[PATCH v2 0/3] can: etas_es58x: report firmware version

[PATCH v2 0/3] can: etas_es58x: report firmware version

[Vincent Mailhol]

The goal of this series is to report the firmware version of ETAS
ES58x devices through ethtool.

The easiest way to do so is by using usb_cache_string so that we do
not have to manage errors.

First patch exports usb_cache_string(). The second patch then does a
small cleanup in the existing function and replace existing code with
usb_cache_string(). The third and final patch reports the firmware
version of the device to the userland through ethtool.

* Changelog *

v1 -> v2:

  * was a single patch. It is now a series of three patches.
  * add a first new patch to export  usb_cache_string().
  * add a second new patch to apply usb_cache_string() to existing code.
  * add missing check on product info string to prevent a buffer overflow.
  * add comma on the last entry of struct es58x_parameters.

Vincent Mailhol (3):
  USB: core: export usb_cache_string()
  can: etas_es58x: use usb_cache_string() to retrieve the product info
    string
  can: etas_es58x: report the firmware version through ethtool

 drivers/net/can/usb/etas_es58x/es581_4.c    |  5 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c | 75 ++++++++++++---------
 drivers/net/can/usb/etas_es58x/es58x_core.h |  8 ++-
 drivers/net/can/usb/etas_es58x/es58x_fd.c   |  5 +-
 drivers/usb/core/message.c                  |  1 +
 drivers/usb/core/usb.h                      |  1 -
 include/linux/usb.h                         |  1 +
 7 files changed, 60 insertions(+), 36 deletions(-)

-- 
2.37.4

[PATCH v2 1/3] USB: core: export usb_cache_string()

[Vincent Mailhol]

usb_cache_string() can also be useful for the drivers so export it.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/usb/core/message.c | 1 +
 drivers/usb/core/usb.h     | 1 -
 include/linux/usb.h        | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 4d59d927ae3e..127fac1af676 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1037,6 +1037,7 @@ char *usb_cache_string(struct usb_device *udev, int index)
 	}
 	return smallbuf;
 }
+EXPORT_SYMBOL_GPL(usb_cache_string);
 
 /*
  * usb_get_device_descriptor - (re)reads the device descriptor (usbcore)
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..0eac7d4285d1 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -47,7 +47,6 @@ extern int usb_get_device_descriptor(struct usb_device *dev,
 extern int usb_set_isoch_delay(struct usb_device *dev);
 extern int usb_get_bos_descriptor(struct usb_device *dev);
 extern void usb_release_bos_descriptor(struct usb_device *dev);
-extern char *usb_cache_string(struct usb_device *udev, int index);
 extern int usb_set_configuration(struct usb_device *dev, int configuration);
 extern int usb_choose_configuration(struct usb_device *udev);
 extern int usb_generic_driver_probe(struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 9ff1ad4dfad1..d2d2f41052c0 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1829,6 +1829,7 @@ static inline int usb_get_ptm_status(struct usb_device *dev, void *data)
 
 extern int usb_string(struct usb_device *dev, int index,
 	char *buf, size_t size);
+extern char *usb_cache_string(struct usb_device *udev, int index);
 
 /* wrappers that also update important state inside usbcore */
 extern int usb_clear_halt(struct usb_device *dev, int pipe);
-- 
2.37.4

Re: [PATCH v2 1/3] USB: core: export usb_cache_string()

[Greg Kroah-Hartman]

On Sat, Nov 05, 2022 at 02:16:02AM +0900, Vincent Mailhol wrote:
> usb_cache_string() can also be useful for the drivers so export it.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/usb/core/message.c | 1 +
>  drivers/usb/core/usb.h     | 1 -
>  include/linux/usb.h        | 1 +
>  3 files changed, 2 insertions(+), 1 deletion(-)

No, sorry, NAK on this, see my comments on patch 2/3.

thanks,

greg k-h

[PATCH v2 2/3] can: etas_es58x: use usb_cache_string() to retrieve the product info string

[Vincent Mailhol]

Instead of allocating memory ourselves and doing all the error
handling, rely on usb_cache_string(). This results in simpler code.

Make es58x_get_product_info() return void. The reason is double:

  1/ by using usb_cache_string() we do not know anymore the root cause
     (is it an allocation issue or input/output issue?)

  2/ Failling to get the product info is not critical. So it is OK to
     continue.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 33 +++------------------
 drivers/net/can/usb/etas_es58x/es58x_core.h |  3 ++
 2 files changed, 7 insertions(+), 29 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 51294b717040..1a17aadfc1dc 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2124,41 +2124,18 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
  * @es58x_dev: ES58X device.
  *
  * Do a synchronous call to get the product information.
- *
- * Return: zero on success, errno when any error occurs.
  */
-static int es58x_get_product_info(struct es58x_device *es58x_dev)
+static void es58x_get_product_info(struct es58x_device *es58x_dev)
 {
-	struct usb_device *udev = es58x_dev->udev;
-	const int es58x_prod_info_idx = 6;
-	/* Empirical tests show a prod_info length of maximum 83,
-	 * below should be more than enough.
-	 */
-	const size_t prod_info_len = 127;
 	char *prod_info;
-	int ret;
 
-	prod_info = kmalloc(prod_info_len, GFP_KERNEL);
+	prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
 	if (!prod_info)
-		return -ENOMEM;
+		return;
 
-	ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
-	if (ret < 0) {
-		dev_err(es58x_dev->dev,
-			"%s: Could not read the product info: %pe\n",
-			__func__, ERR_PTR(ret));
-		goto out_free;
-	}
-	if (ret >= prod_info_len - 1) {
-		dev_warn(es58x_dev->dev,
-			 "%s: Buffer is too small, result might be truncated\n",
-			 __func__);
-	}
 	dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
 
- out_free:
 	kfree(prod_info);
-	return ret < 0 ? ret : 0;
 }
 
 /**
@@ -2243,9 +2220,7 @@ static int es58x_probe(struct usb_interface *intf,
 	if (IS_ERR(es58x_dev))
 		return PTR_ERR(es58x_dev);
 
-	ret = es58x_get_product_info(es58x_dev);
-	if (ret)
-		return ret;
+	es58x_get_product_info(es58x_dev);
 
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
 		ret = es58x_init_netdev(es58x_dev, ch_idx);
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index 640fe0a1df63..9a5a616df783 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -49,6 +49,9 @@
 /* A magic number sent by the ES581.4 to inform it is alive. */
 #define ES58X_HEARTBEAT 0x11
 
+/* USB descriptor index containing the product information string. */
+#define ES58X_PROD_INFO_IDX 6
+
 /**
  * enum es58x_driver_info - Quirks of the device.
  * @ES58X_DUAL_CHANNEL: Device has two CAN channels. If this flag is
-- 
2.37.4

Re: [PATCH v2 2/3] can: etas_es58x: use usb_cache_string() to retrieve the product info string

[Greg Kroah-Hartman]

On Sat, Nov 05, 2022 at 02:16:03AM +0900, Vincent Mailhol wrote:
> Instead of allocating memory ourselves and doing all the error
> handling, rely on usb_cache_string(). This results in simpler code.
> 
> Make es58x_get_product_info() return void. The reason is double:
> 
>   1/ by using usb_cache_string() we do not know anymore the root cause
>      (is it an allocation issue or input/output issue?)
> 
>   2/ Failling to get the product info is not critical. So it is OK to
>      continue.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/net/can/usb/etas_es58x/es58x_core.c | 33 +++------------------
>  drivers/net/can/usb/etas_es58x/es58x_core.h |  3 ++
>  2 files changed, 7 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index 51294b717040..1a17aadfc1dc 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -2124,41 +2124,18 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
>   * @es58x_dev: ES58X device.
>   *
>   * Do a synchronous call to get the product information.
> - *
> - * Return: zero on success, errno when any error occurs.
>   */
> -static int es58x_get_product_info(struct es58x_device *es58x_dev)
> +static void es58x_get_product_info(struct es58x_device *es58x_dev)
>  {
> -	struct usb_device *udev = es58x_dev->udev;
> -	const int es58x_prod_info_idx = 6;
> -	/* Empirical tests show a prod_info length of maximum 83,
> -	 * below should be more than enough.
> -	 */
> -	const size_t prod_info_len = 127;
>  	char *prod_info;
> -	int ret;
>  
> -	prod_info = kmalloc(prod_info_len, GFP_KERNEL);
> +	prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
>  	if (!prod_info)
> -		return -ENOMEM;
> +		return;
>  
> -	ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
> -	if (ret < 0) {
> -		dev_err(es58x_dev->dev,
> -			"%s: Could not read the product info: %pe\n",
> -			__func__, ERR_PTR(ret));
> -		goto out_free;
> -	}
> -	if (ret >= prod_info_len - 1) {
> -		dev_warn(es58x_dev->dev,
> -			 "%s: Buffer is too small, result might be truncated\n",
> -			 __func__);
> -	}
>  	dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);

Wait, why is this driver spamming the kernel log with this information
in the first place?  That should not be happening as when drivers work
properly, they are quiet.

Can you just delete this entirely?  Bonus is that device discovery is
now even faster as you drop the useless "get me the product string" USB
transactions.

And all of that info is in userspace today if userspace really wants it
(through libusb, usbutils, or just reading from a sysfs file.)  There is
no need to add this to individual drivers as well.

So no, please don't do this, just remove this code entirely.

Also note that the USB core can, and will, provide this info if the
kernel is configured to do so, just enable
CONFIG_USB_ANNOUNCE_NEW_DEVICES.  This should not be a per-driver thing
to do.

thanks,

greg k-h

[PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent Mailhol]

ES58x devices report below information in their usb product info
string:

  * the firmware version
  * the bootloader version
  * the hardware revision

Report the firmware version through ethtool_drvinfo::fw_version.
Because struct ethtool_drvinfo has no fields to report the boatloader
version nor the hardware revision, continue to print these in the
kernel log (c.f. es58x_get_product_info()).

While doing so, bump up copyright year of each modified files.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es581_4.c    |  5 ++-
 drivers/net/can/usb/etas_es58x/es58x_core.c | 42 ++++++++++++++++++++-
 drivers/net/can/usb/etas_es58x/es58x_core.h |  5 ++-
 drivers/net/can/usb/etas_es58x/es58x_fd.c   |  5 ++-
 4 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c
index 1bcdcece5ec7..29c03c8b3f07 100644
--- a/drivers/net/can/usb/etas_es58x/es581_4.c
+++ b/drivers/net/can/usb/etas_es58x/es581_4.c
@@ -6,7 +6,7 @@
  *
  * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved.
  * Copyright (c) 2020 ETAS K.K.. All rights reserved.
- * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #include <linux/kernel.h>
@@ -492,7 +492,8 @@ const struct es58x_parameters es581_4_param = {
 	.tx_bulk_max = ES581_4_TX_BULK_MAX,
 	.urb_cmd_header_len = ES581_4_URB_CMD_HEADER_LEN,
 	.rx_urb_max = ES58X_RX_URBS_MAX,
-	.tx_urb_max = ES58X_TX_URBS_MAX
+	.tx_urb_max = ES58X_TX_URBS_MAX,
+	.prod_info_delim = ',',
 };
 
 const struct es58x_operators es581_4_ops = {
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 1a17aadfc1dc..72a60f5f92c8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -7,7 +7,7 @@
  *
  * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved.
  * Copyright (c) 2020 ETAS K.K.. All rights reserved.
- * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #include <linux/ethtool.h>
@@ -1978,7 +1978,47 @@ static const struct net_device_ops es58x_netdev_ops = {
 	.ndo_eth_ioctl = can_eth_ioctl_hwts,
 };
 
+/**
+ * es58x_get_drvinfo() - Get the driver name and firmware version.
+ * @netdev: CAN network device.
+ * @drvinfo: Driver information.
+ *
+ * Populate @drvinfo with the driver name and the firmware version.
+ */
+static void es58x_get_drvinfo(struct net_device *netdev,
+			      struct ethtool_drvinfo *drvinfo)
+{
+	struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
+	char *prod_info, *start, *end;
+
+	strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
+
+	prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+	if (!prod_info)
+		return;
+
+	/* The firmware prefix is either "FW_V" or "FW:" */
+	start = strstr(prod_info, "FW");
+	if (!start)
+		goto free_prod_info;
+	/* Go to first digit */
+	while (!isdigit(*start)) {
+		start++;
+		if (!*start)
+			goto free_prod_info;
+	}
+	end = strchr(start, es58x_dev->param->prod_info_delim);
+	if (!end || end - start >= sizeof(drvinfo->fw_version))
+		goto free_prod_info;
+
+	strncpy(drvinfo->fw_version, start, end - start);
+
+ free_prod_info:
+	kfree(prod_info);
+}
+
 static const struct ethtool_ops es58x_ethtool_ops = {
+	.get_drvinfo = es58x_get_drvinfo,
 	.get_ts_info = can_ethtool_op_get_ts_info_hwts,
 };
 
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index 9a5a616df783..9c2cdb57f34a 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -6,7 +6,7 @@
  *
  * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved.
  * Copyright (c) 2020 ETAS K.K.. All rights reserved.
- * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef __ES58X_COMMON_H__
@@ -309,6 +309,8 @@ struct es58x_priv {
  * @urb_cmd_header_len: Length of the URB command header.
  * @rx_urb_max: Number of RX URB to be allocated during device probe.
  * @tx_urb_max: Number of TX URB to be allocated during device probe.
+ * @prod_info_delim: delimiter of the different fields in the USB
+ *	product information string.
  */
 struct es58x_parameters {
 	const struct can_bittiming_const *bittiming_const;
@@ -327,6 +329,7 @@ struct es58x_parameters {
 	u8 urb_cmd_header_len;
 	u8 rx_urb_max;
 	u8 tx_urb_max;
+	char prod_info_delim;
 };
 
 /**
diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
index c97ffa71fd75..aa7a4866f870 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
@@ -8,7 +8,7 @@
  *
  * Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved.
  * Copyright (c) 2020 ETAS K.K.. All rights reserved.
- * Copyright (c) 2020, 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #include <linux/kernel.h>
@@ -550,7 +550,8 @@ const struct es58x_parameters es58x_fd_param = {
 	.tx_bulk_max = ES58X_FD_TX_BULK_MAX,
 	.urb_cmd_header_len = ES58X_FD_URB_CMD_HEADER_LEN,
 	.rx_urb_max = ES58X_RX_URBS_MAX,
-	.tx_urb_max = ES58X_TX_URBS_MAX
+	.tx_urb_max = ES58X_TX_URBS_MAX,
+	.prod_info_delim = '-',
 };
 
 const struct es58x_operators es58x_fd_ops = {
-- 
2.37.4

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Greg Kroah-Hartman]

On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> ES58x devices report below information in their usb product info
> string:
> 
>   * the firmware version
>   * the bootloader version
>   * the hardware revision
> 
> Report the firmware version through ethtool_drvinfo::fw_version.
> Because struct ethtool_drvinfo has no fields to report the boatloader
> version nor the hardware revision, continue to print these in the
> kernel log (c.f. es58x_get_product_info()).
> 
> While doing so, bump up copyright year of each modified files.

Why not just stick to the normal USB interface here and not try to tie
it into ethtool?  These values are all availble today in sysfs or in
libusb, right?

What workflow wants this added to ethtool?

thanks,

greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> > ES58x devices report below information in their usb product info
> > string:
> >
> >   * the firmware version
> >   * the bootloader version
> >   * the hardware revision
> >
> > Report the firmware version through ethtool_drvinfo::fw_version.
> > Because struct ethtool_drvinfo has no fields to report the boatloader
> > version nor the hardware revision, continue to print these in the
> > kernel log (c.f. es58x_get_product_info()).
> >
> > While doing so, bump up copyright year of each modified files.
>
> Why not just stick to the normal USB interface here and not try to tie
> it into ethtool?  These values are all availble today in sysfs or in
> libusb, right?

The simple answer is ignorance. I am more familiar with ethtool than
libusb and I just did not think to explore that second option.
Thanks for the review, comments taken. I will study sysfs and libusb
and will rework that.

> What workflow wants this added to ethtool?

No workflow. My work is not bound to any company and this driver
maintenance and anything else I am doing on the mailing list at this
time is pure hobby.

Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> > > ES58x devices report below information in their usb product info
> > > string:
> > >
> > >   * the firmware version
> > >   * the bootloader version
> > >   * the hardware revision
> > >
> > > Report the firmware version through ethtool_drvinfo::fw_version.
> > > Because struct ethtool_drvinfo has no fields to report the boatloader
> > > version nor the hardware revision, continue to print these in the
> > > kernel log (c.f. es58x_get_product_info()).
> > >
> > > While doing so, bump up copyright year of each modified files.
> >
> > Why not just stick to the normal USB interface here and not try to tie
> > it into ethtool?  These values are all availble today in sysfs or in
> > libusb, right?
>
> The simple answer is ignorance. I am more familiar with ethtool than
> libusb and I just did not think to explore that second option.
> Thanks for the review, comments taken. I will study sysfs and libusb
> and will rework that.

I double checked following options:
  * CONFIG_USB_ANNOUNCE_NEW_DEVICES
  * lsusb -v from usbutils
  * sysfs

None of those will return the firmware version. The only strings I am
getting are: the Product name, the Manufacturer and the SerialNumber.

I guess you were expecting some default behavior from the device, but
unfortunately, this is not the case.
On this device, the firmware version is stored at some arbitrary
descriptor index (if you ask me: 6). Unless you query that magic
number, the information will not pot up.

So as far as I can see, this does not duplicate existing mechanisms.
With this patch, the firmware version becomes available using:
  $ ethtool -i canX

> > What workflow wants this added to ethtool?
>
> No workflow. My work is not bound to any company and this driver
> maintenance and anything else I am doing on the mailing list at this
> time is pure hobby.
>
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Greg Kroah-Hartman]

On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Nov 05, 2022 at 02:16:04AM +0900, Vincent Mailhol wrote:
> > > > ES58x devices report below information in their usb product info
> > > > string:
> > > >
> > > >   * the firmware version
> > > >   * the bootloader version
> > > >   * the hardware revision
> > > >
> > > > Report the firmware version through ethtool_drvinfo::fw_version.
> > > > Because struct ethtool_drvinfo has no fields to report the boatloader
> > > > version nor the hardware revision, continue to print these in the
> > > > kernel log (c.f. es58x_get_product_info()).
> > > >
> > > > While doing so, bump up copyright year of each modified files.
> > >
> > > Why not just stick to the normal USB interface here and not try to tie
> > > it into ethtool?  These values are all availble today in sysfs or in
> > > libusb, right?
> >
> > The simple answer is ignorance. I am more familiar with ethtool than
> > libusb and I just did not think to explore that second option.
> > Thanks for the review, comments taken. I will study sysfs and libusb
> > and will rework that.
> 
> I double checked following options:
>   * CONFIG_USB_ANNOUNCE_NEW_DEVICES
>   * lsusb -v from usbutils
>   * sysfs
> 
> None of those will return the firmware version. The only strings I am
> getting are: the Product name, the Manufacturer and the SerialNumber.

Those are the default strings that a device can have, so it's good that
the core tries to get them.

Anything other than those are "custom" strings and you can use libusb
for that.  For some reason I thought sysfs also had custom strings, but
as they are so rare I don't know if anyone has tried that.

> I guess you were expecting some default behavior from the device, but
> unfortunately, this is not the case.
> On this device, the firmware version is stored at some arbitrary
> descriptor index (if you ask me: 6). Unless you query that magic
> number, the information will not pot up.
> 
> So as far as I can see, this does not duplicate existing mechanisms.
> With this patch, the firmware version becomes available using:
>   $ ethtool -i canX

It's late right now, and I can't remember the whole USB spec, but I
think the device provides a list of the string ids that are valid for
it.  If so, we can add that to sysfs for any USB device out there, no
matter the string descriptor number.

If not, maybe we can just iterate the 255 values and populate sysfs
files if they are present?  I'll dig up the USB spec tomorrow...

I say do this at the USB core level, that way it works for any USB
device, and you don't have a device-specific sysfs file and custom
userspace code just for this.

Sound reasonable?

greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Alan Stern]

On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > <mailhol.vincent@wanadoo.fr> wrote:
> > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> It's late right now, and I can't remember the whole USB spec, but I
> think the device provides a list of the string ids that are valid for
> it.  If so, we can add that to sysfs for any USB device out there, no
> matter the string descriptor number.

No, there is no such list.

> If not, maybe we can just iterate the 255 values and populate sysfs
> files if they are present?  I'll dig up the USB spec tomorrow...

Yes, we could do that.  But the filename would have to be the string 
id, which is not meaningful.  We wouldn't be able to have labels like 
"product-info" unless somehow a driver could provide the label.

Also, there's the matter of language.  Devices can have string 
descriptors in multiple languages; which one should we show in sysfs?  
All of them?  Right now we use just the default language for the strings 
that we put in sysfs.

> I say do this at the USB core level, that way it works for any USB
> device, and you don't have a device-specific sysfs file and custom
> userspace code just for this.

This is unavoidable to some extent.  Without device-specific information 
or userspace code, there is no way to know which string descriptor 
contains the data you want.

Alan Stern

> Sound reasonable?
> 
> greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Sun. 6 Nov. 2022 at 09:48, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > <mailhol.vincent@wanadoo.fr> wrote:
> > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > It's late right now, and I can't remember the whole USB spec, but I
> > think the device provides a list of the string ids that are valid for
> > it.  If so, we can add that to sysfs for any USB device out there, no
> > matter the string descriptor number.
>
> No, there is no such list.
>
> > If not, maybe we can just iterate the 255 values and populate sysfs
> > files if they are present?  I'll dig up the USB spec tomorrow...
>
> Yes, we could do that.  But the filename would have to be the string
> id, which is not meaningful.  We wouldn't be able to have labels like
> "product-info" unless somehow a driver could provide the label.

My shot on this would be like this:

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 549590e9c644..d0a4fc3ffe07 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -77,6 +77,19 @@ struct ieee1394_device_id {
  * Use the flag values to control which fields are compared.
  */

+/**
+ * struct custom_string - information of custom string and their indexes
+ * @idx: Index of the custom string descriptor.
+ * @label: Mnemotechnic, will be used as a filename for the sysfs entry.
+ *
+ * USB devices might expose some information in some customs strings. Drivers
+ * can use this structure to inform the USB core of where to find these.
+ */
+struct custom_string {
+       __u8 idx;
+       const char *label;
+};
+
 /**
  * struct usb_device_id - identifies USB devices for probing and hotplugging
  * @match_flags: Bit mask controlling which of the other fields are used to
@@ -110,6 +123,9 @@ struct ieee1394_device_id {
  * @driver_info: Holds information used by the driver.  Usually it holds
  *         a pointer to a descriptor understood by the driver, or perhaps
  *         device flags.
+ * @customs_strings_table: devices using customs strings can use this table to
+ *         inform the USB core of how to retrieve them. If used, must
contained an
+ *         empty terminating entry.
  *
  * In most cases, drivers will create a table of device IDs by using
  * USB_DEVICE(), or similar macros designed for that purpose.
@@ -150,6 +166,7 @@ struct usb_device_id {
            /* not matched against */
            kernel_ulong_t  driver_info
                        __attribute__((aligned(sizeof(kernel_ulong_t))));
+           const struct custom_string *custom_strings_table;
 };

 /* Some useful macros to use to create struct usb_device_id */


Then the driver would declare its custom stings like this:

  static const struct custom_string es58x_custom_strings_table[] = {
          { .idx = 6, .label = "product_info" },
          { /* Terminating entry */ }
  };


Finally, the USB core can iterate through it and populate the sysfs
entries using the provided label.


> Also, there's the matter of language.  Devices can have string
> descriptors in multiple languages; which one should we show in sysfs?
> All of them?  Right now we use just the default language for the strings
> that we put in sysfs.

I do not have the knowledge to comment on the multiple languages
issue. FYI, the device which I maintain does not have multiple
languages.

> > I say do this at the USB core level, that way it works for any USB
> > device, and you don't have a device-specific sysfs file and custom
> > userspace code just for this.
>
> This is unavoidable to some extent.  Without device-specific information
> or userspace code, there is no way to know which string descriptor
> contains the data you want.

ACK. I also do not want any userspace code for that. Users should not
need to know a magic number to retrieve the thing.

> Alan Stern
>
> > Sound reasonable?
> >
> > greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Greg Kroah-Hartman]

On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern wrote:
> On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > <mailhol.vincent@wanadoo.fr> wrote:
> > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > It's late right now, and I can't remember the whole USB spec, but I
> > think the device provides a list of the string ids that are valid for
> > it.  If so, we can add that to sysfs for any USB device out there, no
> > matter the string descriptor number.
> 
> No, there is no such list.

Yeah, my fault, nevermind about that, sorry.

> > If not, maybe we can just iterate the 255 values and populate sysfs
> > files if they are present?  I'll dig up the USB spec tomorrow...
> 
> Yes, we could do that.  But the filename would have to be the string 
> id, which is not meaningful.  We wouldn't be able to have labels like 
> "product-info" unless somehow a driver could provide the label.

We could have a directory of strings/ with the individual descriptors in
there as files with the name being the string id.

But that might take a long time to populate, as it can take a few tries
to get the string from a device, and to do that 256 times might be
noticable at device insertion time.

> Also, there's the matter of language.  Devices can have string 
> descriptors in multiple languages; which one should we show in sysfs?  
> All of them?  Right now we use just the default language for the strings 
> that we put in sysfs.
> 
> > I say do this at the USB core level, that way it works for any USB
> > device, and you don't have a device-specific sysfs file and custom
> > userspace code just for this.
> 
> This is unavoidable to some extent.  Without device-specific information 
> or userspace code, there is no way to know which string descriptor 
> contains the data you want.

Agreed.

Ok, for this specific instance, adding the "we know this string id
should be here" as a device-specific sysfs file seems to be the easiest
way forward.

Vincent, want to try that instead?

thanks,

greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern wrote:
> > On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > > <mailhol.vincent@wanadoo.fr> wrote:
> > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > It's late right now, and I can't remember the whole USB spec, but I
> > > think the device provides a list of the string ids that are valid for
> > > it.  If so, we can add that to sysfs for any USB device out there, no
> > > matter the string descriptor number.
> >
> > No, there is no such list.
>
> Yeah, my fault, nevermind about that, sorry.
>
> > > If not, maybe we can just iterate the 255 values and populate sysfs
> > > files if they are present?  I'll dig up the USB spec tomorrow...
> >
> > Yes, we could do that.  But the filename would have to be the string
> > id, which is not meaningful.  We wouldn't be able to have labels like
> > "product-info" unless somehow a driver could provide the label.
>
> We could have a directory of strings/ with the individual descriptors in
> there as files with the name being the string id.
>
> But that might take a long time to populate, as it can take a few tries
> to get the string from a device, and to do that 256 times might be
> noticable at device insertion time.
>
> > Also, there's the matter of language.  Devices can have string
> > descriptors in multiple languages; which one should we show in sysfs?
> > All of them?  Right now we use just the default language for the strings
> > that we put in sysfs.
> >
> > > I say do this at the USB core level, that way it works for any USB
> > > device, and you don't have a device-specific sysfs file and custom
> > > userspace code just for this.
> >
> > This is unavoidable to some extent.  Without device-specific information
> > or userspace code, there is no way to know which string descriptor
> > contains the data you want.
>
> Agreed.
>
> Ok, for this specific instance, adding the "we know this string id
> should be here" as a device-specific sysfs file seems to be the easiest
> way forward.
>
> Vincent, want to try that instead?

OK for me. Will do that and remove the kernel log spam and replace it
by a sysfs entry.

I have two questions:

1/ Can I still export and use usb_cache_string()? In other terms, does
the first patch of this series still apply? This looks like the most
convenient function to retrieve that custom string to me.

2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
users might look for the value in sysfs, while some might use ethtool.
If the info is not available in ethtool, people might simply assume it
is not available at all. So, I would like to provide both.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Greg Kroah-Hartman]

