[v5,2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions

Commit Message

Liming Sun May 17, 2019, 5:49 p.m. UTC

This commit adds the ABI definitions exposed to userspace for
the platform/mellanox/mlxbf-bootctl driver.

Reviewed-by: Vadim Pasternak <[e-mail]>
Signed-off-by: Liming Sun <[e-mail]>
---
 .../ABI/testing/sysfs-platform-mellanox-bootctl    | 58 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-bootctl

Comments

Greg KH May 17, 2019, 5:59 p.m. UTC | #1

On Fri, May 17, 2019 at 01:49:05PM -0400, Liming Sun wrote:
> This commit adds the ABI definitions exposed to userspace for
> the platform/mellanox/mlxbf-bootctl driver.
> 
> Reviewed-by: Vadim Pasternak <[e-mail]>
> Signed-off-by: Liming Sun <[e-mail]>
> ---
>  .../ABI/testing/sysfs-platform-mellanox-bootctl    | 58 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> new file mode 100644
> index 0000000..19a14db
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> @@ -0,0 +1,58 @@
> +What:		/sys/bus/platform/drivers/mlxbf-bootctl/lifecycle_state
> +Date:		May 2019
> +KernelVersion:	5.3
> +Contact:	"Liming Sun <[e-mail]>"
> +Description:
> +		The Life-cycle state of the SoC, which could be one of the
> +		following values.
> +		  Production - Production state and can be updated to secure
> +		  GA Secured - Secure chip and not able to change state
> +		  GA Non-Secured - Non-Secure chip and not able to change state
> +		  RMA - Return Merchandise Authorization

A "driver" does not have a lifecycle state, a "device" does.

You are putting all of these attributes in the wrong place.  Put them on
your device please, not the driver.  driver-specific attributes are
_VERY_ rare, and only for things that can modify/show for all devices
attached to that driver.

thanks,

greg k-h

Liming Sun May 17, 2019, 8:36 p.m. UTC | #2

Thanks Greg for the comments!  Please see my response inline.

Regards,
- Liming

> -----Original Message-----
> From: Greg Kroah-Hartman <[e-mail]>
> Sent: Friday, May 17, 2019 1:59 PM
> To: Liming Sun <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>; Darren Hart <[e-mail]>; Vadim Pasternak <[e-mail]>; David
> Woods <[e-mail]>; [e-mail]; [e-mail]; David S. Miller
> <[e-mail]>; Mauro Carvalho Chehab <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Nicolas Ferre <[e-mail]>; Paul E. McKenney <[e-mail]>
> Subject: Re: [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions
> 
> On Fri, May 17, 2019 at 01:49:05PM -0400, Liming Sun wrote:
> > This commit adds the ABI definitions exposed to userspace for
> > the platform/mellanox/mlxbf-bootctl driver.
> >
> > Reviewed-by: Vadim Pasternak <[e-mail]>
> > Signed-off-by: Liming Sun <[e-mail]>
> > ---
> >  .../ABI/testing/sysfs-platform-mellanox-bootctl    | 58 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 59 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl b/Documentation/ABI/testing/sysfs-platform-mellanox-
> bootctl
> > new file mode 100644
> > index 0000000..19a14db
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > @@ -0,0 +1,58 @@
> > +What:		/sys/bus/platform/drivers/mlxbf-bootctl/lifecycle_state
> > +Date:		May 2019
> > +KernelVersion:	5.3
> > +Contact:	"Liming Sun <[e-mail]>"
> > +Description:
> > +		The Life-cycle state of the SoC, which could be one of the
> > +		following values.
> > +		  Production - Production state and can be updated to secure
> > +		  GA Secured - Secure chip and not able to change state
> > +		  GA Non-Secured - Non-Secure chip and not able to change state
> > +		  RMA - Return Merchandise Authorization
> 
> A "driver" does not have a lifecycle state, a "device" does.
> 
> You are putting all of these attributes in the wrong place.  Put them on
> your device please, not the driver.  driver-specific attributes are
> _VERY_ rare, and only for things that can modify/show for all devices
> attached to that driver.

This driver is running on the ARM processor of the SoC. The 'device' is
the SoC itself. That's to say, there is only one device here attached to
the driver and the driver state will also be the device state.

This interface has been used by user-space applications for a couple of
releases. It'll be great if it could stay in such way for compatibility. Please
advise if this is strongly preferred to move them under devices.

> 
> thanks,
> 
> greg k-h

Greg KH May 18, 2019, 6:35 a.m. UTC | #3

On Fri, May 17, 2019 at 08:36:53PM +0000, Liming Sun wrote:
> Thanks Greg for the comments!  Please see my response inline.
> 
> Regards,
> - Liming
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <[e-mail]>
> > Sent: Friday, May 17, 2019 1:59 PM
> > To: Liming Sun <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>; Darren Hart <[e-mail]>; Vadim Pasternak <[e-mail]>; David
> > Woods <[e-mail]>; [e-mail]; [e-mail]; David S. Miller
> > <[e-mail]>; Mauro Carvalho Chehab <[e-mail]>; Jonathan Cameron
> > <[e-mail]>; Nicolas Ferre <[e-mail]>; Paul E. McKenney <[e-mail]>
> > Subject: Re: [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions
> > 
> > On Fri, May 17, 2019 at 01:49:05PM -0400, Liming Sun wrote:
> > > This commit adds the ABI definitions exposed to userspace for
> > > the platform/mellanox/mlxbf-bootctl driver.
> > >
> > > Reviewed-by: Vadim Pasternak <[e-mail]>
> > > Signed-off-by: Liming Sun <[e-mail]>
> > > ---
> > >  .../ABI/testing/sysfs-platform-mellanox-bootctl    | 58 ++++++++++++++++++++++
> > >  MAINTAINERS                                        |  1 +
> > >  2 files changed, 59 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl b/Documentation/ABI/testing/sysfs-platform-mellanox-
> > bootctl
> > > new file mode 100644
> > > index 0000000..19a14db
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > > @@ -0,0 +1,58 @@
> > > +What:		/sys/bus/platform/drivers/mlxbf-bootctl/lifecycle_state
> > > +Date:		May 2019
> > > +KernelVersion:	5.3
> > > +Contact:	"Liming Sun <[e-mail]>"
> > > +Description:
> > > +		The Life-cycle state of the SoC, which could be one of the
> > > +		following values.
> > > +		  Production - Production state and can be updated to secure
> > > +		  GA Secured - Secure chip and not able to change state
> > > +		  GA Non-Secured - Non-Secure chip and not able to change state
> > > +		  RMA - Return Merchandise Authorization
> > 
> > A "driver" does not have a lifecycle state, a "device" does.
> > 
> > You are putting all of these attributes in the wrong place.  Put them on
> > your device please, not the driver.  driver-specific attributes are
> > _VERY_ rare, and only for things that can modify/show for all devices
> > attached to that driver.
> 
> This driver is running on the ARM processor of the SoC. The 'device' is
> the SoC itself. That's to say, there is only one device here attached to
> the driver and the driver state will also be the device state.

That might be true today, but maybe not tomorrow :)

Anyway, again, this is device state, not driver state.

> This interface has been used by user-space applications for a couple of
> releases. It'll be great if it could stay in such way for compatibility. Please
> advise if this is strongly preferred to move them under devices.

So this is code that is already in the tree, and is just now being
documented?  What .c file(s) is this referring to?

As for "comptability", sysfs is made such that if a file is not present,
userspace should be able to survive, that is why it is
one-value-per-file.  What tool is using this, and where is the source
for it?

thanks,

greg k-h

Liming Sun May 20, 2019, 3:20 p.m. UTC | #4

Please see response inline.

> -----Original Message-----
> From: Greg Kroah-Hartman <[e-mail]>
> Sent: Saturday, May 18, 2019 2:35 AM
> To: Liming Sun <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>; Darren Hart <[e-mail]>; Vadim Pasternak <[e-mail]>; David
> Woods <[e-mail]>; [e-mail]; [e-mail]; David S. Miller
> <[e-mail]>; Mauro Carvalho Chehab <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Nicolas Ferre <[e-mail]>; Paul E. McKenney <[e-mail]>
> Subject: Re: [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions
> 
> On Fri, May 17, 2019 at 08:36:53PM +0000, Liming Sun wrote:
> > Thanks Greg for the comments!  Please see my response inline.
> >
> > Regards,
> > - Liming
> >
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <[e-mail]>
> > > Sent: Friday, May 17, 2019 1:59 PM
> > > To: Liming Sun <[e-mail]>
> > > Cc: Andy Shevchenko <[e-mail]>; Darren Hart <[e-mail]>; Vadim Pasternak <[e-mail]>;
> David
> > > Woods <[e-mail]>; [e-mail]; [e-mail]; David S. Miller
> > > <[e-mail]>; Mauro Carvalho Chehab <[e-mail]>; Jonathan Cameron
> > > <[e-mail]>; Nicolas Ferre <[e-mail]>; Paul E. McKenney <[e-mail]>
> > > Subject: Re: [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions
> > >
> > > On Fri, May 17, 2019 at 01:49:05PM -0400, Liming Sun wrote:
> > > > This commit adds the ABI definitions exposed to userspace for
> > > > the platform/mellanox/mlxbf-bootctl driver.
> > > >
> > > > Reviewed-by: Vadim Pasternak <[e-mail]>
> > > > Signed-off-by: Liming Sun <[e-mail]>
> > > > ---
> > > >  .../ABI/testing/sysfs-platform-mellanox-bootctl    | 58 ++++++++++++++++++++++
> > > >  MAINTAINERS                                        |  1 +
> > > >  2 files changed, 59 insertions(+)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl b/Documentation/ABI/testing/sysfs-platform-mellanox-
> > > bootctl
> > > > new file mode 100644
> > > > index 0000000..19a14db
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > > > @@ -0,0 +1,58 @@
> > > > +What:		/sys/bus/platform/drivers/mlxbf-bootctl/lifecycle_state
> > > > +Date:		May 2019
> > > > +KernelVersion:	5.3
> > > > +Contact:	"Liming Sun <[e-mail]>"
> > > > +Description:
> > > > +		The Life-cycle state of the SoC, which could be one of the
> > > > +		following values.
> > > > +		  Production - Production state and can be updated to secure
> > > > +		  GA Secured - Secure chip and not able to change state
> > > > +		  GA Non-Secured - Non-Secure chip and not able to change state
> > > > +		  RMA - Return Merchandise Authorization
> > >
> > > A "driver" does not have a lifecycle state, a "device" does.
> > >
> > > You are putting all of these attributes in the wrong place.  Put them on
> > > your device please, not the driver.  driver-specific attributes are
> > > _VERY_ rare, and only for things that can modify/show for all devices
> > > attached to that driver.
> >
> > This driver is running on the ARM processor of the SoC. The 'device' is
> > the SoC itself. That's to say, there is only one device here attached to
> > the driver and the driver state will also be the device state.
> 
> That might be true today, but maybe not tomorrow :)
> 
> Anyway, again, this is device state, not driver state.

Yes, I agree. I discussed with the team and will move these attributes
under device (since they could be all considered as device state).

> 
> > This interface has been used by user-space applications for a couple of
> > releases. It'll be great if it could stay in such way for compatibility. Please
> > advise if this is strongly preferred to move them under devices.
> 
> So this is code that is already in the tree, and is just now being
> documented?  What .c file(s) is this referring to?

This code is not in the kernel tree yet. It has been in Mellanox BlueField
SW packages for a couple of releases and is trying to be up-streamed now.

> 
> As for "comptability", sysfs is made such that if a file is not present,
> userspace should be able to survive, that is why it is
> one-value-per-file.  What tool is using this, and where is the source
> for it?

The latest 2.0 code can be found at link
https://github.com/Mellanox/mlxbf-bootctl/tree/2.0

In file mlxbf-bootctl.c, currently it uses the 'drivers' path as sysfs path.
#define SYS_PATH "/sys/bus/platform/drivers/mlx-bootctl". We could
update it to support both paths.

> 
> thanks,
> 
> greg k-h

Greg KH May 20, 2019, 3:53 p.m. UTC | #5

On Mon, May 20, 2019 at 03:20:02PM +0000, Liming Sun wrote:
> Please see response inline.
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <[e-mail]>
> > Sent: Saturday, May 18, 2019 2:35 AM
> > To: Liming Sun <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>; Darren Hart <[e-mail]>; Vadim Pasternak <[e-mail]>; David
> > Woods <[e-mail]>; [e-mail]; [e-mail]; David S. Miller
> > <[e-mail]>; Mauro Carvalho Chehab <[e-mail]>; Jonathan Cameron
> > <[e-mail]>; Nicolas Ferre <[e-mail]>; Paul E. McKenney <[e-mail]>
> > Subject: Re: [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions
> > 
> > On Fri, May 17, 2019 at 08:36:53PM +0000, Liming Sun wrote:
> > > Thanks Greg for the comments!  Please see my response inline.
> > >
> > > Regards,
> > > - Liming
> > >
> > > > -----Original Message-----
> > > > From: Greg Kroah-Hartman <[e-mail]>
> > > > Sent: Friday, May 17, 2019 1:59 PM
> > > > To: Liming Sun <[e-mail]>
> > > > Cc: Andy Shevchenko <[e-mail]>; Darren Hart <[e-mail]>; Vadim Pasternak <[e-mail]>;
> > David
> > > > Woods <[e-mail]>; [e-mail]; [e-mail]; David S. Miller
> > > > <[e-mail]>; Mauro Carvalho Chehab <[e-mail]>; Jonathan Cameron
> > > > <[e-mail]>; Nicolas Ferre <[e-mail]>; Paul E. McKenney <[e-mail]>
> > > > Subject: Re: [PATCH v5 2/2] platform/mellanox/mlxbf-bootctl: Add the ABI definitions
> > > >
> > > > On Fri, May 17, 2019 at 01:49:05PM -0400, Liming Sun wrote:
> > > > > This commit adds the ABI definitions exposed to userspace for
> > > > > the platform/mellanox/mlxbf-bootctl driver.
> > > > >
> > > > > Reviewed-by: Vadim Pasternak <[e-mail]>
> > > > > Signed-off-by: Liming Sun <[e-mail]>
> > > > > ---
> > > > >  .../ABI/testing/sysfs-platform-mellanox-bootctl    | 58 ++++++++++++++++++++++
> > > > >  MAINTAINERS                                        |  1 +
> > > > >  2 files changed, 59 insertions(+)
> > > > >  create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl b/Documentation/ABI/testing/sysfs-platform-mellanox-
> > > > bootctl
> > > > > new file mode 100644
> > > > > index 0000000..19a14db
> > > > > --- /dev/null
> > > > > +++ b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
> > > > > @@ -0,0 +1,58 @@
> > > > > +What:		/sys/bus/platform/drivers/mlxbf-bootctl/lifecycle_state
> > > > > +Date:		May 2019
> > > > > +KernelVersion:	5.3
> > > > > +Contact:	"Liming Sun <[e-mail]>"
> > > > > +Description:
> > > > > +		The Life-cycle state of the SoC, which could be one of the
> > > > > +		following values.
> > > > > +		  Production - Production state and can be updated to secure
> > > > > +		  GA Secured - Secure chip and not able to change state
> > > > > +		  GA Non-Secured - Non-Secure chip and not able to change state
> > > > > +		  RMA - Return Merchandise Authorization
> > > >
> > > > A "driver" does not have a lifecycle state, a "device" does.
> > > >
> > > > You are putting all of these attributes in the wrong place.  Put them on
> > > > your device please, not the driver.  driver-specific attributes are
> > > > _VERY_ rare, and only for things that can modify/show for all devices
> > > > attached to that driver.
> > >
> > > This driver is running on the ARM processor of the SoC. The 'device' is
> > > the SoC itself. That's to say, there is only one device here attached to
> > > the driver and the driver state will also be the device state.
> > 
> > That might be true today, but maybe not tomorrow :)
> > 
> > Anyway, again, this is device state, not driver state.
> 
> Yes, I agree. I discussed with the team and will move these attributes
> under device (since they could be all considered as device state).

Great.

> > > This interface has been used by user-space applications for a couple of
> > > releases. It'll be great if it could stay in such way for compatibility. Please
> > > advise if this is strongly preferred to move them under devices.
> > 
> > So this is code that is already in the tree, and is just now being
> > documented?  What .c file(s) is this referring to?
> 
> This code is not in the kernel tree yet. It has been in Mellanox BlueField
> SW packages for a couple of releases and is trying to be up-streamed now.

Ok, then whatever happened in the past, before anyone in the community
reviewed your code, and merged it into the kernel tree, does not count :)

Where is the first patch in this series?  I'll be glad to review your
actual sysfs implementation if you point me at it.

> > As for "comptability", sysfs is made such that if a file is not present,
> > userspace should be able to survive, that is why it is
> > one-value-per-file.  What tool is using this, and where is the source
> > for it?
> 
> The latest 2.0 code can be found at link
> https://github.com/Mellanox/mlxbf-bootctl/tree/2.0
> 
> In file mlxbf-bootctl.c, currently it uses the 'drivers' path as sysfs path.
> #define SYS_PATH "/sys/bus/platform/drivers/mlx-bootctl". We could
> update it to support both paths.

Please do so.

thanks,

greg k-h

Patch

diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
new file mode 100644
index 0000000..19a14db
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
@@ -0,0 +1,58 @@ 
+What:		/sys/bus/platform/drivers/mlxbf-bootctl/lifecycle_state
+Date:		May 2019
+KernelVersion:	5.3
+Contact:	"Liming Sun <[e-mail]>"
+Description:
+		The Life-cycle state of the SoC, which could be one of the
+		following values.
+		  Production - Production state and can be updated to secure
+		  GA Secured - Secure chip and not able to change state
+		  GA Non-Secured - Non-Secure chip and not able to change state
+		  RMA - Return Merchandise Authorization
+
+What:		/sys/bus/platform/drivers/mlxbf-bootctl/post_reset_wdog
+Date:		May 2019
+KernelVersion:	5.3
+Contact:	"Liming Sun <[e-mail]>"
+Description:
+		The watchdog setting in seconds for the next booting. It's used
+		to reboot the chip and recover it to the old state if the new
+		boot partition fails.
+
+What:		/sys/bus/platform/drivers/mlxbf-bootctl/reset_action
+Date:		May 2019
+KernelVersion:	5.3
+Contact:	"Liming Sun <[e-mail]>"
+Description:
+		The source of the boot stream for the next reset. It could be
+		one of the following values.
+		  external - boot from external source (USB or PCIe)
+		  emmc - boot from the onchip eMMC
+		  emmc_legacy - boot from the onchip eMMC in legacy (slow) mode
+
+What:		/sys/bus/platform/drivers/mlxbf-bootctl/second_reset_action
+Date:		May 2019
+KernelVersion:	5.3
+Contact:	"Liming Sun <[e-mail]>"
+Description:
+		Update the source of the boot stream after next reset. It could
+		be one of the following values and will be applied after next
+		reset.
+		  external - boot from external source (USB or PCIe)
+		  emmc - boot from the onchip eMMC
+		  emmc_legacy - boot from the onchip eMMC in legacy (slow) mode
+		  swap_emmc - swap the primary / secondary boot partition
+		  none - cancel the action
+
+What:		/sys/bus/platform/drivers/mlxbf-bootctl/secure_boot_fuse_state
+Date:		May 2019
+KernelVersion:	5.3
+Contact:	"Liming Sun <[e-mail]>"
+Description:
+		The state of eFuse versions with the following values.
+		  InUse - burnt, valid and currently in use
+		  Used - burnt and valid
+		  Free - not burnt and free to use
+		  Skipped - not burnt but not free (skipped)
+		  Wasted - burnt and invalid
+		  Invalid - not burnt but marked as valid (error state).
diff --git a/MAINTAINERS b/MAINTAINERS
index fb9f9d7..8a9d9ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10018,6 +10018,7 @@  M:	Darren Hart <[e-mail]>
 M:	Vadim Pasternak <[e-mail]>
 L:	[e-mail]
 S:	Supported
+F:	Documentation/ABI/testing/sysfs-platform-mellanox-bootctl
 F:	drivers/platform/mellanox/
 F:	include/linux/platform_data/mlxreg.h