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.
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);