On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern wrote:
> > > On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > > > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > > > <mailhol.vincent@wanadoo.fr> wrote:
> > > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > It's late right now, and I can't remember the whole USB spec, but I
> > > > think the device provides a list of the string ids that are valid for
> > > > it.  If so, we can add that to sysfs for any USB device out there, no
> > > > matter the string descriptor number.
> > >
> > > No, there is no such list.
> >
> > Yeah, my fault, nevermind about that, sorry.
> >
> > > > If not, maybe we can just iterate the 255 values and populate sysfs
> > > > files if they are present?  I'll dig up the USB spec tomorrow...
> > >
> > > Yes, we could do that.  But the filename would have to be the string
> > > id, which is not meaningful.  We wouldn't be able to have labels like
> > > "product-info" unless somehow a driver could provide the label.
> >
> > We could have a directory of strings/ with the individual descriptors in
> > there as files with the name being the string id.
> >
> > But that might take a long time to populate, as it can take a few tries
> > to get the string from a device, and to do that 256 times might be
> > noticable at device insertion time.
> >
> > > Also, there's the matter of language.  Devices can have string
> > > descriptors in multiple languages; which one should we show in sysfs?
> > > All of them?  Right now we use just the default language for the strings
> > > that we put in sysfs.
> > >
> > > > I say do this at the USB core level, that way it works for any USB
> > > > device, and you don't have a device-specific sysfs file and custom
> > > > userspace code just for this.
> > >
> > > This is unavoidable to some extent.  Without device-specific information
> > > or userspace code, there is no way to know which string descriptor
> > > contains the data you want.
> >
> > Agreed.
> >
> > Ok, for this specific instance, adding the "we know this string id
> > should be here" as a device-specific sysfs file seems to be the easiest
> > way forward.
> >
> > Vincent, want to try that instead?
> 
> OK for me. Will do that and remove the kernel log spam and replace it
> by a sysfs entry.
> 
> I have two questions:
> 
> 1/ Can I still export and use usb_cache_string()? In other terms, does
> the first patch of this series still apply? This looks like the most
> convenient function to retrieve that custom string to me.

Everyone seems to just use the usb_string() function, will that not work
for you?

> 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> users might look for the value in sysfs, while some might use ethtool.
> If the info is not available in ethtool, people might simply assume it
> is not available at all. So, I would like to provide both.

That's up to the network developers/maintainers.  I don't know if that's
a required or common api for network devices to have.

thanks,

greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

Le dim. 6 nov. 2022 à 23:22, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> a écrit :
>
> On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> > On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern wrote:
> > > > On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > > > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > > > > <mailhol.vincent@wanadoo.fr> wrote:
> > > > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > It's late right now, and I can't remember the whole USB spec, but I
> > > > > think the device provides a list of the string ids that are valid for
> > > > > it.  If so, we can add that to sysfs for any USB device out there, no
> > > > > matter the string descriptor number.
> > > >
> > > > No, there is no such list.
> > >
> > > Yeah, my fault, nevermind about that, sorry.
> > >
> > > > > If not, maybe we can just iterate the 255 values and populate sysfs
> > > > > files if they are present?  I'll dig up the USB spec tomorrow...
> > > >
> > > > Yes, we could do that.  But the filename would have to be the string
> > > > id, which is not meaningful.  We wouldn't be able to have labels like
> > > > "product-info" unless somehow a driver could provide the label.
> > >
> > > We could have a directory of strings/ with the individual descriptors in
> > > there as files with the name being the string id.
> > >
> > > But that might take a long time to populate, as it can take a few tries
> > > to get the string from a device, and to do that 256 times might be
> > > noticable at device insertion time.
> > >
> > > > Also, there's the matter of language.  Devices can have string
> > > > descriptors in multiple languages; which one should we show in sysfs?
> > > > All of them?  Right now we use just the default language for the strings
> > > > that we put in sysfs.
> > > >
> > > > > I say do this at the USB core level, that way it works for any USB
> > > > > device, and you don't have a device-specific sysfs file and custom
> > > > > userspace code just for this.
> > > >
> > > > This is unavoidable to some extent.  Without device-specific information
> > > > or userspace code, there is no way to know which string descriptor
> > > > contains the data you want.
> > >
> > > Agreed.
> > >
> > > Ok, for this specific instance, adding the "we know this string id
> > > should be here" as a device-specific sysfs file seems to be the easiest
> > > way forward.
> > >
> > > Vincent, want to try that instead?
> >
> > OK for me. Will do that and remove the kernel log spam and replace it
> > by a sysfs entry.
> >
> > I have two questions:
> >
> > 1/ Can I still export and use usb_cache_string()? In other terms, does
> > the first patch of this series still apply? This looks like the most
> > convenient function to retrieve that custom string to me.
>
> Everyone seems to just use the usb_string() function, will that not work
> for you?

It is just that I would have to write two or three lines of code less.
But if you prefer I can use usb_string(), no problem on that.

> > 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> > users might look for the value in sysfs, while some might use ethtool.
> > If the info is not available in ethtool, people might simply assume it
> > is not available at all. So, I would like to provide both.
>
> That's up to the network developers/maintainers.  I don't know if that's
> a required or common api for network devices to have.

My question was more to know if you had any objection if I did so.
From the documentation, there is no indication that this is required.
But I don't like to leave a field empty when I have the information to
fill it.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Greg Kroah-Hartman]

On Sun, Nov 06, 2022 at 11:44:52PM +0900, Vincent MAILHOL wrote:
> Le dim. 6 nov. 2022 à 23:22, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> a écrit :
> >
> > On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> > > On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Sat, Nov 05, 2022 at 08:45:10PM -0400, Alan Stern wrote:
> > > > > On Sat, Nov 05, 2022 at 06:38:35PM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Sun, Nov 06, 2022 at 02:21:11AM +0900, Vincent MAILHOL wrote:
> > > > > > > On Sat. 5 Nov. 2022 at 18:27, Vincent MAILHOL
> > > > > > > <mailhol.vincent@wanadoo.fr> wrote:
> > > > > > > > On Sat. 5 Nov. 2022 at 17:36, Greg Kroah-Hartman
> > > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > It's late right now, and I can't remember the whole USB spec, but I
> > > > > > think the device provides a list of the string ids that are valid for
> > > > > > it.  If so, we can add that to sysfs for any USB device out there, no
> > > > > > matter the string descriptor number.
> > > > >
> > > > > No, there is no such list.
> > > >
> > > > Yeah, my fault, nevermind about that, sorry.
> > > >
> > > > > > If not, maybe we can just iterate the 255 values and populate sysfs
> > > > > > files if they are present?  I'll dig up the USB spec tomorrow...
> > > > >
> > > > > Yes, we could do that.  But the filename would have to be the string
> > > > > id, which is not meaningful.  We wouldn't be able to have labels like
> > > > > "product-info" unless somehow a driver could provide the label.
> > > >
> > > > We could have a directory of strings/ with the individual descriptors in
> > > > there as files with the name being the string id.
> > > >
> > > > But that might take a long time to populate, as it can take a few tries
> > > > to get the string from a device, and to do that 256 times might be
> > > > noticable at device insertion time.
> > > >
> > > > > Also, there's the matter of language.  Devices can have string
> > > > > descriptors in multiple languages; which one should we show in sysfs?
> > > > > All of them?  Right now we use just the default language for the strings
> > > > > that we put in sysfs.
> > > > >
> > > > > > I say do this at the USB core level, that way it works for any USB
> > > > > > device, and you don't have a device-specific sysfs file and custom
> > > > > > userspace code just for this.
> > > > >
> > > > > This is unavoidable to some extent.  Without device-specific information
> > > > > or userspace code, there is no way to know which string descriptor
> > > > > contains the data you want.
> > > >
> > > > Agreed.
> > > >
> > > > Ok, for this specific instance, adding the "we know this string id
> > > > should be here" as a device-specific sysfs file seems to be the easiest
> > > > way forward.
> > > >
> > > > Vincent, want to try that instead?
> > >
> > > OK for me. Will do that and remove the kernel log spam and replace it
> > > by a sysfs entry.
> > >
> > > I have two questions:
> > >
> > > 1/ Can I still export and use usb_cache_string()? In other terms, does
> > > the first patch of this series still apply? This looks like the most
> > > convenient function to retrieve that custom string to me.
> >
> > Everyone seems to just use the usb_string() function, will that not work
> > for you?
> 
> It is just that I would have to write two or three lines of code less.

Odd, should it be used instead where others are calling usb_string()?

> But if you prefer I can use usb_string(), no problem on that.

Try it both ways.  If it's easier with usb_cache_string(), then we can
export it.  It's just odd that it hasn't been exported yet.

> > > 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> > > users might look for the value in sysfs, while some might use ethtool.
> > > If the info is not available in ethtool, people might simply assume it
> > > is not available at all. So, I would like to provide both.
> >
> > That's up to the network developers/maintainers.  I don't know if that's
> > a required or common api for network devices to have.
> 
> My question was more to know if you had any objection if I did so.
> From the documentation, there is no indication that this is required.
> But I don't like to leave a field empty when I have the information to
> fill it.

No objection from me.

thanks,

greg k-h

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Mon. 7 nov. 2022 at 01:18, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Nov 06, 2022 at 11:44:52PM +0900, Vincent MAILHOL wrote:
> > On Sun. 6 Nov. 2022 à 23:22, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> > > > On Sun. 6 Nov. 2022 at 20:25, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > 1/ Can I still export and use usb_cache_string()? In other terms, does
> > > > the first patch of this series still apply? This looks like the most
> > > > convenient function to retrieve that custom string to me.
> > >
> > > Everyone seems to just use the usb_string() function, will that not work
> > > for you?
> >
> > It is just that I would have to write two or three lines of code less.
>
> Odd, should it be used instead where others are calling usb_string()?
>
> > But if you prefer I can use usb_string(), no problem on that.
>
> Try it both ways.  If it's easier with usb_cache_string(), then we can
> export it.  It's just odd that it hasn't been exported yet.

I tried both. Not counting the line breaks, the empty lines and the
comments, the usb_string() version needs 6 more lines. Not a huge
difference but the usb_cache_string() remains easier (at least in my
eyes).

For reference, here is the diff before and after using usb_cache_string():

diff --git a/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
index 4ff0332f6f50..c1d220d0d35f 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
@@ -178,17 +178,10 @@ void es58x_create_file(struct device *dev)
 {
        struct es58x_device *es58x_dev = dev_get_drvdata(dev);
        char *prod_info;
-       int ret;

-       prod_info = kmalloc(ES58X_PROD_INFO_SIZE, GFP_KERNEL);
-       if (!prod_info)
-               return;
-
-        ret = usb_string(es58x_dev->udev, ES58X_PROD_INFO_IDX,
-                        prod_info, ES58X_PROD_INFO_SIZE);
-        if (ret < 0) {
+       prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+       if (!prod_info) {
                dev_warn(dev, "could not retrieve the product info string\n");
-               kfree(prod_info);
                return;
        }

diff --git a/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
b/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
index 62347ffa0214..a204aa5344a8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_sysfs.h
@@ -14,13 +14,6 @@
 /* USB descriptor index containing the product information string. */
 #define ES58X_PROD_INFO_IDX 6

-/* Maximum size for the USB information custom string. USB strings are
- * at most 127 characters and es58x devices only use ASCII (i.e. one
- * byte). Also, empirical observations show a maximum length of 83
- * bytes for the product information.
- */
-#define ES58X_PROD_INFO_SIZE (127 + 1)
-
 void es58x_create_file(struct device *dev);
 void es58x_remove_file(struct device *dev);

Re: [PATCH v2 3/3] can: etas_es58x: report the firmware version through ethtool

[Alan Stern]

On Sun, Nov 06, 2022 at 09:47:05PM +0900, Vincent MAILHOL wrote:
> I have two questions:
> 
> 1/ Can I still export and use usb_cache_string()? In other terms, does
> the first patch of this series still apply? This looks like the most
> convenient function to retrieve that custom string to me.

FWIW, that's okay with me.

> 2/ Is it a no-go to also populate ethtool_drvinfo::fw_version? Some
> users might look for the value in sysfs, while some might use ethtool.
> If the info is not available in ethtool, people might simply assume it
> is not available at all. So, I would like to provide both.

I don't see any reason not to do this.

Alan Stern

[PATCH v3 0/3] can: etas_es58x: report firmware, bootloader and hardware version

[Vincent Mailhol]

The goal of this series is to report the firmware version, the
bootloader version and the hardware revision of ETAS ES58x
devices.

These are already reported in the kernel log but this isn't best
practise. Remove the kernel log and instead export all these in
sysfs. In addition, the firmware version is also reported through
ethtool.


* Changelog *

v2 -> v3:

  * patch 2/3: do not spam the kernel log anymore with the product
    number. Instead parse the product information string, extract the
    firmware version, the bootloadar version and the hardware revision
    and export them through sysfs.

  * patch 2/3: rework the parsing in order not to need additional
    fields in struct es58x_parameters.

  * patch 3/3: only populate ethtool_drvinfo::fw_version because since
    commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo: populate
    drvinfo fields even if callback exits"), there is no need to
    populate ethtool_drvinfo::driver and ethtool_drvinfo::bus_info in
    the driver.

v1 -> v2:

  * was a single patch. It is now a series of three patches.
  * add a first new patch to export  usb_cache_string().
  * add a second new patch to apply usb_cache_string() to existing code.
  * add missing check on product info string to prevent a buffer overflow.
  * add comma on the last entry of struct es58x_parameters.

Vincent Mailhol (3):
  USB: core: export usb_cache_string()
  can: etas_es58x: export firmware, bootloader and hardware versions in
    sysfs
  can: etas_es58x: report firmware-version through ethtool

 drivers/net/can/usb/etas_es58x/Makefile      |   2 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c  |  70 ++----
 drivers/net/can/usb/etas_es58x/es58x_core.h  |  51 ++++
 drivers/net/can/usb/etas_es58x/es58x_sysfs.c | 231 +++++++++++++++++++
 drivers/usb/core/message.c                   |   1 +
 drivers/usb/core/usb.h                       |   1 -
 include/linux/usb.h                          |   1 +
 7 files changed, 309 insertions(+), 48 deletions(-)
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_sysfs.c

-- 
2.37.4

[PATCH v3 1/3] USB: core: export usb_cache_string()

[Vincent Mailhol]

usb_cache_string() can also be useful for the drivers so export it.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/usb/core/message.c | 1 +
 drivers/usb/core/usb.h     | 1 -
 include/linux/usb.h        | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 4d59d927ae3e..127fac1af676 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1037,6 +1037,7 @@ char *usb_cache_string(struct usb_device *udev, int index)
 	}
 	return smallbuf;
 }
