| Message ID | 20240627085104epcms2p5897a3870ea5c6416aa44f94df6c543d7@epcms2p5 |
|---|---|
| State | New |
| Headers | show |
| Series | ufs: core: Remove scsi host only if added | expand |
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.
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.
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.
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>
> 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 --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);
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(-)