Re: [PATCH 15/18] bus: mhi: core: Introduce sysfs entries for MHI

[Manivannan Sadhasivam]

On Sun, Sep 27, 2020 at 12:26:59PM +0200, Greg KH wrote:
> On Mon, Sep 21, 2020 at 09:38:12PM +0530, Manivannan Sadhasivam wrote:
> > From: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > 
> > Introduce sysfs entries to enable userspace clients the ability to read
> > the serial number and the OEM PK Hash values obtained from BHI. OEMs
> > need to read these device-specific hardware information values through
> > userspace for factory testing purposes and cannot be exposed via degbufs
> > as it may remain disabled for performance reasons. Also, update the
> > documentation for ABI to include these entries.
> > 
> > Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> >  Documentation/ABI/stable/sysfs-bus-mhi | 21 ++++++++++
> >  MAINTAINERS                            |  1 +
> >  drivers/bus/mhi/core/init.c            | 53 ++++++++++++++++++++++++++
> >  3 files changed, 75 insertions(+)
> >  create mode 100644 Documentation/ABI/stable/sysfs-bus-mhi
> > 
> > diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
> > new file mode 100644
> > index 000000000000..ecfe7662f8d0
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-bus-mhi
> > @@ -0,0 +1,21 @@
> > +What:		/sys/bus/mhi/devices/.../serialnumber
> > +Date:		Sept 2020
> > +KernelVersion:	5.10
> > +Contact:	Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > +Description:	The file holds the serial number of the client device obtained
> > +		using a BHI (Boot Host Interface) register read after at least
> > +		one attempt to power up the device has been done. If read
> > +		without having the device power on at least once, the file will
> > +		read all 0's.
> > +Users:		Any userspace application or clients interested in device info.
> > +
> > +What:		/sys/bus/mhi/devices/.../oem_pk_hash
> > +Date:		Sept 2020
> > +KernelVersion:	5.10
> > +Contact:	Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > +Description:	The file holds the OEM PK Hash value of the endpoint device
> > +		obtained using a BHI (Boot Host Interface) register read after
> > +		at least one attempt to power up the device has been done. If
> > +		read without having the device power on at least once, the file
> > +		will read all 0's.
> > +Users:		Any userspace application or clients interested in device info.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index deaafb617361..11e7be9b9163 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11323,6 +11323,7 @@ M:	Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> >  L:	linux-arm-msm@xxxxxxxxxxxxxxx
> >  S:	Maintained
> >  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git
> > +F:	Documentation/ABI/stable/sysfs-bus-mhi
> >  F:	Documentation/mhi/
> >  F:	drivers/bus/mhi/
> >  F:	include/linux/mhi.h
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index 61e5885a331a..1b4161eaf0d8 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -76,6 +76,56 @@ const char *to_mhi_pm_state_str(enum mhi_pm_state state)
> >  	return mhi_pm_state_str[index];
> >  }
> >  

[...]

> > +static int mhi_create_sysfs(struct mhi_controller *mhi_cntrl)
> > +{
> > +	return sysfs_create_group(&mhi_cntrl->mhi_dev->dev.kobj,
> > +				  &mhi_sysfs_group);
> 
> You should never have to call a sysfs_* function from a driver or bus
> code, that implies something is wrong :)
> 

Hmm. I think the inspiration is drawn from an old bus driver :(
Will submit a patch later to fix it!

> Just set the sysfs attributes to be the default groups for your
> bus/device/whatever-you-have-here and then the driver core will properly
> manage the creation and removal of these files, in a race-free manner.
> 
> As it is, I think this is racy and will cause userspace confusion, but I
> haven't looked for sure, as you should use the api that guarantees it is
> safe...
> 

Okay. Will use the dev_groups callback to manage the sysfs attributes.

Thanks,
Mani

> thanks,
> 
> greg k-h