[PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Can Guo]

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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] scsi: ufs: Modularize ufs-bsg

[Can Guo]

In order to improve the flexibility of ufs-bsg, modularizing it is a good
choice. This change introduces tristate to ufs-bsg to allow users compile
it as an external module.

Signed-off-by: Can Guo <cang@codeaurora.org>

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index d14c224..d43655a 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -143,7 +143,7 @@ config SCSI_UFS_TI_J721E
 	  If unsure, say N.
 
 config SCSI_UFS_BSG
-	bool "Universal Flash Storage BSG device node"
+	tristate "Universal Flash Storage BSG device node"
 	depends on SCSI_UFSHCD
 	select BLK_DEV_BSGLIB
 	help
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 94c6c5d..904eff1 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
 ufshcd-core-y				+= ufshcd.o ufs-sysfs.o
-ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
+obj-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
 obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 3a2e68f..9c49b4e 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2018 Western Digital Corporation
  */
+#include <linux/platform_device.h>
 #include "ufs_bsg.h"
 
 static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
@@ -158,53 +159,45 @@ static int ufs_bsg_request(struct bsg_job *job)
 
 /**
  * ufs_bsg_remove - detach and remove the added ufs-bsg node
- * @hba: per adapter object
+ * @pdev: Pointer to platform device handle
  *
- * Should be called when unloading the driver.
+ * Return zero for success and non-zero for failure
  */
-void ufs_bsg_remove(struct ufs_hba *hba)
+static int ufs_bsg_remove(struct platform_device *pdev)
 {
-	struct device *bsg_dev = &hba->bsg_dev;
+	struct ufs_hba *hba;
+
+	hba = (struct ufs_hba *)pdev->dev.platform_data;
+	if (!hba)
+		return -ENODEV;
 
 	if (!hba->bsg_queue)
-		return;
+		return 0;
 
 	bsg_remove_queue(hba->bsg_queue);
+	hba->bsg_queue = NULL;
 
-	device_del(bsg_dev);
-	put_device(bsg_dev);
-}
-
-static inline void ufs_bsg_node_release(struct device *dev)
-{
-	put_device(dev->parent);
+	return 0;
 }
 
 /**
- * ufs_bsg_probe - Add ufs bsg device node
- * @hba: per adapter object
+ * ufs_bsg_probe - Probe routine of the driver
+ * @pdev: Pointer to platform device handle
  *
- * Called during initial loading of the driver, and before scsi_scan_host.
+ * Return zero for success and non-zero for failure
  */
-int ufs_bsg_probe(struct ufs_hba *hba)
+static int ufs_bsg_probe(struct platform_device *pdev)
 {
-	struct device *bsg_dev = &hba->bsg_dev;
-	struct Scsi_Host *shost = hba->host;
-	struct device *parent = &shost->shost_gendev;
+	struct ufs_hba *hba;
+	struct device *bsg_dev;
 	struct request_queue *q;
 	int ret;
 
-	device_initialize(bsg_dev);
-
-	bsg_dev->parent = get_device(parent);
-	bsg_dev->release = ufs_bsg_node_release;
-
-	dev_set_name(bsg_dev, "ufs-bsg");
-
-	ret = device_add(bsg_dev);
-	if (ret)
-		goto out;
+	hba = (struct ufs_hba *)pdev->dev.platform_data;
+	if (!hba)
+		return -ENODEV;
 
+	bsg_dev = &pdev->dev;
 	q = bsg_setup_queue(bsg_dev, dev_name(bsg_dev), ufs_bsg_request, NULL, 0);
 	if (IS_ERR(q)) {
 		ret = PTR_ERR(q);
@@ -216,7 +209,19 @@ int ufs_bsg_probe(struct ufs_hba *hba)
 	return 0;
 
 out:
-	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n", shost->host_no);
-	put_device(bsg_dev);
+	dev_err(bsg_dev, "fail to initialize bsg node, err %d\n", ret);
 	return ret;
 }
+
+static struct platform_driver ufs_bsg_driver = {
+	.probe = ufs_bsg_probe,
+	.remove = ufs_bsg_remove,
+	.driver = {
+		.name = "ufs-bsg",
+	},
+};
+
+module_platform_driver(ufs_bsg_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:ufs-bsg");
diff --git a/drivers/scsi/ufs/ufs_bsg.h b/drivers/scsi/ufs/ufs_bsg.h
index d099187..9d922c0 100644
--- a/drivers/scsi/ufs/ufs_bsg.h
+++ b/drivers/scsi/ufs/ufs_bsg.h
@@ -12,12 +12,4 @@
 #include "ufshcd.h"
 #include "ufs.h"
 
-#ifdef CONFIG_SCSI_UFS_BSG
-void ufs_bsg_remove(struct ufs_hba *hba);
-int ufs_bsg_probe(struct ufs_hba *hba);
-#else
-static inline void ufs_bsg_remove(struct ufs_hba *hba) {}
-static inline int ufs_bsg_probe(struct ufs_hba *hba) {return 0; }
-#endif
-
 #endif /* UFS_BSG_H */
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 76f9be7..90dc399 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -393,6 +393,7 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 	void __iomem *mmio_base;
 	int irq, err;
 	struct device *dev = &pdev->dev;
+	struct platform_device *bsg_pdev;
 
 	mmio_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(mmio_base)) {
@@ -440,6 +441,17 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	bsg_pdev = platform_device_register_data(dev, "ufs-bsg",
+						 hba->host->host_no, hba,
+						 sizeof(struct ufs_hba));
+	/* Failure here is non-fatal */
+	if (IS_ERR(bsg_pdev)) {
+		err = PTR_ERR(bsg_pdev);
+		dev_warn(dev, "Register bsg platform device failed %d\n", err);
+	} else {
+		hba->bsg_pdev = bsg_pdev;
+	}
+
 	return 0;
 
 dealloc_host:
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a86b0fd..0160cc3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -42,6 +42,7 @@
 #include <linux/nls.h>
 #include <linux/of.h>
 #include <linux/bitfield.h>
+#include <linux/platform_device.h>
 #include "ufshcd.h"
 #include "ufs_quirks.h"
 #include "unipro.h"
@@ -2093,6 +2094,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	ufshcd_release(hba);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufshcd_send_uic_cmd);
 
 /**
  * ufshcd_map_sg - Map scatter-gather list to prdt
@@ -6024,6 +6026,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(ufshcd_exec_raw_upiu_cmd);
 
 /**
  * ufshcd_eh_device_reset_handler - device reset handler registered to
@@ -7043,9 +7046,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 			}
 			hba->clk_scaling.is_allowed = true;
 		}
-
-		ufs_bsg_probe(hba);
-
 		scsi_scan_host(hba->host);
 		pm_runtime_put_sync(hba->dev);
 	}
@@ -8248,7 +8248,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
  */
 void ufshcd_remove(struct ufs_hba *hba)
 {
-	ufs_bsg_remove(hba);
+	platform_device_unregister(hba->bsg_pdev);
 	ufs_sysfs_remove_nodes(hba->dev);
 	scsi_remove_host(hba->host);
 	scsi_host_put(hba->host);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2740f69..dd86404 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -734,7 +734,7 @@ struct ufs_hba {
 	struct ufs_desc_size desc_size;
 	atomic_t scsi_block_reqs_cnt;
 
-	struct device		bsg_dev;
+	struct platform_device	*bsg_pdev;
 	struct request_queue	*bsg_queue;
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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

-- 

Thanks,

Can Guo.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Greg KH]

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

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: ufs: Put SCSI host after remove it

[cang]

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.