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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
[...]
> -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
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 <[email protected]>
>> ---
>> 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
>>
On Wed 11 Dec 22:01 PST 2019, [email protected] 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 <[email protected]>
> > > ---
> > > 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
>
>
> On Wed 11 Dec 22:01 PST 2019, [email protected] 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 <[email protected]>
> > > > ---
> > > > 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
On 2019-12-12 14:37, Bjorn Andersson wrote:
> On Wed 11 Dec 22:01 PST 2019, [email protected] 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 <[email protected]>
>> > > ---
>> > > 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.
On 2019-12-12 15:00, Avri Altman wrote:
>>
>>
>> On Wed 11 Dec 22:01 PST 2019, [email protected] 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 <[email protected]>
>> > > > ---
>> > > > 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
On Thu 12 Dec 08:53 PST 2019, [email protected] wrote:
> On 2019-12-12 15:00, Avri Altman wrote:
> > > On Wed 11 Dec 22:01 PST 2019, [email protected] 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
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.
On 2019-12-13 02:24, Bjorn Andersson wrote:
> On Thu 12 Dec 08:53 PST 2019, [email protected] wrote:
>
>> On 2019-12-12 15:00, Avri Altman wrote:
>> > > On Wed 11 Dec 22:01 PST 2019, [email protected] 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.
>
> On 2019-12-13 02:24, Bjorn Andersson wrote:
> > On Thu 12 Dec 08:53 PST 2019, [email protected] wrote:
> >
> >> On 2019-12-12 15:00, Avri Altman wrote:
> >> > > On Wed 11 Dec 22:01 PST 2019, [email protected] 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
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.