+EXPORT_SYMBOL_GPL(usb_cache_string);
 
 /*
  * usb_get_device_descriptor - (re)reads the device descriptor (usbcore)
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..0eac7d4285d1 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -47,7 +47,6 @@ extern int usb_get_device_descriptor(struct usb_device *dev,
 extern int usb_set_isoch_delay(struct usb_device *dev);
 extern int usb_get_bos_descriptor(struct usb_device *dev);
 extern void usb_release_bos_descriptor(struct usb_device *dev);
-extern char *usb_cache_string(struct usb_device *udev, int index);
 extern int usb_set_configuration(struct usb_device *dev, int configuration);
 extern int usb_choose_configuration(struct usb_device *udev);
 extern int usb_generic_driver_probe(struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 9ff1ad4dfad1..d2d2f41052c0 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1829,6 +1829,7 @@ static inline int usb_get_ptm_status(struct usb_device *dev, void *data)
 
 extern int usb_string(struct usb_device *dev, int index,
 	char *buf, size_t size);
+extern char *usb_cache_string(struct usb_device *udev, int index);
 
 /* wrappers that also update important state inside usbcore */
 extern int usb_clear_halt(struct usb_device *dev, int pipe);
-- 
2.37.4

Re: [PATCH v3 1/3] USB: core: export usb_cache_string()

[Greg Kroah-Hartman]

On Sun, Nov 13, 2022 at 01:01:06PM +0900, Vincent Mailhol wrote:
> usb_cache_string() can also be useful for the drivers so export it.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/usb/core/message.c | 1 +
>  drivers/usb/core/usb.h     | 1 -
>  include/linux/usb.h        | 1 +
>  3 files changed, 2 insertions(+), 1 deletion(-)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v3 2/3] can: etas_es58x: export firmware, bootloader and hardware versions in sysfs

[Vincent Mailhol]

ES58x devices report below information in their usb product info
string:

  * the firmware version
  * the bootloader version
  * the hardware revision

Parse this string, store the results in struct es58x_dev and create
three new sysfs entries.

Those sysfs entries are not critical to use the device, if parsing
fails, print an informative log message and continue to probe the
device.

Now that these value are available in sysfs, no more need to print
them in the kernel log so remove es58x_get_product_info().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/Makefile      |   2 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c  |  49 +---
 drivers/net/can/usb/etas_es58x/es58x_core.h  |  51 ++++
 drivers/net/can/usb/etas_es58x/es58x_sysfs.c | 231 +++++++++++++++++++
 4 files changed, 286 insertions(+), 47 deletions(-)
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_sysfs.c

diff --git a/drivers/net/can/usb/etas_es58x/Makefile b/drivers/net/can/usb/etas_es58x/Makefile
index a129b4aa0215..8f37aa3e32d3 100644
--- a/drivers/net/can/usb/etas_es58x/Makefile
+++ b/drivers/net/can/usb/etas_es58x/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CAN_ETAS_ES58X) += etas_es58x.o
-etas_es58x-y = es58x_core.o es581_4.o es58x_fd.o
+etas_es58x-y = es58x_core.o es581_4.o es58x_fd.o es58x_sysfs.o
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 25f863b4f5f0..c5109117f8e6 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2119,48 +2119,6 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
 	}
 }
 
-/**
- * es58x_get_product_info() - Get the product information and print them.
- * @es58x_dev: ES58X device.
- *
- * Do a synchronous call to get the product information.
- *
- * Return: zero on success, errno when any error occurs.
- */
-static int es58x_get_product_info(struct es58x_device *es58x_dev)
-{
-	struct usb_device *udev = es58x_dev->udev;
-	const int es58x_prod_info_idx = 6;
-	/* Empirical tests show a prod_info length of maximum 83,
-	 * below should be more than enough.
-	 */
-	const size_t prod_info_len = 127;
-	char *prod_info;
-	int ret;
-
-	prod_info = kmalloc(prod_info_len, GFP_KERNEL);
-	if (!prod_info)
-		return -ENOMEM;
-
-	ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
-	if (ret < 0) {
-		dev_err(es58x_dev->dev,
-			"%s: Could not read the product info: %pe\n",
-			__func__, ERR_PTR(ret));
-		goto out_free;
-	}
-	if (ret >= prod_info_len - 1) {
-		dev_warn(es58x_dev->dev,
-			 "%s: Buffer is too small, result might be truncated\n",
-			 __func__);
-	}
-	dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
-
- out_free:
-	kfree(prod_info);
-	return ret < 0 ? ret : 0;
-}
-
 /**
  * es58x_init_es58x_dev() - Initialize the ES58X device.
  * @intf: USB interface.
@@ -2243,10 +2201,6 @@ static int es58x_probe(struct usb_interface *intf,
 	if (IS_ERR(es58x_dev))
 		return PTR_ERR(es58x_dev);
 
-	ret = es58x_get_product_info(es58x_dev);
-	if (ret)
-		return ret;
-
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
 		ret = es58x_init_netdev(es58x_dev, ch_idx);
 		if (ret) {
@@ -2255,6 +2209,8 @@ static int es58x_probe(struct usb_interface *intf,
 		}
 	}
 
+	es58x_create_file(es58x_dev->dev);
+
 	return ret;
 }
 
@@ -2272,6 +2228,7 @@ static void es58x_disconnect(struct usb_interface *intf)
 	dev_info(&intf->dev, "Disconnecting %s %s\n",
 		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
 
+	es58x_remove_file(es58x_dev->dev);
 	es58x_free_netdevs(es58x_dev);
 	es58x_free_urbs(es58x_dev);
 	usb_set_intfdata(intf, NULL);
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index 640fe0a1df63..d3756ff07da5 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -356,6 +356,45 @@ struct es58x_operators {
 	int (*get_timestamp)(struct es58x_device *es58x_dev);
 };
 
+/**
+ * struct es58x_sw_version - Version number of the firmware or the
+ *	bootloader.
+ * @major: Version major number, represented on two digits.
+ * @minor: Version minor number, represented on two digits.
+ * @revision: Version revision number, represented on two digits.
+ *
+ * The firmware and the bootloader share the same format: "xx.xx.xx"
+ * where 'x' is a digit. Both can be retrieved from the product
+ * information string.
+ *
+ * If @major, @minor and @revision are all zero, the product string
+ * could not be parsed and the version number is invalid.
+ */
+struct es58x_sw_version {
+	u8 major;
+	u8 minor;
+	u8 revision;
+};
+
+/**
+ * struct es58x_hw_revision - Hardware revision number.
+ * @letter: Revision letter.
+ * @major: Version major number, represented on three digits.
+ * @minor: Version minor number, represented on three digits.
+ *
+ * The hardware revision uses its own format: "axxx/xxx" where 'a' is
+ * a letter and 'x' a digit. It can be retrieved from the product
+ * information string.
+ *
+ * If @letter, @major and @minor are all zero, the product string
+ * could not be parsed and the hardware revision number is invalid.
+ */
+struct es58x_hw_revision {
+	char letter;
+	u16 major;
+	u16 minor;
+};
+
 /**
  * struct es58x_device - All information specific to an ES58X device.
  * @dev: Device information.
@@ -373,6 +412,9 @@ struct es58x_operators {
  *	queue wake/stop logic should prevent this URB from getting
  *	empty. Please refer to es58x_get_tx_urb() for more details.
  * @tx_urbs_idle_cnt: number of urbs in @tx_urbs_idle.
+ * @firmware_version: The firmware version number.
+ * @bootloader_version: The bootloader version number.
+ * @hardware_revision: The hardware revision number.
  * @ktime_req_ns: kernel timestamp when es58x_set_realtime_diff_ns()
  *	was called.
  * @realtime_diff_ns: difference in nanoseconds between the clocks of
@@ -408,6 +450,10 @@ struct es58x_device {
 	struct usb_anchor tx_urbs_idle;
 	atomic_t tx_urbs_idle_cnt;
 
+	struct es58x_sw_version firmware_version;
+	struct es58x_sw_version bootloader_version;
+	struct es58x_hw_revision hardware_revision;
+
 	u64 ktime_req_ns;
 	s64 realtime_diff_ns;
 
@@ -674,6 +720,7 @@ static inline enum es58x_flag es58x_get_flags(const struct sk_buff *skb)
 	return es58x_flags;
 }
 
+/* Prototypes from es58x_core.c. */
 int es58x_can_get_echo_skb(struct net_device *netdev, u32 packet_idx,
 			   u64 *tstamps, unsigned int pkts);
 int es58x_tx_ack_msg(struct net_device *netdev, u16 tx_free_entries,
@@ -691,6 +738,10 @@ int es58x_rx_cmd_ret_u32(struct net_device *netdev,
 int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
 		   const void *msg, u16 cmd_len, int channel_idx);
 
+/* Prototypes from es58x_sysfs.c. */
+void es58x_create_file(struct device *dev);
+void es58x_remove_file(struct device *dev);
+
 extern const struct es58x_parameters es581_4_param;
 extern const struct es58x_operators es581_4_ops;
 
diff --git a/drivers/net/can/usb/etas_es58x/es58x_sysfs.c b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
new file mode 100644
index 000000000000..db1d57b6cda5
--- /dev/null
+++ b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces.
+ *
+ * File es58x_sysfs.c: parse the product information string and
+ * populate sysfs.
+ *
+ * Copyright (c) 2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/string.h>
+#include <linux/usb.h>
+
+#include "es58x_core.h"
+
+/* USB descriptor index containing the product information string. */
+#define ES58X_PROD_INFO_IDX 6
+
+/**
+ * es58x_parse_sw_version() - Extract boot loader or firmware version.
+ * @es58x_dev: ES58X device.
+ * @prod_info: USB custom string returned by the device.
+ * @prefix: Select which information should be parsed. Set it to "FW"
+ *	to parse the firmware version or to "BL" to parse the
+ *	bootloader version.
+ *
+ * The @prod_info string contains the firmware and the bootloader
+ * version number all prefixed by a magic string and concatenated with
+ * other numbers. Depending on the device, the firmware (bootloader)
+ * format is either "FW_Vxx.xx.xx" ("BL_Vxx.xx.xx") or "FW:xx.xx.xx"
+ * ("BL:xx.xx.xx") where 'x' represents a digit. @prod_info must
+ * contains the common part of those prefixes: "FW" or "BL".
+ *
+ * Parse @prod_info and store the version number in
+ * &es58x_dev.firmware_version or &es58x_dev.bootloader_version
+ * according to @prefix value.
+ *
+ * Return: zero on success, -EINVAL if @prefix contains an invalid
+ *	value and -EBADMSG if @prod_info could not be parsed.
+ */
+static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
+				  const char *prod_info, const char *prefix)
+{
+	struct es58x_sw_version *version;
+	int major, minor, revision;
+
+	if (!strcmp(prefix, "FW"))
+		version = &es58x_dev->firmware_version;
+	else if (!strcmp(prefix, "BL"))
+		version = &es58x_dev->bootloader_version;
+	else
+		return -EINVAL;
+
+	/* Go to prefix */
+	prod_info = strstr(prod_info, prefix);
+	if (!prod_info)
+		return -EBADMSG;
+	/* Go to beginning of the version number */
+	while (!isdigit(*prod_info)) {
+		prod_info++;
+		if (!*prod_info)
+			return -EBADMSG;
+	}
+
+	if (sscanf(prod_info, "%2u.%2u.%2u", &major, &minor, &revision) != 3)
+		return -EBADMSG;
+
+	version->major = major;
+	version->minor = minor;
+	version->revision = revision;
+
+	return 0;
+}
+
+/**
+ * firmware_version_show() - Emit the firmware version in sysfs.
+ * @dev: Device.
+ * @attr: Device attribute, not used.
+ * @buf: Where to write the firmware version.
+ *
+ * Return: number of bytes written.
+ */
+static ssize_t firmware_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+	struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
+
+	return sysfs_emit(buf, "%02u.%02u.%02u\n",
+			  fw_ver->major, fw_ver->minor, fw_ver->revision);
+}
+static DEVICE_ATTR_RO(firmware_version);
+
+/**
+ * bootloader_version_show() - Emit the bootloader version in sysfs.
+ * @dev: Device.
+ * @attr: Device attribute, not used.
+ * @buf: Where to write the bootloader version.
+ *
+ * Return: number of bytes written.
+ */
+static ssize_t bootloader_version_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+	struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
+
+	return sysfs_emit(buf, "%02u.%02u.%02u\n",
+			  bl_ver->major, bl_ver->minor, bl_ver->revision);
+}
+static DEVICE_ATTR_RO(bootloader_version);
+
+/**
+ * es58x_parse_hw_rev() - Extract hardware revision number
+ * @es58x_dev: ES58X device.
+ * @prod_info: USB custom string returned by the device.
+ *
+ * @prod_info contains the hardware revision prefixed by a magic
+ * string and conquenated together with other numbers. Depending on
+ * the device, the hardware revision format is either
+ * "HW_VER:axxx/xxx" or "HR:axxx/xxx" where 'a' represents a letter
+ * and 'x' a digit.
+ *
+ * Parse @prod_info and store the hardware revision number in
+ * &es58x_dev.hardware_revision.
+ *
+ * Return: zero on success, -EBADMSG if @prod_info could not be
+ *	parsed.
+ */
+static int es58x_parse_hw_rev(struct es58x_device *es58x_dev,
+			      const char *prod_info)
+{
+	char letter;
+	int major, minor;
+
+	/* The only occurrence of 'H' is in the hardware revision prefix. */
+	prod_info = strchr(prod_info, 'H');
+	if (!prod_info)
+		return -EBADMSG;
+	/* Go to beginning of the hardware revision */
+	prod_info = strchr(prod_info, ':');
+	if (!prod_info)
+		return -EBADMSG;
+	prod_info++;
+
+	if (sscanf(prod_info, "%c%3u/%3u", &letter, &major, &minor) != 3)
+		return -EBADMSG;
+
+	es58x_dev->hardware_revision.letter = letter;
+	es58x_dev->hardware_revision.major = major;
+	es58x_dev->hardware_revision.minor = minor;
+
+	return 0;
+}
+
+/**
+ * hardware_revision_show() - Emit the hardware revision in sysfs.
+ * @dev: Device.
+ * @attr: Device attribute, not used.
+ * @buf: Where to write the hardware revision.
+ *
+ * Return: number of bytes written.
+ */
+static ssize_t hardware_revision_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+	struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
+
+	return sysfs_emit(buf, "%c%03u/%03u\n",
+			  hw_rev->letter, hw_rev->major, hw_rev->minor);
+}
+static DEVICE_ATTR_RO(hardware_revision);
+
+/**
+ * es58x_create_file() - Expose the product information through sysfs.
+ * @dev: Device.
+ *
+ * Retrieve the product information string and parse it to extract the
+ * firmware version, the bootloader version and the hardware
+ * revision. Then create three sysfs entries.
+ *
+ * If the parsing fails, simply emit an informative log message and
+ * continue because these files are not critical for the driver to
+ * operate.
+ */
+void es58x_create_file(struct device *dev)
+{
+	struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+	char *prod_info;
+
+	prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+	if (!prod_info) {
+		dev_warn(dev, "could not retrieve the product info string\n");
+		return;
+	}
+
+	if (!es58x_parse_sw_version(es58x_dev, prod_info, "FW"))
+		device_create_file(dev, &dev_attr_firmware_version);
+	else
+		dev_info(dev, "parsing error: can not create firmware_version sysfs entry\n");
+
+	if (!es58x_parse_sw_version(es58x_dev, prod_info, "BL"))
+		device_create_file(dev, &dev_attr_bootloader_version);
+	else
+		dev_info(dev, "parsing error: can not create bootloader_version sysfs entry\n");
+
+	if (!es58x_parse_hw_rev(es58x_dev, prod_info))
+		device_create_file(dev, &dev_attr_hardware_revision);
+	else
+		dev_info(dev, "parsing error: can not create hardware_revision sysfs entry\n");
+
+	kfree(prod_info);
+}
+
+/**
+ * es58x_remove_file() - Remove sysfs files created by es58x_create_file().
+ * @dev: Device.
+ *
+ * device_remove_file() checks if the sysfs file exists before trying
+ * to remove it so it is safe to call it even if these files were not
+ * created by es58x_create_file() due of a parsing error.
+ */
+void es58x_remove_file(struct device *dev)
+{
+	device_remove_file(dev, &dev_attr_firmware_version);
+	device_remove_file(dev, &dev_attr_bootloader_version);
+	device_remove_file(dev, &dev_attr_hardware_revision);
+}
-- 
2.37.4

Re: [PATCH v3 2/3] can: etas_es58x: export firmware, bootloader and hardware versions in sysfs

[Saeed Mahameed]

On 13 Nov 13:01, Vincent Mailhol wrote:
>ES58x devices report below information in their usb product info
>string:
>
>  * the firmware version
>  * the bootloader version
>  * the hardware revision
>
>Parse this string, store the results in struct es58x_dev and create
>three new sysfs entries.
>

will this be the /sys/class/net/XXX sysfs  ?

We try to avoid adding device specific entries in there,

Couldn't you just squeeze the firmware and hw version into the 
ethtool->drvinfo->fw_version

something like: 
fw_version: %3u.%3u.%3u (%c.%3u.%3u)

and bootloader into ethtool->drvinfo->erom_version: 
  * @erom_version: Expansion ROM version string; may be an empty string

Re: [PATCH v3 2/3] can: etas_es58x: export firmware, bootloader and hardware versions in sysfs

[Vincent MAILHOL]

On Wed. 16 Nov. 2022 at 07:50, Saeed Mahameed <saeed@kernel.org> wrote:
> On 13 Nov 13:01, Vincent Mailhol wrote:
> >ES58x devices report below information in their usb product info
> >string:
> >
> >  * the firmware version
> >  * the bootloader version
> >  * the hardware revision
> >
> >Parse this string, store the results in struct es58x_dev and create
> >three new sysfs entries.
> >
>
> will this be the /sys/class/net/XXX sysfs  ?

I am dropping the idea of using sysfs and I am now considering using
devlink following Andrew's message:
https://lore.kernel.org/linux-can/Y3Ef4K5lbilY3EQT@lunn.ch/

> We try to avoid adding device specific entries in there,
>
> Couldn't you just squeeze the firmware and hw version into the
> ethtool->drvinfo->fw_version
>
> something like:
> fw_version: %3u.%3u.%3u (%c.%3u.%3u)

This looks like a hack. There is no way for the end user to know, just
from the ethtool output, what these in brackets values would mean.

> and bootloader into ethtool->drvinfo->erom_version:
>   * @erom_version: Expansion ROM version string; may be an empty string

Same. I considered doing this in the early draft of this series and
dropped the idea because an expansion ROM and a boot loader are two
things different.

I will continue to study devlink and only use the drvinfo only for the
firmware version.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 2/3] can: etas_es58x: export firmware, bootloader and hardware versions in sysfs

[Saeed Mahameed]

On 16 Nov 09:36, Vincent MAILHOL wrote:
>On Wed. 16 Nov. 2022 at 07:50, Saeed Mahameed <saeed@kernel.org> wrote:
>> On 13 Nov 13:01, Vincent Mailhol wrote:
>> >ES58x devices report below information in their usb product info
>> >string:
>> >
>> >  * the firmware version
>> >  * the bootloader version
>> >  * the hardware revision
>> >
>> >Parse this string, store the results in struct es58x_dev and create
>> >three new sysfs entries.
>> >
>>
>> will this be the /sys/class/net/XXX sysfs  ?
>
>I am dropping the idea of using sysfs and I am now considering using
>devlink following Andrew's message:
>https://lore.kernel.org/linux-can/Y3Ef4K5lbilY3EQT@lunn.ch/
>

+1

>> We try to avoid adding device specific entries in there,
>>
>> Couldn't you just squeeze the firmware and hw version into the
>> ethtool->drvinfo->fw_version
>>
>> something like:
>> fw_version: %3u.%3u.%3u (%c.%3u.%3u)
>
>This looks like a hack. There is no way for the end user to know, just
>from the ethtool output, what these in brackets values would mean.

it's not, there is no well defined format for what to put in the version,
as long as it clearly describes what FW is currently running.
at the end of the day, it's just a text you copy&paste when you contact
customer support.

>
>> and bootloader into ethtool->drvinfo->erom_version:
>>   * @erom_version: Expansion ROM version string; may be an empty string
>
>Same. I considered doing this in the early draft of this series and
>dropped the idea because an expansion ROM and a boot loader are two
>things different.
>
>I will continue to study devlink and only use the drvinfo only for the
>firmware version.
>

100% devlink is a great options.

[PATCH v3 3/3] can: etas_es58x: report firmware-version through ethtool

[Vincent Mailhol]

Implement ethtool_ops::get_drvinfo() in order to report the firmware
version.

Firmware version 0.0.0 has a special meaning and just means that we
could not parse the product information string. In such case, do
nothing (i.e. leave the .fw_version string empty).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

*N.B.* Drivers had to also fill ethtool_drvinfo::driver and
ethtool_drvinfo::bus_info. Starting this week, this is not needed
anymore because of commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo:
populate drvinfo fields even if callback exits").

  https://git.kernel.org/netdev/net-next/c/edaf5df22cb8
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index c5109117f8e6..a048e0d40c97 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -1978,7 +1978,28 @@ static const struct net_device_ops es58x_netdev_ops = {
 	.ndo_eth_ioctl = can_eth_ioctl_hwts,
 };
 
