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

[PATCH v2 1/3] 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>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

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

RE: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

[Avri Altman]

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

This is not really part of this patchset, is it? 

> ---
>  drivers/scsi/ufs/ufshcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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

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

[cang]

On 2019-12-11 18:37, Avri Altman 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>
> 
> This is not really part of this patchset, is it?
> 

Hi Avri,

I put this change in the same patchset due to
#1. The main patch has dependency on it
#2. Consider a scenario where platform driver is also compiled as a 
module, say ufs_qcom.ko.
     In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko. If do 
insmod ufs-qcom.ko
     then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without this 
change, because scsi
     host was not release, the new scsi host will have a different host 
number, meaning
     hba->host->host_no will be 1, but not 0. Now if do insmod ufs-bsg.ko 
now, the ufs-bsg dev
     created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep 
trying these operations,
     the ufs-bsg device's name will be messed up. This change make sure 
after ufs-qcom.ko is removed
     and installed back, its hba->host->host_no stays 0, so that ufs-bsg 
device name stays same.

Thanks,

Can Guo.

>> ---
>>  drivers/scsi/ufs/ufshcd.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> 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

RE: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

[Avri Altman]

> 
> 
> On 2019-12-11 18:37, Avri Altman 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>
> >
> > This is not really part of this patchset, is it?
> >
> 
> Hi Avri,
> 
> I put this change in the same patchset due to #1. The main patch has
> dependency on it #2. Consider a scenario where platform driver is also compiled
> as a module, say ufs_qcom.ko.
>      In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko. If do insmod
> ufs-qcom.ko
>      then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without this
> change, because scsi
>      host was not release, the new scsi host will have a different host number,
> meaning
>      hba->host->host_no will be 1, but not 0. Now if do insmod ufs-bsg.ko now,
> the ufs-bsg dev
>      created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep trying these
> operations,
>      the ufs-bsg device's name will be messed up. This change make sure after ufs-
> qcom.ko is removed
>      and installed back, its hba->host->host_no stays 0, so that ufs-bsg device
> name stays same.
Looks like we needed to manage the ufs-bsg nodes using an IDA, instead of host_no?


> 
> Thanks,
> 
> Can Guo.
> 
> >> ---
> >>  drivers/scsi/ufs/ufshcd.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> 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

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

[cang]

On 2019-12-11 19:22, Avri Altman wrote:
>> 
>> 
>> On 2019-12-11 18:37, Avri Altman 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>
>> >
>> > This is not really part of this patchset, is it?
>> >
>> 
>> Hi Avri,
>> 
>> I put this change in the same patchset due to #1. The main patch has
>> dependency on it #2. Consider a scenario where platform driver is also 
>> compiled
>> as a module, say ufs_qcom.ko.
>>      In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko. If 
>> do insmod
>> ufs-qcom.ko
>>      then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without 
>> this
>> change, because scsi
>>      host was not release, the new scsi host will have a different 
>> host number,
>> meaning
>>      hba->host->host_no will be 1, but not 0. Now if do insmod 
>> ufs-bsg.ko now,
>> the ufs-bsg dev
>>      created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep 
>> trying these
>> operations,
>>      the ufs-bsg device's name will be messed up. This change make 
>> sure after ufs-
>> qcom.ko is removed
>>      and installed back, its hba->host->host_no stays 0, so that 
>> ufs-bsg device
>> name stays same.
> Looks like we needed to manage the ufs-bsg nodes using an IDA, instead
> of host_no?
> 
> 

Marking one ufs-bsg dev with host_no still has its advantage. If we have 
more
than one hba host, we can find the right ufs-bsgX dev by reading the 
sg/sd/bsg
device's host->host_unique_id (through SCSI_IOCTL_GET_IDLUN for 
example).
But If ufs-bsg has its own ID, we will lost this "mapping".

Thanks,

Can Guo.

>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>> >> ---
>> >>  drivers/scsi/ufs/ufshcd.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> 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

RE: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

[Avri Altman]

 
> On 2019-12-11 19:22, Avri Altman wrote:
> >>
> >>
> >> On 2019-12-11 18:37, Avri Altman 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>
> >> >
> >> > This is not really part of this patchset, is it?
> >> >
> >>
> >> Hi Avri,
> >>
> >> I put this change in the same patchset due to #1. The main patch has
> >> dependency on it #2. Consider a scenario where platform driver is
> >> also compiled as a module, say ufs_qcom.ko.
> >>      In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko.
> >> If do insmod ufs-qcom.ko
> >>      then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without
> >> this change, because scsi
> >>      host was not release, the new scsi host will have a different
> >> host number, meaning
> >>      hba->host->host_no will be 1, but not 0. Now if do insmod
> >> ufs-bsg.ko now, the ufs-bsg dev
> >>      created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep
> >> trying these operations,
> >>      the ufs-bsg device's name will be messed up. This change make
> >> sure after ufs- qcom.ko is removed
> >>      and installed back, its hba->host->host_no stays 0, so that
> >> ufs-bsg device name stays same.
> > Looks like we needed to manage the ufs-bsg nodes using an IDA, instead
> > of host_no?
> >
> >
> 
> Marking one ufs-bsg dev with host_no still has its advantage. If we have more
> than one hba host, we can find the right ufs-bsgX dev by reading the sg/sd/bsg
> device's host->host_unique_id (through SCSI_IOCTL_GET_IDLUN for example).
> But If ufs-bsg has its own ID, we will lost this "mapping".
OK.

[PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Can Guo]

In order to improve the flexibility of ufs-bsg, modulizing 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>
---
 drivers/scsi/ufs/Kconfig   |  3 ++-
 drivers/scsi/ufs/Makefile  |  2 +-
 drivers/scsi/ufs/ufs_bsg.c | 49 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufs_bsg.h |  8 --------
 drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
 drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
 6 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index d14c224..72620ce 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -38,6 +38,7 @@ config SCSI_UFSHCD
 	select PM_DEVFREQ
 	select DEVFREQ_GOV_SIMPLE_ONDEMAND
 	select NLS
+	select BLK_DEV_BSGLIB
 	---help---
 	This selects the support for UFS devices in Linux, say Y and make
 	  sure that you know the name of your UFS host adapter (the card
@@ -143,7 +144,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..302222f 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
  */
 void ufs_bsg_remove(struct ufs_hba *hba)
 {
-	struct device *bsg_dev = &hba->bsg_dev;
+	struct device *bsg_dev = hba->bsg_dev;
 
 	if (!hba->bsg_queue)
 		return;
 
 	bsg_remove_queue(hba->bsg_queue);
 
+	hba->bsg_dev = NULL;
+	hba->bsg_queue = NULL;
 	device_del(bsg_dev);
 	put_device(bsg_dev);
 }
@@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
 static inline void ufs_bsg_node_release(struct device *dev)
 {
 	put_device(dev->parent);
+	kfree(dev);
 }
 
 /**
@@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct device *dev)
  *
  * Called during initial loading of the driver, and before scsi_scan_host.
  */
-int ufs_bsg_probe(struct ufs_hba *hba)
+static int ufs_bsg_probe(struct ufs_hba *hba)
 {
-	struct device *bsg_dev = &hba->bsg_dev;
+	struct device *bsg_dev;
 	struct Scsi_Host *shost = hba->host;
 	struct device *parent = &shost->shost_gendev;
 	struct request_queue *q;
 	int ret;
 
+	bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
+	if (!bsg_dev)
+		return -ENOMEM;
+
+	hba->bsg_dev = bsg_dev;
 	device_initialize(bsg_dev);
 
 	bsg_dev->parent = get_device(parent);
@@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
 
 out:
 	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n", shost->host_no);
+	hba->bsg_dev = NULL;
 	put_device(bsg_dev);
 	return ret;
 }
+
+static int __init ufs_bsg_init(void)
+{
+	struct list_head *hba_list = NULL;
+	struct ufs_hba *hba;
+	int ret = 0;
+
+	ufshcd_get_hba_list_lock(&hba_list);
+	list_for_each_entry(hba, hba_list, list) {
+		ret = ufs_bsg_probe(hba);
+		if (ret)
+			break;
+	}
+	ufshcd_put_hba_list_unlock();
+
+	return ret;
+}
+
+static void __exit ufs_bsg_exit(void)
+{
+	struct list_head *hba_list = NULL;
+	struct ufs_hba *hba;
+
+	ufshcd_get_hba_list_lock(&hba_list);
+	list_for_each_entry(hba, hba_list, list)
+		ufs_bsg_remove(hba);
+	ufshcd_put_hba_list_unlock();
+}
+
+late_initcall_sync(ufs_bsg_init);
+module_exit(ufs_bsg_exit);
+
+MODULE_ALIAS("ufs-bsg");
+MODULE_LICENSE("GPL v2");
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.c b/drivers/scsi/ufs/ufshcd.c
index a86b0fd..7a83a8f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -108,6 +108,22 @@
 		       16, 4, buf, __len, false);                        \
 } while (0)
 
