| Message ID | 1576328616-30404-2-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Modularize ufs-bsg | expand |
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 <cang@codeaurora.org> > > 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 <cang@codeaurora.org> >> >> 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);
On 2019-12-14 14:24, cang@codeaurora.org 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.
On 2019-12-16 05:55, Bart Van Assche wrote: > On 2019-12-14 14:24, cang@codeaurora.org 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. Hi Bart, This is applied to 5.5/scsi-queue. The two changes I patsed from you are not merged yet, I am still doing code review to them, so there is no blk_cleanup_queue() calls in my code base. I am just saying you may move your blk_cleanup_queue() calls below cancel_work_sync(&hba->eh_work) if my change applies. How do you think? scsi_host_put() was there before but explicitly removed by afa3dfd42d205b106787476647735aa1de1a5d02. I agree with you, without this change, there is memory leak. Thanks, Can Guo.
On 2019-12-15 17:34, cang@codeaurora.org wrote: > This is applied to 5.5/scsi-queue. The two changes I patsed from you are > not merged yet, I am still doing code review to them, so there is no > blk_cleanup_queue() calls in my code base. I am just saying you may move > your blk_cleanup_queue() calls below cancel_work_sync(&hba->eh_work) if > my change applies. How do you think? > > scsi_host_put() was there before but explicitly removed by > afa3dfd42d205b106787476647735aa1de1a5d02. I agree with you, without this > change, there is memory leak. Hi Can, Since your patch restores a call that was removed earlier, please consider adding a Fixes: tag to your patch. Please also have a look at https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=5.6/scsi-queue. As one can see my patches that introduce blk_cleanup_queue() and blk_mq_free_tag_set() calls have already been queued on Martin's 5.6/scsi-queue branch. Bart.
On 2019-12-16 10:39, Bart Van Assche wrote: > On 2019-12-15 17:34, cang@codeaurora.org wrote: >> This is applied to 5.5/scsi-queue. The two changes I patsed from you >> are >> not merged yet, I am still doing code review to them, so there is no >> blk_cleanup_queue() calls in my code base. I am just saying you may >> move >> your blk_cleanup_queue() calls below cancel_work_sync(&hba->eh_work) >> if >> my change applies. How do you think? >> >> scsi_host_put() was there before but explicitly removed by >> afa3dfd42d205b106787476647735aa1de1a5d02. I agree with you, without >> this >> change, there is memory leak. > > Hi Can, > > Since your patch restores a call that was removed earlier, please > consider adding a Fixes: tag to your patch. > > Please also have a look at > https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=5.6/scsi-queue. > As one can see my patches that introduce blk_cleanup_queue() and > blk_mq_free_tag_set() calls have already been queued on Martin's > 5.6/scsi-queue branch. > > Bart. Hi Bart, Sure, I will add the Fixes tag and rebase my changes. How about the logic part of this change? Does it look good to you? Sorry I was not aware of that your changes have been applied to 5.6/scsi-queue. I am still trying to get it tested on my setups... Anyways, aside of hba->cmd_queue, tearing down hba->tmf_queue before scsi_remove_host() may be problem too. Requests can still be sent before and during scsi_remove_host(). If a request timed out, task abort will be invoked to abort the request, during which hba->tmf_queue is expected to be present. Please correct me if I am wrong. Thanks, Can Guo.
On 2019-12-16 11:12, cang@codeaurora.org wrote: > On 2019-12-16 10:39, Bart Van Assche wrote: >> On 2019-12-15 17:34, cang@codeaurora.org wrote: >>> This is applied to 5.5/scsi-queue. The two changes I patsed from you >>> are >>> not merged yet, I am still doing code review to them, so there is no >>> blk_cleanup_queue() calls in my code base. I am just saying you may >>> move >>> your blk_cleanup_queue() calls below cancel_work_sync(&hba->eh_work) >>> if >>> my change applies. How do you think? >>> >>> scsi_host_put() was there before but explicitly removed by >>> afa3dfd42d205b106787476647735aa1de1a5d02. I agree with you, without >>> this >>> change, there is memory leak. >> >> Hi Can, >> >> Since your patch restores a call that was removed earlier, please >> consider adding a Fixes: tag to your patch. >> >> Please also have a look at >> https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=5.6/scsi-queue. >> As one can see my patches that introduce blk_cleanup_queue() and >> blk_mq_free_tag_set() calls have already been queued on Martin's >> 5.6/scsi-queue branch. >> >> Bart. > > Hi Bart, > > Sure, I will add the Fixes tag and rebase my changes. How about the > logic > part of this change? Does it look good to you? > > Sorry I was not aware of that your changes have been applied to > 5.6/scsi-queue. > I am still trying to get it tested on my setups... > Anyways, aside of hba->cmd_queue, tearing down hba->tmf_queue before > scsi_remove_host() may be problem too. Requests can still be > sent before and during scsi_remove_host(). If a request timed out, > task abort will be invoked to abort the request, during which > hba->tmf_queue is expected to be present. Please correct me if I am > wrong. > > Thanks, > > Can Guo. Hi Bart Just found that I should also remove the ufshcd_dealloc_host() called in ufshcd_pci_remove() to make sure the deallocation is only handled by ufshcd_remove(). Thanks, Can Guo.
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 <cang@codeaurora.org> >> >> 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, As SCSI host is allocated in ufshcd_platform_init() during platform drive probe, it is much more appropriate if platform driver calls ufshcd_dealloc_host() in their own drv->remove() path. How do you think if I change it as below? If it is OK to you, please ignore my previous mails. diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 3d4582e..ea45756 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct platform_device *pdev) pm_runtime_get_sync(&(pdev)->dev); ufshcd_remove(hba); + ufshcd_dealloc_host(hba); return 0; } Thanks, Can Guo.
On 12/16/19 6:31 AM, cang@codeaurora.org wrote: > As SCSI host is allocated in ufshcd_platform_init() during platform > drive probe, it is much more appropriate if platform driver calls > ufshcd_dealloc_host() in their own drv->remove() path. How do you > think if I change it as below? If it is OK to you, please ignore my > previous mails. > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 3d4582e..ea45756 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct platform_device > *pdev) > > pm_runtime_get_sync(&(pdev)->dev); > ufshcd_remove(hba); > + ufshcd_dealloc_host(hba); > return 0; > } Hi Can, Apparently some UFS drivers call ufshcd_remove() only and others (PCIe) call both ufshcd_remove() and ufshcd_dealloc_host(). I think that the above change will cause trouble for the PCIe driver unless the ufshcd_dealloc_host() call is removed from ufshcd_pci_remove(). Thanks, Bart.
On 12/15/19 7:12 PM, cang@codeaurora.org wrote: > Sure, I will add the Fixes tag and rebase my changes. How about the logic > part of this change? Does it look good to you? Hi Can, You may want to ask someone who is more familiar with the UFS driver than I to have a look. I'm not a UFS expert ... > Sorry I was not aware of that your changes have been applied to > 5.6/scsi-queue. > I am still trying to get it tested on my setups... > Anyways, aside of hba->cmd_queue, tearing down hba->tmf_queue before > scsi_remove_host() may be problem too. Requests can still be > sent before and during scsi_remove_host(). If a request timed out, > task abort will be invoked to abort the request, during which > hba->tmf_queue is expected to be present. Please correct me if I am wrong. I agree that the code I added in ufshcd_remove() probably needs to be moved somewhere below the scsi_remove_host() call. Thanks, Bart.
On Mon, Dec 16, 2019 at 10:31:29PM +0800, cang@codeaurora.org wrote: > 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 <cang@codeaurora.org> > > > > > > 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, > > As SCSI host is allocated in ufshcd_platform_init() during platform > drive probe, it is much more appropriate if platform driver calls > ufshcd_dealloc_host() in their own drv->remove() path. How do you > think if I change it as below? If it is OK to you, please ignore my > previous mails. > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 3d4582e..ea45756 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct platform_device > *pdev) > > pm_runtime_get_sync(&(pdev)->dev); > ufshcd_remove(hba); > + ufshcd_dealloc_host(hba); > return 0; > } Wait, why is this a platform device? Don't you hang off of a pci device? Or am I missing something earlier in this patchset? thanks, greg k-h
On 2019-12-17 01:39, Bart Van Assche wrote: > On 12/16/19 6:31 AM, cang@codeaurora.org wrote: >> As SCSI host is allocated in ufshcd_platform_init() during platform >> drive probe, it is much more appropriate if platform driver calls >> ufshcd_dealloc_host() in their own drv->remove() path. How do you >> think if I change it as below? If it is OK to you, please ignore my >> previous mails. >> >> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c >> index 3d4582e..ea45756 100644 >> --- a/drivers/scsi/ufs/ufs-qcom.c >> +++ b/drivers/scsi/ufs/ufs-qcom.c >> @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct >> platform_device *pdev) >> >> pm_runtime_get_sync(&(pdev)->dev); >> ufshcd_remove(hba); >> + ufshcd_dealloc_host(hba); >> return 0; >> } > > Hi Can, > > Apparently some UFS drivers call ufshcd_remove() only and others > (PCIe) call both ufshcd_remove() and ufshcd_dealloc_host(). I think > that the above change will cause trouble for the PCIe driver unless > the ufshcd_dealloc_host() call is removed from ufshcd_pci_remove(). > > Thanks, > > Bart. Hi Bart, You may get me wrong. I mean we should do like what ufshcd-pci.c does. As driver probe routine allocates SCSI host, then driver remove() should de-allocate it. Meaning ufs_qcom_remove() should call both ufshcd_remove() and ufshcd_dealloc_host(). diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 3d4582e..ea45756 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct platform_device *pdev) pm_runtime_get_sync(&(pdev)->dev); ufshcd_remove(hba); + ufshcd_dealloc_host(hba); return 0; } Thanks, Can Guo.
On 2019-12-17 02:05, Greg KH wrote: > On Mon, Dec 16, 2019 at 10:31:29PM +0800, cang@codeaurora.org wrote: >> 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 <cang@codeaurora.org> >> > > >> > > 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, >> >> As SCSI host is allocated in ufshcd_platform_init() during platform >> drive probe, it is much more appropriate if platform driver calls >> ufshcd_dealloc_host() in their own drv->remove() path. How do you >> think if I change it as below? If it is OK to you, please ignore my >> previous mails. >> >> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c >> index 3d4582e..ea45756 100644 >> --- a/drivers/scsi/ufs/ufs-qcom.c >> +++ b/drivers/scsi/ufs/ufs-qcom.c >> @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct >> platform_device >> *pdev) >> >> pm_runtime_get_sync(&(pdev)->dev); >> ufshcd_remove(hba); >> + ufshcd_dealloc_host(hba); >> return 0; >> } > > Wait, why is this a platform device? Don't you hang off of a pci > device? Or am I missing something earlier in this patchset? > > thanks, > > greg k-h Hi Greg, I am not saying someone is a platform device here. My point is whoever allocates the SCSI host in its drv->probe(), should de-allocate it in its own drv->remove(), just like what ufshcd-pci.c does. Thanks, Can Guo.
On 12/16/19 4:46 PM, cang@codeaurora.org wrote: > On 2019-12-17 01:39, Bart Van Assche wrote: >> Apparently some UFS drivers call ufshcd_remove() only and others >> (PCIe) call both ufshcd_remove() and ufshcd_dealloc_host(). I think >> that the above change will cause trouble for the PCIe driver unless >> the ufshcd_dealloc_host() call is removed from ufshcd_pci_remove(). > > You may get me wrong. I mean we should do like what ufshcd-pci.c does. > As driver probe routine allocates SCSI host, then driver remove() should > de-allocate it. Meaning ufs_qcom_remove() should call both ufshcd_remove() > and ufshcd_dealloc_host(). > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 3d4582e..ea45756 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct platform_device > *pdev) > > pm_runtime_get_sync(&(pdev)->dev); > ufshcd_remove(hba); > + ufshcd_dealloc_host(hba); > return 0; > } Hi Can, If it is possible to move the ufshcd_dealloc_host() into ufshcd_remove() then I would prefer to do that. If all UFS transport drivers need that call then I think that call should happen from the UFS core instead of from the transport drivers. Thanks, Bart.
On 2019-12-17 09:15, Bart Van Assche wrote: > On 12/16/19 4:46 PM, cang@codeaurora.org wrote: >> On 2019-12-17 01:39, Bart Van Assche wrote: >>> Apparently some UFS drivers call ufshcd_remove() only and others >>> (PCIe) call both ufshcd_remove() and ufshcd_dealloc_host(). I think >>> that the above change will cause trouble for the PCIe driver unless >>> the ufshcd_dealloc_host() call is removed from ufshcd_pci_remove(). >> >> You may get me wrong. I mean we should do like what ufshcd-pci.c does. >> As driver probe routine allocates SCSI host, then driver remove() >> should >> de-allocate it. Meaning ufs_qcom_remove() should call both >> ufshcd_remove() >> and ufshcd_dealloc_host(). >> >> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c >> index 3d4582e..ea45756 100644 >> --- a/drivers/scsi/ufs/ufs-qcom.c >> +++ b/drivers/scsi/ufs/ufs-qcom.c >> @@ -3239,6 +3239,7 @@ static int ufs_qcom_remove(struct >> platform_device *pdev) >> >> pm_runtime_get_sync(&(pdev)->dev); >> ufshcd_remove(hba); >> + ufshcd_dealloc_host(hba); >> return 0; >> } > > Hi Can, > > If it is possible to move the ufshcd_dealloc_host() into > ufshcd_remove() then I would prefer to do that. If all UFS transport > drivers need that call then I think that call should happen from the > UFS core instead of from the transport drivers. > > Thanks, > > Bart. Yeah, that is an once for all solution, but I not sure if PCI folks are OK if I remove the ufshcd_dealloc_host() call from their driver. In next version, I will try to make such change and see. Thanks, Can Guo.
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);
In ufshcd_remove(), after SCSI host is removed, put it once so that its resources can be released. Signed-off-by: Can Guo <cang@codeaurora.org>