+/**
+ * es58x_get_drvinfo() - Get the firmware version.
+ * @netdev: CAN network device.
+ * @drvinfo: Driver information.
+ *
+ * Populate @drvinfo with the firmware version. The core will populate
+ * the rest.
+ */
+static void es58x_get_drvinfo(struct net_device *netdev,
+			      struct ethtool_drvinfo *drvinfo)
+{
+	struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
+	struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
+
+	if (fw_ver->major || fw_ver->minor || fw_ver->revision)
+		snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
+			 "%02u.%02u.%02u",
+			 fw_ver->major, fw_ver->minor, fw_ver->revision);
+}
+
 static const struct ethtool_ops es58x_ethtool_ops = {
+	.get_drvinfo = es58x_get_drvinfo,
 	.get_ts_info = can_ethtool_op_get_ts_info_hwts,
 };
 
-- 
2.37.4

Re: [PATCH v3 0/3] can: etas_es58x: report firmware, bootloader and hardware version

[Andrew Lunn]

On Sun, Nov 13, 2022 at 01:01:05PM +0900, Vincent Mailhol wrote:
> The goal of this series is to report the firmware version, the
> bootloader version and the hardware revision of ETAS ES58x
> devices.
> 
> These are already reported in the kernel log but this isn't best
> practise. Remove the kernel log and instead export all these in
> sysfs. In addition, the firmware version is also reported through
> ethtool.

Sorry to only comment on version 3, rather than version 1. I don't
normally look at CAN patches.

Have you considered using devlink?

https://www.kernel.org/doc/html/latest/networking/devlink/devlink-info.html

fw and asic.id would cover two of your properties. Maybe talk to Jiri
about the bootloader. It might make sense to add it is a new common
property, or to use a custom property.

devlink has the advantage of being a well defined, standardised API,
rather than just random, per device sys files.

There might also be other interesting features in devlink, once you
have basic support. Many Ethernet switch drivers use devlink regions
to dump all the registers, for example. Since there is a bootloader, i
assume the firmware is upgradeable? devlink supports that.

	  Andrew

Re: [PATCH v3 0/3] can: etas_es58x: report firmware, bootloader and hardware version

[Vincent MAILHOL]

On Mon. 14 Nov. 2022 at 02:03, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Nov 13, 2022 at 01:01:05PM +0900, Vincent Mailhol wrote:
> > The goal of this series is to report the firmware version, the
> > bootloader version and the hardware revision of ETAS ES58x
> > devices.
> >
> > These are already reported in the kernel log but this isn't best
> > practise. Remove the kernel log and instead export all these in
> > sysfs. In addition, the firmware version is also reported through
> > ethtool.
>
> Sorry to only comment on version 3, rather than version 1. I don't
> normally look at CAN patches.

Actually, I only started to CC linux-usb mailing from version 2.
Regardless, thanks a lot, this is a valuable feedback.

> Have you considered using devlink?
>
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-info.html

I have not thought about this (I simply did not know the existence of
this feature). A first quick look makes me think it is a good idea. I
will continue to investigate.

> fw and asic.id would cover two of your properties. Maybe talk to Jiri
> about the bootloader. It might make sense to add it is a new common
> property, or to use a custom property.

I will try to report the firmware version and the hardware version in
a first step and then see what we can do for the bootloader.

> devlink has the advantage of being a well defined, standardised API,
> rather than just random, per device sys files.

ACK.

> There might also be other interesting features in devlink, once you
> have basic support. Many Ethernet switch drivers use devlink regions
> to dump all the registers, for example.

I am aware of ethtool_drvinfo (which I implemented in the last patch
of this series to report the firmware version).
Do you have any reference of how to dump the other registers?

> Since there is a bootloader, i
> assume the firmware is upgradeable? devlink supports that.

True, it is upgradeable, however, I do not have an environment to test
for upgrades so there are no plans right now to develop an upgrade
feature.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 0/3] can: etas_es58x: report firmware, bootloader and hardware version

[Andrew Lunn]

> Do you have any reference of how to dump the other registers?

Have a look at drivers/net/dsa/mv88e6xxx/devlink.c, all the code with
mv88e6xxx_region_. This ethernet switch chip has multiple banks of
registers, one per port of the switch, and two global. It also has a
few other tables which can be interesting to dump in their raw format.
There is also a user space tool to pritty print them.

    Andrew

[PATCH v4 0/6] can: etas_es58x: report firmware, bootloader and hardware version

[Vincent Mailhol]

The goal of this series is to report the firmware version, the
bootloader version and the hardware revision of ETAS ES58x
devices.

These are already reported in the kernel log but this isn't best
practice. Remove the kernel log and instead export all these through
devlink. In addition, the firmware version is also reported through
ethtool.

---
* Changelog *

v3 -> v4:

  * major rework to use devlink instead of sysfs following Andrew's
    comment.

  * split the series in 6 patches.

  * [PATCH 1/6] add Acked-by: Greg Kroah-Hartman

v2 -> v3:

  * patch 2/3: do not spam the kernel log anymore with the product
    number. Instead parse the product information string, extract the
    firmware version, the bootloadar version and the hardware revision
    and export them through sysfs.

  * patch 2/3: rework the parsing in order not to need additional
    fields in struct es58x_parameters.

  * patch 3/3: only populate ethtool_drvinfo::fw_version because since
    commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo: populate
    drvinfo fields even if callback exits"), there is no need to
    populate ethtool_drvinfo::driver and ethtool_drvinfo::bus_info in
    the driver.

v1 -> v2:

  * was a single patch. It is now a series of three patches.
  * add a first new patch to export  usb_cache_string().
  * add a second new patch to apply usb_cache_string() to existing code.
  * add missing check on product info string to prevent a buffer overflow.
  * add comma on the last entry of struct es58x_parameters.

Vincent Mailhol (6):
  USB: core: export usb_cache_string()
  can: etas_es58x: add devlink support
  can: etas_es58x: export product information through
    devlink_ops::info_get()
  can: etas_es58x: remove es58x_get_product_info()
  can: etas_es58x: report the firmware version through ethtool
  Documentation: devlink: add devlink documentation for the etas_es58x
    driver

 .../networking/devlink/etas_es58x.rst         |  33 +++
 MAINTAINERS                                   |   1 +
 drivers/net/can/usb/Kconfig                   |   1 +
 drivers/net/can/usb/etas_es58x/Makefile       |   2 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c   |  85 +++----
 drivers/net/can/usb/etas_es58x/es58x_core.h   |  73 ++++++
 .../net/can/usb/etas_es58x/es58x_devlink.c    | 207 ++++++++++++++++++
 drivers/usb/core/message.c                    |   1 +
 drivers/usb/core/usb.h                        |   1 -
 include/linux/usb.h                           |   1 +
 10 files changed, 352 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/networking/devlink/etas_es58x.rst
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_devlink.c

-- 
2.37.4

[PATCH v4 1/6] USB: core: export usb_cache_string()

[Vincent Mailhol]

usb_cache_string() can also be useful for the drivers so export it.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
For reference, acked by Greg here:
https://lore.kernel.org/linux-usb/Y3zyCz5HbGdsxmRT@kroah.com/
---
 drivers/usb/core/message.c | 1 +
 drivers/usb/core/usb.h     | 1 -
 include/linux/usb.h        | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 4d59d927ae3e..127fac1af676 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1037,6 +1037,7 @@ char *usb_cache_string(struct usb_device *udev, int index)
 	}
 	return smallbuf;
 }
+EXPORT_SYMBOL_GPL(usb_cache_string);
 
 /*
  * usb_get_device_descriptor - (re)reads the device descriptor (usbcore)
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..0eac7d4285d1 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -47,7 +47,6 @@ extern int usb_get_device_descriptor(struct usb_device *dev,
 extern int usb_set_isoch_delay(struct usb_device *dev);
 extern int usb_get_bos_descriptor(struct usb_device *dev);
 extern void usb_release_bos_descriptor(struct usb_device *dev);
-extern char *usb_cache_string(struct usb_device *udev, int index);
 extern int usb_set_configuration(struct usb_device *dev, int configuration);
 extern int usb_choose_configuration(struct usb_device *udev);
 extern int usb_generic_driver_probe(struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 9ff1ad4dfad1..d2d2f41052c0 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1829,6 +1829,7 @@ static inline int usb_get_ptm_status(struct usb_device *dev, void *data)
 
 extern int usb_string(struct usb_device *dev, int index,
 	char *buf, size_t size);
+extern char *usb_cache_string(struct usb_device *udev, int index);
 
 /* wrappers that also update important state inside usbcore */
 extern int usb_clear_halt(struct usb_device *dev, int pipe);
-- 
2.37.4

[PATCH v4 2/6] can: etas_es58x: add devlink support

[Vincent Mailhol]

Add basic support for devlink. The callbacks of struct devlink_ops
will be implemented next.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/Kconfig                    |  1 +
 drivers/net/can/usb/etas_es58x/Makefile        |  2 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c    | 13 ++++++++++---
 drivers/net/can/usb/etas_es58x/es58x_core.h    |  6 ++++++
 drivers/net/can/usb/etas_es58x/es58x_devlink.c | 13 +++++++++++++
 5 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_devlink.c

diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index 8c6fea661530..445504ababce 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -30,6 +30,7 @@ config CAN_ESD_USB
 config CAN_ETAS_ES58X
 	tristate "ETAS ES58X CAN/USB interfaces"
 	select CRC16
+	select NET_DEVLINK
 	help
 	  This driver supports the ES581.4, ES582.1 and ES584.1 interfaces
 	  from ETAS GmbH (https://www.etas.com/en/products/es58x.php).
diff --git a/drivers/net/can/usb/etas_es58x/Makefile b/drivers/net/can/usb/etas_es58x/Makefile
index a129b4aa0215..d6667ebe259f 100644
--- a/drivers/net/can/usb/etas_es58x/Makefile
+++ b/drivers/net/can/usb/etas_es58x/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CAN_ETAS_ES58X) += etas_es58x.o
-etas_es58x-y = es58x_core.o es581_4.o es58x_fd.o
+etas_es58x-y = es58x_core.o es58x_devlink.o es581_4.o es58x_fd.o
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 4c97102202bf..c6e598e4800c 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/usb.h>
+#include <net/devlink.h>
 
 #include "es58x_core.h"
 
@@ -2174,6 +2175,7 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
 {
 	struct device *dev = &intf->dev;
 	struct es58x_device *es58x_dev;
+	struct devlink *devlink;
 	const struct es58x_parameters *param;
 	const struct es58x_operators *ops;
 	struct usb_device *udev = interface_to_usbdev(intf);
@@ -2196,11 +2198,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
 		ops = &es581_4_ops;
 	}
 
-	es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
-				 GFP_KERNEL);
-	if (!es58x_dev)
+	devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
+				dev);
+	if (!devlink)
 		return ERR_PTR(-ENOMEM);
 
+	es58x_dev = devlink_priv(devlink);
 	es58x_dev->param = param;
 	es58x_dev->ops = ops;
 	es58x_dev->dev = dev;
@@ -2247,6 +2250,8 @@ static int es58x_probe(struct usb_interface *intf,
 	if (ret)
 		return ret;
 
+	devlink_register(priv_to_devlink(es58x_dev));
+
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
 		ret = es58x_init_netdev(es58x_dev, ch_idx);
 		if (ret) {
@@ -2272,8 +2277,10 @@ static void es58x_disconnect(struct usb_interface *intf)
 	dev_info(&intf->dev, "Disconnecting %s %s\n",
 		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
 
+	devlink_unregister(priv_to_devlink(es58x_dev));
 	es58x_free_netdevs(es58x_dev);
 	es58x_free_urbs(es58x_dev);
+	devlink_free(priv_to_devlink(es58x_dev));
 	usb_set_intfdata(intf, NULL);
 }
 
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index 4a082fd69e6f..bf24375580e5 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -674,6 +674,7 @@ static inline enum es58x_flag es58x_get_flags(const struct sk_buff *skb)
 	return es58x_flags;
 }
 
+/* es58x_core.c. */
 int es58x_can_get_echo_skb(struct net_device *netdev, u32 packet_idx,
 			   u64 *tstamps, unsigned int pkts);
 int es58x_tx_ack_msg(struct net_device *netdev, u16 tx_free_entries,
@@ -691,9 +692,14 @@ int es58x_rx_cmd_ret_u32(struct net_device *netdev,
 int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
 		   const void *msg, u16 cmd_len, int channel_idx);
 
+/* es58x_devlink.c. */
+extern const struct devlink_ops es58x_dl_ops;
+
+/* es581_4.c. */
 extern const struct es58x_parameters es581_4_param;
 extern const struct es58x_operators es581_4_ops;
 
+/* es58x_fd.c. */
 extern const struct es58x_parameters es58x_fd_param;
 extern const struct es58x_operators es58x_fd_ops;
 
diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
new file mode 100644
index 000000000000..af6ca7ada23f
--- /dev/null
+++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces.
+ *
+ * File es58x_devlink.c: report the product information using devlink.
+ *
+ * Copyright (c) 2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ */
+
+#include <net/devlink.h>
+
+const struct devlink_ops es58x_dl_ops = {
+};
-- 
2.37.4

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Andrew Lunn]

> @@ -2196,11 +2198,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
>  		ops = &es581_4_ops;
>  	}
>  
> -	es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
> -				 GFP_KERNEL);
> -	if (!es58x_dev)
> +	devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
> +				dev);
> +	if (!devlink)
>  		return ERR_PTR(-ENOMEM);
>  
> +	es58x_dev = devlink_priv(devlink);

That is 'interesting'. Makes me wonder about lifetimes of different
objects. Previously your es58x_dev structure would disappear when the
driver is released, or an explicit call to devm_kfree(). Now it
disappears when devlink_free() is called. Any danger of use after free
here?

USB devices always make me wonder about life times rules since they
are probably the mode dynamic sort of device the kernel has the
handle, them just abruptly disappearing.

>  	es58x_dev->param = param;
>  	es58x_dev->ops = ops;
>  	es58x_dev->dev = dev;
> @@ -2247,6 +2250,8 @@ static int es58x_probe(struct usb_interface *intf,
>  	if (ret)
>  		return ret;
>  
> +	devlink_register(priv_to_devlink(es58x_dev));
> +
>  	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
>  		ret = es58x_init_netdev(es58x_dev, ch_idx);
>  		if (ret) {
> @@ -2272,8 +2277,10 @@ static void es58x_disconnect(struct usb_interface *intf)
>  	dev_info(&intf->dev, "Disconnecting %s %s\n",
>  		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
>  
> +	devlink_unregister(priv_to_devlink(es58x_dev));
>  	es58x_free_netdevs(es58x_dev);
>  	es58x_free_urbs(es58x_dev);
> +	devlink_free(priv_to_devlink(es58x_dev));
>  	usb_set_intfdata(intf, NULL);

Should devlink_free() be after usb_set_inftdata()?

       Andrew

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Vincent MAILHOL]

On Tue. 27 Nov. 2022 at 01:51, Andrew Lunn <andrew@lunn.ch> wrote:
> > @@ -2196,11 +2198,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
> >               ops = &es581_4_ops;
> >       }
> >
> > -     es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
> > -                              GFP_KERNEL);
> > -     if (!es58x_dev)
> > +     devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
> > +                             dev);
> > +     if (!devlink)
> >               return ERR_PTR(-ENOMEM);
> >
> > +     es58x_dev = devlink_priv(devlink);
>
> That is 'interesting'.

Another interesting thing I found is:
https://elixir.bootlin.com/linux/v6.1-rc6/source/drivers/net/ethernet/intel/ice/ice_devlink.c#L866

Because devlink does not have an equivalent to devm_kzalloc(), that
driver uses devm_add_action_or_reset() instead. But any other drivers
will call devlink_free() in their disconnect function. So here, I just
followed the trend.

> Makes me wonder about lifetimes of different
> objects. Previously your es58x_dev structure would disappear when the
> driver is released, or an explicit call to devm_kfree(). Now it
> disappears when devlink_free() is called.

Even before that, this driver used to release es58x_dev in its
disconnect() function. I changed it to use devm_kzalloc() last year
after discovering its existence.
  https://git.kernel.org/torvalds/linux/c/6bde4c7fd845

>Any danger of use after free here?

devlink_alloc() allocates one continuous block for both the devlink
and the device priv (struct es58x_dev here):
  https://elixir.bootlin.com/linux/v6.1-rc6/source/net/core/devlink.c#L9629

So calling devlink_free() also releases struct es58x_dev.

> USB devices always make me wonder about life times rules since they
> are probably the mode dynamic sort of device the kernel has the
> handle, them just abruptly disappearing.
>
> >       es58x_dev->param = param;
> >       es58x_dev->ops = ops;
> >       es58x_dev->dev = dev;
> > @@ -2247,6 +2250,8 @@ static int es58x_probe(struct usb_interface *intf,
> >       if (ret)
> >               return ret;
> >
> > +     devlink_register(priv_to_devlink(es58x_dev));
> > +
> >       for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
> >               ret = es58x_init_netdev(es58x_dev, ch_idx);
> >               if (ret) {
> > @@ -2272,8 +2277,10 @@ static void es58x_disconnect(struct usb_interface *intf)
> >       dev_info(&intf->dev, "Disconnecting %s %s\n",
> >                es58x_dev->udev->manufacturer, es58x_dev->udev->product);
> >
> > +     devlink_unregister(priv_to_devlink(es58x_dev));
> >       es58x_free_netdevs(es58x_dev);
> >       es58x_free_urbs(es58x_dev);
> > +     devlink_free(priv_to_devlink(es58x_dev));
> >       usb_set_intfdata(intf, NULL);
>
> Should devlink_free() be after usb_set_inftdata()?

A look at
  $ git grep -W "usb_set_intfdata(.*NULL)"

shows that the two patterns (freeing before or after
usb_set_intfdata()) coexist.

You are raising an important question here. usb_set_intfdata() does
not have documentation that freeing before it is risky. And the
documentation of usb_driver::disconnect says that:
  "@disconnect: Called when the interface is no longer accessible,
   usually because its device has been (or is being) disconnected
   or the driver module is being unloaded."
  Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130

So the interface no longer being accessible makes me assume that the
order does not matter. If it indeed matters, then this is a foot gun
and there is some clean-up work waiting for us on many drivers.

@Greg, any thoughts on whether or not the order of usb_set_intfdata()
and resource freeing matters or not?


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Alan Stern]

On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote:
> > Should devlink_free() be after usb_set_inftdata()?
> 
> A look at
>   $ git grep -W "usb_set_intfdata(.*NULL)"
> 
> shows that the two patterns (freeing before or after
> usb_set_intfdata()) coexist.
> 
> You are raising an important question here. usb_set_intfdata() does
> not have documentation that freeing before it is risky. And the
> documentation of usb_driver::disconnect says that:
>   "@disconnect: Called when the interface is no longer accessible,
>    usually because its device has been (or is being) disconnected
>    or the driver module is being unloaded."
>   Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130
> 
> So the interface no longer being accessible makes me assume that the
> order does not matter. If it indeed matters, then this is a foot gun
> and there is some clean-up work waiting for us on many drivers.
> 
> @Greg, any thoughts on whether or not the order of usb_set_intfdata()
> and resource freeing matters or not?

In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the 
USB core does it for them after the ->disconnect() callback returns.

But if a driver does make the call, it should be careful to ensure that 
the call happens _after_ the driver is finished using the interface-data 
pointer.  For example, after all outstanding URBs have completed, if the
completion handlers will need to call usb_get_intfdata().

Remember, the interface-data pointer is owned by the driver.  Nothing 
else in the kernel uses it.  So the driver merely has to be careful not 
to clear the pointer while it is still using it.

Alan Stern

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Vincent MAILHOL]

On Mon. 28 Nov. 2022 at 00:41, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote:
> > > Should devlink_free() be after usb_set_inftdata()?
> >
> > A look at
> >   $ git grep -W "usb_set_intfdata(.*NULL)"
> >
> > shows that the two patterns (freeing before or after
> > usb_set_intfdata()) coexist.
> >
> > You are raising an important question here. usb_set_intfdata() does
> > not have documentation that freeing before it is risky. And the
> > documentation of usb_driver::disconnect says that:
> >   "@disconnect: Called when the interface is no longer accessible,
> >    usually because its device has been (or is being) disconnected
> >    or the driver module is being unloaded."
> >   Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130
> >
> > So the interface no longer being accessible makes me assume that the
> > order does not matter. If it indeed matters, then this is a foot gun
> > and there is some clean-up work waiting for us on many drivers.
> >
> > @Greg, any thoughts on whether or not the order of usb_set_intfdata()
> > and resource freeing matters or not?
>
> In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the
> USB core does it for them after the ->disconnect() callback returns.

Interesting. This fact is widely unknown, cf:
  $ git grep "usb_set_intfdata(.*NULL)" | wc -l
  215

I will do some clean-up later on, at least for the CAN USB drivers.

> But if a driver does make the call, it should be careful to ensure that
> the call happens _after_ the driver is finished using the interface-data
> pointer.  For example, after all outstanding URBs have completed, if the
> completion handlers will need to call usb_get_intfdata().

ACK. I understand that it should be called *after* the completion of
any ongoing task.

My question was more on:

        devlink_free(priv_to_devlink(es58x_dev));
        usb_set_intfdata(intf, NULL);

VS.

        usb_set_intfdata(intf, NULL);
        devlink_free(priv_to_devlink(es58x_dev));

From your comments, I understand that both are fine.

> Remember, the interface-data pointer is owned by the driver.  Nothing
> else in the kernel uses it.  So the driver merely has to be careful not
> to clear the pointer while it is still using it.

Thanks for your comments!


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Vincent MAILHOL]

