[PATCH net-next v2] ch9200: do return USB errors from control_write()

[PATCH net-next v2] ch9200: do return USB errors from control_write()

[Sergey Shtylyov]

Compared with control_read(), control_write() looks really strange:
it ignores any errors returned by usb_control_msg(), always returning
0 instead, despite overriding a positive result of usb_control_msg()
(indicating short transfer) to -EINVAL before doing that. Drop that
dubious *return* and propagate USB errors to the callers...

Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>

---
Changes in version 2:
- dropped the Fixes tag, retargeting the patch to the net-next.git repo;
- dropped [RFT] from the subject.

 drivers/net/usb/ch9200.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index a206ffa76f1b..3a81e9e96fd3 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -168,8 +168,6 @@ static int control_write(struct usbnet *dev, unsigned char request,
 		err = -EINVAL;
 	kfree(buf);
 
-	return 0;
-
 err_out:
 	return err;
 }
-- 
2.55.0

Re: [PATCH net-next v2] ch9200: do return USB errors from control_write()

[Andrew Lunn]

On Tue, Sep 15, 2026 at 09:58:04PM +0300, Sergey Shtylyov wrote:
> Compared with control_read(), control_write() looks really strange:
> it ignores any errors returned by usb_control_msg(), always returning
> 0 instead, despite overriding a positive result of usb_control_msg()
> (indicating short transfer) to -EINVAL before doing that. Drop that
> dubious *return* and propagate USB errors to the callers...
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2] ch9200: do return USB errors from control_write()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: after removing `return 0;` from control_write()
  (drivers/net/usb/ch9200.c:137-171), the helper now returns the…

--

ch9200: do return USB errors from control_write()

This removes the unconditional "return 0;" at the end of control_write()
in drivers/net/usb/ch9200.c so that the function falls through to
"return err;" instead. The stated intent is to propagate usb_control_msg()
errors to the callers.

> diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
> index a206ffa76f1b9..3a81e9e96fd33 100644
> --- a/drivers/net/usb/ch9200.c
> +++ b/drivers/net/usb/ch9200.c
> @@ -168,8 +168,6 @@ static int control_write(struct usbnet *dev, unsigned char request,
>  		err = -EINVAL;
>  	kfree(buf);
>  
> -	return 0;
> -
>  err_out:
>  	return err;
>  }

[Severity: High]
The commit message says this will "propagate USB errors to the callers",
but do any of the callers actually consume the value?

There are two in-tree callers.  ch9200_mdio_write() is void and calls it
as a bare statement:

drivers/net/usb/ch9200.c:ch9200_mdio_write() {
	...
	control_write(dev, REQUEST_WRITE, 0, loc * 2, buff, 0x02,
		      CONTROL_TIMEOUT_MS);
}

ch9200_bind() stores the result six times and overwrites it every time,
with the last store clobbered by get_mac_address():

drivers/net/usb/ch9200.c:ch9200_bind() {
	...
	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
			       0x02, CONTROL_TIMEOUT_MS);
	...
	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_CTRL, data, 0x02,
			       CONTROL_TIMEOUT_MS);

	retval = get_mac_address(dev, addr);
	eth_hw_addr_set(dev->net, addr);

	return retval;
}

Since usbnet_probe() only aborts on a negative bind() status:

drivers/net/usb/usbnet.c:usbnet_probe() {
	...
		status = info->bind(dev, udev);
		if (status < 0)
			goto out1;
}

bind still reports success when every register write failed, for example
when a failed MAC_REG_THRESHOLD/MAC_REG_FIFO_DEPTH write is followed by a
successful MAC_REG_CTRL = 0x0D5F write that arms DMA/receiver/transmitter
with unprogrammed FIFO and flow-control watermarks, or when the
MAC_REG_CTRL write itself fails and the netdev is registered with the
datapath never enabled.  Is the observable behaviour of the driver changed
at all by this patch?

There is a second point about the resulting contract.  On success
usb_control_msg() returns the transferred length, so control_write() now
returns a positive value (2 for these register writes) rather than 0:

	err = usb_control_msg(dev->udev, ...);
	if (err >= 0 && err < size)
		err = -EINVAL;

Would the obvious caller-side follow-up "if (retval) return retval;" then
treat every successful 2-byte register write as a failure?  Would it be
better to normalize success to 0 in control_write(), or to check
"retval < 0" in each caller and update the callers in the same patch so
the errors being returned are actually acted upon?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915185805.38712-1-s.shtylyov%40auroraos.dev

Re: [PATCH net-next v2] ch9200: do return USB errors from control_write()

[Sergey Shtylyov]

On 9/16/26 10:23 AM, netdev-bot+sashiko@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> - [High] Incomplete fix: after removing `return 0;` from control_write()
>   (drivers/net/usb/ch9200.c:137-171), the helper now returns the…
> 
> --
> 
> ch9200: do return USB errors from control_write()
> 
> This removes the unconditional "return 0;" at the end of control_write()
> in drivers/net/usb/ch9200.c so that the function falls through to
> "return err;" instead. The stated intent is to propagate usb_control_msg()
> errors to the callers.
> 
>> diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
>> index a206ffa76f1b9..3a81e9e96fd33 100644
>> --- a/drivers/net/usb/ch9200.c
>> +++ b/drivers/net/usb/ch9200.c
>> @@ -168,8 +168,6 @@ static int control_write(struct usbnet *dev, unsigned char request,
>>               err = -EINVAL;
>>       kfree(buf);
>>
>> -     return 0;
>> -
>>  err_out:
>>       return err;
>>  }
> 
> [Severity: High]

   But why? 8-)

> The commit message says this will "propagate USB errors to the callers",
> but do any of the callers actually consume the value?

   Most will -- after another patch:

https://lore.kernel.org/all/20260914202503.52920-1-s.shtylyov@auroraos.dev/

[...]

> There is a second point about the resulting contract.  On success
> usb_control_msg() returns the transferred length, so control_write() now
> returns a positive value (2 for these register writes) rather than 0:
> 
>         err = usb_control_msg(dev->udev, ...);
>         if (err >= 0 && err < size)
>                 err = -EINVAL;
> 
> Would the obvious caller-side follow-up "if (retval) return retval;" then
> treat every successful 2-byte register write as a failure?  Would it be
> better to normalize success to 0 in control_write(), or to check
> "retval < 0" in each caller and update the callers in the same patch so
> the errors being returned are actually acted upon?
   That's what the patch I linked to does...

[...]

MBR, Sergey