diff mbox series

net: ax88179: add proper error handling of usb read errors

Message ID 20220404151036.265901-1-k.kahurani@gmail.com
State Superseded
Headers show
Series net: ax88179: add proper error handling of usb read errors | expand

Commit Message

David Kahurani April 4, 2022, 3:10 p.m. UTC 
Reads that are lesser than the requested size lead to uninit-value bugs. Qualify
such reads as errors and handle them correctly.

ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -71
ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0002: -71
=====================================================
BUG: KMSAN: uninit-value in ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
BUG: KMSAN: uninit-value in ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
 ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
 ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
 ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
 usbnet_probe+0x1284/0x4140 drivers/net/usb/usbnet.c:1747
 usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396
 really_probe+0x67d/0x1510 drivers/base/dd.c:596
 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
 driver_probe_device drivers/base/dd.c:781 [inline]
 __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
 __device_attach+0x593/0x8e0 drivers/base/dd.c:969
 device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
 bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487
 device_add+0x1d3e/0x2400 drivers/base/core.c:3394
 usb_set_configuration+0x37e9/0x3ed0 drivers/usb/core/message.c:2170
 usb_generic_driver_probe+0x13c/0x300 drivers/usb/core/generic.c:238
 usb_probe_device+0x309/0x570 drivers/usb/core/driver.c:293
 really_probe+0x67d/0x1510 drivers/base/dd.c:596
 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
 driver_probe_device drivers/base/dd.c:781 [inline]
 __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
 __device_attach+0x593/0x8e0 drivers/base/dd.c:969
 device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016

Signed-off-by: David Kahurani <k.kahurani@gmail.com>
Reported-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
---
 drivers/net/usb/ax88179_178a.c | 255 +++++++++++++++++++++++++++------
 1 file changed, 213 insertions(+), 42 deletions(-)

Comments

Paolo Abeni April 5, 2022, 9:44 a.m. UTC | #1 
On Mon, 2022-04-04 at 18:10 +0300, David Kahurani wrote:
> @@ -354,8 +369,15 @@ static int ax88179_mdio_read(struct net_device *netdev, int phy_id, int loc)
>  {
>  	struct usbnet *dev = netdev_priv(netdev);
>  	u16 res;
> +	int ret;
> +
> +	ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
> +
> +	if (ret < 0){

For the records: another recurring indentation issue is the lack of a
whitespace after the ')'.

Please address all the issues reported by scritps/checkpatch.pl before
submitting the next version, thanks!

Paolo
Andy Shevchenko April 11, 2022, 3:11 p.m. UTC | #2 
On Tue, Apr 5, 2022 at 3:05 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
> On 4/4/22 18:10, David Kahurani wrote:
> > Reads that are lesser than the requested size lead to uninit-value bugs. Qualify
> > such reads as errors and handle them correctly.

> I'd personally cut this log a bit and would add this part of the initial
> report
>
> Local variable eeprom.i created at:
>   ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1045 [inline]
>   ax88179_led_setting+0x2e2/0x30b0 drivers/net/usb/ax88179_178a.c:1168
>   ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
>
> Since it shows exactly where problem comes from.
>
> I do not insist, just IMO

I insist though. It will reduce the resource consumption (i.e. storage
and network load on cloning / pulling) a lot (taking into account
multiplier of how many Linux kernel source copies are around the
globe) and hence become more environmentally friendly.
David Kahurani April 13, 2022, 12:36 p.m. UTC | #3 
On Mon, Apr 4, 2022 at 6:32 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:

Hi Dan

> >       int ret;
> >       int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> > @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> >       ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> >                value, index, data, size);
> >
> > -     if (unlikely(ret < 0))
> > +     if (unlikely(ret < size)) {
> > +             ret = ret < 0 ? ret : -ENODATA;
> > +
> >               netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> >                           index, ret);
> > +     }
> >
> >       return ret;
>
> It would be better to make __ax88179_read_cmd() return 0 on success
> instead of returning size on success.  Non-standard returns lead to bugs.
>