On Mon. 28 Nov. 2022 at 10:34, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Mon. 28 Nov. 2022 at 00:41, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote:
> > > > Should devlink_free() be after usb_set_inftdata()?
> > >
> > > A look at
> > >   $ git grep -W "usb_set_intfdata(.*NULL)"
> > >
> > > shows that the two patterns (freeing before or after
> > > usb_set_intfdata()) coexist.
> > >
> > > You are raising an important question here. usb_set_intfdata() does
> > > not have documentation that freeing before it is risky. And the
> > > documentation of usb_driver::disconnect says that:
> > >   "@disconnect: Called when the interface is no longer accessible,
> > >    usually because its device has been (or is being) disconnected
> > >    or the driver module is being unloaded."
> > >   Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130
> > >
> > > So the interface no longer being accessible makes me assume that the
> > > order does not matter. If it indeed matters, then this is a foot gun
> > > and there is some clean-up work waiting for us on many drivers.
> > >
> > > @Greg, any thoughts on whether or not the order of usb_set_intfdata()
> > > and resource freeing matters or not?
> >
> > In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the
> > USB core does it for them after the ->disconnect() callback returns.
>
> Interesting. This fact is widely unknown, cf:
>   $ git grep "usb_set_intfdata(.*NULL)" | wc -l
>   215
>
> I will do some clean-up later on, at least for the CAN USB drivers.
>
> > But if a driver does make the call, it should be careful to ensure that
> > the call happens _after_ the driver is finished using the interface-data
> > pointer.  For example, after all outstanding URBs have completed, if the
> > completion handlers will need to call usb_get_intfdata().
>
> ACK. I understand that it should be called *after* the completion of
> any ongoing task.
>
> My question was more on:
>
>         devlink_free(priv_to_devlink(es58x_dev));
>         usb_set_intfdata(intf, NULL);
>
> VS.
>
>         usb_set_intfdata(intf, NULL);
>         devlink_free(priv_to_devlink(es58x_dev));
>
> From your comments, I understand that both are fine.

Do we agree that the usb-skeleton is doing it wrong?
  https://elixir.bootlin.com/linux/latest/source/drivers/usb/usb-skeleton.c#L567
usb_set_intfdata(interface, NULL) is called before deregistering the
interface and terminating the outstanding URBs!

> > Remember, the interface-data pointer is owned by the driver.  Nothing
> > else in the kernel uses it.  So the driver merely has to be careful not
> > to clear the pointer while it is still using it.
>
> Thanks for your comments!
>
>
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Alan Stern]

On Mon, Nov 28, 2022 at 02:32:23PM +0900, Vincent MAILHOL wrote:
> On Mon. 28 Nov. 2022 at 10:34, Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > On Mon. 28 Nov. 2022 at 00:41, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote:
> > > > > Should devlink_free() be after usb_set_inftdata()?
> > > >
> > > > A look at
> > > >   $ git grep -W "usb_set_intfdata(.*NULL)"
> > > >
> > > > shows that the two patterns (freeing before or after
> > > > usb_set_intfdata()) coexist.
> > > >
> > > > You are raising an important question here. usb_set_intfdata() does
> > > > not have documentation that freeing before it is risky. And the
> > > > documentation of usb_driver::disconnect says that:
> > > >   "@disconnect: Called when the interface is no longer accessible,
> > > >    usually because its device has been (or is being) disconnected
> > > >    or the driver module is being unloaded."
> > > >   Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130
> > > >
> > > > So the interface no longer being accessible makes me assume that the
> > > > order does not matter. If it indeed matters, then this is a foot gun
> > > > and there is some clean-up work waiting for us on many drivers.
> > > >
> > > > @Greg, any thoughts on whether or not the order of usb_set_intfdata()
> > > > and resource freeing matters or not?
> > >
> > > In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the
> > > USB core does it for them after the ->disconnect() callback returns.
> >
> > Interesting. This fact is widely unknown, cf:
> >   $ git grep "usb_set_intfdata(.*NULL)" | wc -l
> >   215
> >
> > I will do some clean-up later on, at least for the CAN USB drivers.
> >
> > > But if a driver does make the call, it should be careful to ensure that
> > > the call happens _after_ the driver is finished using the interface-data
> > > pointer.  For example, after all outstanding URBs have completed, if the
> > > completion handlers will need to call usb_get_intfdata().
> >
> > ACK. I understand that it should be called *after* the completion of
> > any ongoing task.
> >
> > My question was more on:
> >
> >         devlink_free(priv_to_devlink(es58x_dev));
> >         usb_set_intfdata(intf, NULL);
> >
> > VS.
> >
> >         usb_set_intfdata(intf, NULL);
> >         devlink_free(priv_to_devlink(es58x_dev));
> >
> > From your comments, I understand that both are fine.
> 
> Do we agree that the usb-skeleton is doing it wrong?
>   https://elixir.bootlin.com/linux/latest/source/drivers/usb/usb-skeleton.c#L567
> usb_set_intfdata(interface, NULL) is called before deregistering the
> interface and terminating the outstanding URBs!

Going through the usb-skeleton.c source code, you will find that 
usb_get_intfdata() is called from only a few routines:

	skel_open()
	skel_disconnect()
	skel_suspend()
	skel_pre_reset()
	skel_post_reset()

Of those, all but the first are called only by the USB core and they are 
mutually exclusive with disconnect processing (except for 
skel_disconnect() itself, of course).  So they don't matter.

The first, skel_open(), can be called as a result of actions by the 
user, so the driver needs to ensure that this can't happen after it 
clears the interface-data pointer.  The user can open the device file at 
any time before the minor number is given back, so it is not proper to 
call usb_set_intfdata(interface, NULL) before usb_deregister_dev() -- 
but the driver does exactly this!

(Well, it's not quite that bad.  skel_open() does check whether the 
interface-data pointer value it gets from usb_get_intfdata() is NULL.  
But it's still a race.)

So yes, the current code is wrong.  And in fact, it will still be wrong 
even after the usb_set_intfdata(interface, NULL) line is removed, 
because there is no synchronization between skel_open() and 
skel_disconnect().  It is possible for skel_disconnect() to run to 
completion and the USB core to clear the interface-data pointer all 
while skel_open() is running.  The driver needs a static private mutex 
to synchronize opens with unregistrations.  (This is a general 
phenomenon, true of all drivers that have a user interface such as a 
device file.)

The driver _does_ have a per-instance mutex, dev->io_mutex, to 
synchronize I/O with disconnects.  But that's separate from 
synchronizing opens with unregistrations, because at open time the 
driver doesn't yet know the address of the private data structure or 
even if the structure is still allocated.  So obviously it can't use a 
mutex that is embedded within the private data structure for this 
purpose.

Alan Stern

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Vincent MAILHOL]

On Tue. 29 Nov. 2022 at 00:50, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, Nov 28, 2022 at 02:32:23PM +0900, Vincent MAILHOL wrote:
> > On Mon. 28 Nov. 2022 at 10:34, Vincent MAILHOL
> > <mailhol.vincent@wanadoo.fr> wrote:
> > > On Mon. 28 Nov. 2022 at 00:41, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote:
> > > > > > Should devlink_free() be after usb_set_inftdata()?
> > > > >
> > > > > A look at
> > > > >   $ git grep -W "usb_set_intfdata(.*NULL)"
> > > > >
> > > > > shows that the two patterns (freeing before or after
> > > > > usb_set_intfdata()) coexist.
> > > > >
> > > > > You are raising an important question here. usb_set_intfdata() does
> > > > > not have documentation that freeing before it is risky. And the
> > > > > documentation of usb_driver::disconnect says that:
> > > > >   "@disconnect: Called when the interface is no longer accessible,
> > > > >    usually because its device has been (or is being) disconnected
> > > > >    or the driver module is being unloaded."
> > > > >   Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130
> > > > >
> > > > > So the interface no longer being accessible makes me assume that the
> > > > > order does not matter. If it indeed matters, then this is a foot gun
> > > > > and there is some clean-up work waiting for us on many drivers.
> > > > >
> > > > > @Greg, any thoughts on whether or not the order of usb_set_intfdata()
> > > > > and resource freeing matters or not?
> > > >
> > > > In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the
> > > > USB core does it for them after the ->disconnect() callback returns.
> > >
> > > Interesting. This fact is widely unknown, cf:
> > >   $ git grep "usb_set_intfdata(.*NULL)" | wc -l
> > >   215
> > >
> > > I will do some clean-up later on, at least for the CAN USB drivers.
> > >
> > > > But if a driver does make the call, it should be careful to ensure that
> > > > the call happens _after_ the driver is finished using the interface-data
> > > > pointer.  For example, after all outstanding URBs have completed, if the
> > > > completion handlers will need to call usb_get_intfdata().
> > >
> > > ACK. I understand that it should be called *after* the completion of
> > > any ongoing task.
> > >
> > > My question was more on:
> > >
> > >         devlink_free(priv_to_devlink(es58x_dev));
> > >         usb_set_intfdata(intf, NULL);
> > >
> > > VS.
> > >
> > >         usb_set_intfdata(intf, NULL);
> > >         devlink_free(priv_to_devlink(es58x_dev));
> > >
> > > From your comments, I understand that both are fine.
> >
> > Do we agree that the usb-skeleton is doing it wrong?
> >   https://elixir.bootlin.com/linux/latest/source/drivers/usb/usb-skeleton.c#L567
> > usb_set_intfdata(interface, NULL) is called before deregistering the
> > interface and terminating the outstanding URBs!
>
> Going through the usb-skeleton.c source code, you will find that
> usb_get_intfdata() is called from only a few routines:
>
>         skel_open()
>         skel_disconnect()
>         skel_suspend()
>         skel_pre_reset()
>         skel_post_reset()
>
> Of those, all but the first are called only by the USB core and they are
> mutually exclusive with disconnect processing (except for
> skel_disconnect() itself, of course).  So they don't matter.
>
> The first, skel_open(), can be called as a result of actions by the
> user, so the driver needs to ensure that this can't happen after it
> clears the interface-data pointer.  The user can open the device file at
> any time before the minor number is given back, so it is not proper to
> call usb_set_intfdata(interface, NULL) before usb_deregister_dev() --
> but the driver does exactly this!
>
> (Well, it's not quite that bad.  skel_open() does check whether the
> interface-data pointer value it gets from usb_get_intfdata() is NULL.
> But it's still a race.)
>
> So yes, the current code is wrong.  And in fact, it will still be wrong
> even after the usb_set_intfdata(interface, NULL) line is removed,
> because there is no synchronization between skel_open() and
> skel_disconnect().

ACK. I did not look outside of skel_disconnect(). Regardless, I think
that removing the usb_set_intdata(interface, NULL) is still one step
in the good direction despite the other synchronisation issues. I sent
a patch for that which Greg already pick-up:
  https://git.kernel.org/gregkh/usb/c/c568f8bb41a4

>It is possible for skel_disconnect() to run to
> completion and the USB core to clear the interface-data pointer all
> while skel_open() is running.  The driver needs a static private mutex
> to synchronize opens with unregistrations.  (This is a general
> phenomenon, true of all drivers that have a user interface such as a
> device file.)
>
> The driver _does_ have a per-instance mutex, dev->io_mutex, to
> synchronize I/O with disconnects.  But that's separate from
> synchronizing opens with unregistrations, because at open time the
> driver doesn't yet know the address of the private data structure or
> even if the structure is still allocated.  So obviously it can't use a
> mutex that is embedded within the private data structure for this
> purpose.

ACK. However, I have other priorities, I will not invest more time to
dig in the usb-skeleton.c

Thank you for the answer! That was a long but interesting diversion
from the initial topic :)


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Andrew Lunn]

> > But if a driver does make the call, it should be careful to ensure that
> > the call happens _after_ the driver is finished using the interface-data
> > pointer.  For example, after all outstanding URBs have completed, if the
> > completion handlers will need to call usb_get_intfdata().
> 
> ACK. I understand that it should be called *after* the completion of
> any ongoing task.

What sometimes gets people is /sys, /proc. etc. A process can have
such a file open when the device is unplugged. If the read needs to
make use of your private data structure, you need to guarantee it
still exists.  Ideally the core needs to wait and not call the
disconnect until all such files are closed. Probably the USB core
does, it is such an obvious issue, but i have no knowledge of USB.

	Andrew

Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

[Vincent MAILHOL]

On Mon. 28 Nov. 2022 at 22:45, Andrew Lunn <andrew@lunn.ch> wrote:
> > > But if a driver does make the call, it should be careful to ensure that
> > > the call happens _after_ the driver is finished using the interface-data
> > > pointer.  For example, after all outstanding URBs have completed, if the
> > > completion handlers will need to call usb_get_intfdata().
> >
> > ACK. I understand that it should be called *after* the completion of
> > any ongoing task.
>
> What sometimes gets people is /sys, /proc. etc. A process can have
> such a file open when the device is unplugged. If the read needs to
> make use of your private data structure, you need to guarantee it
> still exists.  Ideally the core needs to wait and not call the
> disconnect until all such files are closed. Probably the USB core
> does, it is such an obvious issue, but i have no knowledge of USB.

For USB drivers, the parallel of what you are describing are the URBs
(USB request Buffers). The URB are sent asynchronously to the device.
Each URB has a completion handler:
  https://elixir.bootlin.com/linux/v6.0/source/include/linux/usb.h#L1443
It is important to wait for all outstanding URB to complete before
releasing your resources. But once you are able to guarantee that any
ongoing actions were completed, the order in which you kfree() or
usb_set_intfdata() to NULL matters less.

Of course, the USB drivers could also have some /sys/ or /proc/ files
opened, but this is not the case of the etas_es58x. By the way, the
handling of outstanding URBs is done by es58x_free_urbs():
  https://elixir.bootlin.com/linux/v6.0/source/drivers/net/can/usb/etas_es58x/es58x_core.c#L1745
which is called just before the devlink_free() and the usb_set_intfdata().

[PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent Mailhol]

ES58x devices report below product information through a custom usb
string:

  * the firmware version
  * the bootloader version
  * the hardware revision

Parse this string, store the results in struct es58x_dev, export the
firmware version through devlink's "fw" name and the hardware revision
through devlink's "board.rev" name.

devlink does not yet have a name suited for the bootloader and so this
last piece of information is exposed to the userland for through a
custom name: "bl".

Those devlink entries are not critical to use the device, if parsing
fails, print an informative log message and continue to probe the
device.

In addition to that, report the device serial number which is
available in usb_device::serial.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c   |   1 +
 drivers/net/can/usb/etas_es58x/es58x_core.h   |  67 ++++++
 .../net/can/usb/etas_es58x/es58x_devlink.c    | 194 ++++++++++++++++++
 3 files changed, 262 insertions(+)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index c6e598e4800c..d29c1bf90d73 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2250,6 +2250,7 @@ static int es58x_probe(struct usb_interface *intf,
 	if (ret)
 		return ret;
 
+	es58x_parse_product_info(es58x_dev);
 	devlink_register(priv_to_devlink(es58x_dev));
 
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index bf24375580e5..9481f0764131 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -356,6 +356,39 @@ struct es58x_operators {
 	int (*get_timestamp)(struct es58x_device *es58x_dev);
 };
 
+/**
+ * struct es58x_sw_version - Version number of the firmware or the
+ *	bootloader.
+ * @major: Version major number, represented on two digits.
+ * @minor: Version minor number, represented on two digits.
+ * @revision: Version revision number, represented on two digits.
+ *
+ * The firmware and the bootloader share the same format: "xx.xx.xx"
+ * where 'x' is a digit. Both can be retrieved from the product
+ * information string.
+ */
+struct es58x_sw_version {
+	u8 major;
+	u8 minor;
+	u8 revision;
+};
+
+/**
+ * struct es58x_hw_revision - Hardware revision number.
+ * @letter: Revision letter.
+ * @major: Version major number, represented on three digits.
+ * @minor: Version minor number, represented on three digits.
+ *
+ * The hardware revision uses its own format: "axxx/xxx" where 'a' is
+ * a letter and 'x' a digit. It can be retrieved from the product
+ * information string.
+ */
+struct es58x_hw_revision {
+	char letter;
+	u16 major;
+	u16 minor;
+};
+
 /**
  * struct es58x_device - All information specific to an ES58X device.
  * @dev: Device information.
@@ -373,6 +406,9 @@ struct es58x_operators {
  *	queue wake/stop logic should prevent this URB from getting
  *	empty. Please refer to es58x_get_tx_urb() for more details.
  * @tx_urbs_idle_cnt: number of urbs in @tx_urbs_idle.
+ * @firmware_version: The firmware version number.
+ * @bootloader_version: The bootloader version number.
+ * @hardware_revision: The hardware revision number.
  * @ktime_req_ns: kernel timestamp when es58x_set_realtime_diff_ns()
  *	was called.
  * @realtime_diff_ns: difference in nanoseconds between the clocks of
@@ -408,6 +444,10 @@ struct es58x_device {
 	struct usb_anchor tx_urbs_idle;
 	atomic_t tx_urbs_idle_cnt;
 
+	struct es58x_sw_version firmware_version;
+	struct es58x_sw_version bootloader_version;
+	struct es58x_hw_revision hardware_revision;
+
 	u64 ktime_req_ns;
 	s64 realtime_diff_ns;
 
@@ -420,6 +460,32 @@ struct es58x_device {
 	union es58x_urb_cmd rx_cmd_buf;
 };
 
+/**
+ * es58x_sw_version_is_set() - Check if the version is a valid number.
+ * @sw_ver: Version number of either the firmware or the bootloader.
+ *
+ * If &es58x_sw_version.major, &es58x_sw_version.minor and
+ * &es58x_sw_version.revision are all zero, the product string could
+ * not be parsed and the version number is invalid.
+ */
+static inline bool es58x_sw_version_is_set(struct es58x_sw_version *sw_ver)
+{
+	return sw_ver->major || sw_ver->minor || sw_ver->revision;
+}
+
+/**
+ * es58x_hw_revision_is_set() - Check if the revision is a valid number.
+ * @hw_rev: Revision number of the hardware.
+ *
+ * If &es58x_hw_revision.letter, &es58x_hw_revision.major and
+ * &es58x_hw_revision.minor are all zero, the product string could not
+ * be parsed and the hardware revision number is invalid.
+ */
+static inline bool es58x_hw_revision_is_set(struct es58x_hw_revision *hw_rev)
+{
+	return hw_rev->letter || hw_rev->major || hw_rev->minor;
+}
+
 /**
  * es58x_sizeof_es58x_device() - Calculate the maximum length of
  *	struct es58x_device.
@@ -693,6 +759,7 @@ int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
 		   const void *msg, u16 cmd_len, int channel_idx);
 
 /* es58x_devlink.c. */
+void es58x_parse_product_info(struct es58x_device *es58x_dev);
 extern const struct devlink_ops es58x_dl_ops;
 
 /* es581_4.c. */
diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
index af6ca7ada23f..7b67682b952e 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_devlink.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
@@ -7,7 +7,201 @@
  * Copyright (c) 2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/usb.h>
 #include <net/devlink.h>
 
+#include "es58x_core.h"
+
+/* USB descriptor index containing the product information string. */
+#define ES58X_PROD_INFO_IDX 6
+
+/**
+ * es58x_parse_sw_version() - Extract boot loader or firmware version.
+ * @es58x_dev: ES58X device.
+ * @prod_info: USB custom string returned by the device.
+ * @prefix: Select which information should be parsed. Set it to "FW"
+ *	to parse the firmware version or to "BL" to parse the
+ *	bootloader version.
+ *
+ * The @prod_info string contains the firmware and the bootloader
+ * version number all prefixed by a magic string and concatenated with
+ * other numbers. Depending on the device, the firmware (bootloader)
+ * format is either "FW_Vxx.xx.xx" ("BL_Vxx.xx.xx") or "FW:xx.xx.xx"
+ * ("BL:xx.xx.xx") where 'x' represents a digit. @prod_info must
+ * contains the common part of those prefixes: "FW" or "BL".
+ *
+ * Parse @prod_info and store the version number in
+ * &es58x_dev.firmware_version or &es58x_dev.bootloader_version
+ * according to @prefix value.
+ *
+ * Return: zero on success, -EINVAL if @prefix contains an invalid
+ *	value and -EBADMSG if @prod_info could not be parsed.
+ */
+static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
+				  const char *prod_info, const char *prefix)
+{
+	struct es58x_sw_version *version;
+	int major, minor, revision;
+
+	if (!strcmp(prefix, "FW"))
+		version = &es58x_dev->firmware_version;
+	else if (!strcmp(prefix, "BL"))
+		version = &es58x_dev->bootloader_version;
+	else
+		return -EINVAL;
+
+	/* Go to prefix */
+	prod_info = strstr(prod_info, prefix);
+	if (!prod_info)
+		return -EBADMSG;
+	/* Go to beginning of the version number */
+	while (!isdigit(*prod_info)) {
+		prod_info++;
+		if (!*prod_info)
+			return -EBADMSG;
+	}
+
+	if (sscanf(prod_info, "%2u.%2u.%2u", &major, &minor, &revision) != 3)
+		return -EBADMSG;
+
+	version->major = major;
+	version->minor = minor;
+	version->revision = revision;
+
+	return 0;
+}
+
+/**
+ * es58x_parse_hw_rev() - Extract hardware revision number.
+ * @es58x_dev: ES58X device.
+ * @prod_info: USB custom string returned by the device.
+ *
+ * @prod_info contains the hardware revision prefixed by a magic
+ * string and conquenated together with other numbers. Depending on
+ * the device, the hardware revision format is either
+ * "HW_VER:axxx/xxx" or "HR:axxx/xxx" where 'a' represents a letter
+ * and 'x' a digit.
+ *
+ * Parse @prod_info and store the hardware revision number in
+ * &es58x_dev.hardware_revision.
+ *
+ * Return: zero on success, -EBADMSG if @prod_info could not be
+ *	parsed.
+ */
+static int es58x_parse_hw_rev(struct es58x_device *es58x_dev,
+			      const char *prod_info)
+{
+	char letter;
+	int major, minor;
+
+	/* The only occurrence of 'H' is in the hardware revision prefix. */
+	prod_info = strchr(prod_info, 'H');
+	if (!prod_info)
+		return -EBADMSG;
+	/* Go to beginning of the hardware revision */
+	prod_info = strchr(prod_info, ':');
+	if (!prod_info)
+		return -EBADMSG;
+	prod_info++;
+
+	if (sscanf(prod_info, "%c%3u/%3u", &letter, &major, &minor) != 3)
+		return -EBADMSG;
+
+	es58x_dev->hardware_revision.letter = letter;
+	es58x_dev->hardware_revision.major = major;
+	es58x_dev->hardware_revision.minor = minor;
+
+	return 0;
+}
+
+/**
+ * es58x_parse_product_info() - Parse the ES58x product information
+ *	string.
+ * @es58x_dev: ES58X device.
+ *
+ * Retrieve the product information string and parse it to extract the
+ * firmware version, the bootloader version and the hardware
+ * revision.
+ *
+ * If the function fails, simply emit a log message and continue
+ * because product information is not critical for the driver to
+ * operate.
+ */
+void es58x_parse_product_info(struct es58x_device *es58x_dev)
+{
+	char *prod_info;
+
+	prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+	if (!prod_info) {
+		dev_warn(es58x_dev->dev,
+			 "could not retrieve the product info string\n");
+		return;
+	}
+
+	if (es58x_parse_sw_version(es58x_dev, prod_info, "FW") ||
+	    es58x_parse_sw_version(es58x_dev, prod_info, "BL") ||
+	    es58x_parse_hw_rev(es58x_dev, prod_info))
+		dev_info(es58x_dev->dev,
+			 "could not parse product info: '%s'\n", prod_info);
+
+	kfree(prod_info);
+}
+
+/**
+ * es58x_devlink_info_get() - Report the product information.
+ * @devlink: Devlink.
+ * @req: skb wrapper where to put requested information.
+ * @extack: Unused.
+ *
+ * Report the firmware version, the bootloader version, the hardware
+ * revision and the serial number through netlink.
+ *
+ * Return: zero on success, errno when any error occurs.
+ */
+static int es58x_devlink_info_get(struct devlink *devlink,
+				  struct devlink_info_req *req,
+				  struct netlink_ext_ack *extack)
+{
+	struct es58x_device *es58x_dev = devlink_priv(devlink);
+	struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
+	struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
+	struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
+	char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
+	int ret = 0;
+
+	if (es58x_sw_version_is_set(fw_ver)) {
+		snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
+			 fw_ver->major, fw_ver->minor, fw_ver->revision);
+		ret = devlink_info_version_running_put(req,
+						       DEVLINK_INFO_VERSION_GENERIC_FW,
+						       buf);
+		if (ret)
+			return ret;
+	}
+
+	if (es58x_sw_version_is_set(bl_ver)) {
+		snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
+			 bl_ver->major, bl_ver->minor, bl_ver->revision);
+		ret = devlink_info_version_running_put(req, "bl", buf);
+		if (ret)
+			return ret;
+	}
+
+	if (es58x_hw_revision_is_set(hw_rev)) {
+		snprintf(buf, sizeof(buf), "%c%03u/%03u",
+			 hw_rev->letter, hw_rev->major, hw_rev->minor);
+		ret = devlink_info_version_fixed_put(req,
+						     DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,
+						     buf);
+		if (ret)
+			return ret;
+	}
+
+	return devlink_info_serial_number_put(req, es58x_dev->udev->serial);
+}
+
 const struct devlink_ops es58x_dl_ops = {
+	.info_get = es58x_devlink_info_get,
 };