+static LIST_HEAD(ufs_hba_list);
+static DEFINE_MUTEX(ufs_hba_list_lock);
+
+void ufshcd_get_hba_list_lock(struct list_head **list)
+{
+	mutex_lock(&ufs_hba_list_lock);
+	*list = &ufs_hba_list;
+}
+EXPORT_SYMBOL_GPL(ufshcd_get_hba_list_lock);
+
+void ufshcd_put_hba_list_unlock(void)
+{
+	mutex_unlock(&ufs_hba_list_lock);
+}
+EXPORT_SYMBOL_GPL(ufshcd_put_hba_list_unlock);
+
 int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		     const char *prefix)
 {
@@ -2093,6 +2109,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 +6041,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 +7061,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 +8263,16 @@ int ufshcd_shutdown(struct ufs_hba *hba)
  */
 void ufshcd_remove(struct ufs_hba *hba)
 {
-	ufs_bsg_remove(hba);
+	struct device *bsg_dev = hba->bsg_dev;
+
+	mutex_lock(&ufs_hba_list_lock);
+	list_del(&hba->list);
+	if (hba->bsg_queue) {
+		bsg_remove_queue(hba->bsg_queue);
+		device_del(bsg_dev);
+		put_device(bsg_dev);
+	}
+	mutex_unlock(&ufs_hba_list_lock);
 	ufs_sysfs_remove_nodes(hba->dev);
 	scsi_remove_host(hba->host);
 	scsi_host_put(hba->host);
@@ -8494,6 +8518,10 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	async_schedule(ufshcd_async_scan, hba);
 	ufs_sysfs_add_nodes(hba->dev);
 
+	mutex_lock(&ufs_hba_list_lock);
+	list_add_tail(&hba->list, &ufs_hba_list);
+	mutex_unlock(&ufs_hba_list_lock);
+
 	return 0;
 
 out_remove_scsi_host:
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2740f69..893debc 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -74,6 +74,9 @@
 
 struct ufs_hba;
 
+void ufshcd_get_hba_list_lock(struct list_head **list);
+void ufshcd_put_hba_list_unlock(void);
+
 enum dev_cmd_type {
 	DEV_CMD_TYPE_NOP		= 0x0,
 	DEV_CMD_TYPE_QUERY		= 0x1,
@@ -473,6 +476,7 @@ struct ufs_stats {
 
 /**
  * struct ufs_hba - per adapter private structure
+ * @list: Anchored at ufs_hba_list
  * @mmio_base: UFSHCI base register address
  * @ucdl_base_addr: UFS Command Descriptor base address
  * @utrdl_base_addr: UTP Transfer Request Descriptor base address
@@ -527,6 +531,7 @@ struct ufs_stats {
  * @scsi_block_reqs_cnt: reference counting for scsi block requests
  */
 struct ufs_hba {
+	struct list_head list;
 	void __iomem *mmio_base;
 
 	/* Virtual memory reference */
@@ -734,7 +739,7 @@ struct ufs_hba {
 	struct ufs_desc_size desc_size;
 	atomic_t scsi_block_reqs_cnt;
 
-	struct device		bsg_dev;
+	struct device		*bsg_dev;
 	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 v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bjorn Andersson]

On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:

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

Can you please elaborate on what this "flexibility" is and why it's a
good thing?

> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/Kconfig   |  3 ++-
>  drivers/scsi/ufs/Makefile  |  2 +-
>  drivers/scsi/ufs/ufs_bsg.c | 49 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/scsi/ufs/ufs_bsg.h |  8 --------
>  drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
>  drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
>  6 files changed, 87 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index d14c224..72620ce 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -38,6 +38,7 @@ config SCSI_UFSHCD
>  	select PM_DEVFREQ
>  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	select NLS
> +	select BLK_DEV_BSGLIB

Why is this needed?

>  	---help---
>  	This selects the support for UFS devices in Linux, say Y and make
>  	  sure that you know the name of your UFS host adapter (the card
> @@ -143,7 +144,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..302222f 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
>   */
>  void ufs_bsg_remove(struct ufs_hba *hba)
>  {
> -	struct device *bsg_dev = &hba->bsg_dev;
> +	struct device *bsg_dev = hba->bsg_dev;
>  
>  	if (!hba->bsg_queue)
>  		return;
>  
>  	bsg_remove_queue(hba->bsg_queue);
>  
> +	hba->bsg_dev = NULL;
> +	hba->bsg_queue = NULL;
>  	device_del(bsg_dev);
>  	put_device(bsg_dev);
>  }
> @@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
>  static inline void ufs_bsg_node_release(struct device *dev)
>  {
>  	put_device(dev->parent);
> +	kfree(dev);
>  }
>  
>  /**
> @@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct device *dev)
>   *
>   * Called during initial loading of the driver, and before scsi_scan_host.
>   */
> -int ufs_bsg_probe(struct ufs_hba *hba)
> +static int ufs_bsg_probe(struct ufs_hba *hba)
>  {
> -	struct device *bsg_dev = &hba->bsg_dev;
> +	struct device *bsg_dev;
>  	struct Scsi_Host *shost = hba->host;
>  	struct device *parent = &shost->shost_gendev;
>  	struct request_queue *q;
>  	int ret;
>  
> +	bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
> +	if (!bsg_dev)
> +		return -ENOMEM;
> +
> +	hba->bsg_dev = bsg_dev;
>  	device_initialize(bsg_dev);
>  
>  	bsg_dev->parent = get_device(parent);
> @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
>  
>  out:
>  	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n", shost->host_no);
> +	hba->bsg_dev = NULL;
>  	put_device(bsg_dev);
>  	return ret;
>  }
> +
> +static int __init ufs_bsg_init(void)
> +{
> +	struct list_head *hba_list = NULL;
> +	struct ufs_hba *hba;
> +	int ret = 0;
> +
> +	ufshcd_get_hba_list_lock(&hba_list);
> +	list_for_each_entry(hba, hba_list, list) {
> +		ret = ufs_bsg_probe(hba);
> +		if (ret)
> +			break;
> +	}

So what happens if I go CONFIG_SCSI_UFS_BSG=y and
CONFIG_SCSI_UFS_QCOM=y?

Wouldn't that mean that ufs_bsg_init() is called before ufshcd_init()
has added the controller to the list? And even in the even that they are
both =m, what happens if they are invoked in the "wrong" order?

> +	ufshcd_put_hba_list_unlock();
> +
> +	return ret;
> +}
> +
> +static void __exit ufs_bsg_exit(void)
> +{
> +	struct list_head *hba_list = NULL;
> +	struct ufs_hba *hba;
> +
> +	ufshcd_get_hba_list_lock(&hba_list);
> +	list_for_each_entry(hba, hba_list, list)
> +		ufs_bsg_remove(hba);
> +	ufshcd_put_hba_list_unlock();
> +}
> +
> +late_initcall_sync(ufs_bsg_init);
> +module_exit(ufs_bsg_exit);
> +
> +MODULE_ALIAS("ufs-bsg");

The purpose of MODULE_ALIAS() is to facilitate module autoloading, but
as you probe the bsg device from the initcall of the bsg driver itself I
don't see how that would happen, and as such I don't think this alias
has a purpose.

> +MODULE_LICENSE("GPL v2");
> 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.c b/drivers/scsi/ufs/ufshcd.c
> index a86b0fd..7a83a8f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -108,6 +108,22 @@
>  		       16, 4, buf, __len, false);                        \
>  } while (0)
>  
> +static LIST_HEAD(ufs_hba_list);
> +static DEFINE_MUTEX(ufs_hba_list_lock);
> +
> +void ufshcd_get_hba_list_lock(struct list_head **list)
> +{
> +	mutex_lock(&ufs_hba_list_lock);
> +	*list = &ufs_hba_list;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_get_hba_list_lock);
> +
> +void ufshcd_put_hba_list_unlock(void)
> +{
> +	mutex_unlock(&ufs_hba_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_put_hba_list_unlock);
> +
>  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>  		     const char *prefix)
>  {
> @@ -2093,6 +2109,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 +6041,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 +7061,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 +8263,16 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>   */
>  void ufshcd_remove(struct ufs_hba *hba)
>  {
> -	ufs_bsg_remove(hba);
> +	struct device *bsg_dev = hba->bsg_dev;
> +
> +	mutex_lock(&ufs_hba_list_lock);
> +	list_del(&hba->list);
> +	if (hba->bsg_queue) {
> +		bsg_remove_queue(hba->bsg_queue);
> +		device_del(bsg_dev);

Am I reading this correct in that you probe the bsg_dev form initcall
and you delete it as the ufshcd instance is removed? That's not okay.

Regards,
Bjorn

> +		put_device(bsg_dev);
> +	}
> +	mutex_unlock(&ufs_hba_list_lock);
>  	ufs_sysfs_remove_nodes(hba->dev);
>  	scsi_remove_host(hba->host);
>  	scsi_host_put(hba->host);
> @@ -8494,6 +8518,10 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	async_schedule(ufshcd_async_scan, hba);
>  	ufs_sysfs_add_nodes(hba->dev);
>  
> +	mutex_lock(&ufs_hba_list_lock);
> +	list_add_tail(&hba->list, &ufs_hba_list);
> +	mutex_unlock(&ufs_hba_list_lock);
> +
>  	return 0;
>  
>  out_remove_scsi_host:
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 2740f69..893debc 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -74,6 +74,9 @@
>  
>  struct ufs_hba;
>  
> +void ufshcd_get_hba_list_lock(struct list_head **list);
> +void ufshcd_put_hba_list_unlock(void);
> +
>  enum dev_cmd_type {
>  	DEV_CMD_TYPE_NOP		= 0x0,
>  	DEV_CMD_TYPE_QUERY		= 0x1,
> @@ -473,6 +476,7 @@ struct ufs_stats {
>  
>  /**
>   * struct ufs_hba - per adapter private structure
> + * @list: Anchored at ufs_hba_list
>   * @mmio_base: UFSHCI base register address
>   * @ucdl_base_addr: UFS Command Descriptor base address
>   * @utrdl_base_addr: UTP Transfer Request Descriptor base address
> @@ -527,6 +531,7 @@ struct ufs_stats {
>   * @scsi_block_reqs_cnt: reference counting for scsi block requests
>   */
>  struct ufs_hba {
> +	struct list_head list;
>  	void __iomem *mmio_base;
>  
>  	/* Virtual memory reference */
> @@ -734,7 +739,7 @@ struct ufs_hba {
>  	struct ufs_desc_size desc_size;
>  	atomic_t scsi_block_reqs_cnt;
>  
> -	struct device		bsg_dev;
> +	struct device		*bsg_dev;
>  	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 v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-12 12:53, Bjorn Andersson wrote:
> On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
> 
>> In order to improve the flexibility of ufs-bsg, modulizing it is a 
>> good
>> choice. This change introduces tristate to ufs-bsg to allow users 
>> compile
>> it as an external module.
> 
> Can you please elaborate on what this "flexibility" is and why it's a
> good thing?
> 

ufs-bsg is a helpful gadget for debug/test purpose. But neither
disabling it nor enabling it is the best way on a commercialized
device. Disabling it means we cannot use it, while enabling it
by default will expose all the DEVM/UIC/TM interfaces to user space,
which is not "safe" on a commercialized device to let users play with 
it.
Making it a module can resolve this, because only vendors can install it
as they have the root permissions.

>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/Kconfig   |  3 ++-
>>  drivers/scsi/ufs/Makefile  |  2 +-
>>  drivers/scsi/ufs/ufs_bsg.c | 49 
>> +++++++++++++++++++++++++++++++++++++++++++---
>>  drivers/scsi/ufs/ufs_bsg.h |  8 --------
>>  drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
>>  drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
>>  6 files changed, 87 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
>> index d14c224..72620ce 100644
>> --- a/drivers/scsi/ufs/Kconfig
>> +++ b/drivers/scsi/ufs/Kconfig
>> @@ -38,6 +38,7 @@ config SCSI_UFSHCD
>>  	select PM_DEVFREQ
>>  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>>  	select NLS
>> +	select BLK_DEV_BSGLIB
> 
> Why is this needed?
> 

Because ufshcd.c needs to call some funcs defined in bsg lib.

>>  	---help---
>>  	This selects the support for UFS devices in Linux, say Y and make
>>  	  sure that you know the name of your UFS host adapter (the card
>> @@ -143,7 +144,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..302222f 100644
>> --- a/drivers/scsi/ufs/ufs_bsg.c
>> +++ b/drivers/scsi/ufs/ufs_bsg.c
>> @@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
>>   */
>>  void ufs_bsg_remove(struct ufs_hba *hba)
>>  {
>> -	struct device *bsg_dev = &hba->bsg_dev;
>> +	struct device *bsg_dev = hba->bsg_dev;
>> 
>>  	if (!hba->bsg_queue)
>>  		return;
>> 
>>  	bsg_remove_queue(hba->bsg_queue);
>> 
>> +	hba->bsg_dev = NULL;
>> +	hba->bsg_queue = NULL;
>>  	device_del(bsg_dev);
>>  	put_device(bsg_dev);
>>  }
>> @@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
>>  static inline void ufs_bsg_node_release(struct device *dev)
>>  {
>>  	put_device(dev->parent);
>> +	kfree(dev);
>>  }
>> 
>>  /**
>> @@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct 
>> device *dev)
>>   *
>>   * Called during initial loading of the driver, and before 
>> scsi_scan_host.
>>   */
>> -int ufs_bsg_probe(struct ufs_hba *hba)
>> +static int ufs_bsg_probe(struct ufs_hba *hba)
>>  {
>> -	struct device *bsg_dev = &hba->bsg_dev;
>> +	struct device *bsg_dev;
>>  	struct Scsi_Host *shost = hba->host;
>>  	struct device *parent = &shost->shost_gendev;
>>  	struct request_queue *q;
>>  	int ret;
>> 
>> +	bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
>> +	if (!bsg_dev)
>> +		return -ENOMEM;
>> +
>> +	hba->bsg_dev = bsg_dev;
>>  	device_initialize(bsg_dev);
>> 
>>  	bsg_dev->parent = get_device(parent);
>> @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
>> 
>>  out:
>>  	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n", 
>> shost->host_no);
>> +	hba->bsg_dev = NULL;
>>  	put_device(bsg_dev);
>>  	return ret;
>>  }
>> +
>> +static int __init ufs_bsg_init(void)
>> +{
>> +	struct list_head *hba_list = NULL;
>> +	struct ufs_hba *hba;
>> +	int ret = 0;
>> +
>> +	ufshcd_get_hba_list_lock(&hba_list);
>> +	list_for_each_entry(hba, hba_list, list) {
>> +		ret = ufs_bsg_probe(hba);
>> +		if (ret)
>> +			break;
>> +	}
> 
> So what happens if I go CONFIG_SCSI_UFS_BSG=y and
> CONFIG_SCSI_UFS_QCOM=y?
> 
> Wouldn't that mean that ufs_bsg_init() is called before ufshcd_init()
> has added the controller to the list? And even in the even that they 
> are
> both =m, what happens if they are invoked in the "wrong" order?
> 

In the case that CONFIG_SCSI_UFS_BSG=y and CONFIG_SCSI_UFS_QCOM=y,
I give late_initcall_sync(ufs_bsg_init) to make sure ufs_bsg_init
is invoked only after platform driver is probed. I tested this 
combination.

In the case that both of them are "m", installing ufs-bsg before 
ufs-qcom
is installed would have no effect as ufs_hba_list is empty, which is 
expected.
And in real cases, as the UFS is the boot device, UFS driver will always
be probed during bootup.

>> +	ufshcd_put_hba_list_unlock();
>> +
>> +	return ret;
>> +}
>> +
>> +static void __exit ufs_bsg_exit(void)
>> +{
>> +	struct list_head *hba_list = NULL;
>> +	struct ufs_hba *hba;
>> +
>> +	ufshcd_get_hba_list_lock(&hba_list);
>> +	list_for_each_entry(hba, hba_list, list)
>> +		ufs_bsg_remove(hba);
>> +	ufshcd_put_hba_list_unlock();
>> +}
>> +
>> +late_initcall_sync(ufs_bsg_init);
>> +module_exit(ufs_bsg_exit);
>> +
>> +MODULE_ALIAS("ufs-bsg");
> 
> The purpose of MODULE_ALIAS() is to facilitate module autoloading, but
> as you probe the bsg device from the initcall of the bsg driver itself 
> I
> don't see how that would happen, and as such I don't think this alias
> has a purpose.
> 

Good point, will remove it.

>> +MODULE_LICENSE("GPL v2");
>> 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.c b/drivers/scsi/ufs/ufshcd.c
>> index a86b0fd..7a83a8f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -108,6 +108,22 @@
>>  		       16, 4, buf, __len, false);                        \
>>  } while (0)
>> 
>> +static LIST_HEAD(ufs_hba_list);
>> +static DEFINE_MUTEX(ufs_hba_list_lock);
>> +
>> +void ufshcd_get_hba_list_lock(struct list_head **list)
>> +{
>> +	mutex_lock(&ufs_hba_list_lock);
>> +	*list = &ufs_hba_list;
>> +}
>> +EXPORT_SYMBOL_GPL(ufshcd_get_hba_list_lock);
>> +
>> +void ufshcd_put_hba_list_unlock(void)
>> +{
>> +	mutex_unlock(&ufs_hba_list_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(ufshcd_put_hba_list_unlock);
>> +
>>  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>>  		     const char *prefix)
>>  {
>> @@ -2093,6 +2109,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 +6041,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 +7061,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 +8263,16 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>>   */
>>  void ufshcd_remove(struct ufs_hba *hba)
>>  {
>> -	ufs_bsg_remove(hba);
>> +	struct device *bsg_dev = hba->bsg_dev;
>> +
>> +	mutex_lock(&ufs_hba_list_lock);
>> +	list_del(&hba->list);
>> +	if (hba->bsg_queue) {
>> +		bsg_remove_queue(hba->bsg_queue);
>> +		device_del(bsg_dev);
> 
> Am I reading this correct in that you probe the bsg_dev form initcall
> and you delete it as the ufshcd instance is removed? That's not okay.
> 
> Regards,
> Bjorn
> 

If ufshcd is removed, its ufs-bsg, if exists, should also be removed.
Could you please enlighten me a better way to do this? Thanks.

Regards,
Can Guo.

>> +		put_device(bsg_dev);
>> +	}
>> +	mutex_unlock(&ufs_hba_list_lock);
>>  	ufs_sysfs_remove_nodes(hba->dev);
>>  	scsi_remove_host(hba->host);
>>  	scsi_host_put(hba->host);
>> @@ -8494,6 +8518,10 @@ int ufshcd_init(struct ufs_hba *hba, void 
>> __iomem *mmio_base, unsigned int irq)
>>  	async_schedule(ufshcd_async_scan, hba);
>>  	ufs_sysfs_add_nodes(hba->dev);
>> 
>> +	mutex_lock(&ufs_hba_list_lock);
>> +	list_add_tail(&hba->list, &ufs_hba_list);
>> +	mutex_unlock(&ufs_hba_list_lock);
>> +
>>  	return 0;
>> 
>>  out_remove_scsi_host:
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 2740f69..893debc 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -74,6 +74,9 @@
>> 
>>  struct ufs_hba;
>> 
>> +void ufshcd_get_hba_list_lock(struct list_head **list);
>> +void ufshcd_put_hba_list_unlock(void);
>> +
>>  enum dev_cmd_type {
>>  	DEV_CMD_TYPE_NOP		= 0x0,
>>  	DEV_CMD_TYPE_QUERY		= 0x1,
>> @@ -473,6 +476,7 @@ struct ufs_stats {
>> 
>>  /**
>>   * struct ufs_hba - per adapter private structure
>> + * @list: Anchored at ufs_hba_list
>>   * @mmio_base: UFSHCI base register address
>>   * @ucdl_base_addr: UFS Command Descriptor base address
>>   * @utrdl_base_addr: UTP Transfer Request Descriptor base address
>> @@ -527,6 +531,7 @@ struct ufs_stats {
>>   * @scsi_block_reqs_cnt: reference counting for scsi block requests
>>   */
>>  struct ufs_hba {
>> +	struct list_head list;
>>  	void __iomem *mmio_base;
>> 
>>  	/* Virtual memory reference */
>> @@ -734,7 +739,7 @@ struct ufs_hba {
>>  	struct ufs_desc_size desc_size;
>>  	atomic_t scsi_block_reqs_cnt;
>> 
>> -	struct device		bsg_dev;
>> +	struct device		*bsg_dev;
>>  	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 v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bjorn Andersson]

On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:

> On 2019-12-12 12:53, Bjorn Andersson wrote:
> > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
> > 
> > > In order to improve the flexibility of ufs-bsg, modulizing it is a
> > > good
> > > choice. This change introduces tristate to ufs-bsg to allow users
> > > compile
> > > it as an external module.
> > 
> > Can you please elaborate on what this "flexibility" is and why it's a
> > good thing?
> > 
> 
> ufs-bsg is a helpful gadget for debug/test purpose. But neither
> disabling it nor enabling it is the best way on a commercialized
> device. Disabling it means we cannot use it, while enabling it
> by default will expose all the DEVM/UIC/TM interfaces to user space,
> which is not "safe" on a commercialized device to let users play with it.
> Making it a module can resolve this, because only vendors can install it
> as they have the root permissions.
> 
> > > 
> > > Signed-off-by: Can Guo <cang@codeaurora.org>
> > > ---
> > >  drivers/scsi/ufs/Kconfig   |  3 ++-
> > >  drivers/scsi/ufs/Makefile  |  2 +-
> > >  drivers/scsi/ufs/ufs_bsg.c | 49
> > > +++++++++++++++++++++++++++++++++++++++++++---
> > >  drivers/scsi/ufs/ufs_bsg.h |  8 --------
> > >  drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
> > >  drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
> > >  6 files changed, 87 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> > > index d14c224..72620ce 100644
> > > --- a/drivers/scsi/ufs/Kconfig
> > > +++ b/drivers/scsi/ufs/Kconfig
> > > @@ -38,6 +38,7 @@ config SCSI_UFSHCD
> > >  	select PM_DEVFREQ
> > >  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
> > >  	select NLS
> > > +	select BLK_DEV_BSGLIB
> > 
> > Why is this needed?
> > 
> 
> Because ufshcd.c needs to call some funcs defined in bsg lib.
> 
> > >  	---help---
> > >  	This selects the support for UFS devices in Linux, say Y and make
> > >  	  sure that you know the name of your UFS host adapter (the card
> > > @@ -143,7 +144,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..302222f 100644
> > > --- a/drivers/scsi/ufs/ufs_bsg.c
> > > +++ b/drivers/scsi/ufs/ufs_bsg.c
> > > @@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
> > >   */
> > >  void ufs_bsg_remove(struct ufs_hba *hba)
> > >  {
> > > -	struct device *bsg_dev = &hba->bsg_dev;
> > > +	struct device *bsg_dev = hba->bsg_dev;
> > > 
> > >  	if (!hba->bsg_queue)
> > >  		return;
> > > 
> > >  	bsg_remove_queue(hba->bsg_queue);
> > > 
> > > +	hba->bsg_dev = NULL;
> > > +	hba->bsg_queue = NULL;
> > >  	device_del(bsg_dev);
> > >  	put_device(bsg_dev);
> > >  }
> > > @@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
> > >  static inline void ufs_bsg_node_release(struct device *dev)
> > >  {
> > >  	put_device(dev->parent);
> > > +	kfree(dev);
> > >  }
> > > 
> > >  /**
> > > @@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct
> > > device *dev)
> > >   *
> > >   * Called during initial loading of the driver, and before
> > > scsi_scan_host.
> > >   */
> > > -int ufs_bsg_probe(struct ufs_hba *hba)
> > > +static int ufs_bsg_probe(struct ufs_hba *hba)
> > >  {
> > > -	struct device *bsg_dev = &hba->bsg_dev;
> > > +	struct device *bsg_dev;
> > >  	struct Scsi_Host *shost = hba->host;
> > >  	struct device *parent = &shost->shost_gendev;
> > >  	struct request_queue *q;
> > >  	int ret;
> > > 
> > > +	bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
> > > +	if (!bsg_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	hba->bsg_dev = bsg_dev;
> > >  	device_initialize(bsg_dev);
> > > 
> > >  	bsg_dev->parent = get_device(parent);
> > > @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
> > > 
> > >  out:
> > >  	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n",
> > > shost->host_no);
> > > +	hba->bsg_dev = NULL;
> > >  	put_device(bsg_dev);
> > >  	return ret;
> > >  }
> > > +
> > > +static int __init ufs_bsg_init(void)
> > > +{
> > > +	struct list_head *hba_list = NULL;
> > > +	struct ufs_hba *hba;
> > > +	int ret = 0;
> > > +
> > > +	ufshcd_get_hba_list_lock(&hba_list);
> > > +	list_for_each_entry(hba, hba_list, list) {
> > > +		ret = ufs_bsg_probe(hba);
> > > +		if (ret)
> > > +			break;
> > > +	}
> > 
> > So what happens if I go CONFIG_SCSI_UFS_BSG=y and
> > CONFIG_SCSI_UFS_QCOM=y?
> > 
> > Wouldn't that mean that ufs_bsg_init() is called before ufshcd_init()
> > has added the controller to the list? And even in the even that they are
> > both =m, what happens if they are invoked in the "wrong" order?
> > 
> 
> In the case that CONFIG_SCSI_UFS_BSG=y and CONFIG_SCSI_UFS_QCOM=y,
> I give late_initcall_sync(ufs_bsg_init) to make sure ufs_bsg_init
> is invoked only after platform driver is probed. I tested this combination.
> 
> In the case that both of them are "m", installing ufs-bsg before ufs-qcom
> is installed would have no effect as ufs_hba_list is empty, which is
> expected.

Why is it the expected behavior that bsg may or may not probe depending
on the driver load order and potentially timing of the initialization.

> And in real cases, as the UFS is the boot device, UFS driver will always
> be probed during bootup.
> 

The UFS driver will load and probe because it's mentioned in the
devicetree, but if either the ufs drivers or any of its dependencies
(phy, resets, clocks, etc) are built as modules it might very well
finish probing after lateinitcall.

So in the even that the bsg is =y and any of these drivers are =m, or if
you're having bad luck with your timing, the list will be empty.

As described below, if bsg=m, then there's nothing that will load the
module and the bsg will not probe...

[..]
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
[..]
> > >  void ufshcd_remove(struct ufs_hba *hba)
> > >  {
> > > -	ufs_bsg_remove(hba);
> > > +	struct device *bsg_dev = hba->bsg_dev;
> > > +
> > > +	mutex_lock(&ufs_hba_list_lock);
> > > +	list_del(&hba->list);
> > > +	if (hba->bsg_queue) {
> > > +		bsg_remove_queue(hba->bsg_queue);
> > > +		device_del(bsg_dev);
> > 
> > Am I reading this correct in that you probe the bsg_dev form initcall
> > and you delete it as the ufshcd instance is removed? That's not okay.
> > 
> > Regards,
> > Bjorn
> > 
> 
> If ufshcd is removed, its ufs-bsg, if exists, should also be removed.
> Could you please enlighten me a better way to do this? Thanks.
> 

It's the asymmetry that I don't like.

Perhaps if you instead make ufshcd platform_device_register_data() the
bsg device you would solve the probe ordering, the remove will be
symmetric and module autoloading will work as well (although then you
need a MODULE_ALIAS of platform:device-name).

Regards,
Bjorn

RE: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Avri Altman]

> 
> 
> On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
> 
> > On 2019-12-12 12:53, Bjorn Andersson wrote:
> > > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
> > >
> > > > In order to improve the flexibility of ufs-bsg, modulizing it is a
> > > > good choice. This change introduces tristate to ufs-bsg to allow
> > > > users compile it as an external module.
> > >
> > > Can you please elaborate on what this "flexibility" is and why it's
> > > a good thing?
> > >
> >
> > ufs-bsg is a helpful gadget for debug/test purpose. But neither
> > disabling it nor enabling it is the best way on a commercialized
> > device. Disabling it means we cannot use it, while enabling it by
> > default will expose all the DEVM/UIC/TM interfaces to user space,
> > which is not "safe" on a commercialized device to let users play with it.
> > Making it a module can resolve this, because only vendors can install
> > it as they have the root permissions.
Agree.
We see that the public ufs-utils (https://github.com/westerndigitalcorporation/ufs-utils) that uses this infrastructure,
is gaining momentum, and currently being used not only by chipset and flash vendors,
but by end customers as well.
This change will e.g. enable, field application engineers to debug issues in a safer mode.

> >
> > > >
> > > > Signed-off-by: Can Guo <cang@codeaurora.org>
> > > > ---
> > > >  drivers/scsi/ufs/Kconfig   |  3 ++-
> > > >  drivers/scsi/ufs/Makefile  |  2 +-  drivers/scsi/ufs/ufs_bsg.c |
> > > > 49
> > > > +++++++++++++++++++++++++++++++++++++++++++---
> > > >  drivers/scsi/ufs/ufs_bsg.h |  8 --------
> > > > drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
> > > >  drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
> > > >  6 files changed, 87 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> > > > index d14c224..72620ce 100644
> > > > --- a/drivers/scsi/ufs/Kconfig
> > > > +++ b/drivers/scsi/ufs/Kconfig
> > > > @@ -38,6 +38,7 @@ config SCSI_UFSHCD
> > > >   select PM_DEVFREQ
> > > >   select DEVFREQ_GOV_SIMPLE_ONDEMAND
> > > >   select NLS
> > > > + select BLK_DEV_BSGLIB
> > >
> > > Why is this needed?
> > >
> >
> > Because ufshcd.c needs to call some funcs defined in bsg lib.
> >
> > > >   ---help---
> > > >   This selects the support for UFS devices in Linux, say Y and make
> > > >     sure that you know the name of your UFS host adapter (the card
> > > > @@ -143,7 +144,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..302222f 100644
> > > > --- a/drivers/scsi/ufs/ufs_bsg.c
> > > > +++ b/drivers/scsi/ufs/ufs_bsg.c
> > > > @@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
> > > >   */
> > > >  void ufs_bsg_remove(struct ufs_hba *hba)  {
> > > > - struct device *bsg_dev = &hba->bsg_dev;
> > > > + struct device *bsg_dev = hba->bsg_dev;
> > > >
> > > >   if (!hba->bsg_queue)
> > > >           return;
> > > >
> > > >   bsg_remove_queue(hba->bsg_queue);
> > > >
> > > > + hba->bsg_dev = NULL;
> > > > + hba->bsg_queue = NULL;
> > > >   device_del(bsg_dev);
> > > >   put_device(bsg_dev);
> > > >  }
> > > > @@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
> > > >  static inline void ufs_bsg_node_release(struct device *dev)
> > > >  {
> > > >   put_device(dev->parent);
> > > > + kfree(dev);
> > > >  }
> > > >
> > > >  /**
> > > > @@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct
> > > > device *dev)
> > > >   *
> > > >   * Called during initial loading of the driver, and before
> > > > scsi_scan_host.
> > > >   */
> > > > -int ufs_bsg_probe(struct ufs_hba *hba)
> > > > +static int ufs_bsg_probe(struct ufs_hba *hba)
> > > >  {
> > > > - struct device *bsg_dev = &hba->bsg_dev;
> > > > + struct device *bsg_dev;
> > > >   struct Scsi_Host *shost = hba->host;
> > > >   struct device *parent = &shost->shost_gendev;
> > > >   struct request_queue *q;
> > > >   int ret;
> > > >
> > > > + bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
> > > > + if (!bsg_dev)
> > > > +         return -ENOMEM;
> > > > +
> > > > + hba->bsg_dev = bsg_dev;
> > > >   device_initialize(bsg_dev);
> > > >
> > > >   bsg_dev->parent = get_device(parent);
> > > > @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
> > > >
> > > >  out:
> > > >   dev_err(bsg_dev, "fail to initialize a bsg dev %d\n",
> > > > shost->host_no);
> > > > + hba->bsg_dev = NULL;
> > > >   put_device(bsg_dev);
> > > >   return ret;
> > > >  }
> > > > +
> > > > +static int __init ufs_bsg_init(void)
> > > > +{
> > > > + struct list_head *hba_list = NULL;
> > > > + struct ufs_hba *hba;
> > > > + int ret = 0;
> > > > +
> > > > + ufshcd_get_hba_list_lock(&hba_list);
> > > > + list_for_each_entry(hba, hba_list, list) {
> > > > +         ret = ufs_bsg_probe(hba);
> > > > +         if (ret)
> > > > +                 break;
> > > > + }
> > >
> > > So what happens if I go CONFIG_SCSI_UFS_BSG=y and
> > > CONFIG_SCSI_UFS_QCOM=y?
> > >
> > > Wouldn't that mean that ufs_bsg_init() is called before ufshcd_init()
> > > has added the controller to the list? And even in the even that they are
> > > both =m, what happens if they are invoked in the "wrong" order?
> > >
> >
> > In the case that CONFIG_SCSI_UFS_BSG=y and CONFIG_SCSI_UFS_QCOM=y,
> > I give late_initcall_sync(ufs_bsg_init) to make sure ufs_bsg_init
> > is invoked only after platform driver is probed. I tested this combination.
> >
> > In the case that both of them are "m", installing ufs-bsg before ufs-qcom
> > is installed would have no effect as ufs_hba_list is empty, which is
> > expected.
> 
> Why is it the expected behavior that bsg may or may not probe depending
> on the driver load order and potentially timing of the initialization.
> 
> > And in real cases, as the UFS is the boot device, UFS driver will always
> > be probed during bootup.
> >
> 
> The UFS driver will load and probe because it's mentioned in the
> devicetree, but if either the ufs drivers or any of its dependencies
> (phy, resets, clocks, etc) are built as modules it might very well
> finish probing after lateinitcall.
> 
> So in the even that the bsg is =y and any of these drivers are =m, or if
> you're having bad luck with your timing, the list will be empty.
> 
> As described below, if bsg=m, then there's nothing that will load the
> module and the bsg will not probe...
Right.
bsg=y and ufshcd=m is a bad idea, and should be avoided.

> 
> [..]
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> [..]
> > > >  void ufshcd_remove(struct ufs_hba *hba)
> > > >  {
> > > > - ufs_bsg_remove(hba);
> > > > + struct device *bsg_dev = hba->bsg_dev;
> > > > +
> > > > + mutex_lock(&ufs_hba_list_lock);
> > > > + list_del(&hba->list);
> > > > + if (hba->bsg_queue) {
> > > > +         bsg_remove_queue(hba->bsg_queue);
> > > > +         device_del(bsg_dev);
> > >
> > > Am I reading this correct in that you probe the bsg_dev form initcall
> > > and you delete it as the ufshcd instance is removed? That's not okay.
> > >
> > > Regards,
> > > Bjorn
> > >
> >
> > If ufshcd is removed, its ufs-bsg, if exists, should also be removed.
> > Could you please enlighten me a better way to do this? Thanks.
> >
> 
> It's the asymmetry that I don't like.
> 
> Perhaps if you instead make ufshcd platform_device_register_data() the
> bsg device you would solve the probe ordering, the remove will be
> symmetric and module autoloading will work as well (although then you
> need a MODULE_ALIAS of platform:device-name).
> 
> Regards,
> Bjorn

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-12 15:00, Avri Altman wrote:
>> 
>> 
>> On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
>> 
>> > On 2019-12-12 12:53, Bjorn Andersson wrote:
>> > > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
>> > >
>> > > > In order to improve the flexibility of ufs-bsg, modulizing it is a
>> > > > good choice. This change introduces tristate to ufs-bsg to allow
>> > > > users compile it as an external module.
>> > >
>> > > Can you please elaborate on what this "flexibility" is and why it's
>> > > a good thing?
>> > >
>> >
>> > ufs-bsg is a helpful gadget for debug/test purpose. But neither
>> > disabling it nor enabling it is the best way on a commercialized
>> > device. Disabling it means we cannot use it, while enabling it by
>> > default will expose all the DEVM/UIC/TM interfaces to user space,
>> > which is not "safe" on a commercialized device to let users play with it.
>> > Making it a module can resolve this, because only vendors can install
>> > it as they have the root permissions.
> Agree.
> We see that the public ufs-utils
> (https://github.com/westerndigitalcorporation/ufs-utils) that uses
> this infrastructure,
> is gaining momentum, and currently being used not only by chipset and
> flash vendors,
> but by end customers as well.
> This change will e.g. enable, field application engineers to debug
> issues in a safer mode.
> 

True, thank you for the comments.

>> >
>> > > >
>> > > > Signed-off-by: Can Guo <cang@codeaurora.org>
>> > > > ---
>> > > >  drivers/scsi/ufs/Kconfig   |  3 ++-
>> > > >  drivers/scsi/ufs/Makefile  |  2 +-  drivers/scsi/ufs/ufs_bsg.c |
>> > > > 49
>> > > > +++++++++++++++++++++++++++++++++++++++++++---
>> > > >  drivers/scsi/ufs/ufs_bsg.h |  8 --------
>> > > > drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
>> > > >  drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
>> > > >  6 files changed, 87 insertions(+), 18 deletions(-)
>> > > >
>> > > > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
>> > > > index d14c224..72620ce 100644
>> > > > --- a/drivers/scsi/ufs/Kconfig
>> > > > +++ b/drivers/scsi/ufs/Kconfig
>> > > > @@ -38,6 +38,7 @@ config SCSI_UFSHCD
>> > > >   select PM_DEVFREQ
>> > > >   select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> > > >   select NLS
>> > > > + select BLK_DEV_BSGLIB
>> > >
>> > > Why is this needed?
>> > >
>> >
>> > Because ufshcd.c needs to call some funcs defined in bsg lib.
>> >
>> > > >   ---help---
>> > > >   This selects the support for UFS devices in Linux, say Y and make
>> > > >     sure that you know the name of your UFS host adapter (the card
>> > > > @@ -143,7 +144,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..302222f 100644
>> > > > --- a/drivers/scsi/ufs/ufs_bsg.c
>> > > > +++ b/drivers/scsi/ufs/ufs_bsg.c
>> > > > @@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
>> > > >   */
>> > > >  void ufs_bsg_remove(struct ufs_hba *hba)  {
>> > > > - struct device *bsg_dev = &hba->bsg_dev;
>> > > > + struct device *bsg_dev = hba->bsg_dev;
>> > > >
>> > > >   if (!hba->bsg_queue)
>> > > >           return;
>> > > >
>> > > >   bsg_remove_queue(hba->bsg_queue);
>> > > >
>> > > > + hba->bsg_dev = NULL;
>> > > > + hba->bsg_queue = NULL;
>> > > >   device_del(bsg_dev);
>> > > >   put_device(bsg_dev);
>> > > >  }
>> > > > @@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
>> > > >  static inline void ufs_bsg_node_release(struct device *dev)
>> > > >  {
>> > > >   put_device(dev->parent);
>> > > > + kfree(dev);
>> > > >  }
>> > > >
>> > > >  /**
>> > > > @@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct
>> > > > device *dev)
>> > > >   *
>> > > >   * Called during initial loading of the driver, and before
>> > > > scsi_scan_host.
>> > > >   */
>> > > > -int ufs_bsg_probe(struct ufs_hba *hba)
>> > > > +static int ufs_bsg_probe(struct ufs_hba *hba)
>> > > >  {
>> > > > - struct device *bsg_dev = &hba->bsg_dev;
>> > > > + struct device *bsg_dev;
>> > > >   struct Scsi_Host *shost = hba->host;
>> > > >   struct device *parent = &shost->shost_gendev;
>> > > >   struct request_queue *q;
>> > > >   int ret;
>> > > >
>> > > > + bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
>> > > > + if (!bsg_dev)
>> > > > +         return -ENOMEM;
>> > > > +
>> > > > + hba->bsg_dev = bsg_dev;
>> > > >   device_initialize(bsg_dev);
>> > > >
>> > > >   bsg_dev->parent = get_device(parent);
>> > > > @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
>> > > >
>> > > >  out:
>> > > >   dev_err(bsg_dev, "fail to initialize a bsg dev %d\n",
>> > > > shost->host_no);
>> > > > + hba->bsg_dev = NULL;
>> > > >   put_device(bsg_dev);
>> > > >   return ret;
>> > > >  }
>> > > > +
>> > > > +static int __init ufs_bsg_init(void)
>> > > > +{
>> > > > + struct list_head *hba_list = NULL;
>> > > > + struct ufs_hba *hba;
>> > > > + int ret = 0;
>> > > > +
>> > > > + ufshcd_get_hba_list_lock(&hba_list);
>> > > > + list_for_each_entry(hba, hba_list, list) {
>> > > > +         ret = ufs_bsg_probe(hba);
>> > > > +         if (ret)
>> > > > +                 break;
>> > > > + }
>> > >
>> > > So what happens if I go CONFIG_SCSI_UFS_BSG=y and
>> > > CONFIG_SCSI_UFS_QCOM=y?
>> > >
>> > > Wouldn't that mean that ufs_bsg_init() is called before ufshcd_init()
>> > > has added the controller to the list? And even in the even that they are
>> > > both =m, what happens if they are invoked in the "wrong" order?
>> > >
>> >
>> > In the case that CONFIG_SCSI_UFS_BSG=y and CONFIG_SCSI_UFS_QCOM=y,
>> > I give late_initcall_sync(ufs_bsg_init) to make sure ufs_bsg_init
>> > is invoked only after platform driver is probed. I tested this combination.
>> >
>> > In the case that both of them are "m", installing ufs-bsg before ufs-qcom
>> > is installed would have no effect as ufs_hba_list is empty, which is
>> > expected.
>> 
>> Why is it the expected behavior that bsg may or may not probe 
>> depending
>> on the driver load order and potentially timing of the initialization.
>> 
>> > And in real cases, as the UFS is the boot device, UFS driver will always
>> > be probed during bootup.
>> >
>> 
>> The UFS driver will load and probe because it's mentioned in the
>> devicetree, but if either the ufs drivers or any of its dependencies
>> (phy, resets, clocks, etc) are built as modules it might very well
>> finish probing after lateinitcall.
>> 
>> So in the even that the bsg is =y and any of these drivers are =m, or 
>> if
>> you're having bad luck with your timing, the list will be empty.
>> 
>> As described below, if bsg=m, then there's nothing that will load the
>> module and the bsg will not probe...
> Right.
> bsg=y and ufshcd=m is a bad idea, and should be avoided.
> 

Yeah, I will get it addressed in the next patchset.

Thanks,
Can Guo.

>> 
>> [..]
>> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> [..]
>> > > >  void ufshcd_remove(struct ufs_hba *hba)
>> > > >  {
>> > > > - ufs_bsg_remove(hba);
>> > > > + struct device *bsg_dev = hba->bsg_dev;
>> > > > +
>> > > > + mutex_lock(&ufs_hba_list_lock);
>> > > > + list_del(&hba->list);
>> > > > + if (hba->bsg_queue) {
>> > > > +         bsg_remove_queue(hba->bsg_queue);
>> > > > +         device_del(bsg_dev);
>> > >
>> > > Am I reading this correct in that you probe the bsg_dev form initcall
>> > > and you delete it as the ufshcd instance is removed? That's not okay.
>> > >
>> > > Regards,
>> > > Bjorn
>> > >
>> >
>> > If ufshcd is removed, its ufs-bsg, if exists, should also be removed.
>> > Could you please enlighten me a better way to do this? Thanks.
>> >
>> 
>> It's the asymmetry that I don't like.
>> 
>> Perhaps if you instead make ufshcd platform_device_register_data() the
>> bsg device you would solve the probe ordering, the remove will be
>> symmetric and module autoloading will work as well (although then you
>> need a MODULE_ALIAS of platform:device-name).
>> 
>> Regards,
>> Bjorn

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bjorn Andersson]

On Thu 12 Dec 08:53 PST 2019, cang@codeaurora.org wrote:

> On 2019-12-12 15:00, Avri Altman wrote:
> > > On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
> > > > On 2019-12-12 12:53, Bjorn Andersson wrote:
> > > > > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
[..]
> > > > And in real cases, as the UFS is the boot device, UFS driver will always
> > > > be probed during bootup.
> > > >
> > > 
> > > The UFS driver will load and probe because it's mentioned in the
> > > devicetree, but if either the ufs drivers or any of its dependencies
> > > (phy, resets, clocks, etc) are built as modules it might very well
> > > finish probing after lateinitcall.
> > > 
> > > So in the even that the bsg is =y and any of these drivers are =m,
> > > or if
> > > you're having bad luck with your timing, the list will be empty.
> > > 
> > > As described below, if bsg=m, then there's nothing that will load the
> > > module and the bsg will not probe...
> > Right.
> > bsg=y and ufshcd=m is a bad idea, and should be avoided.
> > 
> 
> Yeah, I will get it addressed in the next patchset.
> 

If you build this around platform_device_register_data() from ufshcd I
don't see a reason to add additional restrictions on this combination
(even though it might not make much sense for people to use this
combination).

Regards,
Bjorn

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-13 02:24, Bjorn Andersson wrote:
> On Thu 12 Dec 08:53 PST 2019, cang@codeaurora.org wrote:
> 
>> On 2019-12-12 15:00, Avri Altman wrote:
>> > > On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
>> > > > On 2019-12-12 12:53, Bjorn Andersson wrote:
>> > > > > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
> [..]
>> > > > And in real cases, as the UFS is the boot device, UFS driver will always
>> > > > be probed during bootup.
>> > > >
>> > >
>> > > The UFS driver will load and probe because it's mentioned in the
>> > > devicetree, but if either the ufs drivers or any of its dependencies
>> > > (phy, resets, clocks, etc) are built as modules it might very well
>> > > finish probing after lateinitcall.
>> > >
>> > > So in the even that the bsg is =y and any of these drivers are =m,
>> > > or if
>> > > you're having bad luck with your timing, the list will be empty.
>> > >
>> > > As described below, if bsg=m, then there's nothing that will load the
>> > > module and the bsg will not probe...
>> > Right.
>> > bsg=y and ufshcd=m is a bad idea, and should be avoided.
>> >
>> 
>> Yeah, I will get it addressed in the next patchset.
>> 
> 
> If you build this around platform_device_register_data() from ufshcd I
> don't see a reason to add additional restrictions on this combination
> (even though it might not make much sense for people to use this
> combination).
> 
> Regards,
> Bjorn

Agree, thanks.

Regards,
Can Guo.

RE: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Avri Altman]

 
> 
> On 2019-12-13 02:24, Bjorn Andersson wrote:
> > On Thu 12 Dec 08:53 PST 2019, cang@codeaurora.org wrote:
> >
> >> On 2019-12-12 15:00, Avri Altman wrote:
> >> > > On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
> >> > > > On 2019-12-12 12:53, Bjorn Andersson wrote:
> >> > > > > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
> > [..]
> >> > > > And in real cases, as the UFS is the boot device, UFS driver
> >> > > > will always be probed during bootup.
> >> > > >
> >> > >
> >> > > The UFS driver will load and probe because it's mentioned in the
> >> > > devicetree, but if either the ufs drivers or any of its
> >> > > dependencies (phy, resets, clocks, etc) are built as modules it
> >> > > might very well finish probing after lateinitcall.
> >> > >
> >> > > So in the even that the bsg is =y and any of these drivers are
> >> > > =m, or if you're having bad luck with your timing, the list will
> >> > > be empty.
> >> > >
> >> > > As described below, if bsg=m, then there's nothing that will load
> >> > > the module and the bsg will not probe...
> >> > Right.
> >> > bsg=y and ufshcd=m is a bad idea, and should be avoided.
> >> >
> >>
> >> Yeah, I will get it addressed in the next patchset.
> >>
> >
> > If you build this around platform_device_register_data() from ufshcd I
> > don't see a reason to add additional restrictions on this combination
> > (even though it might not make much sense for people to use this
> > combination).
> >
> > Regards,
> > Bjorn
> 
> Agree, thanks.
While at it, maybe you can add few words in the "BSG Support" paragraph,
In Documentation/scsi/ufs.txt.

Thanks,
Avri

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-12 14:37, Bjorn Andersson wrote:
> On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
> 
>> On 2019-12-12 12:53, Bjorn Andersson wrote:
>> > On Wed 11 Dec 00:49 PST 2019, Can Guo wrote:
>> >
>> > > In order to improve the flexibility of ufs-bsg, modulizing it is a
>> > > good
>> > > choice. This change introduces tristate to ufs-bsg to allow users
>> > > compile
>> > > it as an external module.
>> >
>> > Can you please elaborate on what this "flexibility" is and why it's a
>> > good thing?
>> >
>> 
>> ufs-bsg is a helpful gadget for debug/test purpose. But neither
>> disabling it nor enabling it is the best way on a commercialized
>> device. Disabling it means we cannot use it, while enabling it
>> by default will expose all the DEVM/UIC/TM interfaces to user space,
>> which is not "safe" on a commercialized device to let users play with 
>> it.
>> Making it a module can resolve this, because only vendors can install 
>> it
>> as they have the root permissions.
>> 
>> > >
>> > > Signed-off-by: Can Guo <cang@codeaurora.org>
>> > > ---
>> > >  drivers/scsi/ufs/Kconfig   |  3 ++-
>> > >  drivers/scsi/ufs/Makefile  |  2 +-
>> > >  drivers/scsi/ufs/ufs_bsg.c | 49
>> > > +++++++++++++++++++++++++++++++++++++++++++---
>> > >  drivers/scsi/ufs/ufs_bsg.h |  8 --------
>> > >  drivers/scsi/ufs/ufshcd.c  | 36 ++++++++++++++++++++++++++++++----
>> > >  drivers/scsi/ufs/ufshcd.h  |  7 ++++++-
>> > >  6 files changed, 87 insertions(+), 18 deletions(-)
>> > >
>> > > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
>> > > index d14c224..72620ce 100644
>> > > --- a/drivers/scsi/ufs/Kconfig
>> > > +++ b/drivers/scsi/ufs/Kconfig
>> > > @@ -38,6 +38,7 @@ config SCSI_UFSHCD
>> > >  	select PM_DEVFREQ
>> > >  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> > >  	select NLS
>> > > +	select BLK_DEV_BSGLIB
>> >
>> > Why is this needed?
>> >
>> 
>> Because ufshcd.c needs to call some funcs defined in bsg lib.
>> 
>> > >  	---help---
>> > >  	This selects the support for UFS devices in Linux, say Y and make
>> > >  	  sure that you know the name of your UFS host adapter (the card
>> > > @@ -143,7 +144,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..302222f 100644
>> > > --- a/drivers/scsi/ufs/ufs_bsg.c
>> > > +++ b/drivers/scsi/ufs/ufs_bsg.c
>> > > @@ -164,13 +164,15 @@ static int ufs_bsg_request(struct bsg_job *job)
>> > >   */
>> > >  void ufs_bsg_remove(struct ufs_hba *hba)
>> > >  {
>> > > -	struct device *bsg_dev = &hba->bsg_dev;
>> > > +	struct device *bsg_dev = hba->bsg_dev;
>> > >
>> > >  	if (!hba->bsg_queue)
>> > >  		return;
>> > >
>> > >  	bsg_remove_queue(hba->bsg_queue);
>> > >
>> > > +	hba->bsg_dev = NULL;
>> > > +	hba->bsg_queue = NULL;
>> > >  	device_del(bsg_dev);
>> > >  	put_device(bsg_dev);
>> > >  }
>> > > @@ -178,6 +180,7 @@ void ufs_bsg_remove(struct ufs_hba *hba)
>> > >  static inline void ufs_bsg_node_release(struct device *dev)
>> > >  {
>> > >  	put_device(dev->parent);
>> > > +	kfree(dev);
>> > >  }
>> > >
>> > >  /**
>> > > @@ -186,14 +189,19 @@ static inline void ufs_bsg_node_release(struct
>> > > device *dev)
>> > >   *
>> > >   * Called during initial loading of the driver, and before
>> > > scsi_scan_host.
>> > >   */
>> > > -int ufs_bsg_probe(struct ufs_hba *hba)
>> > > +static int ufs_bsg_probe(struct ufs_hba *hba)
>> > >  {
>> > > -	struct device *bsg_dev = &hba->bsg_dev;
>> > > +	struct device *bsg_dev;
>> > >  	struct Scsi_Host *shost = hba->host;
>> > >  	struct device *parent = &shost->shost_gendev;
>> > >  	struct request_queue *q;
>> > >  	int ret;
>> > >
>> > > +	bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
>> > > +	if (!bsg_dev)
>> > > +		return -ENOMEM;
>> > > +
>> > > +	hba->bsg_dev = bsg_dev;
>> > >  	device_initialize(bsg_dev);
>> > >
>> > >  	bsg_dev->parent = get_device(parent);
>> > > @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
>> > >
>> > >  out:
>> > >  	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n",
>> > > shost->host_no);
>> > > +	hba->bsg_dev = NULL;
>> > >  	put_device(bsg_dev);
>> > >  	return ret;
>> > >  }
>> > > +
>> > > +static int __init ufs_bsg_init(void)
>> > > +{
>> > > +	struct list_head *hba_list = NULL;
>> > > +	struct ufs_hba *hba;
>> > > +	int ret = 0;
>> > > +
>> > > +	ufshcd_get_hba_list_lock(&hba_list);
>> > > +	list_for_each_entry(hba, hba_list, list) {
>> > > +		ret = ufs_bsg_probe(hba);
>> > > +		if (ret)
>> > > +			break;
>> > > +	}
>> >
>> > So what happens if I go CONFIG_SCSI_UFS_BSG=y and
>> > CONFIG_SCSI_UFS_QCOM=y?
>> >
>> > Wouldn't that mean that ufs_bsg_init() is called before ufshcd_init()
>> > has added the controller to the list? And even in the even that they are
>> > both =m, what happens if they are invoked in the "wrong" order?
>> >
>> 
>> In the case that CONFIG_SCSI_UFS_BSG=y and CONFIG_SCSI_UFS_QCOM=y,
>> I give late_initcall_sync(ufs_bsg_init) to make sure ufs_bsg_init
>> is invoked only after platform driver is probed. I tested this 
>> combination.
>> 
>> In the case that both of them are "m", installing ufs-bsg before 
>> ufs-qcom
>> is installed would have no effect as ufs_hba_list is empty, which is
>> expected.
> 
> Why is it the expected behavior that bsg may or may not probe depending
> on the driver load order and potentially timing of the initialization.
> 
>> And in real cases, as the UFS is the boot device, UFS driver will 
>> always
>> be probed during bootup.
>> 
> 
> The UFS driver will load and probe because it's mentioned in the
> devicetree, but if either the ufs drivers or any of its dependencies
> (phy, resets, clocks, etc) are built as modules it might very well
> finish probing after lateinitcall.
> 
> So in the even that the bsg is =y and any of these drivers are =m, or 
> if
> you're having bad luck with your timing, the list will be empty.
> 
> As described below, if bsg=m, then there's nothing that will load the
> module and the bsg will not probe...
> 
> [..]
>> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> [..]
>> > >  void ufshcd_remove(struct ufs_hba *hba)
>> > >  {
>> > > -	ufs_bsg_remove(hba);
>> > > +	struct device *bsg_dev = hba->bsg_dev;
>> > > +
>> > > +	mutex_lock(&ufs_hba_list_lock);
>> > > +	list_del(&hba->list);
>> > > +	if (hba->bsg_queue) {
>> > > +		bsg_remove_queue(hba->bsg_queue);
>> > > +		device_del(bsg_dev);
>> >
>> > Am I reading this correct in that you probe the bsg_dev form initcall
>> > and you delete it as the ufshcd instance is removed? That's not okay.
>> >
>> > Regards,
>> > Bjorn
>> >
>> 
>> If ufshcd is removed, its ufs-bsg, if exists, should also be removed.
>> Could you please enlighten me a better way to do this? Thanks.
>> 
> 
> It's the asymmetry that I don't like.
> 
> Perhaps if you instead make ufshcd platform_device_register_data() the
> bsg device you would solve the probe ordering, the remove will be
> symmetric and module autoloading will work as well (although then you
> need a MODULE_ALIAS of platform:device-name).
> 
> Regards,
> Bjorn

Thanks for the suggestion! I didn't even think about this before. I
will go with the platform_device_register_data() way, it will be much
easier. After I get my new patchset tested I will upload it for review.

Best Regards,
Can Guo.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bart Van Assche]

On 2019-12-11 22:37, Bjorn Andersson wrote:
> It's the asymmetry that I don't like.
> 
> Perhaps if you instead make ufshcd platform_device_register_data() the
> bsg device you would solve the probe ordering, the remove will be
> symmetric and module autoloading will work as well (although then you
> need a MODULE_ALIAS of platform:device-name).

Hi Bjorn,

From Documentation/driver-api/driver-model/platform.rst:
"Platform devices are devices that typically appear as autonomous
entities in the system. This includes legacy port-based devices and
host bridges to peripheral buses, and most controllers integrated
into system-on-chip platforms.  What they usually have in common
is direct addressing from a CPU bus.  Rarely, a platform_device will
be connected through a segment of some other kind of bus; but its
registers will still be directly addressable."

Do you agree that the above description is not a good match for the
ufs-bsg kernel module?

Thanks,

Bart.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-16 05:49, Bart Van Assche wrote:
> On 2019-12-11 22:37, Bjorn Andersson wrote:
>> It's the asymmetry that I don't like.
>> 
>> Perhaps if you instead make ufshcd platform_device_register_data() the
>> bsg device you would solve the probe ordering, the remove will be
>> symmetric and module autoloading will work as well (although then you
>> need a MODULE_ALIAS of platform:device-name).
> 
> Hi Bjorn,
> 
> From Documentation/driver-api/driver-model/platform.rst:
> "Platform devices are devices that typically appear as autonomous
> entities in the system. This includes legacy port-based devices and
> host bridges to peripheral buses, and most controllers integrated
> into system-on-chip platforms.  What they usually have in common
> is direct addressing from a CPU bus.  Rarely, a platform_device will
> be connected through a segment of some other kind of bus; but its
> registers will still be directly addressable."
> 
> Do you agree that the above description is not a good match for the
> ufs-bsg kernel module?
> 
> Thanks,
> 
> Bart.

Hi Bart,

I missed this one.
How about making it a plain device and add it from ufs driver?

Thanks,

Can Guo.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bart Van Assche]

On 12/15/19 8:36 PM, cang@codeaurora.org wrote:
> On 2019-12-16 05:49, Bart Van Assche wrote:
>> On 2019-12-11 22:37, Bjorn Andersson wrote:
>>> It's the asymmetry that I don't like.
>>>
>>> Perhaps if you instead make ufshcd platform_device_register_data() the
>>> bsg device you would solve the probe ordering, the remove will be
>>> symmetric and module autoloading will work as well (although then you
>>> need a MODULE_ALIAS of platform:device-name).
>>
>> Hi Bjorn,
>>
>> From Documentation/driver-api/driver-model/platform.rst:
>> "Platform devices are devices that typically appear as autonomous
>> entities in the system. This includes legacy port-based devices and
>> host bridges to peripheral buses, and most controllers integrated
>> into system-on-chip platforms.  What they usually have in common
>> is direct addressing from a CPU bus.  Rarely, a platform_device will
>> be connected through a segment of some other kind of bus; but its
>> registers will still be directly addressable."
>>
>> Do you agree that the above description is not a good match for the
>> ufs-bsg kernel module?
>
> I missed this one.
> How about making it a plain device and add it from ufs driver?

Hi Can,

Since the ufs_bsg kernel module already creates one device node under 
/dev/bsg for each UFS host I don't think that we need to create any 
additional device nodes for ufs-bsg devices. My proposal is to modify 
the original patch 2/3 from this series as follows:
* Use module_init() instead of late_initcall_sync().
* Remove the ufshcd_get_hba_list_lock() and
   ufshcd_put_hba_list_unlock() functions.
* Implement a notification mechanism in the UFS core that invokes a
   callback function after an UFS host has been created and also after an
   UFS host has been removed.
* Register for these notifications from inside the ufs-bsg driver.
* During registration for notifications, invoke the UFS host creation
   callback function for all known UFS hosts.
* If the UFS core is unloaded, invoke the UFS host removal callback
   function for all known UFS hosts.

I think there are several examples of similar notification mechanisms in 
the Linux kernel, e.g. the probe and remove callback functions in struct 
pci_driver.

Bart.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Greg KH]

On Mon, Dec 16, 2019 at 09:22:21AM -0800, Bart Van Assche wrote:
> On 12/15/19 8:36 PM, cang@codeaurora.org wrote:
> > On 2019-12-16 05:49, Bart Van Assche wrote:
> > > On 2019-12-11 22:37, Bjorn Andersson wrote:
> > > > It's the asymmetry that I don't like.
> > > > 
> > > > Perhaps if you instead make ufshcd platform_device_register_data() the
> > > > bsg device you would solve the probe ordering, the remove will be
> > > > symmetric and module autoloading will work as well (although then you
> > > > need a MODULE_ALIAS of platform:device-name).
> > > 
> > > Hi Bjorn,
> > > 
> > > From Documentation/driver-api/driver-model/platform.rst:
> > > "Platform devices are devices that typically appear as autonomous
> > > entities in the system. This includes legacy port-based devices and
> > > host bridges to peripheral buses, and most controllers integrated
> > > into system-on-chip platforms.  What they usually have in common
> > > is direct addressing from a CPU bus.  Rarely, a platform_device will
> > > be connected through a segment of some other kind of bus; but its
> > > registers will still be directly addressable."
> > > 
> > > Do you agree that the above description is not a good match for the
> > > ufs-bsg kernel module?
> > 
> > I missed this one.
> > How about making it a plain device and add it from ufs driver?
> 
> Hi Can,
> 
> Since the ufs_bsg kernel module already creates one device node under
> /dev/bsg for each UFS host I don't think that we need to create any
> additional device nodes for ufs-bsg devices. My proposal is to modify the
> original patch 2/3 from this series as follows:
> * Use module_init() instead of late_initcall_sync().
> * Remove the ufshcd_get_hba_list_lock() and
>   ufshcd_put_hba_list_unlock() functions.
> * Implement a notification mechanism in the UFS core that invokes a
>   callback function after an UFS host has been created and also after an
>   UFS host has been removed.

You want to be a bus and have a device on it.

> * Register for these notifications from inside the ufs-bsg driver.

You want to be a bus.

> * During registration for notifications, invoke the UFS host creation
>   callback function for all known UFS hosts.

You want to be a bus.

> * If the UFS core is unloaded, invoke the UFS host removal callback
>   function for all known UFS hosts.

Again, a bus would do all of this for you "for free", right?

Take a look at the crazy "virtual bus" code that the RDMA people are
proposing if you want to try to use something like that, or just take
200 lines of code and be your own bus and devices that hang off of it.
That sounds exactly what you are looking for here.

> I think there are several examples of similar notification mechanisms in the
> Linux kernel, e.g. the probe and remove callback functions in struct
> pci_driver.

Look, a bus!  :)

thanks,

greg k-h

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-17 01:22, Bart Van Assche wrote:
> On 12/15/19 8:36 PM, cang@codeaurora.org wrote:
>> On 2019-12-16 05:49, Bart Van Assche wrote:
>>> On 2019-12-11 22:37, Bjorn Andersson wrote:
>>>> It's the asymmetry that I don't like.
>>>> 
>>>> Perhaps if you instead make ufshcd platform_device_register_data() 
>>>> the
>>>> bsg device you would solve the probe ordering, the remove will be
>>>> symmetric and module autoloading will work as well (although then 
>>>> you
>>>> need a MODULE_ALIAS of platform:device-name).
>>> 
>>> Hi Bjorn,
>>> 
>>> From Documentation/driver-api/driver-model/platform.rst:
>>> "Platform devices are devices that typically appear as autonomous
>>> entities in the system. This includes legacy port-based devices and
>>> host bridges to peripheral buses, and most controllers integrated
>>> into system-on-chip platforms.  What they usually have in common
>>> is direct addressing from a CPU bus.  Rarely, a platform_device will
>>> be connected through a segment of some other kind of bus; but its
>>> registers will still be directly addressable."
>>> 
>>> Do you agree that the above description is not a good match for the
>>> ufs-bsg kernel module?
>> 
>> I missed this one.
>> How about making it a plain device and add it from ufs driver?
> 
> Hi Can,
> 
> Since the ufs_bsg kernel module already creates one device node under
> /dev/bsg for each UFS host I don't think that we need to create any
> additional device nodes for ufs-bsg devices. My proposal is to modify
> the original patch 2/3 from this series as follows:
> * Use module_init() instead of late_initcall_sync().
> * Remove the ufshcd_get_hba_list_lock() and
>   ufshcd_put_hba_list_unlock() functions.
> * Implement a notification mechanism in the UFS core that invokes a
>   callback function after an UFS host has been created and also after 
> an
>   UFS host has been removed.
> * Register for these notifications from inside the ufs-bsg driver.
> * During registration for notifications, invoke the UFS host creation
>   callback function for all known UFS hosts.
> * If the UFS core is unloaded, invoke the UFS host removal callback
>   function for all known UFS hosts.
> 
> I think there are several examples of similar notification mechanisms
> in the Linux kernel, e.g. the probe and remove callback functions in
> struct pci_driver.
> 
> Bart.

Hi Bart,

Even in the current ufs_bsg.c, it creates two devices, one is ufs-bsg,
one is the char dev node under /dev/bsg. Why this becomes a problem
after make it a module?

I took a look into the pci_driver, it is no different than making 
ufs-bsg
a plain device. The only special place about pci_driver is that it has 
its
own probe() and remove(), and the probe() in its bus_type calls the
probe() in pci_driver. Meaning the bus->probe() is an intermediate call
used to pass whatever needed by pci_driver->probe().

Of course we can also do this, but isn't it too much for ufs-bsg?
For our case, calling set_dev_drvdata(bsg_dev, hba) to pass hba to
ufs_bsg.c would be enough.

If you take a look at the V3 patch, the change makes the ufs_bsg.c
much conciser. platform_device_register_data() does everything for us,
initialize the device, set device name, provide the match func,
bus type and release func.

Since ufs-bsg is somewhat not a platform device, we can still add it
as a plain device, just need a few more lines to get it initialized.
This allows us leverage kernel's device driver model. Just like Greg
commented, we don't need to re-implement the mechanism again.

Thanks,
Can Guo.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bart Van Assche]

On 12/17/19 12:56 AM, cang@codeaurora.org wrote:
> Even in the current ufs_bsg.c, it creates two devices, one is ufs-bsg,
> one is the char dev node under /dev/bsg. Why this becomes a problem
> after make it a module?
> 
> I took a look into the pci_driver, it is no different than making ufs-bsg
> a plain device. The only special place about pci_driver is that it has its
> own probe() and remove(), and the probe() in its bus_type calls the
> probe() in pci_driver. Meaning the bus->probe() is an intermediate call
> used to pass whatever needed by pci_driver->probe().
> 
> Of course we can also do this, but isn't it too much for ufs-bsg?
> For our case, calling set_dev_drvdata(bsg_dev, hba) to pass hba to
> ufs_bsg.c would be enough.
> 
> If you take a look at the V3 patch, the change makes the ufs_bsg.c
> much conciser. platform_device_register_data() does everything for us,
> initialize the device, set device name, provide the match func,
> bus type and release func.
> 
> Since ufs-bsg is somewhat not a platform device, we can still add it
> as a plain device, just need a few more lines to get it initialized.
> This allows us leverage kernel's device driver model. Just like Greg
> commented, we don't need to re-implement the mechanism again.

Hi Can,

Since ufs-bsg is not a platform device I think it would be wrong to 
model ufs-bsg devices as platform devices.

Please have a look at the bus_register() and bus_unregister() functions 
as Greg KH asked. Using the bus abstraction is not that hard. An example 
is e.g. available in the scsi_debug driver, namely the pseudo_lld_bus.

Thanks,

Bart.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[cang]

On 2019-12-18 02:19, Bart Van Assche wrote:
> On 12/17/19 12:56 AM, cang@codeaurora.org wrote:
>> Even in the current ufs_bsg.c, it creates two devices, one is ufs-bsg,
>> one is the char dev node under /dev/bsg. Why this becomes a problem
>> after make it a module?
>> 
>> I took a look into the pci_driver, it is no different than making 
>> ufs-bsg
>> a plain device. The only special place about pci_driver is that it has 
>> its
>> own probe() and remove(), and the probe() in its bus_type calls the
>> probe() in pci_driver. Meaning the bus->probe() is an intermediate 
>> call
>> used to pass whatever needed by pci_driver->probe().
>> 
>> Of course we can also do this, but isn't it too much for ufs-bsg?
>> For our case, calling set_dev_drvdata(bsg_dev, hba) to pass hba to
>> ufs_bsg.c would be enough.
>> 
>> If you take a look at the V3 patch, the change makes the ufs_bsg.c
>> much conciser. platform_device_register_data() does everything for us,
>> initialize the device, set device name, provide the match func,
>> bus type and release func.
>> 
>> Since ufs-bsg is somewhat not a platform device, we can still add it
>> as a plain device, just need a few more lines to get it initialized.
>> This allows us leverage kernel's device driver model. Just like Greg
>> commented, we don't need to re-implement the mechanism again.
> 
> Hi Can,
> 
> Since ufs-bsg is not a platform device I think it would be wrong to
> model ufs-bsg devices as platform devices.
> 
> Please have a look at the bus_register() and bus_unregister()
> functions as Greg KH asked. Using the bus abstraction is not that
> hard. An example is e.g. available in the scsi_debug driver, namely
> the pseudo_lld_bus.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Yes, I am talking the same here. Since platform device is not an option
for ufs-bsg, to make it a plain device we would need to do 
bus_register()
and bus_unregister(). And also do device_initialize() and device_add().

Thanks,
Can Guo.

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Vignesh Raghavendra]

Hi,

On 11/12/19 2:19 pm, Can Guo wrote:
> In order to improve the flexibility of ufs-bsg, modulizing 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>
> ---
[...]
> -int ufs_bsg_probe(struct ufs_hba *hba)
> +static int ufs_bsg_probe(struct ufs_hba *hba)
>  {
> -	struct device *bsg_dev = &hba->bsg_dev;
> +	struct device *bsg_dev;
>  	struct Scsi_Host *shost = hba->host;
>  	struct device *parent = &shost->shost_gendev;
>  	struct request_queue *q;
>  	int ret;
>  
> +	bsg_dev = kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
> +	if (!bsg_dev)
> +		return -ENOMEM;
> +
> +	hba->bsg_dev = bsg_dev;
>  	device_initialize(bsg_dev);
>  
>  	bsg_dev->parent = get_device(parent);
> @@ -217,6 +225,41 @@ int ufs_bsg_probe(struct ufs_hba *hba)
>  
>  out:
>  	dev_err(bsg_dev, "fail to initialize a bsg dev %d\n", shost->host_no);
> +	hba->bsg_dev = NULL;

Don't we need to free the associated memory before assigning to NULL?
Alternatively can allocation be made with devm_ APIs instead?

>  	put_device(bsg_dev);
>  	return ret;
>  }
> +
> +static int __init ufs_bsg_init(void)
> +{
> +	struct list_head *hba_list = NULL;
> +	struct ufs_hba *hba;
> +	int ret = 0;
> +
> +	ufshcd_get_hba_list_lock(&hba_list);
> +	list_for_each_entry(hba, hba_list, list) {
> +		ret = ufs_bsg_probe(hba);
> +		if (ret)
> +			break;
> +	}

So IIUC, if ufs_bsg_probe() fails for one of the hba instances in the
list, then we fail to create bsg device for all remaining instances that
follow, which seems too harsh.

Regards
Vignesh

Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

[Bart Van Assche]

On 12/11/19 3:49 AM, Can Guo wrote:
> In order to improve the flexibility of ufs-bsg, modulizing it is a good
> choice. This change introduces tristate to ufs-bsg to allow users compile
> it as an external module.

Did you perhaps mean "modularize" instead of "modulize"? Additionally, 
should "modulizing" perhaps be changed into "modularizing"?

> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index d14c224..72620ce 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -38,6 +38,7 @@ config SCSI_UFSHCD
>   	select PM_DEVFREQ
>   	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>   	select NLS
> +	select BLK_DEV_BSGLIB
>   	---help---
>   	This selects the support for UFS devices in Linux, say Y and make
>   	  sure that you know the name of your UFS host adapter (the card

I do not understand the above change. Doesn't moving the BSG code into a 
separate module remove the dependency of SCSI_UFSHCD on BLK_DEV_BSGLIB?

> +static int __init ufs_bsg_init(void)
> +{
> +	struct list_head *hba_list = NULL;
> +	struct ufs_hba *hba;
> +	int ret = 0;
> +
> +	ufshcd_get_hba_list_lock(&hba_list);
> +	list_for_each_entry(hba, hba_list, list) {
> +		ret = ufs_bsg_probe(hba);
> +		if (ret)
> +			break;
> +	}
> +	ufshcd_put_hba_list_unlock();
> +
> +	return ret;
> +}

What if ufs_bsg_probe() succeeds for some UFS adapters but not for 
others? Shouldn't ufs_bgs_remove() be called in that case for the 
adapters for which ufs_bsg_probe() succeeded?

> +late_initcall_sync(ufs_bsg_init);
> +module_exit(ufs_bsg_exit);

Why late_initcall_sync() instead of module_init()?

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a86b0fd..7a83a8f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -108,6 +108,22 @@
>   		       16, 4, buf, __len, false);                        \
>   } while (0)
>   
> +static LIST_HEAD(ufs_hba_list);
> +static DEFINE_MUTEX(ufs_hba_list_lock);
> +
> +void ufshcd_get_hba_list_lock(struct list_head **list)
> +{
> +	mutex_lock(&ufs_hba_list_lock);
> +	*list = &ufs_hba_list;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_get_hba_list_lock);

Please make ufshcd_get_hba_list_lock() return the list_head pointer 
instead of the above.

Thanks,

Bart.