| Message ID | 0101016ef425ef65-5c4508cc-5e76-4107-bb27-270f66acaa9a-000000@us-west-2.amazonses.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Modulize ufs-bsg | expand |
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 >
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 >>
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
> > > 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
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.
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
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
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.
> > 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
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.
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.
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.
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
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.
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.
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.
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; };
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(-)