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

[netdev-bot+sashiko@kernel.org]

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