diff mbox series

ufs: core: Remove scsi host only if added

Message ID 20240627085104epcms2p5897a3870ea5c6416aa44f94df6c543d7@epcms2p5
State New
Headers show
Series ufs: core: Remove scsi host only if added | expand

Commit Message

Kyoungrul Kim June 27, 2024, 8:51 a.m. UTC
If host tries to remove ufshcd driver from a ufs device, it would cause
a kernel panic if ufshcd_async_scan fails during ufshcd_probe_hba before
adding a scsi host with scsi_add_host and MCQ is enabled since scsi host
has been defered after MCQ configuration introduced by Commit
0cab4023ec7b ("scsi: ufs: core: Defer adding host to SCSI if MCQ is
supported").

To guarantee that scsi host is removed only if it's been added, set the
scsi_host_added flag to true after adding a scsi host and check whether
it's set or not before removing the scsi host.

Signed-off-by: Kyoungrul Kim <k831.kim@samsung.com>
Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Kyoungrul Kim June 28, 2024, 2:18 a.m. UTC | #1
Yes, scsi_add_host has been moved the async function (ufshcd_device_init)as MCQ patch applied.
So the ufshcd driver attaches the device without knowing whether the probe fails or not.
and if host tries to remove ufshcd driver, it makes kernel panic.
So it became necessary to check whether to add a host or not.
 
--------- Original Message ---------
Sender : Bart Van Assche <bvanassche@acm.org>
Date : 2024-06-28 01:25 (GMT+9)
Title : Re: [PATCH] ufs: core: Remove scsi host only if added
 
On 6/27/24 1:51 AM, 김경률 wrote:
> @@ -10638,6 +10639,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>                           dev_err(hba->dev, "scsi_add_host failed\n");
>                           goto out_disable;
>                   }
> +                hba->scsi_host_added = true;
>           }


Why has no "hba->scsi_host_added = true" assignment been added in
ufshcd_device_init()? Isn't that a bug?

Bart.
Bart Van Assche June 28, 2024, 7:30 p.m. UTC | #2
On 6/27/24 7:18 PM, Kyoungrul Kim wrote:
> Yes, scsi_add_host has been moved the async function (ufshcd_device_init)as MCQ patch applied.
> So the ufshcd driver attaches the device without knowing whether the probe fails or not.
> and if host tries to remove ufshcd driver, it makes kernel panic.
> So it became necessary to check whether to add a host or not.

There are two scsi_add_host() calls in the UFS driver and this patch
only adds one "hba->scsi_host_added = true" assignment. Shouldn't this
patch add two such assignments?

Thanks,

Bart.
Kyoungrul Kim June 29, 2024, 4:39 a.m. UTC | #3
Yes, you are right. The ufshcd_init() and ufshcd_device_init() call scsi_add_host().
However, the ufs_device_init() already has the "hba->scsi_host_added = true" assignment.
So, I added only one assignment.
 
 
--------- Original Message ---------
Sender : Bart Van Assche <bvanassche@acm.org>
Date : 2024-06-29 04:30 (GMT+9)
Title : Re: [PATCH] ufs: core: Remove scsi host only if added
 
On 6/27/24 7:18 PM, Kyoungrul Kim wrote:
> Yes, scsi_add_host has been moved the async function (ufshcd_device_init)as MCQ patch applied.
> So the ufshcd driver attaches the device without knowing whether the probe fails or not.
> and if host tries to remove ufshcd driver, it makes kernel panic.
> So it became necessary to check whether to add a host or not.

There are two scsi_add_host() calls in the UFS driver and this patch
only adds one "hba->scsi_host_added = true" assignment. Shouldn't this
patch add two such assignments?

Thanks,

Bart.
Bart Van Assche July 1, 2024, 5:12 p.m. UTC | #4
On 6/27/24 1:51 AM, 김경률 wrote:
> If host tries to remove ufshcd driver from a ufs device, it would cause
> a kernel panic if ufshcd_async_scan fails during ufshcd_probe_hba before
> adding a scsi host with scsi_add_host and MCQ is enabled since scsi host
> has been defered after MCQ configuration introduced by Commit
> 0cab4023ec7b ("scsi: ufs: core: Defer adding host to SCSI if MCQ is
> supported").
> 
> To guarantee that scsi host is removed only if it's been added, set the
> scsi_host_added flag to true after adding a scsi host and check whether
> it's set or not before removing the scsi host.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Martin K. Petersen July 5, 2024, 2:51 a.m. UTC | #5
> If host tries to remove ufshcd driver from a ufs device, it would
> cause a kernel panic if ufshcd_async_scan fails during
> ufshcd_probe_hba before adding a scsi host with scsi_add_host and MCQ
> is enabled since scsi host has been defered after MCQ configuration
> introduced by Commit 0cab4023ec7b ("scsi: ufs: core: Defer adding host
> to SCSI if MCQ is supported").
>
> To guarantee that scsi host is removed only if it's been added, set
> the scsi_host_added flag to true after adding a scsi host and check
> whether it's set or not before removing the scsi host.

Applied to 6.11/scsi-staging, thanks!
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a0f8e930167d..101aa08195ce 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10359,7 +10359,8 @@  void ufshcd_remove(struct ufs_hba *hba)
 	blk_mq_destroy_queue(hba->tmf_queue);
 	blk_put_queue(hba->tmf_queue);
 	blk_mq_free_tag_set(&hba->tmf_tag_set);
-	scsi_remove_host(hba->host);
+	if (hba->scsi_host_added)
+		scsi_remove_host(hba->host);
 	/* disable interrupts */
 	ufshcd_disable_intr(hba, hba->intr_mask);
 	ufshcd_hba_stop(hba);
@@ -10638,6 +10639,7 @@  int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 			dev_err(hba->dev, "scsi_add_host failed\n");
 			goto out_disable;
 		}
+		hba->scsi_host_added = true;
 	}
 
 	hba->tmf_tag_set = (struct blk_mq_tag_set) {
@@ -10720,7 +10722,8 @@  int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 free_tmf_tag_set:
 	blk_mq_free_tag_set(&hba->tmf_tag_set);
 out_remove_scsi_host:
-	scsi_remove_host(hba->host);
+	if (hba->scsi_host_added)
+		scsi_remove_host(hba->host);
 out_disable:
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);