can: etas_es58x: report the firmware version through ethtool
Commit Message
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_print_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 | 140 +++++++++++++-------
drivers/net/can/usb/etas_es58x/es58x_core.h | 11 +-
drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +-
4 files changed, 108 insertions(+), 53 deletions(-)
Comments
On 04.11.2022 16:36:59, 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_print_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 | 140 +++++++++++++-------
> drivers/net/can/usb/etas_es58x/es58x_core.h | 11 +-
> drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +-
> 4 files changed, 108 insertions(+), 53 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..1d6ae7b279cf 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 = ','
Nitpick: you can add a trailing "," here, makes the diff smaller on the
next change :)
> };
>
> 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 51294b717040..7c20a73169f3 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,10 +1978,6 @@ static const struct net_device_ops es58x_netdev_ops = {
> .ndo_eth_ioctl = can_eth_ioctl_hwts,
> };
>
> -static const struct ethtool_ops es58x_ethtool_ops = {
> - .get_ts_info = can_ethtool_op_get_ts_info_hwts,
> -};
> -
> /**
> * es58x_set_mode() - Change network device mode.
> * @netdev: CAN network device.
> @@ -2062,6 +2058,96 @@ static void es58x_init_priv(struct es58x_device *es58x_dev,
> can->do_set_mode = es58x_set_mode;
> }
>
> +/**
> + * es58x_get_product_info() - Get the product information.
> + * @es58x_dev: ES58X device.
> + * @prod_info: Buffer where to store the product information.
> + * @prod_info_len: Length of @prod_info.
> + *
> + * 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,
> + char *prod_info, size_t prod_info_len)
> +{
> + struct usb_device *udev = es58x_dev->udev;
> + const int es58x_prod_info_idx = 6;
> + int ret;
> +
> + 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));
> + return ret;
> + }
> + if (ret >= prod_info_len - 1) {
> + dev_warn(es58x_dev->dev,
> + "%s: Buffer is too small, result might be truncated\n",
> + __func__);
> + }
You can use usb_cache_string() that puts the requested string into a
kmalloc()ed buffer:
| https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/core/message.c#L1018
...and avoids having the large stack frame.
> +
> + return 0;
> +}
> +
> +/**
> + * es58x_print_product_info() - Print the product information.
> + * @es58x_dev: ES58X device.
> + *
> + * Return: zero on success, errno when any error occurs.
> + */
> +static int es58x_print_product_info(struct es58x_device *es58x_dev)
> +{
> + char prod_info[ES58X_USB_STRING_SIZE];
Stack size in the kernel is limited.
> + int ret;
> +
> + ret = es58x_get_product_info(es58x_dev, prod_info, sizeof(prod_info));
> + if (ret == 0)
> + dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
> +
> + return ret;
> +}
> +
> +/**
> + * 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 firwmware 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[ES58X_USB_STRING_SIZE];
> + char *start, *end;
> +
> + strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
> +
> + if (es58x_get_product_info(es58x_dev, prod_info, sizeof(prod_info)) < 0)
> + return;
> +
> + /* The firmware prefix is either "FW_V" or "FW:" */
> + start = strstr(prod_info, "FW");
> + if (!start)
> + return;
> + /* Go to first digit */
> + while (!isdigit(*start))
> + start++;
Don't trust the input. Check for end of string before accessing it.
> +
> + end = strchr(start, es58x_dev->param->prod_info_delim);
> + if (!end || end - start >= sizeof(drvinfo->fw_version))
> + return;
> +
> + strscpy(drvinfo->fw_version, start, end - start + 1);
Are you misusing strscpy() here? The last parameter should be the size
of the dest buffer, not the src buffer.
> +}
> +
> +static const struct ethtool_ops es58x_ethtool_ops = {
> + .get_drvinfo = es58x_get_drvinfo,
> + .get_ts_info = can_ethtool_op_get_ts_info_hwts,
> +};
> +
> /**
> * es58x_init_netdev() - Initialize the network device.
> * @es58x_dev: ES58X device.
> @@ -2119,48 +2205,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,7 +2287,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);
> + ret = es58x_print_product_info(es58x_dev);
> if (ret)
> return ret;
>
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
> index 640fe0a1df63..3a9f6582c06d 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__
> @@ -49,6 +49,12 @@
> /* A magic number sent by the ES581.4 to inform it is alive. */
> #define ES58X_HEARTBEAT 0x11
>
> +/* USB strings are at most 127 characters and es58x devices only use
> + * ASCII (i.e. one byte). Also, the maximum observed length is 83
> + * bytes.
> + */
> +#define ES58X_USB_STRING_SIZE (127 + 1)
> +
> /**
> * enum es58x_driver_info - Quirks of the device.
> * @ES58X_DUAL_CHANNEL: Device has two CAN channels. If this flag is
> @@ -306,6 +312,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;
> @@ -324,6 +332,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..3d781b89df4a 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
Marc
On Fri 4 nov. 2022 at 21:06, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 04.11.2022 16:36:59, 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_print_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 | 140 +++++++++++++-------
> > drivers/net/can/usb/etas_es58x/es58x_core.h | 11 +-
> > drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +-
> > 4 files changed, 108 insertions(+), 53 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..1d6ae7b279cf 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 = ','
>
> Nitpick: you can add a trailing "," here, makes the diff smaller on the
> next change :)
ACK.
> > };
> >
> > 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 51294b717040..7c20a73169f3 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,10 +1978,6 @@ static const struct net_device_ops es58x_netdev_ops = {
> > .ndo_eth_ioctl = can_eth_ioctl_hwts,
> > };
> >
> > -static const struct ethtool_ops es58x_ethtool_ops = {
> > - .get_ts_info = can_ethtool_op_get_ts_info_hwts,
> > -};
> > -
> > /**
> > * es58x_set_mode() - Change network device mode.
> > * @netdev: CAN network device.
> > @@ -2062,6 +2058,96 @@ static void es58x_init_priv(struct es58x_device *es58x_dev,
> > can->do_set_mode = es58x_set_mode;
> > }
> >
> > +/**
> > + * es58x_get_product_info() - Get the product information.
> > + * @es58x_dev: ES58X device.
> > + * @prod_info: Buffer where to store the product information.
> > + * @prod_info_len: Length of @prod_info.
> > + *
> > + * 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,
> > + char *prod_info, size_t prod_info_len)
> > +{
> > + struct usb_device *udev = es58x_dev->udev;
> > + const int es58x_prod_info_idx = 6;
> > + int ret;
> > +
> > + 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));
> > + return ret;
> > + }
> > + if (ret >= prod_info_len - 1) {
> > + dev_warn(es58x_dev->dev,
> > + "%s: Buffer is too small, result might be truncated\n",
> > + __func__);
> > + }
>
> You can use usb_cache_string() that puts the requested string into a
> kmalloc()ed buffer:
>
> | https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/core/message.c#L1018
>
> ...and avoids having the large stack frame.
I saw that one a long time ago, before I started sending patches on
the mailing list, but couldn't use it because it is not
EXPORT_SYMBOL_GPLed. I was also too shy to send a patch to change
it...
I guess I will add the export and use it.
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * es58x_print_product_info() - Print the product information.
> > + * @es58x_dev: ES58X device.
> > + *
> > + * Return: zero on success, errno when any error occurs.
> > + */
> > +static int es58x_print_product_info(struct es58x_device *es58x_dev)
> > +{
> > + char prod_info[ES58X_USB_STRING_SIZE];
>
> Stack size in the kernel is limited.
'make checkstack' tels me:
0x00000000000008300 es58x_get_drvinfo []: 448
0x00000000000003ae0 es58x_print_product_info []: 448
My understanding is that anything under 512 is still acceptable. c.f.
https://www.kernel.org/doc/html/v4.12/process/submit-checklist.html
Regardless, I agree that using usb_cache_string() is better.
> > + int ret;
> > +
> > + ret = es58x_get_product_info(es58x_dev, prod_info, sizeof(prod_info));
> > + if (ret == 0)
> > + dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * 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 firwmware 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[ES58X_USB_STRING_SIZE];
> > + char *start, *end;
> > +
> > + strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
> > +
> > + if (es58x_get_product_info(es58x_dev, prod_info, sizeof(prod_info)) < 0)
> > + return;
> > +
> > + /* The firmware prefix is either "FW_V" or "FW:" */
> > + start = strstr(prod_info, "FW");
> > + if (!start)
> > + return;
> > + /* Go to first digit */
> > + while (!isdigit(*start))
> > + start++;
>
> Don't trust the input. Check for end of string before accessing it.
You are right. Now I feel ashamed of making this mistake.
> > +
> > + end = strchr(start, es58x_dev->param->prod_info_delim);
> > + if (!end || end - start >= sizeof(drvinfo->fw_version))
> > + return;
> > +
> > + strscpy(drvinfo->fw_version, start, end - start + 1);
>
> Are you misusing strscpy() here? The last parameter should be the size
> of the dest buffer, not the src buffer.
Indeed, the documentation clearly specifies that it should be the size
of the destination. I will use strncpy() instead. I already checked
that (end - start) is strictly smaller than the destination size above
so it will be fine.
Yours sincerely,
Vincent Mailhol
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
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
> 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
@@ -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 = {
@@ -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,10 +1978,6 @@ static const struct net_device_ops es58x_netdev_ops = {
.ndo_eth_ioctl = can_eth_ioctl_hwts,
};
-static const struct ethtool_ops es58x_ethtool_ops = {
- .get_ts_info = can_ethtool_op_get_ts_info_hwts,
-};
-
/**
* es58x_set_mode() - Change network device mode.
* @netdev: CAN network device.
@@ -2062,6 +2058,96 @@ static void es58x_init_priv(struct es58x_device *es58x_dev,
can->do_set_mode = es58x_set_mode;
}
+/**
+ * es58x_get_product_info() - Get the product information.
+ * @es58x_dev: ES58X device.
+ * @prod_info: Buffer where to store the product information.
+ * @prod_info_len: Length of @prod_info.
+ *
+ * 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,
+ char *prod_info, size_t prod_info_len)
+{
+ struct usb_device *udev = es58x_dev->udev;
+ const int es58x_prod_info_idx = 6;
+ int ret;
+
+ 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));
+ return ret;
+ }
+ if (ret >= prod_info_len - 1) {
+ dev_warn(es58x_dev->dev,
+ "%s: Buffer is too small, result might be truncated\n",
+ __func__);
+ }
+
+ return 0;
+}
+
+/**
+ * es58x_print_product_info() - Print the product information.
+ * @es58x_dev: ES58X device.
+ *
+ * Return: zero on success, errno when any error occurs.
+ */
+static int es58x_print_product_info(struct es58x_device *es58x_dev)
+{
+ char prod_info[ES58X_USB_STRING_SIZE];
+ int ret;
+
+ ret = es58x_get_product_info(es58x_dev, prod_info, sizeof(prod_info));
+ if (ret == 0)
+ dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
+
+ return ret;
+}
+
+/**
+ * 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 firwmware 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[ES58X_USB_STRING_SIZE];
+ char *start, *end;
+
+ strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
+
+ if (es58x_get_product_info(es58x_dev, prod_info, sizeof(prod_info)) < 0)
+ return;
+
+ /* The firmware prefix is either "FW_V" or "FW:" */
+ start = strstr(prod_info, "FW");
+ if (!start)
+ return;
+ /* Go to first digit */
+ while (!isdigit(*start))
+ start++;
+
+ end = strchr(start, es58x_dev->param->prod_info_delim);
+ if (!end || end - start >= sizeof(drvinfo->fw_version))
+ return;
+
+ strscpy(drvinfo->fw_version, start, end - start + 1);
+}
+
+static const struct ethtool_ops es58x_ethtool_ops = {
+ .get_drvinfo = es58x_get_drvinfo,
+ .get_ts_info = can_ethtool_op_get_ts_info_hwts,
+};
+
/**
* es58x_init_netdev() - Initialize the network device.
* @es58x_dev: ES58X device.
@@ -2119,48 +2205,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,7 +2287,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);
+ ret = es58x_print_product_info(es58x_dev);
if (ret)
return ret;
@@ -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__
@@ -49,6 +49,12 @@
/* A magic number sent by the ES581.4 to inform it is alive. */
#define ES58X_HEARTBEAT 0x11
+/* USB strings are at most 127 characters and es58x devices only use
+ * ASCII (i.e. one byte). Also, the maximum observed length is 83
+ * bytes.
+ */
+#define ES58X_USB_STRING_SIZE (127 + 1)
+
/**
* enum es58x_driver_info - Quirks of the device.
* @ES58X_DUAL_CHANNEL: Device has two CAN channels. If this flag is
@@ -306,6 +312,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;
@@ -324,6 +332,7 @@ struct es58x_parameters {
u8 urb_cmd_header_len;
u8 rx_urb_max;
u8 tx_urb_max;
+ char prod_info_delim;
};
/**
@@ -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 = {