Re: [PATCH] net: can: es58x: fully try proceeding with partial allocations

[Vincent Mailhol <mailhol@kernel.org>]

On 11/02/2026 at 11:40, Oliver Neukum wrote:
> Since b1979778e985 ("can: etas_es58x: allow partial RX URB
> allocation to succeed") es58x_alloc_rx_urbs() sees a failure
> to allocate and submit the full number of URBs as a success
> and the driver will continue at reduced performance.
> 
> However, if you do so, there is no point in abandoning further
> allocations or submissions, just because an earlier one failed.
> 
> Fixes: b1979778e985 ("can: etas_es58x: allow partial RX URB allocation to succeed")
> Signed-off-by: Oliver Neukum <oneukum@suse.com>

Thanks. I left some minor comments, but the logic is already
good. Here is my review tag in advance:

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>

> ---
>  drivers/net/can/usb/etas_es58x/es58x_core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index 19fa51821a89..9dc66932267f 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -1706,12 +1706,13 @@ static int es58x_alloc_rx_urbs(struct es58x_device *es58x_dev)

While at it, can you fix the documentation (which was already
incorrect in the last commmit)?

- * Return: zero on success, errno when any error occurs.
+ * Return: zero if one or more allocation succeeded, errno if all of
+ *	them failed.

>  	u8 *buf;
>  	int i;
>  	int ret = -EINVAL;
> +	int allocated = 0;

Nitpick: put allocated next to i as they are both related:

	int i, allocated = 0;

or

	int allocated;
	int i = 0;

at your convenience.

>  
>  	for (i = 0; i < param->rx_urb_max; i++) {
>  		ret = es58x_alloc_urb(es58x_dev, &urb, &buf, rx_buf_len,
>  				      GFP_KERNEL);
>  		if (ret)
> -			break;
> +			continue;
>  
>  		usb_fill_bulk_urb(urb, es58x_dev->udev, es58x_dev->rx_pipe,
>  				  buf, rx_buf_len, es58x_read_bulk_callback,
> @@ -1723,18 +1724,18 @@ static int es58x_alloc_rx_urbs(struct es58x_device *es58x_dev)
>  			usb_unanchor_urb(urb);
>  			usb_free_coherent(es58x_dev->udev, rx_buf_len,
>  					  buf, urb->transfer_dma);
> -			usb_free_urb(urb);
> -			break;
> +		} else {
> +			allocated++;
>  		}
>  		usb_free_urb(urb);
>  	}
>  
> -	if (i == 0) {
> +	if (allocated == 0) {
>  		dev_err(dev, "%s: Could not setup any rx URBs\n", __func__);
>  		return ret;
>  	}
>  	dev_dbg(dev, "%s: Allocated %d rx URBs each of size %u\n",
> -		__func__, i, rx_buf_len);
> +		__func__, allocated, rx_buf_len);
>  
>  	return 0;
>  }


Yours sincerely,
Vincent Mailhol