| Message ID | 20250412183829.41342-5-qasdev00@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: ch9200: fix various bugs and improve qinheng ch9200 driver | expand |
On Tue, Apr 15, 2025 at 08:47:08PM -0700, Jakub Kicinski wrote: > On Sat, 12 Apr 2025 19:38:28 +0100 Qasim Ijaz wrote: > > retval = usbnet_get_endpoints(dev, intf); > > - if (retval) > > + if (retval < 0) > > return retval; > > This change is unnecessary ? Commit message speaks of control_write(), > this is usbnet_get_endpoints(). So this change was done mainly for consistency with the other error checks in the function. Essentially in my one of my previous patches (<https://lore.kernel.org/all/20250317175117.GI688833@kernel.org/>) I was using "if (retval)" for error handling, however after Simon's recommendation to use "if (retval < 0)" I changed this. In this particular function I took Simons advice but then noticed that the usbnet_get_endpoints() check was still using "if (retval)" so I decided to make it the same as the others. The behaviour is still the same regardless of it we do "if (retval < 0)" or "if (retval)" for checking usbnet_get_endpoints() since it returns 0 on success or negative on failure. So in ch9200_bind: In the first case of "if (retval)", if the usbnet_get_endpoints() function fails and returns negative then we execute this branch and it returns negative, if it succeeds with 0 then the ch9200_bind function continues. In the second case of "if (retval < 0)", if the usbnet_get_endpoints() function fails and returns negative then we execute this branch and it returns negative, if it succeeds with 0 then ch9200_bind function continues. If you like I can include this in the patch description for clarity or remove it entirely.
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index 4f1d2e9045a9..187bbfc991f5 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -338,12 +338,12 @@ static int get_mac_address(struct usbnet *dev, unsigned char *data) static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf) { - int retval = 0; + int retval; unsigned char data[2]; u8 addr[ETH_ALEN]; retval = usbnet_get_endpoints(dev, intf); - if (retval) + if (retval < 0) return retval; dev->mii.dev = dev->net; @@ -361,32 +361,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); if (retval < 0) @@ -394,7 +406,7 @@ static int ch9200_bind(struct usbnet *dev, struct usb_interface *intf) eth_hw_addr_set(dev->net, addr); - return retval; + return 0; } static const struct driver_info ch9200_info = {
The ch9200_bind() function has no error handling for any control_write() calls. Fix this by checking if any control_write() call fails and propagate the error to the caller. Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") Reviewed-by: Simon Horman <horms@kernel.org> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> --- drivers/net/usb/ch9200.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)