Message-ID: <20221104115211.533zif7ip4f5tvky@pengutronix.de>
Date:   Fri, 4 Nov 2022 12:52:11 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc:     linux-can@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] can: etas_es58x: report the firmware version through
 ethtool

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@...adoo.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@...adoo.fr>
> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@...adoo.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@...adoo.fr>
> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@...adoo.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@...adoo.fr>
> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@...adoo.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@...adoo.fr>
> + * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@...adoo.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

-- 
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 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists