* Re: [PATCH net-next v2] ch9200: do return USB errors from control_write()
2026-09-15 18:58 [PATCH net-next v2] ch9200: do return USB errors from control_write() Sergey Shtylyov
@ 2026-09-15 21:53 ` Andrew Lunn
2026-09-16 7:23 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Lunn @ 2026-09-15 21:53 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-usb, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next v2] ch9200: do return USB errors from control_write()
2026-09-15 18:58 [PATCH net-next v2] ch9200: do return USB errors from control_write() Sergey Shtylyov
2026-09-15 21:53 ` Andrew Lunn
@ 2026-09-16 7:23 ` netdev-bot+sashiko
2026-09-16 16:21 ` Sergey Shtylyov
1 sibling, 1 reply; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-16 7:23 UTC (permalink / raw)
To: s.shtylyov
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, linux-usb, netdev,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread