On Fri 4 nov. 2022 at 21:06, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > */
> >
> > #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@xxxxxxxxxx>
> > + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > */
> >
> > #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
