[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

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

[Jakub Kicinski]

On Wed, 16 Sep 2026 19:21:04 +0300 Sergey Shtylyov wrote:
> > 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...

Please fix the issues in one series or not at all.
We have too many patches in flight these days to be chasing people's
linked series. Too many LLM-happy "helpers".

Please explain how you discovered the issue and how you tested the fix.