-- 
2.37.4

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Andrew Lunn]

> +struct es58x_sw_version {
> +	u8 major;
> +	u8 minor;
> +	u8 revision;
> +};

> +static int es58x_devlink_info_get(struct devlink *devlink,
> +				  struct devlink_info_req *req,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct es58x_device *es58x_dev = devlink_priv(devlink);
> +	struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
> +	struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
> +	struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
> +	char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
> +	int ret = 0;
> +
> +	if (es58x_sw_version_is_set(fw_ver)) {
> +		snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
> +			 fw_ver->major, fw_ver->minor, fw_ver->revision);

I see you have been very careful here, but i wonder if you might still
get some compiler/static code analyser warnings here. As far as i
remember %02u does not limit it to two characters. If the number is
bigger than 99, it will take three characters. And your types are u8,
so the compiler could consider these to be 3 characters each. So you
end up truncating. Which you look to of done correctly, but i wonder
if some over zealous checker will report it? Maybe consider
"xxx.xxx.xxx"?

Nice paranoid code by the way. I'm not the best at spotting potential
buffer overflows, but this code looks good. The only question i had
left was how well sscanf() deals with UTF-8.

     Andrew

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent MAILHOL]

Hi Andrew,

Thank you for the review and the interesting comments on the parsing.

On. 27 Nov. 2022 at 02:37, Andrew Lunn <andrew@lunn.ch> wrote:
> > +struct es58x_sw_version {
> > +     u8 major;
> > +     u8 minor;
> > +     u8 revision;
> > +};
>
> > +static int es58x_devlink_info_get(struct devlink *devlink,
> > +                               struct devlink_info_req *req,
> > +                               struct netlink_ext_ack *extack)
> > +{
> > +     struct es58x_device *es58x_dev = devlink_priv(devlink);
> > +     struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
> > +     struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
> > +     struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
> > +     char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
> > +     int ret = 0;
> > +
> > +     if (es58x_sw_version_is_set(fw_ver)) {
> > +             snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
> > +                      fw_ver->major, fw_ver->minor, fw_ver->revision);
>
> I see you have been very careful here, but i wonder if you might still
> get some compiler/static code analyser warnings here. As far as i
> remember %02u does not limit it to two characters.

I checked, none of gcc and clang would trigger a warning even for a
'make W=12'. More generally speaking, I made sure that my driver is
free of any W=12.
(except from the annoying spam from GENMASK_INPUT_CHECK for which my
attempts to silence it were rejected:
https://lore.kernel.org/all/20220426161658.437466-1-mailhol.vincent@wanadoo.fr/
).

> If the number is
> bigger than 99, it will take three characters. And your types are u8,
> so the compiler could consider these to be 3 characters each. So you
> end up truncating. Which you look to of done correctly, but i wonder
> if some over zealous checker will report it?

That zealous check is named -Wformat-truncation in gcc (I did not find
it in clang). Even W=3 doesn't report it so I consider this to be
fine.

> Maybe consider "xxx.xxx.xxx"?

If I do that, I also need to consider the maximum length of the
hardware revision would be "a/xxxxx/xxxxx" because the numbers are
u16. The declaration would become:

        char buf[max(sizeof("xxx.xxx.xxx"), sizeof("axxxxx/xxxxx"))];

Because no such warning exists in the kernel, I do not think the above
line to be a good trade off. I would like to keep things as they are,
it is easier to read. That said, I will add an extra check in
es58x_parse_sw_version() and es58x_parse_hw_revision() to assert that
the number are not bigger than 99 for the software version (and not
bigger than 999 for the hardware revision). That way the code will
guarantee that the truncation can never occur.

> Nice paranoid code by the way. I'm not the best at spotting potential
> buffer overflows, but this code looks good. The only question i had
> left was how well sscanf() deals with UTF-8.

It does not consider UTF-8. The %u is a _parse_integer_limit() in disguise.
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3637
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L70

_parse_integer_limit() just check for ASCII digits so the first UTF-8
character would make the function return.
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/kstrtox.c#L65

For example, this:
  "ＦＷ:03.00.06"
would fail parsing because sscanf() will not be able to match the
first byte of the UTF-8 'Ｆ' with 'F'.

Another example:
  "FW:０３.００.０６"
would also fail parsing because _parse_integer_limit() will not
recognize the first byte of UTF-8 '０' as a valid ASCII digit and
return early.

To finish, a very edge case:
  "FW:03.00.0６"
would incorrectly succeed. It will parse "FW:03.00.0" successfully and
will return when encountering the UTF-8 '６'. But I am not willing to
cover that edge case. If the device goes into this level of
perversion, I do not care any more as long as it does not result in
undefined behaviour.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent MAILHOL]

On Sun. 27 Nov. 2022 at 12:42, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Hi Andrew,
>
> Thank you for the review and the interesting comments on the parsing.
>
> On. 27 Nov. 2022 at 02:37, Andrew Lunn <andrew@lunn.ch> wrote:
> > > +struct es58x_sw_version {
> > > +     u8 major;
> > > +     u8 minor;
> > > +     u8 revision;
> > > +};
> >
> > > +static int es58x_devlink_info_get(struct devlink *devlink,
> > > +                               struct devlink_info_req *req,
> > > +                               struct netlink_ext_ack *extack)
> > > +{
> > > +     struct es58x_device *es58x_dev = devlink_priv(devlink);
> > > +     struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
> > > +     struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
> > > +     struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
> > > +     char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
> > > +     int ret = 0;
> > > +
> > > +     if (es58x_sw_version_is_set(fw_ver)) {
> > > +             snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
> > > +                      fw_ver->major, fw_ver->minor, fw_ver->revision);
> >
> > I see you have been very careful here, but i wonder if you might still
> > get some compiler/static code analyser warnings here. As far as i
> > remember %02u does not limit it to two characters.
>
> I checked, none of gcc and clang would trigger a warning even for a
> 'make W=12'. More generally speaking, I made sure that my driver is
> free of any W=12.
> (except from the annoying spam from GENMASK_INPUT_CHECK for which my
> attempts to silence it were rejected:
> https://lore.kernel.org/all/20220426161658.437466-1-mailhol.vincent@wanadoo.fr/
> ).
>
> > If the number is
> > bigger than 99, it will take three characters. And your types are u8,
> > so the compiler could consider these to be 3 characters each. So you
> > end up truncating. Which you look to of done correctly, but i wonder
> > if some over zealous checker will report it?
>
> That zealous check is named -Wformat-truncation in gcc (I did not find
> it in clang). Even W=3 doesn't report it so I consider this to be
> fine.
>
> > Maybe consider "xxx.xxx.xxx"?
>
> If I do that, I also need to consider the maximum length of the
> hardware revision would be "a/xxxxx/xxxxx" because the numbers are
> u16. The declaration would become:
>
>         char buf[max(sizeof("xxx.xxx.xxx"), sizeof("axxxxx/xxxxx"))];
>
> Because no such warning exists in the kernel, I do not think the above
> line to be a good trade off. I would like to keep things as they are,
> it is easier to read. That said, I will add an extra check in
> es58x_parse_sw_version() and es58x_parse_hw_revision() to assert that
> the number are not bigger than 99 for the software version (and not
> bigger than 999 for the hardware revision). That way the code will
> guarantee that the truncation can never occur.

Never mind. I forgot that I already accounted for that. The "%2u"
format in sscanf() will detect if the number is three or more digits.
So I am thinking of leaving everything as-is.

> > Nice paranoid code by the way. I'm not the best at spotting potential
> > buffer overflows, but this code looks good. The only question i had
> > left was how well sscanf() deals with UTF-8.
>
> It does not consider UTF-8. The %u is a _parse_integer_limit() in disguise.
>   https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3637
>   https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L70
>
> _parse_integer_limit() just check for ASCII digits so the first UTF-8
> character would make the function return.
>   https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/kstrtox.c#L65
>
> For example, this:
>   "ＦＷ:03.00.06"
> would fail parsing because sscanf() will not be able to match the
> first byte of the UTF-8 'Ｆ' with 'F'.
>
> Another example:
>   "FW:０３.００.０６"
> would also fail parsing because _parse_integer_limit() will not
> recognize the first byte of UTF-8 '０' as a valid ASCII digit and
> return early.
>
> To finish, a very edge case:
>   "FW:03.00.0６"
> would incorrectly succeed. It will parse "FW:03.00.0" successfully and
> will return when encountering the UTF-8 '６'. But I am not willing to
> cover that edge case. If the device goes into this level of
> perversion, I do not care any more as long as it does not result in
> undefined behaviour.
>
>
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Andrew Lunn]

> I checked, none of gcc and clang would trigger a warning even for a
> 'make W=12'. More generally speaking, I made sure that my driver is
> free of any W=12.

That is good enough for me.

> I do not care any more as long as it does not result in
> undefined behaviour.

Agreed. Hopefully sscanf cannot go completely wrong and go off the end
of the buffer. That i would care about. Bit i guess the USB fuzzers
would of hit such problems already.

	Andrew

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent MAILHOL]

On Mon. 28 Nov. 2022 at 00:08, Andrew Lunn <andrew@lunn.ch> wrote:
> > I checked, none of gcc and clang would trigger a warning even for a
> > 'make W=12'. More generally speaking, I made sure that my driver is
> > free of any W=12.
>
> That is good enough for me.
>
> > I do not care any more as long as it does not result in
> > undefined behaviour.
>
> Agreed. Hopefully sscanf cannot go completely wrong and go off the end
> of the buffer. That i would care about. Bit i guess the USB fuzzers
> would of hit such problems already.

On the surface, the sscanf() seems OK. It will break the while loop
when reaching the end of the format:
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3429
or the end of the string:
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3501
(I am skipping details here, there are other branches that will break
the while loop and all of them look good).

And me not being the first person using sscanf(), I hope that if a bug
existed, it would have already been spotted by some static
analysis/fuzzing/code review :)

That said, I think I answered all your comments. Can I get your
reviewed-by or ack tag? Thank you!


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Andrew Lunn]

> That said, I think I answered all your comments. Can I get your
> reviewed-by or ack tag? Thank you!

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Andrew Lunn]

> devlink does not yet have a name suited for the bootloader and so this
> last piece of information is exposed to the userland for through a
> custom name: "bl".

Jiri, what do you think about 'bl'? Is it too short, not well known
enough? It could easily be 'bootloader'.

	Andrew

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent MAILHOL]

On Mon. 28 Nov. 2022 at 22:49, Andrew Lunn <andrew@lunn.ch> wrote:
> > devlink does not yet have a name suited for the bootloader and so this
> > last piece of information is exposed to the userland for through a
> > custom name: "bl".
>
> Jiri, what do you think about 'bl'? Is it too short, not well known
> enough? It could easily be 'bootloader'.

For the record, I name it "bl" by analogy with the firmware which is
named "fw". My personal preference would have been to name the fields
without any abbreviations: "firmware", "bootloader" and
"hardware.revision" (for reference ethtool -i uses
"firmware-version"). But I tried to put my personal taste aside and
try to fit with the devlink trends to abbreviate things. Thus the name
"bl".


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Jakub Kicinski]

On Mon, 28 Nov 2022 23:43:19 +0900 Vincent MAILHOL wrote:
> On Mon. 28 Nov. 2022 at 22:49, Andrew Lunn <andrew@lunn.ch> wrote:
> > > devlink does not yet have a name suited for the bootloader and so this
> > > last piece of information is exposed to the userland for through a
> > > custom name: "bl".  
> >
> > Jiri, what do you think about 'bl'? Is it too short, not well known
> > enough? It could easily be 'bootloader'.  
> 
> For the record, I name it "bl" by analogy with the firmware which is
> named "fw". My personal preference would have been to name the fields
> without any abbreviations: "firmware", "bootloader" and
> "hardware.revision" (for reference ethtool -i uses
> "firmware-version"). But I tried to put my personal taste aside and
> try to fit with the devlink trends to abbreviate things. Thus the name
> "bl".

Agreed, I thought "fw" is sufficiently universally understood to be used
but "bl" is most definitely not :S  I'd suggest "fw.bootloader". Also
don't hesitate to add that to the "well known" list in devlink.h, 
I reckon it will be used by others sooner or later.

Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent MAILHOL]

On Tue. 29 Nov. 2022 at 07:27, Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 28 Nov 2022 23:43:19 +0900 Vincent MAILHOL wrote:
> > On Mon. 28 Nov. 2022 at 22:49, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > devlink does not yet have a name suited for the bootloader and so this
> > > > last piece of information is exposed to the userland for through a
> > > > custom name: "bl".
> > >
> > > Jiri, what do you think about 'bl'? Is it too short, not well known
> > > enough? It could easily be 'bootloader'.
> >
> > For the record, I name it "bl" by analogy with the firmware which is
> > named "fw". My personal preference would have been to name the fields
> > without any abbreviations: "firmware", "bootloader" and
> > "hardware.revision" (for reference ethtool -i uses
> > "firmware-version"). But I tried to put my personal taste aside and
> > try to fit with the devlink trends to abbreviate things. Thus the name
> > "bl".
>
> Agreed, I thought "fw" is sufficiently universally understood to be used
> but "bl" is most definitely not :S  I'd suggest "fw.bootloader". Also
> don't hesitate to add that to the "well known" list in devlink.h,
> I reckon it will be used by others sooner or later.

I like the "fw.bootloader" suggestion. A bootloader is technically
still a firmware. I will send a separate patch to add the entry to
devlink.h and only then send the v5.

[PATCH v4 4/6] can: etas_es58x: remove es58x_get_product_info()

[Vincent Mailhol]

Now that the product information is available under devlink, no more
need to print them in the kernel log. Remove es58x_get_product_info().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 52 ++-------------------
 1 file changed, 3 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index d29c1bf90d73..e81ef23d8698 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2120,48 +2120,6 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
 	}
 }
 
-/**
- * es58x_get_product_info() - Get the product information and print them.
- * @es58x_dev: ES58X device.
- *
- * Do a synchronous call to get the product information.
- *
- * Return: zero on success, errno when any error occurs.
- */
-static int es58x_get_product_info(struct es58x_device *es58x_dev)
-{
-	struct usb_device *udev = es58x_dev->udev;
-	const int es58x_prod_info_idx = 6;
-	/* Empirical tests show a prod_info length of maximum 83,
-	 * below should be more than enough.
-	 */
-	const size_t prod_info_len = 127;
-	char *prod_info;
-	int ret;
-
-	prod_info = kmalloc(prod_info_len, GFP_KERNEL);
-	if (!prod_info)
-		return -ENOMEM;
-
-	ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
-	if (ret < 0) {
-		dev_err(es58x_dev->dev,
-			"%s: Could not read the product info: %pe\n",
-			__func__, ERR_PTR(ret));
-		goto out_free;
-	}
-	if (ret >= prod_info_len - 1) {
-		dev_warn(es58x_dev->dev,
-			 "%s: Buffer is too small, result might be truncated\n",
-			 __func__);
-	}
-	dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
-
- out_free:
-	kfree(prod_info);
-	return ret < 0 ? ret : 0;
-}
-
 /**
  * es58x_init_es58x_dev() - Initialize the ES58X device.
  * @intf: USB interface.
@@ -2240,28 +2198,24 @@ static int es58x_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
 	struct es58x_device *es58x_dev;
-	int ch_idx, ret;
+	int ch_idx;
 
 	es58x_dev = es58x_init_es58x_dev(intf, id->driver_info);
 	if (IS_ERR(es58x_dev))
 		return PTR_ERR(es58x_dev);
 
-	ret = es58x_get_product_info(es58x_dev);
-	if (ret)
-		return ret;
-
 	es58x_parse_product_info(es58x_dev);
 	devlink_register(priv_to_devlink(es58x_dev));
 
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
-		ret = es58x_init_netdev(es58x_dev, ch_idx);
+		int ret = es58x_init_netdev(es58x_dev, ch_idx);
 		if (ret) {
 			es58x_free_netdevs(es58x_dev);
 			return ret;
 		}
 	}
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.37.4

Re: [PATCH v4 4/6] can: etas_es58x: remove es58x_get_product_info()

[Andrew Lunn]

On Sun, Nov 27, 2022 at 01:22:09AM +0900, Vincent Mailhol wrote:
> Now that the product information is available under devlink, no more
> need to print them in the kernel log. Remove es58x_get_product_info().
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

There is a slim chance this will break something paring the kernel
log, but you are not really supposed to do that.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v4 4/6] can: etas_es58x: remove es58x_get_product_info()

[Vincent MAILHOL]

On Mon. 28 Nov. 2022 at 22:47, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Nov 27, 2022 at 01:22:09AM +0900, Vincent Mailhol wrote:
> > Now that the product information is available under devlink, no more
> > need to print them in the kernel log. Remove es58x_get_product_info().
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> There is a slim chance this will break something paring the kernel
> log, but you are not really supposed to do that.

Greg made it clear that this should disappear:
  https://lore.kernel.org/linux-can/Y2YdH4dd8u%2FeUEXg@kroah.com/
and I agree.

I do not recognize the kernel log as being a stable interface to the
userland that we should not break.

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thank you!

[PATCH v4 5/6] can: etas_es58x: report the firmware version through ethtool

[Vincent Mailhol]

Implement ethtool_ops::get_drvinfo() in order to report the firmware
version.

Firmware version 0.0.0 has a special meaning and just means that we
could not parse the product information string. In such case, do
nothing (i.e. leave the .fw_version string empty).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

*N.B.* Drivers had to also fill ethtool_drvinfo::driver and
ethtool_drvinfo::bus_info. Starting this month, this is not needed
anymore because of commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo:
populate drvinfo fields even if callback exits").

  https://git.kernel.org/netdev/net-next/c/edaf5df22cb8
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index e81ef23d8698..12968aef41af 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -1979,7 +1979,28 @@ static const struct net_device_ops es58x_netdev_ops = {
 	.ndo_eth_ioctl = can_eth_ioctl_hwts,
 };
 
+/**
+ * es58x_get_drvinfo() - Get the firmware version.
+ * @netdev: CAN network device.
+ * @drvinfo: Driver information.
+ *
+ * Populate @drvinfo with the firmware version. The core will populate
+ * the rest.
+ */
+static void es58x_get_drvinfo(struct net_device *netdev,
+			      struct ethtool_drvinfo *drvinfo)
+{
+	struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
+	struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
+
+	if (es58x_sw_version_is_set(fw_ver))
+		snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
+			 "%02u.%02u.%02u",
+			 fw_ver->major, fw_ver->minor, fw_ver->revision);
+}
+
 static const struct ethtool_ops es58x_ethtool_ops = {
+	.get_drvinfo = es58x_get_drvinfo,
 	.get_ts_info = can_ethtool_op_get_ts_info_hwts,
 };
 
