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

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 = {
-- 
2.39.5

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

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.