Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 5 Nov 2022 09:21:51 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc:     linux-can@vger.kernel.org, Marc Kleine-Budde <mkl@pengutronix.de>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v2 2/3] can: etas_es58x: use usb_cache_string() to
 retrieve the product info string
Message-ID: <Y2YdH4dd8u/eUEXg@kroah.com>
References: <20221104073659.414147-1-mailhol.vincent@wanadoo.fr>
 <20221104171604.24052-1-mailhol.vincent@wanadoo.fr>
 <20221104171604.24052-3-mailhol.vincent@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221104171604.24052-3-mailhol.vincent@wanadoo.fr>
Precedence: bulk

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