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

The successful register writes, done via the USB control requests in
control_write(), seem vital for the normal functioning of the device,
however the driver's bind() method ignores error codes returned from
control_write().  Do bail out of ch9200_bind() on failed writes...

Found by Linux Verification Center (linuxtesting.org) with the Svace
static analysis tool.

Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Sergey Shtylyov <redacted>

---
Changes in version 2:
- switched to checking for the negative error values instead of non-zero;
- added the Fixes tag, retargeting the patch to the net.git repo;
- dropped [RFT] from the subject.

 drivers/net/usb/ch9200.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index a206ffa76f1b..ab3cd3902ed7 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;
 
 	data[0] = 0xA0;
 	data[1] = 0x90;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FIFO_DEPTH, data,
 			       0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0x30;
 	data[1] = 0x00;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_PAUSE, data,
 			       0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0x17;
 	data[1] = 0xD8;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_FLOW_CONTROL,
 			       data, 0x02, CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	/* Undocumented register */
 	data[0] = 0x01;
 	data[1] = 0x00;
 	retval = control_write(dev, REQUEST_WRITE, 0, 254, data, 0x02,
 			       CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	data[0] = 0x5F;
 	data[1] = 0x0D;
 	retval = control_write(dev, REQUEST_WRITE, 0, MAC_REG_CTRL, data, 0x02,
 			       CONTROL_TIMEOUT_MS);
+	if (retval < 0)
+		return retval;
 
 	retval = get_mac_address(dev, addr);
 	eth_hw_addr_set(dev->net, addr);

-- 
2.55.0

Hello!

   Duh, forgot to add v2 to the subject... :-/

MBR, Sergey

On Mon, Sep 14, 2026 at 11:25:02PM +0300, Sergey Shtylyov wrote:

The successful register writes, done via the USB control requests in
control_write(), seem vital for the normal functioning of the device,
however the driver's bind() method ignores error codes returned from
control_write().  Do bail out of ch9200_bind() on failed writes...

Found by Linux Verification Center (linuxtesting.org) with the Svace
static analysis tool.

Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices")
Signed-off-by: Sergey Shtylyov <redacted>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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.

[Severity: High]

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:

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.

A STALL of the vendor OUT request (-EPIPE), a timeout (-ETIMEDOUT), a
disconnect during bind (-ENODEV/-ESHUTDOWN), a protocol error (-EPROTO),
or a short control transfer (normalized to -EINVAL just above) are all
turned into 0, so ch9200_bind() still falls through to
get_mac_address()/eth_hw_addr_set() and reports success with
MAC_REG_THRESHOLD, MAC_REG_FIFO_DEPTH, MAC_REG_PAUSE,
MAC_REG_FLOW_CONTROL, register 254 and MAC_REG_CTRL (which arms TX/RX)
left unprogrammed.

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.

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.

[ ... remaining four checks have the same question ... ]

 	retval = get_mac_address(dev, addr);
 	eth_hw_addr_set(dev->net, addr);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914202503.52920-1-s.shtylyov%40auroraos.dev

On 9/16/26 2:20 AM, netdev-bot+sashiko@kernel.org 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.

quoted

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

On 9/16/26 18:58, Sergey Shtylyov wrote:

On 9/16/26 2:20 AM, netdev-bot+sashiko@kernel.org wrote:

quoted

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.

quoted

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-)

quoted

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)...

quoted

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. :-)

[...]

quoted

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...

quoted

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?

IMHO the patches are so strictly interconnected that should land into the same (net-next) series. No fixes tags in net-next patches.

Thanks,

Paolo