Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: l4EqO1RyPSj6XyheOjjY6x2Y2rz1/j4GoodUQEjpdVarfei35xiT8VaDQS+xLh80gu9tCvy9qr
 TLl2GXEdRfO5lc320/tHe3+CHvEwNWtj4PCEOUzlaeXERzv5Ozfo7GJt+THwQo0qO4x7ppjxyu
 aU2/u6yibMaJ4OPYULFS/ZLDtby8GYIacJKb9HXnlppjEAJEoI2DzNFR/mbDhOpldv0/0qELFD
 MkX0OQ7vWlVNLkxzqzA0pQJ1+lJZO+LUq85WjJMwpeGhImF2/6TvjamcwO8ECYnwThtMtKmvJs
 ij4=
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        "cang@codeaurora.org" <cang@codeaurora.org>
CC:     "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        "nguyenb@codeaurora.org" <nguyenb@codeaurora.org>,
        "rnayak@codeaurora.org" <rnayak@codeaurora.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "kernel-team@android.com" <kernel-team@android.com>,
        "saravanak@google.com" <saravanak@google.com>,
        "salyzyn@google.com" <salyzyn@google.com>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Pedro Sousa <pedrom.sousa@synopsys.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Evan Green <evgreen@chromium.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Stephen Boyd <swboyd@chromium.org>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Bean Huo <beanhuo@micron.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Venkat Gopalakrishnan <venkatg@codeaurora.org>,
        Tomas Winkler <tomas.winkler@intel.com>,
        Arnd Bergmann <arnd@arndb.de>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg
Thread-Topic: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg
Thread-Index: AQHVr//eTRAsaCi7o0KCA+PFpTUx1Ke18FKAgAAc25KAAAI5UA==
Date:   Thu, 12 Dec 2019 07:00:50 +0000
Message-ID: <MN2PR04MB69919AA0C345E7D6620C3ADFFC550@MN2PR04MB6991.namprd04.prod.outlook.com>
References: <1576054123-16417-1-git-send-email-cang@codeaurora.org>
 <0101016ef425ef65-5c4508cc-5e76-4107-bb27-270f66acaa9a-000000@us-west-2.amazonses.com>
 <20191212045357.GA415177@yoga>
 <0101016ef8b2e2f8-72260b08-e6ad-42fc-bd4b-4a0a72c5c9b3-000000@us-west-2.amazonses.com>
 <20191212063703.GC415177@yoga>
In-Reply-To: <20191212063703.GC415177@yoga>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ec18eeb5-cda9-497d-af88-08d77ed105a2
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Dec 2019 07:00:50.0910
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: uEsKkXgL46Vhu7scVdwBiBqcxIpRXu6HhVL13MpwTeZsfGkYnnrVxxmsx+4pAZA37JwS2FDDIecNa00YdYAhHg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR04MB7022
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


>=20
>=20
> On Wed 11 Dec 22:01 PST 2019, cang@codeaurora.org wrote:
>=20
> > 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 i=
t.
> > 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/westerndigitalcorporat=
ion/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 i=
n 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) +=3D
> > > > cdns-pltfrm.o
> > > >  obj-$(CONFIG_SCSI_UFS_QCOM) +=3D ufs-qcom.o
> > > >  obj-$(CONFIG_SCSI_UFSHCD) +=3D ufshcd-core.o
> > > >  ufshcd-core-y                            +=3D ufshcd.o ufs-sysfs.o
> > > > -ufshcd-core-$(CONFIG_SCSI_UFS_BSG)       +=3D ufs_bsg.o
> > > > +obj-$(CONFIG_SCSI_UFS_BSG)       +=3D ufs_bsg.o
> > > >  obj-$(CONFIG_SCSI_UFSHCD_PCI) +=3D ufshcd-pci.o
> > > >  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) +=3D ufshcd-pltfrm.o
> > > >  obj-$(CONFIG_SCSI_UFS_HISI) +=3D 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 *jo=
b)
> > > >   */
> > > >  void ufs_bsg_remove(struct ufs_hba *hba)  {
> > > > - struct device *bsg_dev =3D &hba->bsg_dev;
> > > > + struct device *bsg_dev =3D hba->bsg_dev;
> > > >
> > > >   if (!hba->bsg_queue)
> > > >           return;
> > > >
> > > >   bsg_remove_queue(hba->bsg_queue);
> > > >
> > > > + hba->bsg_dev =3D NULL;
> > > > + hba->bsg_queue =3D 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(struc=
t
> > > > 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 =3D &hba->bsg_dev;
> > > > + struct device *bsg_dev;
> > > >   struct Scsi_Host *shost =3D hba->host;
> > > >   struct device *parent =3D &shost->shost_gendev;
> > > >   struct request_queue *q;
> > > >   int ret;
> > > >
> > > > + bsg_dev =3D kzalloc(sizeof(*bsg_dev), GFP_KERNEL);
> > > > + if (!bsg_dev)
> > > > +         return -ENOMEM;
> > > > +
> > > > + hba->bsg_dev =3D bsg_dev;
> > > >   device_initialize(bsg_dev);
> > > >
> > > >   bsg_dev->parent =3D 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 =3D NULL;
> > > >   put_device(bsg_dev);
> > > >   return ret;
> > > >  }
> > > > +
> > > > +static int __init ufs_bsg_init(void)
> > > > +{
> > > > + struct list_head *hba_list =3D NULL;
> > > > + struct ufs_hba *hba;
> > > > + int ret =3D 0;
> > > > +
> > > > + ufshcd_get_hba_list_lock(&hba_list);
> > > > + list_for_each_entry(hba, hba_list, list) {
> > > > +         ret =3D ufs_bsg_probe(hba);
> > > > +         if (ret)
> > > > +                 break;
> > > > + }
> > >
> > > So what happens if I go CONFIG_SCSI_UFS_BSG=3Dy and
> > > CONFIG_SCSI_UFS_QCOM=3Dy?
> > >
> > > 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 =3Dm, what happens if they are invoked in the "wrong" order?
> > >
> >
> > In the case that CONFIG_SCSI_UFS_BSG=3Dy and CONFIG_SCSI_UFS_QCOM=3Dy,
> > 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 combinat=
ion.
> >
> > In the case that both of them are "m", installing ufs-bsg before ufs-qc=
om
> > is installed would have no effect as ufs_hba_list is empty, which is
> > expected.
>=20
> 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.
>=20
> > And in real cases, as the UFS is the boot device, UFS driver will alway=
s
> > be probed during bootup.
> >
>=20
> 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.
>=20
> So in the even that the bsg is =3Dy and any of these drivers are =3Dm, or=
 if
> you're having bad luck with your timing, the list will be empty.
>=20
> As described below, if bsg=3Dm, then there's nothing that will load the
> module and the bsg will not probe...
Right.
bsg=3Dy and ufshcd=3Dm is a bad idea, and should be avoided.

>=20
> [..]
> > > > 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 =3D 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.
> >
>=20
> It's the asymmetry that I don't like.
>=20
> 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).
>=20
> Regards,
> Bjorn