I don't suppose this would have much effect on the structure of the
code and indeed plan to do this but just some minor clarification.

Isn't it standard for reader functions to return the number of bytes read?

Regards,
David.

>
> > @@ -1060,16 +1151,30 @@ static int ax88179_check_eeprom(struct usbnet *dev)
> >
> >               jtimeout = jiffies + delay;
> >               do {
> > -                     ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> > -                                      1, 1, &buf);
> > +                 ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> > +                                        1, 1, &buf);
> > +
> > +                 if (ret < 0) {
> > +                         netdev_dbg(dev->net,
> > +                                    "Failed to read SROM_CMD: %d\n",
> > +                                    ret);
> > +                         return ret;
> > +                 }
> >
> >                       if (time_after(jiffies, jtimeout))
> >                               return -EINVAL;
>
> The indenting here is wrong.  Run scripts/checkpatch.pl on your patches.
>
> regards,
> dan carpenter
>
Dan Carpenter April 13, 2022, 3:32 p.m. UTC | #4 
On Wed, Apr 13, 2022 at 03:36:57PM +0300, David Kahurani wrote:
> On Mon, Apr 4, 2022 at 6:32 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> Hi Dan
> 
> > >       int ret;
> > >       int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> > > @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> > >       ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > >                value, index, data, size);
> > >
> > > -     if (unlikely(ret < 0))
> > > +     if (unlikely(ret < size)) {
> > > +             ret = ret < 0 ? ret : -ENODATA;
> > > +
> > >               netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> > >                           index, ret);
> > > +     }
> > >
> > >       return ret;
> >
> > It would be better to make __ax88179_read_cmd() return 0 on success
> > instead of returning size on success.  Non-standard returns lead to bugs.
> >
> 
> I don't suppose this would have much effect on the structure of the
> code and indeed plan to do this but just some minor clarification.
> 
> Isn't it standard for reader functions to return the number of bytes read?
> 

Not really.

There are some functions that do it, but it has historically lead to bug
after bug.  For example, see commit 719b8f2850d3 ("USB: add
usb_control_msg_send() and usb_control_msg_recv()") where USB is moving
away from that to avoid bugs.

If you return zero on success then it's simple:

	if (ret)
		return ret;

If you return the bytes people will try call kinds of things:

	if (ret)
		return ret;

Bug: Now the driver is broken.  (Not everyone can test the hardware).

	if (ret != size)
		return ret;

Bug: returns a positive.

	if (ret != size)
		return -EIO;

Bug: forgot to propagate the error code.

	if (ret < sizeof(foo))
		return -EIO;

Bug: because of type promotion negative error codes are treated as
     success.

	if (ret < 0)
		return ret;

Bug: buffer partially filled.  Information leak.

If you return the bytes then the only correct way to write error
handling is:

	if (ret < 0)
		return ret;
	if (ret != size)
		return -EIO;

regards,
dan carpenter
Oliver Neukum April 14, 2022, 7:31 a.m. UTC | #5 
On 13.04.22 17:32, Dan Carpenter wrote:
>
> Bug: buffer partially filled.  Information leak.
>
> If you return the bytes then the only correct way to write error
> handling is:
>
> 	if (ret < 0)
> 		return ret;
> 	if (ret != size)
> 		return -EIO;
>
You have to make up your mind on whether you ever need to read
answer of a length not known before you try it. The alternative of
passing a pointer to an integer for length is worse.

    Regards
        Oliver
Dan Carpenter April 14, 2022, 8:21 a.m. UTC | #6 
On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote:
> 
> 
> On 13.04.22 17:32, Dan Carpenter wrote:
> >
> > Bug: buffer partially filled.  Information leak.
> >
> > If you return the bytes then the only correct way to write error
> > handling is:
> >
> > 	if (ret < 0)
> > 		return ret;
> > 	if (ret != size)
> > 		return -EIO;
> >
> You have to make up your mind on whether you ever need to read
> answer of a length not known before you try it. The alternative of
> passing a pointer to an integer for length is worse.

How is it worse?  Can you give an example, so I will write a static
checker rule for it?

There used to be more APIs that consistently caused bug after bug where
we mixed positives success values with negative error codes.  We
converted some bad offenders to return the positive as a parameter
and I was really happy about that.

Another example I used to see a lot is request_irq() saved to an
unsigned.  These days I think GCC warns about that?  Maybe the build
bots get to it before I do.

regards,
dan carpenter
Oliver Neukum April 14, 2022, 9:13 a.m. UTC | #7 
On 14.04.22 10:21, Dan Carpenter wrote:
> On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote:
>>
>> On 13.04.22 17:32, Dan Carpenter wrote:
>>> Bug: buffer partially filled.  Information leak.
>>>
>>> If you return the bytes then the only correct way to write error
>>> handling is:
>>>
>>> 	if (ret < 0)
>>> 		return ret;
>>> 	if (ret != size)
>>> 		return -EIO;
>>>
>> You have to make up your mind on whether you ever need to read
>> answer of a length not known before you try it. The alternative of
>> passing a pointer to an integer for length is worse.
> How is it worse?  Can you give an example, so I will write a static
> checker rule for it?
My favorite example would be:

int usb_bulk_msg(struct usb_device *usb_dev, unsigned int pipe,
                void *data, int len, int *actual_length, int timeout)

The actual_length parameter is horrible. Now, there are situations like this
where this pattern is unavoidable, because in addition to an error you
can get a partial completion of an operation.

Do I see it correctly that you would prefer this pattern even if
you could report either an error or a result in the function?
> There used to be more APIs that consistently caused bug after bug where
> we mixed positives success values with negative error codes.  We
> converted some bad offenders to return the positive as a parameter
> and I was really happy about that.
>
> Another example I used to see a lot is request_irq() saved to an
> unsigned.  These days I think GCC warns about that?  Maybe the build
> bots get to it before I do.
>
That needs to be checked for.. In fact while we are at it, do we check
for integer overflows?

    Regards
        Oliver
diff mbox series

Patch

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e2fa56b92..b5e114bed 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -185,8 +185,9 @@  static const struct {
 	{7, 0xcc, 0x4c, 0x18, 8},
 };
 
-static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
-			      u16 size, void *data, int in_pm)
+static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
+		                           u16 value, u16 index, u16 size,
+					   void *data, int in_pm)
 {
 	int ret;
 	int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
@@ -201,9 +202,12 @@  static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
 	ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 		 value, index, data, size);
 
-	if (unlikely(ret < 0))
+	if (unlikely(ret < size)) {
+		ret = ret < 0 ? ret : -ENODATA;
+
 		netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
 			    index, ret);
+	}
 
 	return ret;
 }
@@ -249,19 +253,26 @@  static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
 	}
 }
 
-static int ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
-				 u16 index, u16 size, void *data)
+static int __must_check ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd,
+		                              u16 value, u16 index, u16 size,
+					      void *data)
 {
 	int ret;
 
 	if (2 == size) {
 		u16 buf;
 		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+
+		if (ret < 0)
+			return ret;
 		le16_to_cpus(&buf);
 		*((u16 *)data) = buf;
 	} else if (4 == size) {
 		u32 buf;
 		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+
+		if (ret < 0)
+			return ret;
 		le32_to_cpus(&buf);
 		*((u32 *)data) = buf;
 	} else {
@@ -290,19 +301,23 @@  static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
 	return ret;
 }
 
-static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
-			    u16 size, void *data)
+static int __must_check ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
+                                         u16 index, u16 size, void *data)
 {
 	int ret;
 
 	if (2 == size) {
 		u16 buf = 0;
 		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+		if (ret < 0)
+			return ret;
 		le16_to_cpus(&buf);
 		*((u16 *)data) = buf;
 	} else if (4 == size) {
 		u32 buf = 0;
 		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+		if (ret < 0)
+			return ret;
 		le32_to_cpus(&buf);
 		*((u32 *)data) = buf;
 	} else {
@@ -354,8 +369,15 @@  static int ax88179_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	u16 res;
+	int ret;
+
+	ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
+
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+		return ret;
+	}
 
-	ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
 	return res;
 }
 
@@ -427,19 +449,31 @@  static int ax88179_suspend(struct usb_interface *intf, pm_message_t message)
 	struct usbnet *dev = usb_get_intfdata(intf);
 	u16 tmp16;
 	u8 tmp8;
+	int ret;
 
 	usbnet_suspend(intf, message);
 
 	/* Disable RX path */
-	ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
-			      2, 2, &tmp16);
+	ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+			            2, 2, &tmp16);
+
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read MEDIUM_STATUS_MODE: %d\n",
+			   ret);
+		return ret;
+	}
+
 	tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
 	ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
 			       2, 2, &tmp16);
 
 	/* Force bulk-in zero length */
-	ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
-			      2, 2, &tmp16);
+	ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
+                                    2, 2, &tmp16);
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d\n", ret);
+		return ret;
+	}
 
 	tmp16 |= AX_PHYPWR_RSTCTL_BZ | AX_PHYPWR_RSTCTL_IPRL;
 	ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
@@ -462,6 +496,7 @@  static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
 {
 	u16 tmp16;
 	u8 tmp8;
+	int ret;
 	int (*fnr)(struct usbnet *, u8, u16, u16, u16, void *);
 	int (*fnw)(struct usbnet *, u8, u16, u16, u16, const void *);
 
@@ -481,11 +516,19 @@  static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
 
 	/* Enable Auto Detach bit */
 	tmp8 = 0;
-	fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+	ret = fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+	if (ret < 0) {
+		netdev_dbg(dev->net, "Failed to read CLK_SELECT: %d", ret);
+		return ret;
+	}
 	tmp8 |= AX_CLK_SELECT_ULR;
 	fnw(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
 
-	fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+	ret = fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+	if (ret < 0) {
+		netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d", ret);
+		return ret;
+	}
 	tmp16 |= AX_PHYPWR_RSTCTL_AT;
 	fnw(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
 
@@ -497,6 +540,7 @@  static int ax88179_resume(struct usb_interface *intf)
 	struct usbnet *dev = usb_get_intfdata(intf);
 	u16 tmp16;
 	u8 tmp8;
+	int ret;
 
 	usbnet_link_change(dev, 0, 0);
 
@@ -515,7 +559,14 @@  static int ax88179_resume(struct usb_interface *intf)
 	ax88179_auto_detach(dev, 1);
 
 	/* Enable clock */
-	ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC,  AX_CLK_SELECT, 1, 1, &tmp8);
+	ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC,  AX_CLK_SELECT, 1, 1, &tmp8);
+
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read CLK_SELECT %d\n", ret);
+
+		return ret;
+	}
+
 	tmp8 |= AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
 	ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
 	msleep(100);
@@ -951,23 +1002,45 @@  static int
 ax88179_set_features(struct net_device *net, netdev_features_t features)
 {
 	u8 tmp;
+	int ret;
 	struct usbnet *dev = netdev_priv(net);
 	netdev_features_t changed = net->features ^ features;
 
 	if (changed & NETIF_F_IP_CSUM) {
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+				       1, 1, &tmp);
+		if (ret < 0){
+			netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+				   ret);
+			return ret;
+		}
+
 		tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
 		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
 	}
 
 	if (changed & NETIF_F_IPV6_CSUM) {
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+				       1, 1, &tmp);
+		if (ret < 0){
+			netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+				   ret);
+			return ret;
+		}
+
 		tmp ^= AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
 		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
 	}
 
 	if (changed & NETIF_F_RXCSUM) {
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL,
+				       1, 1, &tmp);
+		if (ret < 0){
+			netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+				   ret);
+			return ret;
+		}
+
 		tmp ^= AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
 		       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
 		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
@@ -980,19 +1053,36 @@  static int ax88179_change_mtu(struct net_device *net, int new_mtu)
 {
 	struct usbnet *dev = netdev_priv(net);
 	u16 tmp16;
+	int ret;
 
 	net->mtu = new_mtu;
 	dev->hard_mtu = net->mtu + net->hard_header_len;
 
 	if (net->mtu > 1500) {
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
-				 2, 2, &tmp16);
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+				       AX_MEDIUM_STATUS_MODE,
+				       2, 2, &tmp16);
+		if (ret < 0){
+			netdev_dbg(dev->net,
+				   "Failed to read MEDIUM_STATUS_MODE %d\n",
+				   ret);
+			return ret;
+		}
+
 		tmp16 |= AX_MEDIUM_JUMBO_EN;
 		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
 				  2, 2, &tmp16);
 	} else {
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
-				 2, 2, &tmp16);
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+			               AX_MEDIUM_STATUS_MODE,
+				       2, 2, &tmp16);
+		if (ret < 0){
+			netdev_dbg(dev->net,
+				   "Failed to read MEDIUM_STATUS_MODE %d\n",
+				   ret);
+			return ret;
+		}
+
 		tmp16 &= ~AX_MEDIUM_JUMBO_EN;
 		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
 				  2, 2, &tmp16);
@@ -1045,6 +1135,7 @@  static int ax88179_check_eeprom(struct usbnet *dev)
 	u8 i, buf, eeprom[20];
 	u16 csum, delay = HZ / 10;
 	unsigned long jtimeout;
+	int ret;
 
 	/* Read EEPROM content */
 	for (i = 0; i < 6; i++) {
@@ -1060,16 +1151,30 @@  static int ax88179_check_eeprom(struct usbnet *dev)
 
 		jtimeout = jiffies + delay;
 		do {
-			ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
-					 1, 1, &buf);
+		    ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+					   1, 1, &buf);
+
+		    if (ret < 0) {
+			    netdev_dbg(dev->net,
+				       "Failed to read SROM_CMD: %d\n",
+			               ret);
+			    return ret;
+		    }
 
 			if (time_after(jiffies, jtimeout))
 				return -EINVAL;
 
 		} while (buf & EEP_BUSY);
 
-		__ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
-				   2, 2, &eeprom[i * 2], 0);
+		ret = __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+				         2, 2, &eeprom[i * 2], 0);
+
+		if (ret < 0) {
+			netdev_dbg(dev->net,
+				   "Failed to read SROM_DATA_LOW: %d\n",
+			           ret);
+			return ret;
+		}
 
 		if ((i == 0) && (eeprom[0] == 0xFF))
 			return -EINVAL;
@@ -1149,12 +1254,19 @@  static int ax88179_convert_old_led(struct usbnet *dev, u16 *ledvalue)
 
 static int ax88179_led_setting(struct usbnet *dev)
 {
+	int ret;
 	u8 ledfd, value = 0;
 	u16 tmp, ledact, ledlink, ledvalue = 0, delay = HZ / 10;
 	unsigned long jtimeout;
 
 	/* Check AX88179 version. UA1 or UA2*/
-	ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1, &value);
+	ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1, &value);
+
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read GENERAL_STATUS: %d\n",
+			   ret);
+		return ret;
+	}
 
 	if (!(value & AX_SECLD)) {	/* UA1 */
 		value = AX_GPIO_CTRL_GPIO3EN | AX_GPIO_CTRL_GPIO2EN |
@@ -1178,20 +1290,40 @@  static int ax88179_led_setting(struct usbnet *dev)
 
 		jtimeout = jiffies + delay;
 		do {
-			ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
-					 1, 1, &value);
+			ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+					       1, 1, &value);
+			if (ret < 0){
+				netdev_dbg(dev->net,
+					   "Failed to read SROM_CMD: %d\n",
+					   ret);
+				return ret;
+			}
 
 			if (time_after(jiffies, jtimeout))
 				return -EINVAL;
 
 		} while (value & EEP_BUSY);
 
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
 				 1, 1, &value);
+		if (ret < 0){
+			netdev_dbg(dev->net, "Failed to read SROM_DATA_HIGH: %d\n",
+				   ret);
+			return ret;
+		}
+
+
 		ledvalue = (value << 8);
 
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
-				 1, 1, &value);
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+				       1, 1, &value);
+
+		if (ret < 0) {
+			netdev_dbg(dev->net, "Failed to read SROM_DATA_LOW: %d",
+				   ret);
+			return ret;
+		}
+
 		ledvalue |= value;
 
 		/* load internal ROM for defaule setting */
@@ -1213,12 +1345,22 @@  static int ax88179_led_setting(struct usbnet *dev)
 	ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
 			  GMII_PHYPAGE, 2, &tmp);
 
-	ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
-			 GMII_LED_ACT, 2, &ledact);
+	ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+			       GMII_LED_ACT, 2, &ledact);
 
-	ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+		return ret;
+	}
+
+	ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
 			 GMII_LED_LINK, 2, &ledlink);
 
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+		return ret;
+	}
+
 	ledact &= GMII_LED_ACTIVE_MASK;
 	ledlink &= GMII_LED_LINK_MASK;
 
@@ -1295,6 +1437,7 @@  static int ax88179_led_setting(struct usbnet *dev)
 static void ax88179_get_mac_addr(struct usbnet *dev)
 {
 	u8 mac[ETH_ALEN];
+	int ret;
 
 	memset(mac, 0, sizeof(mac));
 
@@ -1303,8 +1446,12 @@  static void ax88179_get_mac_addr(struct usbnet *dev)
 		netif_dbg(dev, ifup, dev->net,
 			  "MAC address read from device tree");
 	} else {
-		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
+		ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
 				 ETH_ALEN, mac);
+
+		if (ret < 0)
+			netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
+
 		netif_dbg(dev, ifup, dev->net,
 			  "MAC address read from ASIX chip");
 	}
@@ -1572,6 +1719,7 @@  static int ax88179_link_reset(struct usbnet *dev)
 	u16 mode, tmp16, delay = HZ / 10;
 	u32 tmp32 = 0x40000000;
 	unsigned long jtimeout;
+	int ret;
 
 	jtimeout = jiffies + delay;
 	while (tmp32 & 0x40000000) {
@@ -1581,7 +1729,13 @@  static int ax88179_link_reset(struct usbnet *dev)
 				  &ax179_data->rxctl);
 
 		/*link up, check the usb device control TX FIFO full or empty*/
-		ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+		ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+
+		if (ret < 0) {
+			netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
+				   ret);
+			return ret;
+		}
 
 		if (time_after(jiffies, jtimeout))
 			return 0;
@@ -1590,11 +1744,21 @@  static int ax88179_link_reset(struct usbnet *dev)
 	mode = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
 	       AX_MEDIUM_RXFLOW_CTRLEN;
 
-	ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
-			 1, 1, &link_sts);
+	ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
+			       1, 1, &link_sts);
 
-	ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
-			 GMII_PHY_PHYSR, 2, &tmp16);
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read LINK_STATUS: %d", ret);
+		return ret;
+	}
+
+	ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+			       GMII_PHY_PHYSR, 2, &tmp16);
+
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+		return ret;
+	}
 
 	if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
 		return 0;
@@ -1732,9 +1896,16 @@  static int ax88179_reset(struct usbnet *dev)
 static int ax88179_stop(struct usbnet *dev)
 {
 	u16 tmp16;
+	int ret;
 
-	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+	ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
 			 2, 2, &tmp16);
+
+	if (ret < 0){
+		netdev_dbg(dev->net, "Failed to read STATUS_MODE: %d\n", ret);
+		return ret;
+	}
+
 	tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
 	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
 			  2, 2, &tmp16);