-- 
2.37.4

Re: [PATCH v4 5/6] can: etas_es58x: report the firmware version through ethtool

[Andrew Lunn]

On Sun, Nov 27, 2022 at 01:22:10AM +0900, Vincent Mailhol wrote:
> Implement ethtool_ops::get_drvinfo() in order to report the firmware
> version.
> 
> Firmware version 0.0.0 has a special meaning and just means that we
> could not parse the product information string. In such case, do
> nothing (i.e. leave the .fw_version string empty).
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v4 5/6] can: etas_es58x: report the firmware version through ethtool

[Jakub Kicinski]

On Sun, 27 Nov 2022 01:22:10 +0900 Vincent Mailhol wrote:
> Implement ethtool_ops::get_drvinfo() in order to report the firmware
> version.
> 
> Firmware version 0.0.0 has a special meaning and just means that we
> could not parse the product information string. In such case, do
> nothing (i.e. leave the .fw_version string empty).

devlink_compat_running_version() does not work?

Re: [PATCH v4 5/6] can: etas_es58x: report the firmware version through ethtool

[Vincent MAILHOL]

On Tue. 29 Nov. 2022 at 07:29, Jakub Kicinski <kuba@kernel.org> wrote:
> On Sun, 27 Nov 2022 01:22:10 +0900 Vincent Mailhol wrote:
> > Implement ethtool_ops::get_drvinfo() in order to report the firmware
> > version.
> >
> > Firmware version 0.0.0 has a special meaning and just means that we
> > could not parse the product information string. In such case, do
> > nothing (i.e. leave the .fw_version string empty).
>
> devlink_compat_running_version() does not work?

I was not aware of this one. Thank you for pointing this out.
If I correctly understand, devlink_compat_running_version() is
supposed to allow ethtool to retrieve the firmware version from
devlink, right?

Currently it does not work. I guess it is because I am not using
SET_NETDEV_DEVLINK_PORT()? I initially thought that this was optional.
I will continue to investigate and see if it is possible to completely
remove the .get_drvinfo() callback.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 5/6] can: etas_es58x: report the firmware version through ethtool

[Jakub Kicinski]

On Wed, 30 Nov 2022 02:12:27 +0900 Vincent MAILHOL wrote:
> I was not aware of this one. Thank you for pointing this out.
> If I correctly understand, devlink_compat_running_version() is
> supposed to allow ethtool to retrieve the firmware version from
> devlink, right?

Yes.

> Currently it does not work. I guess it is because I am not using
> SET_NETDEV_DEVLINK_PORT()? I initially thought that this was optional.

It's optional but breaks the linking hence the fallback can't kick in.
I guess "optional-ity" is a spectrum :)

> I will continue to investigate and see if it is possible to completely
> remove the .get_drvinfo() callback.

[PATCH v4 6/6] Documentation: devlink: add devlink documentation for the etas_es58x driver

[Vincent Mailhol]

List all the version information reported by the etas_es58x
driver. Also, update MAINTAINERS with the newly created file.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 .../networking/devlink/etas_es58x.rst         | 33 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/networking/devlink/etas_es58x.rst

diff --git a/Documentation/networking/devlink/etas_es58x.rst b/Documentation/networking/devlink/etas_es58x.rst
new file mode 100644
index 000000000000..83f59713eed5
--- /dev/null
+++ b/Documentation/networking/devlink/etas_es58x.rst
@@ -0,0 +1,33 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==========================
+etas_es58x devlink support
+==========================
+
+This document describes the devlink features implemented by the
+``etas_es58x`` device driver.
+
+Info versions
+=============
+
+The ``etas_es58x`` driver reports the following versions
+
+.. list-table:: devlink info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``fw``
+     - running
+     - Version of firmware running on the device. Also available
+       through ``ethtool -i``.
+   * - ``bl``
+     - running
+     - Version of the bootloader running on the device.
+   * - ``board.rev``
+     - fixed
+     - The hardware revision of the device.
+   * - ``serial_number``
+     - fixed
+     - The USB serial number. Also available through ``lsusb -v``.
diff --git a/MAINTAINERS b/MAINTAINERS
index 61fe86968111..d95642683fc4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7686,6 +7686,7 @@ ETAS ES58X CAN/USB DRIVER
 M:	Vincent Mailhol <mailhol.vincent@wanadoo.fr>
 L:	linux-can@vger.kernel.org
 S:	Maintained
+F:	Documentation/networking/devlink/etas_es58x.rst
 F:	drivers/net/can/usb/etas_es58x/
 
 ETHERNET BRIDGE
-- 
2.37.4

[PATCH v5 0/7] can: etas_es58x: report firmware, bootloader and hardware version

[Vincent Mailhol]

The goal of this series is to report the firmware version, the
bootloader version and the hardware revision of ETAS ES58x
devices.

These are already reported in the kernel log but this isn't best
practice. Remove the kernel log and instead export all these through
devlink. The devlink core automatically exports the firmware and the
bootloader version to ethtool, so no need to implement the
ethtool_ops::get_drvinfo() callback anymore.

Patch one and two implement the core support for devlink (at device
level) and devlink port (at the network interface level).

Patch three export usb_cache_string() and patch four add a new info
attribute to devlink.h. Both are prerequisites for patch five.

Patch five is the actual goal: it parses the product information from
a custom usb string returned by the device and expose them through
devlink.

Patch six removes the product information from the kernel log.

Finally, patch seven add a devlink documentation page with list all
the information attributes reported by the driver.


* Sample outputs following this series *

| $ devlink dev info 
| usb/1-9:1.1:
|   serial_number 0108954
|   versions:
|       fixed:
|         board.rev B012/000
|       running:
|         fw 04.00.01
|         fw.bootloader 02.00.00

| $ devlink port show can0
| usb/1-9:1.1/0: type eth netdev can0 flavour physical port 0 splittable false

| $ ethtool -i can0
| driver: etas_es58x
| version: 6.1.0-rc7+
| firmware-version: 04.00.01 02.00.00 
| expansion-rom-version: 
| bus-info: 1-9:1.1
| supports-statistics: no
| supports-test: no
| supports-eeprom-access: no
| supports-register-dump: no
| supports-priv-flags: no

---
* Changelog *

v4 -> v5:

  * [PATH 2/7] add devlink port support. This extends devlink to the
    network interface.

  * thanks to devlink port, 'ethtool -i' is now able to retrieve the
    firmware version from devlink. No need to implement the
    ethtool_ops::get_drvinfo() callback anymore: remove one patch from
    the series.

  * [PATCH 4/7] A new patch to add a new info attribute for the
    bootloader version in devlink.h. This patch was initially sent as
    a standalone patch here:
      https://lore.kernel.org/netdev/20221129031406.3849872-1-mailhol.vincent@wanadoo.fr/
    Merging it to this series so that it is both added and used at the
    same time.

  * [PATCH 5/7] use the newly info attribute defined in patch 4/7 to
    report the bootloader version instead of the custom string "bl".

  * [PATCH 5/7] because the series does not implement
    ethtool_ops::get_drvinfo() anymore, the two helper functions
    es58x_sw_version_is_set() and es58x_hw_revision_is_set() are only
    used in devlink.c. Move them from es58x_core.h to es58x_devlink.c.

  * [PATCH 5/7] small rework of the helper function
    es58x_hw_revision_is_set(): it is OK to only check the letter (if
    the letter is '\0', it will not be possible to print the next
    numbers).

  * [PATCH 5/7 and 6/7] add reviewed-by Andrew Lunn tag.

  * [PATCH 7/7] Now, 'ethtool -i' reports both the firmware version
    and the bootloader version (this is how the core export the
    information from devlink to ethtool). Update the documentation to
    reflect this fact.

  * Reoder the patches.

v3 -> v4:

  * major rework to use devlink instead of sysfs following Andrew's
    comment.

  * split the series in 6 patches.

  * [PATCH 1/6] add Acked-by: Greg Kroah-Hartman

v2 -> v3:

  * patch 2/3: do not spam the kernel log anymore with the product
    number. Instead parse the product information string, extract the
    firmware version, the bootloadar version and the hardware revision
    and export them through sysfs.

  * patch 2/3: rework the parsing in order not to need additional
    fields in struct es58x_parameters.

  * patch 3/3: only populate ethtool_drvinfo::fw_version because since
    commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo: populate
    drvinfo fields even if callback exits"), there is no need to
    populate ethtool_drvinfo::driver and ethtool_drvinfo::bus_info in
    the driver.

v1 -> v2:

  * was a single patch. It is now a series of three patches.
  * add a first new patch to export  usb_cache_string().
  * add a second new patch to apply usb_cache_string() to existing code.
  * add missing check on product info string to prevent a buffer overflow.
  * add comma on the last entry of struct es58x_parameters.

Vincent Mailhol (7):
  can: etas_es58x: add devlink support
  can: etas_es58x: add devlink port support
  USB: core: export usb_cache_string()
  net: devlink: add DEVLINK_INFO_VERSION_GENERIC_FW_BOOTLOADER
  can: etas_es58x: export product information through
    devlink_ops::info_get()
  can: etas_es58x: remove es58x_get_product_info()
  Documentation: devlink: add devlink documentation for the etas_es58x
    driver

 .../networking/devlink/devlink-info.rst       |   5 +
 .../networking/devlink/etas_es58x.rst         |  36 +++
 MAINTAINERS                                   |   1 +
 drivers/net/can/usb/Kconfig                   |   1 +
 drivers/net/can/usb/etas_es58x/Makefile       |   2 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c   |  98 +++-----
 drivers/net/can/usb/etas_es58x/es58x_core.h   |  50 ++++
 .../net/can/usb/etas_es58x/es58x_devlink.c    | 235 ++++++++++++++++++
 drivers/usb/core/message.c                    |   1 +
 drivers/usb/core/usb.h                        |   1 -
 include/linux/usb.h                           |   1 +
 include/net/devlink.h                         |   2 +
 12 files changed, 372 insertions(+), 61 deletions(-)
 create mode 100644 Documentation/networking/devlink/etas_es58x.rst
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_devlink.c

-- 
2.37.4

[PATCH v5 1/7] can: etas_es58x: add devlink support

[Vincent Mailhol]

Add basic support for devlink at the device level. The callbacks of
struct devlink_ops will be implemented next.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/Kconfig                    |  1 +
 drivers/net/can/usb/etas_es58x/Makefile        |  2 +-
 drivers/net/can/usb/etas_es58x/es58x_core.c    | 13 ++++++++++---
 drivers/net/can/usb/etas_es58x/es58x_core.h    |  6 ++++++
 drivers/net/can/usb/etas_es58x/es58x_devlink.c | 13 +++++++++++++
 5 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_devlink.c

diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index 8c6fea661530..445504ababce 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -30,6 +30,7 @@ config CAN_ESD_USB
 config CAN_ETAS_ES58X
 	tristate "ETAS ES58X CAN/USB interfaces"
 	select CRC16
+	select NET_DEVLINK
 	help
 	  This driver supports the ES581.4, ES582.1 and ES584.1 interfaces
 	  from ETAS GmbH (https://www.etas.com/en/products/es58x.php).
diff --git a/drivers/net/can/usb/etas_es58x/Makefile b/drivers/net/can/usb/etas_es58x/Makefile
index a129b4aa0215..d6667ebe259f 100644
--- a/drivers/net/can/usb/etas_es58x/Makefile
+++ b/drivers/net/can/usb/etas_es58x/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CAN_ETAS_ES58X) += etas_es58x.o
-etas_es58x-y = es58x_core.o es581_4.o es58x_fd.o
+etas_es58x-y = es58x_core.o es58x_devlink.o es581_4.o es58x_fd.o
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 5aba16849603..aeffe61faed8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/usb.h>
+#include <net/devlink.h>
 
 #include "es58x_core.h"
 
@@ -2177,6 +2178,7 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
 {
 	struct device *dev = &intf->dev;
 	struct es58x_device *es58x_dev;
+	struct devlink *devlink;
 	const struct es58x_parameters *param;
 	const struct es58x_operators *ops;
 	struct usb_device *udev = interface_to_usbdev(intf);
@@ -2199,11 +2201,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
 		ops = &es581_4_ops;
 	}
 
-	es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
-				 GFP_KERNEL);
-	if (!es58x_dev)
+	devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
+				dev);
+	if (!devlink)
 		return ERR_PTR(-ENOMEM);
 
+	es58x_dev = devlink_priv(devlink);
 	es58x_dev->param = param;
 	es58x_dev->ops = ops;
 	es58x_dev->dev = dev;
