[PATCH net RFT] ch9200: do return USB errors from control_write()

[PATCH net RFT] 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...

Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>

---
 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 RFT] 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: control_write() in drivers/net/usb/ch9200.c now
  returns the real USB/short-transfer/allocation error (negative errno…

--

ch9200: do return USB errors from control_write()

The patch removes the unconditional "return 0" from control_write() in
drivers/net/usb/ch9200.c so that the value computed from usb_control_msg()
(and the -ENOMEM from kmemdup()) reaches the callers.  The commit is tagged
Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices").

> 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 the intent is to "propagate USB errors to the
callers", but do any of the callers in this driver actually look at the
returned value?

There are only two call sites.  ch9200_mdio_write() is the void
mii_if_info::mdio_write callback and drops the result completely:

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

so the BMCR write issued by mii_nway_restart() from ch9200_bind() still
fails silently.

The other call site is ch9200_bind(), which assigns to retval six times
without ever testing it, and then overwrites it one last time:

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_FIFO_DEPTH, 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 get_mac_address() returns only 0 or -EINVAL, doesn't every newly
propagated control_write() error get discarded here?  If the three MAC
address reads succeed while, say, the MAC_REG_CTRL write fails, ch9200_bind()
returns 0, usbnet_probe() only aborts on "status < 0", and the netdev is
registered with MAC_REG_CTRL, the FIFO depth, the threshold and the flow
control registers never programmed.

Would it make sense to add the call site checks in the same patch, e.g.

	retval = control_write(...);
	if (retval < 0)
		return retval;

otherwise the error swallowing that the Fixes: tag refers to stays
reachable and the change has no observable effect.

One related detail for whoever adds those checks: on success control_write()
now returns the transferred byte count, not 0, because the short transfer
test leaves err == size untouched:

	if (err >= 0 && err < size)
		err = -EINVAL;

So only "if (retval < 0)" tests are valid at the call sites; a later
"if (retval)" check would abort probe on success.  Would it be preferable to
normalise success to 0 in control_write() instead?

If the intent is only a preparatory cleanup with no functional change, could
the changelog say so and drop or justify the Fixes: tag, given that stable
and AUTOSEL will otherwise pick up a no-op?

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

Re: [PATCH net RFT] ch9200: do return USB errors from control_write()

[Sergey Shtylyov]

On 9/8/26 11:01 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: control_write() in drivers/net/usb/ch9200.c now
>   returns the real USB/short-transfer/allocation error (negative errno…

   Allocation error (you mean -ENOMEM?) was returned before this patch...
> --
> 
> ch9200: do return USB errors from control_write()
> 
> The patch removes the unconditional "return 0" from control_write() in
> drivers/net/usb/ch9200.c so that the value computed from usb_control_msg()
> (and the -ENOMEM from kmemdup()) reaches the callers.  The commit is tagged

   Error from kmemdup() surely reached the callers before the patch...

> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices").
> 
>> 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 the intent is to "propagate USB errors to the
> callers", but do any of the callers in this driver actually look at the
> returned value?

   Well, you're correct here...

> There are only two call sites.  ch9200_mdio_write() is the void
> mii_if_info::mdio_write callback and drops the result completely:
> 
> drivers/net/usb/ch9200.c:ch9200_mdio_write() {
>         ...
>         control_write(dev, REQUEST_WRITE, 0, loc * 2, buff, 0x02,
>                       CONTROL_TIMEOUT_MS);
> }

   Indeed...

> so the BMCR write issued by mii_nway_restart() from ch9200_bind() still
> fails silently.
> 
> The other call site is ch9200_bind(), which assigns to retval six times
> without ever testing it, and then overwrites it one last time:
> 
> 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_FIFO_DEPTH, 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 get_mac_address() returns only 0 or -EINVAL, doesn't every newly
> propagated control_write() error get discarded here?  If the three MAC
> address reads succeed while, say, the MAC_REG_CTRL write fails, ch9200_bind()
> returns 0, usbnet_probe() only aborts on "status < 0", and the netdev is
> registered with MAC_REG_CTRL, the FIFO depth, the threshold and the flow
> control registers never programmed.

   You're correct again...

> Would it make sense to add the call site checks in the same patch, e.g.
> 
>         retval = control_write(...);
>         if (retval < 0)
>                 return retval;
> 
> otherwise the error swallowing that the Fixes: tag refers to stays
> reachable and the change has no observable effect.

   This was done by another patch:

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

> One related detail for whoever adds those checks: on success control_write()
> now returns the transferred byte count, not 0, because the short transfer
> test leaves err == size untouched:
> 
>         if (err >= 0 && err < size)
>                 err = -EINVAL;
> 
> So only "if (retval < 0)" tests are valid at the call sites; a later
> "if (retval)" check would abort probe on success.  Would it be preferable to
> normalise success to 0 in control_write() instead?

   You're correct again, I was sloppy in the patch I just linked to...

> If the intent is only a preparatory cleanup with no functional change, could
> the changelog say so and drop or justify the Fixes: tag, given that stable
> and AUTOSEL will otherwise pick up a no-op?

   Yes, the Fixes tag shouldn't have been there in the 1st place. I'll resubmit
the patch for net-next.git instead of net.git...

[...]

MBR, Sergey