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 <[email protected]>
Reported-by: [email protected]
---
drivers/net/usb/ax88179_178a.c | 255 +++++++++++++++++++++++++++------
1 file changed, 213 insertions(+), 42 deletions(-)
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);
--
2.25.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
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master horms-ipvs/master linus/master v5.18-rc1 next-20220405]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/David-Kahurani/net-ax88179-add-proper-error-handling-of-usb-read-errors/20220404-231226
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 692930cc435099580a4b9e32fa781b0688c18439
config: i386-randconfig-m021-20220404 (https://download.01.org/0day-ci/archive/20220406/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
smatch warnings:
drivers/net/usb/ax88179_178a.c:1164 ax88179_check_eeprom() warn: inconsistent indenting
vim +1164 drivers/net/usb/ax88179_178a.c
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1132
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1133 static int ax88179_check_eeprom(struct usbnet *dev)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1134 {
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1135 u8 i, buf, eeprom[20];
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1136 u16 csum, delay = HZ / 10;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1137 unsigned long jtimeout;
c006257c3aa36f6 David Kahurani 2022-04-04 1138 int ret;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1139
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1140 /* Read EEPROM content */
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1141 for (i = 0; i < 6; i++) {
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1142 buf = i;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1143 if (ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_SROM_ADDR,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1144 1, 1, &buf) < 0)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1145 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1146
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1147 buf = EEP_RD;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1148 if (ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1149 1, 1, &buf) < 0)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1150 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1151
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1152 jtimeout = jiffies + delay;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1153 do {
c006257c3aa36f6 David Kahurani 2022-04-04 1154 ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1155 1, 1, &buf);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1156
c006257c3aa36f6 David Kahurani 2022-04-04 1157 if (ret < 0) {
c006257c3aa36f6 David Kahurani 2022-04-04 1158 netdev_dbg(dev->net,
c006257c3aa36f6 David Kahurani 2022-04-04 1159 "Failed to read SROM_CMD: %d\n",
c006257c3aa36f6 David Kahurani 2022-04-04 1160 ret);
c006257c3aa36f6 David Kahurani 2022-04-04 1161 return ret;
c006257c3aa36f6 David Kahurani 2022-04-04 1162 }
c006257c3aa36f6 David Kahurani 2022-04-04 1163
e2ca90c276e1fc4 Freddy Xin 2013-03-02 @1164 if (time_after(jiffies, jtimeout))
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1165 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1166
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1167 } while (buf & EEP_BUSY);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1168
c006257c3aa36f6 David Kahurani 2022-04-04 1169 ret = __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1170 2, 2, &eeprom[i * 2], 0);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1171
c006257c3aa36f6 David Kahurani 2022-04-04 1172 if (ret < 0) {
c006257c3aa36f6 David Kahurani 2022-04-04 1173 netdev_dbg(dev->net,
c006257c3aa36f6 David Kahurani 2022-04-04 1174 "Failed to read SROM_DATA_LOW: %d\n",
c006257c3aa36f6 David Kahurani 2022-04-04 1175 ret);
c006257c3aa36f6 David Kahurani 2022-04-04 1176 return ret;
c006257c3aa36f6 David Kahurani 2022-04-04 1177 }
c006257c3aa36f6 David Kahurani 2022-04-04 1178
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1179 if ((i == 0) && (eeprom[0] == 0xFF))
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1180 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1181 }
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1182
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1183 csum = eeprom[6] + eeprom[7] + eeprom[8] + eeprom[9];
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1184 csum = (csum >> 8) + (csum & 0xff);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1185 if ((csum + eeprom[10]) != 0xff)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1186 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1187
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1188 return 0;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1189 }
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1190
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, Apr 13, 2022 at 03:36:57PM +0300, David Kahurani wrote:
> On Mon, Apr 4, 2022 at 6:32 PM Dan Carpenter <[email protected]> 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
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
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
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