In ufshcd_remove(), after SCSI host is removed, put it once so that its
resources can be released.
Signed-off-by: Can Guo <[email protected]>
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b5966fa..a86b0fd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
ufs_bsg_remove(hba);
ufs_sysfs_remove_nodes(hba->dev);
scsi_remove_host(hba->host);
+ scsi_host_put(hba->host);
/* disable interrupts */
ufshcd_disable_intr(hba, hba->intr_mask);
ufshcd_hba_stop(hba, true);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 12/14/19 8:03 AM, Can Guo wrote:
> In ufshcd_remove(), after SCSI host is removed, put it once so that its
> resources can be released.
>
> Signed-off-by: Can Guo <[email protected]>
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b5966fa..a86b0fd 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
> ufs_bsg_remove(hba);
> ufs_sysfs_remove_nodes(hba->dev);
> scsi_remove_host(hba->host);
> + scsi_host_put(hba->host);
> /* disable interrupts */
> ufshcd_disable_intr(hba, hba->intr_mask);
> ufshcd_hba_stop(hba, true);
Hi Can,
The UFS driver may queue work asynchronously and that asynchronous work
may refer to the SCSI host, e.g. ufshcd_err_handler(). Is it guaranteed
that all that asynchronous work has finished before scsi_host_put() is
called?
Thanks,
Bart.
On 2019-12-15 02:32, Bart Van Assche wrote:
> On 12/14/19 8:03 AM, Can Guo wrote:
>> In ufshcd_remove(), after SCSI host is removed, put it once so that
>> its
>> resources can be released.
>>
>> Signed-off-by: Can Guo <[email protected]>
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index b5966fa..a86b0fd 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
>> ufs_bsg_remove(hba);
>> ufs_sysfs_remove_nodes(hba->dev);
>> scsi_remove_host(hba->host);
>> + scsi_host_put(hba->host);
>> /* disable interrupts */
>> ufshcd_disable_intr(hba, hba->intr_mask);
>> ufshcd_hba_stop(hba, true);
>
> Hi Can,
>
> The UFS driver may queue work asynchronously and that asynchronous
> work may refer to the SCSI host, e.g. ufshcd_err_handler(). Is it
> guaranteed that all that asynchronous work has finished before
> scsi_host_put() is called?
>
> Thanks,
>
> Bart.
Hi Bart,
Thanks for pointing it out. I noticed that you are changing this
path too in below 2 changes.
https://marc.info/?l=linux-scsi&m=157591520015924&w=2
https://marc.info/?l=linux-scsi&m=157591519915923&w=2
Actually the async works you pointed may also affect your change,
because you may tear down the hba->cmd_queue too early, as there can be
devm commands sent by clock gating, eeh_work and eh_work after that
point,
meaning when blk_get_request is called in exec_dev_cmd(), hba->cmd_queue
may have been released already.
@@ -8263,6 +8232,7 @@ void ufshcd_remove(struct ufs_hba *hba)
{
ufs_bsg_remove(hba);
ufs_sysfs_remove_nodes(hba->dev);
+ blk_cleanup_queue(hba->cmd_queue);
scsi_remove_host(hba->host);
/* disable interrupts */
ufshcd_disable_intr(hba, hba->intr_mask);
How do you think if I replace my patch with below one?
In this way, you can also move blk_cleanup_queue() behind
cancel_work_sync(eh_work).
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b5966fa..bd4ae75 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8251,15 +8251,17 @@ void ufshcd_remove(struct ufs_hba *hba)
ufs_bsg_remove(hba);
ufs_sysfs_remove_nodes(hba->dev);
scsi_remove_host(hba->host);
- /* disable interrupts */
- ufshcd_disable_intr(hba, hba->intr_mask);
- ufshcd_hba_stop(hba, true);
-
ufshcd_exit_clk_scaling(hba);
ufshcd_exit_clk_gating(hba);
if (ufshcd_is_clkscaling_supported(hba))
device_remove_file(hba->dev,
&hba->clk_scaling.enable_attr);
+ cancel_work_sync(&hba->eeh_work);
+ cancel_work_sync(&hba->eh_work);
+ /* disable interrupts */
+ ufshcd_disable_intr(hba, hba->intr_mask);
+ ufshcd_hba_stop(hba, true);
ufshcd_hba_exit(hba);
+ ufshcd_dealloc_host(hba);
}
EXPORT_SYMBOL_GPL(ufshcd_remove);
--
Thanks,
Can Guo.
On 2019-12-14 14:24, [email protected] wrote:
> How do you think if I replace my patch with below one?
> In this way, you can also move blk_cleanup_queue() behind
> cancel_work_sync(eh_work).
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b5966fa..bd4ae75 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8251,15 +8251,17 @@ void ufshcd_remove(struct ufs_hba *hba)
> ufs_bsg_remove(hba);
> ufs_sysfs_remove_nodes(hba->dev);
> scsi_remove_host(hba->host);
> - /* disable interrupts */
> - ufshcd_disable_intr(hba, hba->intr_mask);
> - ufshcd_hba_stop(hba, true);
> -
> ufshcd_exit_clk_scaling(hba);
> ufshcd_exit_clk_gating(hba);
> if (ufshcd_is_clkscaling_supported(hba))
> device_remove_file(hba->dev,
> &hba->clk_scaling.enable_attr);
> + cancel_work_sync(&hba->eeh_work);
> + cancel_work_sync(&hba->eh_work);
> + /* disable interrupts */
> + ufshcd_disable_intr(hba, hba->intr_mask);
> + ufshcd_hba_stop(hba, true);
> ufshcd_hba_exit(hba);
> + ufshcd_dealloc_host(hba);
> }
> EXPORT_SYMBOL_GPL(ufshcd_remove);
Hi Can,
To which kernel tree does the above patch apply? I'm asking this because
I don't see the recently added blk_cleanup_queue() calls in the above
patch. Please start from Martin's latest scsi-queue branch when
preparing SCSI patches.
Additionally, is it on purpose that there is no scsi_host_put() call in
the above code? I'd like to keep that call because without that call a
memory leak will occur when unloading the ufshcd-core kernel driver.
Thanks,
Bart.