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@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: 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@xxxxxxxxxxxx/

[...]

> 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