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

[Sergey Shtylyov]

On 9/8/26 11:01 AM, netdev-bot+sashiko@xxxxxxxxxx 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@xxxxxxxxxxxx/

> 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