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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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