[v2] ch9200: do return USB errors from control_write()

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

Posted by Sergey Shtylyov 1 week, 2 days ago

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()

Posted by netdev-bot+sashiko@kernel.org 1 week, 1 day ago

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()

Posted by Sergey Shtylyov 1 week, 1 day ago

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()

Posted by Jakub Kicinski 6 days, 4 hours ago

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.

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

Posted by Sergey Shtylyov 3 days, 10 hours ago

On 9/19/26 4:21 AM, Jakub Kicinski 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.

   OK. Series against net-next, right?

> We have too many patches in flight these days to be chasing people's
> linked series. Too many LLM-happy "helpers".

   IMHO these 2 patches are not so heavily linked, that's why I posted
them separately... And no AI was involved, of course. :-)

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

   As I noted in the description of the patch [1], the issue was reported
by the static analyzer. For the patch [2], I saw a problem with my own eyes.
   I don't have the hardware, so no testing; there was some hope that e.g.
Matthew Garret (the driver's original submitter) could be able to test but
his email bounced...

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

MBR, Sergey

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

Posted by Jakub Kicinski 3 days, 9 hours ago

On Mon, 21 Sep 2026 22:59:10 +0300 Sergey Shtylyov wrote:
> >>    That's what the patch I linked to does...  
> > 
> > Please fix the issues in one series or not at all.  
> 
>    OK. Series against net-next, right?

yes

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

Posted by Sergey Shtylyov 3 days, 9 hours ago

On 9/21/26 10:59 PM, Sergey Shtylyov wrote:
[...]

>> Please explain how you discovered the issue and how you tested the fix.
> 
>    As I noted in the description of the patch [1], the issue was reported
> by the static analyzer. For the patch [2], I saw a problem with my own eyes.
>    I don't have the hardware, so no testing; there was some hope that e.g.
> Matthew Garret (the driver's original submitter) could be able to test but
> his email bounced...

   Sorry, it's Garrett. From searching on lore it appears he still uses that
same email, so not sure why it bounced...
 
[...]

MBR, Sergey

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

Posted by Andrew Lunn 1 week, 2 days ago

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