Re: [PATCH net] ch9200: return error on failed register writes in ch9200_bind()

[Sergey Shtylyov]

On 9/16/26 2:20 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] Ineffective fix: the six newly added `if (retval < 0) return
>   retval;` checks in ch9200_bind() (drivers/net/usb/ch9200.c) cannot…
> 
> --
> 
> ch9200: return error on failed register writes in ch9200_bind()
> 
> The patch adds error checks after the six control_write() calls in
> ch9200_bind() so that a failed vendor register write aborts the bind
> instead of being ignored.  It carries a Fixes tag for the original driver
> commit 4a476bd6d1d9.
> 
>> diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
>> index a206ffa76f1b9..ab3cd3902ed72 100644
>> --- a/drivers/net/usb/ch9200.c
>> +++ b/drivers/net/usb/ch9200.c
>> @@ -360,32 +360,44 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf)
>>       data[1] = 0x0F;
>>       retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_THRESHOLD, data,
>>                              0x02, CONTROL_TIMEOUT_MS);
>> +     if (retval < 0)
>> +             return retval;
> 
> [Severity: High]

   But why? 8-)

> Can these new checks ever observe a failed register write?  control_write()
> in the same file discards the USB transfer status on its normal path:

   By "failed register write" I meant any error in control_write(), including
kmemdup() failure (which precludes the USB request from being sent at all)...

> drivers/net/usb/ch9200.c:control_write() {
>         ...
>         err = usb_control_msg(dev->udev,
>                               usb_sndctrlpipe(dev->udev, 0),
>                               request, request_type, value, index, buf, size,
>                               timeout);
>         if (err >= 0 && err < size)
>                 err = -EINVAL;
>         kfree(buf);
> 
>         return 0;
> 
> err_out:
>         return err;
> }
> 
> The only return err; is at err_out:, which is reached solely from the
> kmemdup() -ENOMEM branch.  All six patched call sites pass a non-NULL
> 2-byte data buffer, so the only negative value the new checks can see is
> -ENOMEM from that allocation.

   I kinda noticed. :-)

[...]

> The sibling helper control_read() already returns err on its normal path,
> and ch9200_mdio_read() checks it with if (ret < 0), so the convention
> control_write() is expected to follow already exists in this file.

   And I'm extending it to control_write(), but in another patch...

> Would it make more sense to change control_write() to return err after
> kfree(buf) instead of (or in addition to) adding the caller-side checks?
> As it stands the commit message claim that this makes the driver "bail out
> of ch9200_bind() on failed writes" does not seem to hold for actual
> transfer failures.

   I'm getting bored...
   If any *human* maintainer insists that I merge the 2 patches, I surely
will... Or maybe I should resubmit both against net.git (net-next.git?) as
a series?

[...]

MBR, Sergey