@@ -2250,6 +2253,8 @@ static int es58x_probe(struct usb_interface *intf,
 	if (ret)
 		return ret;
 
+	devlink_register(priv_to_devlink(es58x_dev));
+
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
 		ret = es58x_init_netdev(es58x_dev, ch_idx);
 		if (ret) {
@@ -2275,8 +2280,10 @@ static void es58x_disconnect(struct usb_interface *intf)
 	dev_info(&intf->dev, "Disconnecting %s %s\n",
 		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
 
+	devlink_unregister(priv_to_devlink(es58x_dev));
 	es58x_free_netdevs(es58x_dev);
 	es58x_free_urbs(es58x_dev);
+	devlink_free(priv_to_devlink(es58x_dev));
 	usb_set_intfdata(intf, NULL);
 }
 
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index 4a082fd69e6f..bf24375580e5 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -674,6 +674,7 @@ static inline enum es58x_flag es58x_get_flags(const struct sk_buff *skb)
 	return es58x_flags;
 }
 
+/* es58x_core.c. */
 int es58x_can_get_echo_skb(struct net_device *netdev, u32 packet_idx,
 			   u64 *tstamps, unsigned int pkts);
 int es58x_tx_ack_msg(struct net_device *netdev, u16 tx_free_entries,
@@ -691,9 +692,14 @@ int es58x_rx_cmd_ret_u32(struct net_device *netdev,
 int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
 		   const void *msg, u16 cmd_len, int channel_idx);
 
+/* es58x_devlink.c. */
+extern const struct devlink_ops es58x_dl_ops;
+
+/* es581_4.c. */
 extern const struct es58x_parameters es581_4_param;
 extern const struct es58x_operators es581_4_ops;
 
+/* es58x_fd.c. */
 extern const struct es58x_parameters es58x_fd_param;
 extern const struct es58x_operators es58x_fd_ops;
 
diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
new file mode 100644
index 000000000000..af6ca7ada23f
--- /dev/null
+++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces.
+ *
+ * File es58x_devlink.c: report the product information using devlink.
+ *
+ * Copyright (c) 2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ */
+
+#include <net/devlink.h>
+
+const struct devlink_ops es58x_dl_ops = {
+};
-- 
2.37.4

[PATCH v5 2/7] can: etas_es58x: add devlink port support

[Vincent Mailhol]

Add support for devlink port which extends the devlink support to the
network interface level. For now, the etas_es58x driver will only rely
on the default features that devlink port has to offer and not
implement additional feature ones.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 34 ++++++++++++++++-----
 drivers/net/can/usb/etas_es58x/es58x_core.h |  3 ++
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index aeffe61faed8..de884de9fe57 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2039,10 +2039,16 @@ static int es58x_set_mode(struct net_device *netdev, enum can_mode mode)
  * @es58x_dev: ES58X device.
  * @priv: ES58X private parameters related to the network device.
  * @channel_idx: Index of the network device.
+ *
+ * Return: zero on success, errno if devlink port could not be
+ *	properly registered.
  */
-static void es58x_init_priv(struct es58x_device *es58x_dev,
-			    struct es58x_priv *priv, int channel_idx)
+static int es58x_init_priv(struct es58x_device *es58x_dev,
+			   struct es58x_priv *priv, int channel_idx)
 {
+	struct devlink_port_attrs attrs = {
+		.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL,
+	};
 	const struct es58x_parameters *param = es58x_dev->param;
 	struct can_priv *can = &priv->can;
 
@@ -2061,6 +2067,10 @@ static void es58x_init_priv(struct es58x_device *es58x_dev,
 	can->state = CAN_STATE_STOPPED;
 	can->ctrlmode_supported = param->ctrlmode_supported;
 	can->do_set_mode = es58x_set_mode;
+
+	devlink_port_attrs_set(&priv->devlink_port, &attrs);
+	return devlink_port_register(priv_to_devlink(es58x_dev),
+				     &priv->devlink_port, channel_idx);
 }
 
 /**
@@ -2084,7 +2094,10 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
 	}
 	SET_NETDEV_DEV(netdev, dev);
 	es58x_dev->netdev[channel_idx] = netdev;
-	es58x_init_priv(es58x_dev, es58x_priv(netdev), channel_idx);
+	ret = es58x_init_priv(es58x_dev, es58x_priv(netdev), channel_idx);
+	if (ret)
+		goto free_candev;
+	SET_NETDEV_DEVLINK_PORT(netdev, &es58x_priv(netdev)->devlink_port);
 
 	netdev->netdev_ops = &es58x_netdev_ops;
 	netdev->ethtool_ops = &es58x_ethtool_ops;
@@ -2092,16 +2105,20 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
 	netdev->dev_port = channel_idx;
 
 	ret = register_candev(netdev);
-	if (ret) {
-		es58x_dev->netdev[channel_idx] = NULL;
-		free_candev(netdev);
-		return ret;
-	}
+	if (ret)
+		goto devlink_port_unregister;
 
 	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
 				       es58x_dev->param->dql_min_limit);
 
 	return ret;
+
+ devlink_port_unregister:
+	devlink_port_unregister(&es58x_priv(netdev)->devlink_port);
+ free_candev:
+	es58x_dev->netdev[channel_idx] = NULL;
+	free_candev(netdev);
+	return ret;
 }
 
 /**
@@ -2118,6 +2135,7 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
 		if (!netdev)
 			continue;
 		unregister_candev(netdev);
+		devlink_port_unregister(&es58x_priv(netdev)->devlink_port);
 		es58x_dev->netdev[i] = NULL;
 		free_candev(netdev);
 	}
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index bf24375580e5..a76789119229 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -17,6 +17,7 @@
 #include <linux/netdevice.h>
 #include <linux/types.h>
 #include <linux/usb.h>
+#include <net/devlink.h>
 
 #include "es581_4.h"
 #include "es58x_fd.h"
@@ -230,6 +231,7 @@ union es58x_urb_cmd {
  * @can: struct can_priv must be the first member (Socket CAN relies
  *	on the fact that function netdev_priv() returns a pointer to
  *	a struct can_priv).
+ * @devlink_port: devlink instance for the network interface.
  * @es58x_dev: pointer to the corresponding ES58X device.
  * @tx_urb: Used as a buffer to concatenate the TX messages and to do
  *	a bulk send. Please refer to es58x_start_xmit() for more
@@ -255,6 +257,7 @@ union es58x_urb_cmd {
  */
 struct es58x_priv {
 	struct can_priv can;
+	struct devlink_port devlink_port;
 	struct es58x_device *es58x_dev;
 	struct urb *tx_urb;
 
-- 
2.37.4

[PATCH v5 3/7] USB: core: export usb_cache_string()

[Vincent Mailhol]

usb_cache_string() can also be useful for the drivers so export it.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
For reference, acked by Greg here:
https://lore.kernel.org/linux-usb/Y3zyCz5HbGdsxmRT@kroah.com/
---
 drivers/usb/core/message.c | 1 +
 drivers/usb/core/usb.h     | 1 -
 include/linux/usb.h        | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 4d59d927ae3e..127fac1af676 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1037,6 +1037,7 @@ char *usb_cache_string(struct usb_device *udev, int index)
 	}
 	return smallbuf;
 }
+EXPORT_SYMBOL_GPL(usb_cache_string);
 
 /*
  * usb_get_device_descriptor - (re)reads the device descriptor (usbcore)
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..0eac7d4285d1 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -47,7 +47,6 @@ extern int usb_get_device_descriptor(struct usb_device *dev,
 extern int usb_set_isoch_delay(struct usb_device *dev);
 extern int usb_get_bos_descriptor(struct usb_device *dev);
 extern void usb_release_bos_descriptor(struct usb_device *dev);
-extern char *usb_cache_string(struct usb_device *udev, int index);
 extern int usb_set_configuration(struct usb_device *dev, int configuration);
 extern int usb_choose_configuration(struct usb_device *udev);
 extern int usb_generic_driver_probe(struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 9ff1ad4dfad1..d2d2f41052c0 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1829,6 +1829,7 @@ static inline int usb_get_ptm_status(struct usb_device *dev, void *data)
 
 extern int usb_string(struct usb_device *dev, int index,
 	char *buf, size_t size);
+extern char *usb_cache_string(struct usb_device *udev, int index);
 
 /* wrappers that also update important state inside usbcore */
 extern int usb_clear_halt(struct usb_device *dev, int pipe);
-- 
2.37.4

[PATCH v5 4/7] net: devlink: add DEVLINK_INFO_VERSION_GENERIC_FW_BOOTLOADER

[Vincent Mailhol]

As discussed in [1], abbreviating the bootloader to "bl" might not be
well understood. Instead, a bootloader technically being a firmware,
name it "fw.bootloader".

Add a new macro to devlink.h to formalize this new info attribute name
and update the documentation.

[1] https://lore.kernel.org/netdev/20221128142723.2f826d20@kernel.org/

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
This was initially sent as a standalone patch here:

  https://lore.kernel.org/netdev/20221129031406.3849872-1-mailhol.vincent@wanadoo.fr/

Merging it to this series so that it is both added and used.
---
 Documentation/networking/devlink/devlink-info.rst | 5 +++++
 include/net/devlink.h                             | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
index 7572bf6de5c1..1242b0e6826b 100644
--- a/Documentation/networking/devlink/devlink-info.rst
+++ b/Documentation/networking/devlink/devlink-info.rst
@@ -198,6 +198,11 @@ fw.bundle_id
 
 Unique identifier of the entire firmware bundle.
 
+fw.bootloader
+-------------
+
+Version of the bootloader.
+
 Future work
 ===========
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 074a79b8933f..2f552b90b5c6 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -621,6 +621,8 @@ enum devlink_param_generic_id {
 #define DEVLINK_INFO_VERSION_GENERIC_FW_ROCE	"fw.roce"
 /* Firmware bundle identifier */
 #define DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID	"fw.bundle_id"
+/* Bootloader */
+#define DEVLINK_INFO_VERSION_GENERIC_FW_BOOTLOADER	"fw.bootloader"
 
 /**
  * struct devlink_flash_update_params - Flash Update parameters
-- 
2.37.4

[PATCH v5 5/7] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent Mailhol]

ES58x devices report below product information through a custom usb
string:

  * the firmware version
  * the bootloader version
  * the hardware revision

Parse this string, store the results in struct es58x_dev, export:

  * the firmware version through devlink's "fw" name
  * the bootloader version through devlink's "fw.bootloader" name
  * the hardware revisionthrough devlink's "board.rev" name

Those devlink entries are not critical to use the device, if parsing
fails, print an informative log message and continue to probe the
device.

In addition to that, use usb_device::serial to report the device
serial number.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c   |   1 +
 drivers/net/can/usb/etas_es58x/es58x_core.h   |  41 ++++
 .../net/can/usb/etas_es58x/es58x_devlink.c    | 222 ++++++++++++++++++
 3 files changed, 264 insertions(+)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index de884de9fe57..4d6d5a4ac06e 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2271,6 +2271,7 @@ static int es58x_probe(struct usb_interface *intf,
 	if (ret)
 		return ret;
 
+	es58x_parse_product_info(es58x_dev);
 	devlink_register(priv_to_devlink(es58x_dev));
 
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index a76789119229..c1ba1a4e8857 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -359,6 +359,39 @@ struct es58x_operators {
 	int (*get_timestamp)(struct es58x_device *es58x_dev);
 };
 
+/**
+ * struct es58x_sw_version - Version number of the firmware or the
+ *	bootloader.
+ * @major: Version major number, represented on two digits.
+ * @minor: Version minor number, represented on two digits.
+ * @revision: Version revision number, represented on two digits.
+ *
+ * The firmware and the bootloader share the same format: "xx.xx.xx"
+ * where 'x' is a digit. Both can be retrieved from the product
+ * information string.
+ */
+struct es58x_sw_version {
+	u8 major;
+	u8 minor;
+	u8 revision;
+};
+
+/**
+ * struct es58x_hw_revision - Hardware revision number.
+ * @letter: Revision letter.
+ * @major: Version major number, represented on three digits.
+ * @minor: Version minor number, represented on three digits.
+ *
+ * The hardware revision uses its own format: "axxx/xxx" where 'a' is
+ * a letter and 'x' a digit. It can be retrieved from the product
+ * information string.
+ */
+struct es58x_hw_revision {
+	char letter;
+	u16 major;
+	u16 minor;
+};
+
 /**
  * struct es58x_device - All information specific to an ES58X device.
  * @dev: Device information.
@@ -376,6 +409,9 @@ struct es58x_operators {
  *	queue wake/stop logic should prevent this URB from getting
  *	empty. Please refer to es58x_get_tx_urb() for more details.
  * @tx_urbs_idle_cnt: number of urbs in @tx_urbs_idle.
+ * @firmware_version: The firmware version number.
+ * @bootloader_version: The bootloader version number.
+ * @hardware_revision: The hardware revision number.
  * @ktime_req_ns: kernel timestamp when es58x_set_realtime_diff_ns()
  *	was called.
  * @realtime_diff_ns: difference in nanoseconds between the clocks of
@@ -411,6 +447,10 @@ struct es58x_device {
 	struct usb_anchor tx_urbs_idle;
 	atomic_t tx_urbs_idle_cnt;
 
+	struct es58x_sw_version firmware_version;
+	struct es58x_sw_version bootloader_version;
+	struct es58x_hw_revision hardware_revision;
+
 	u64 ktime_req_ns;
 	s64 realtime_diff_ns;
 
@@ -696,6 +736,7 @@ int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
 		   const void *msg, u16 cmd_len, int channel_idx);
 
 /* es58x_devlink.c. */
+void es58x_parse_product_info(struct es58x_device *es58x_dev);
 extern const struct devlink_ops es58x_dl_ops;
 
 /* es581_4.c. */
diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
index af6ca7ada23f..9fba29e2f57c 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_devlink.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
@@ -7,7 +7,229 @@
  * Copyright (c) 2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/usb.h>
 #include <net/devlink.h>
 
+#include "es58x_core.h"
+
+/* USB descriptor index containing the product information string. */
+#define ES58X_PROD_INFO_IDX 6
+
+/**
+ * es58x_parse_sw_version() - Extract boot loader or firmware version.
+ * @es58x_dev: ES58X device.
+ * @prod_info: USB custom string returned by the device.
+ * @prefix: Select which information should be parsed. Set it to "FW"
+ *	to parse the firmware version or to "BL" to parse the
+ *	bootloader version.
+ *
+ * The @prod_info string contains the firmware and the bootloader
+ * version number all prefixed by a magic string and concatenated with
+ * other numbers. Depending on the device, the firmware (bootloader)
+ * format is either "FW_Vxx.xx.xx" ("BL_Vxx.xx.xx") or "FW:xx.xx.xx"
+ * ("BL:xx.xx.xx") where 'x' represents a digit. @prod_info must
+ * contains the common part of those prefixes: "FW" or "BL".
+ *
+ * Parse @prod_info and store the version number in
+ * &es58x_dev.firmware_version or &es58x_dev.bootloader_version
+ * according to @prefix value.
+ *
+ * Return: zero on success, -EINVAL if @prefix contains an invalid
+ *	value and -EBADMSG if @prod_info could not be parsed.
+ */
+static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
+				  const char *prod_info, const char *prefix)
+{
+	struct es58x_sw_version *version;
+	int major, minor, revision;
+
+	if (!strcmp(prefix, "FW"))
+		version = &es58x_dev->firmware_version;
+	else if (!strcmp(prefix, "BL"))
+		version = &es58x_dev->bootloader_version;
+	else
+		return -EINVAL;
+
+	/* Go to prefix */
+	prod_info = strstr(prod_info, prefix);
+	if (!prod_info)
+		return -EBADMSG;
+	/* Go to beginning of the version number */
+	while (!isdigit(*prod_info)) {
+		prod_info++;
+		if (!*prod_info)
+			return -EBADMSG;
+	}
+
+	if (sscanf(prod_info, "%2u.%2u.%2u", &major, &minor, &revision) != 3)
+		return -EBADMSG;
+
+	version->major = major;
+	version->minor = minor;
+	version->revision = revision;
+
+	return 0;
+}
+
+/**
+ * es58x_parse_hw_rev() - Extract hardware revision number.
+ * @es58x_dev: ES58X device.
+ * @prod_info: USB custom string returned by the device.
+ *
+ * @prod_info contains the hardware revision prefixed by a magic
+ * string and conquenated together with other numbers. Depending on
+ * the device, the hardware revision format is either
+ * "HW_VER:axxx/xxx" or "HR:axxx/xxx" where 'a' represents a letter
+ * and 'x' a digit.
+ *
+ * Parse @prod_info and store the hardware revision number in
+ * &es58x_dev.hardware_revision.
+ *
+ * Return: zero on success, -EBADMSG if @prod_info could not be
+ *	parsed.
+ */
+static int es58x_parse_hw_rev(struct es58x_device *es58x_dev,
+			      const char *prod_info)
+{
+	char letter;
+	int major, minor;
+
+	/* The only occurrence of 'H' is in the hardware revision prefix. */
+	prod_info = strchr(prod_info, 'H');
+	if (!prod_info)
+		return -EBADMSG;
+	/* Go to beginning of the hardware revision */
+	prod_info = strchr(prod_info, ':');
+	if (!prod_info)
+		return -EBADMSG;
+	prod_info++;
+
+	if (sscanf(prod_info, "%c%3u/%3u", &letter, &major, &minor) != 3)
+		return -EBADMSG;
+
+	es58x_dev->hardware_revision.letter = letter;
+	es58x_dev->hardware_revision.major = major;
+	es58x_dev->hardware_revision.minor = minor;
+
+	return 0;
+}
+
+/**
+ * es58x_parse_product_info() - Parse the ES58x product information
+ *	string.
+ * @es58x_dev: ES58X device.
+ *
+ * Retrieve the product information string and parse it to extract the
+ * firmware version, the bootloader version and the hardware
+ * revision.
+ *
+ * If the function fails, simply emit a log message and continue
+ * because product information is not critical for the driver to
+ * operate.
+ */
+void es58x_parse_product_info(struct es58x_device *es58x_dev)
+{
+	char *prod_info;
+
+	prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+	if (!prod_info) {
+		dev_warn(es58x_dev->dev,
+			 "could not retrieve the product info string\n");
+		return;
+	}
+
+	if (es58x_parse_sw_version(es58x_dev, prod_info, "FW") ||
+	    es58x_parse_sw_version(es58x_dev, prod_info, "BL") ||
+	    es58x_parse_hw_rev(es58x_dev, prod_info))
+		dev_info(es58x_dev->dev,
+			 "could not parse product info: '%s'\n", prod_info);
+
+	kfree(prod_info);
+}
+
+/**
+ * es58x_sw_version_is_set() - Check if the version is a valid number.
+ * @sw_ver: Version number of either the firmware or the bootloader.
+ *
+ * If &es58x_sw_version.major, &es58x_sw_version.minor and
+ * &es58x_sw_version.revision are all zero, the product string could
+ * not be parsed and the version number is invalid.
+ */
+static inline bool es58x_sw_version_is_set(struct es58x_sw_version *sw_ver)
+{
+	return sw_ver->major || sw_ver->minor || sw_ver->revision;
+}
+
+/**
+ * es58x_hw_revision_is_set() - Check if the revision is a valid number.
+ * @hw_rev: Revision number of the hardware.
+ *
+ * If &es58x_hw_revision.letter is the null character, the product
+ * string could not be parsed and the hardware revision number is
+ * invalid.
+ */
+static inline bool es58x_hw_revision_is_set(struct es58x_hw_revision *hw_rev)
+{
+	return hw_rev->letter != '\0';
+}
+
+/**
+ * es58x_devlink_info_get() - Report the product information.
+ * @devlink: Devlink.
+ * @req: skb wrapper where to put requested information.
+ * @extack: Unused.
+ *
+ * Report the firmware version, the bootloader version, the hardware
+ * revision and the serial number through netlink.
+ *
+ * Return: zero on success, errno when any error occurs.
+ */
+static int es58x_devlink_info_get(struct devlink *devlink,
+				  struct devlink_info_req *req,
+				  struct netlink_ext_ack *extack)
+{
+	struct es58x_device *es58x_dev = devlink_priv(devlink);
+	struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
+	struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
+	struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
+	char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
+	int ret = 0;
+
+	if (es58x_sw_version_is_set(fw_ver)) {
+		snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
+			 fw_ver->major, fw_ver->minor, fw_ver->revision);
+		ret = devlink_info_version_running_put(req,
+						       DEVLINK_INFO_VERSION_GENERIC_FW,
+						       buf);
+		if (ret)
+			return ret;
+	}
+
+	if (es58x_sw_version_is_set(bl_ver)) {
+		snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
+			 bl_ver->major, bl_ver->minor, bl_ver->revision);
+		ret = devlink_info_version_running_put(req,
+						       DEVLINK_INFO_VERSION_GENERIC_FW_BOOTLOADER,
+						       buf);
+		if (ret)
+			return ret;
+	}
+
+	if (es58x_hw_revision_is_set(hw_rev)) {
+		snprintf(buf, sizeof(buf), "%c%03u/%03u",
+			 hw_rev->letter, hw_rev->major, hw_rev->minor);
+		ret = devlink_info_version_fixed_put(req,
+						     DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,
+						     buf);
+		if (ret)
+			return ret;
+	}
+
+	return devlink_info_serial_number_put(req, es58x_dev->udev->serial);
+}
+
 const struct devlink_ops es58x_dl_ops = {
+	.info_get = es58x_devlink_info_get,
 };
-- 
2.37.4

[PATCH v5 6/7] can: etas_es58x: remove es58x_get_product_info()

[Vincent Mailhol]

Now that the product information are available under devlink, no more
need to print them in the kernel log. Remove es58x_get_product_info().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 52 ++-------------------
 1 file changed, 3 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 4d6d5a4ac06e..0c7f7505632c 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2141,48 +2141,6 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
 	}
 }
 
-/**
- * es58x_get_product_info() - Get the product information and print them.
- * @es58x_dev: ES58X device.
- *
- * Do a synchronous call to get the product information.
- *
- * Return: zero on success, errno when any error occurs.
- */
-static int es58x_get_product_info(struct es58x_device *es58x_dev)
-{
-	struct usb_device *udev = es58x_dev->udev;
-	const int es58x_prod_info_idx = 6;
-	/* Empirical tests show a prod_info length of maximum 83,
-	 * below should be more than enough.
-	 */
-	const size_t prod_info_len = 127;
-	char *prod_info;
-	int ret;
-
-	prod_info = kmalloc(prod_info_len, GFP_KERNEL);
-	if (!prod_info)
-		return -ENOMEM;
-
-	ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
-	if (ret < 0) {
-		dev_err(es58x_dev->dev,
-			"%s: Could not read the product info: %pe\n",
-			__func__, ERR_PTR(ret));
-		goto out_free;
-	}
-	if (ret >= prod_info_len - 1) {
-		dev_warn(es58x_dev->dev,
-			 "%s: Buffer is too small, result might be truncated\n",
-			 __func__);
-	}
-	dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
-
- out_free:
-	kfree(prod_info);
-	return ret < 0 ? ret : 0;
-}
-
 /**
  * es58x_init_es58x_dev() - Initialize the ES58X device.
  * @intf: USB interface.
@@ -2261,28 +2219,24 @@ static int es58x_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
 	struct es58x_device *es58x_dev;
-	int ch_idx, ret;
+	int ch_idx;
 
 	es58x_dev = es58x_init_es58x_dev(intf, id->driver_info);
 	if (IS_ERR(es58x_dev))
 		return PTR_ERR(es58x_dev);
 
-	ret = es58x_get_product_info(es58x_dev);
-	if (ret)
-		return ret;
-
 	es58x_parse_product_info(es58x_dev);
 	devlink_register(priv_to_devlink(es58x_dev));
 
 	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
-		ret = es58x_init_netdev(es58x_dev, ch_idx);
+		int ret = es58x_init_netdev(es58x_dev, ch_idx);
 		if (ret) {
 			es58x_free_netdevs(es58x_dev);
 			return ret;
 		}
 	}
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.37.4

[PATCH v5 7/7] Documentation: devlink: add devlink documentation for the etas_es58x driver

[Vincent Mailhol]

List all the version information reported by the etas_es58x driver
through devlink. Also, update MAINTAINERS with the newly created file.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 .../networking/devlink/etas_es58x.rst         | 36 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 37 insertions(+)
 create mode 100644 Documentation/networking/devlink/etas_es58x.rst

diff --git a/Documentation/networking/devlink/etas_es58x.rst b/Documentation/networking/devlink/etas_es58x.rst
new file mode 100644
index 000000000000..9893e57b625a
--- /dev/null
+++ b/Documentation/networking/devlink/etas_es58x.rst
@@ -0,0 +1,36 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==========================
+etas_es58x devlink support
+==========================
+
+This document describes the devlink features implemented by the
+``etas_es58x`` device driver.
+
+Info versions
+=============
+
+The ``etas_es58x`` driver reports the following versions
+
+.. list-table:: devlink info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``fw``
+     - running
+     - Version of the firmware running on the device. Also available
+       through ``ethtool -i`` as the first member of the
+       ``firmware-version``.
+   * - ``bl``
+     - running
+     - Version of the bootloader running on the device. Also available
+       through ``ethtool -i`` as the second member of the
+       ``firmware-version``.
+   * - ``board.rev``
+     - fixed
+     - The hardware revision of the device.
+   * - ``serial_number``
+     - fixed
+     - The USB serial number. Also available through ``lsusb -v``.
diff --git a/MAINTAINERS b/MAINTAINERS
index 955c1be1efb2..71f4f8776779 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7682,6 +7682,7 @@ ETAS ES58X CAN/USB DRIVER
 M:	Vincent Mailhol <mailhol.vincent@wanadoo.fr>
 L:	linux-can@vger.kernel.org
 S:	Maintained
+F:	Documentation/networking/devlink/etas_es58x.rst
 F:	drivers/net/can/usb/etas_es58x/
 
 ETHERNET BRIDGE
-- 
2.37.4

Re: [PATCH v5 7/7] Documentation: devlink: add devlink documentation for the etas_es58x driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

On 01.12.2022 02:46:58, Vincent Mailhol wrote:
> List all the version information reported by the etas_es58x driver
> through devlink. Also, update MAINTAINERS with the newly created file.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  .../networking/devlink/etas_es58x.rst         | 36 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 37 insertions(+)
>  create mode 100644 Documentation/networking/devlink/etas_es58x.rst
> 
> diff --git a/Documentation/networking/devlink/etas_es58x.rst b/Documentation/networking/devlink/etas_es58x.rst
> new file mode 100644
> index 000000000000..9893e57b625a
> --- /dev/null
> +++ b/Documentation/networking/devlink/etas_es58x.rst
> @@ -0,0 +1,36 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==========================
> +etas_es58x devlink support
> +==========================
> +
> +This document describes the devlink features implemented by the
> +``etas_es58x`` device driver.
> +
> +Info versions
> +=============
> +
> +The ``etas_es58x`` driver reports the following versions
> +
> +.. list-table:: devlink info versions implemented
> +   :widths: 5 5 90
> +
> +   * - Name
> +     - Type
> +     - Description
> +   * - ``fw``
> +     - running
> +     - Version of the firmware running on the device. Also available
> +       through ``ethtool -i`` as the first member of the
> +       ``firmware-version``.
> +   * - ``bl``
            ^^
            fw.bootloader?

Fixed that up while applying.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 7/7] Documentation: devlink: add devlink documentation for the etas_es58x driver

[Vincent MAILHOL]

On Fri 2 Dec. 2022 at 21:49, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 01.12.2022 02:46:58, Vincent Mailhol wrote:
> > List all the version information reported by the etas_es58x driver
> > through devlink. Also, update MAINTAINERS with the newly created file.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> >  .../networking/devlink/etas_es58x.rst         | 36 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 37 insertions(+)
> >  create mode 100644 Documentation/networking/devlink/etas_es58x.rst
> >
> > diff --git a/Documentation/networking/devlink/etas_es58x.rst b/Documentation/networking/devlink/etas_es58x.rst
> > new file mode 100644
> > index 000000000000..9893e57b625a
> > --- /dev/null
> > +++ b/Documentation/networking/devlink/etas_es58x.rst
> > @@ -0,0 +1,36 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +==========================
> > +etas_es58x devlink support
> > +==========================
> > +
> > +This document describes the devlink features implemented by the
> > +``etas_es58x`` device driver.
> > +
> > +Info versions
> > +=============
> > +
> > +The ``etas_es58x`` driver reports the following versions
> > +
> > +.. list-table:: devlink info versions implemented
> > +   :widths: 5 5 90
> > +
> > +   * - Name
> > +     - Type
> > +     - Description
> > +   * - ``fw``
> > +     - running
> > +     - Version of the firmware running on the device. Also available
> > +       through ``ethtool -i`` as the first member of the
> > +       ``firmware-version``.
> > +   * - ``bl``
>             ^^
>             fw.bootloader?
>
> Fixed that up while applying.

Thanks for catching this. "fw" was the name in v4. "fw.bootloader" is
indeed correct.


Yours sincerely,
Vincent Mailhol