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

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

[Oliver Neukum]

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>
---
 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)
 	u8 *buf;
 	int i;
 	int ret = -EINVAL;
+	int allocated = 0;
 
 	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;
 }
-- 
2.53.0

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

[Szymon Wilczek]

Thanks for catching this and improving the logic. It looks correct to me.

Acked-by: Szymon Wilczek <swilczek.lx@gmail.com>

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

[Vincent Mailhol]

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