Messages in this thread | | | Date | Thu, 12 Dec 2019 16:53:51 +0000 | From | cang@codeauro ... | Subject | Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg |
| |
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
| |