Reads that are lesser than the requested size lead to uninit-value bugs.
In this particular case a variable which was supposed to be initialized
after a read is left uninitialized after a partial read.
Qualify such reads as errors and handle them correctly and while at it
convert the reader functions to return zero on success for easier error
handling.
Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
gigabit ethernet adapter driver")
Signed-off-by: David Kahurani <[email protected]>
Reported-and-tested-by: [email protected]
---
drivers/net/usb/ax88179_178a.c | 279 ++++++++++++++++++++++++++-------
1 file changed, 226 insertions(+), 53 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e2fa56b92..5113ed63f 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,11 +202,15 @@ 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;
+ }
- return ret;
+ return 0;
}
static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
@@ -249,26 +254,33 @@ 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)
+ 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)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
ret = __ax88179_read_cmd(dev, cmd, value, index, size, data, 1);
+ if (ret)
+ return ret;
}
- return ret;
+ return 0;
}
static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
@@ -290,26 +302,32 @@ 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)
+ 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)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
ret = __ax88179_read_cmd(dev, cmd, value, index, size, data, 0);
+ if (ret)
+ return ret;
}
- return ret;
+ return 0;
}
static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
@@ -354,8 +372,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) {
+ 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;
}
@@ -416,7 +441,7 @@ ax88179_phy_write_mmd_indirect(struct usbnet *dev, u16 prtad, u16 devad,
ret = ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
MII_MMD_DATA, 2, &data);
- if (ret < 0)
+ if (ret)
return ret;
return 0;
@@ -427,19 +452,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) {
+ 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) {
+ 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 +499,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 +519,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) {
+ 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) {
+ 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 +543,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 +562,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) {
+ 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);
@@ -601,7 +655,7 @@ ax88179_get_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
ret = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM, i, 1, 2,
&eeprom_buff[i - first_word],
0);
- if (ret < 0) {
+ if (ret) {
kfree(eeprom_buff);
return -EIO;
}
@@ -645,7 +699,7 @@ ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
if (eeprom->offset & 1) {
ret = ax88179_read_cmd(dev, AX_ACCESS_EEPROM, first_word, 1, 2,
&eeprom_buff[0]);
- if (ret < 0) {
+ if (ret) {
netdev_err(net, "Failed to read EEPROM at offset 0x%02x.\n", first_word);
goto free;
}
@@ -654,7 +708,7 @@ ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
if ((eeprom->offset + eeprom->len) & 1) {
ret = ax88179_read_cmd(dev, AX_ACCESS_EEPROM, last_word, 1, 2,
&eeprom_buff[last_word - first_word]);
- if (ret < 0) {
+ if (ret) {
netdev_err(net, "Failed to read EEPROM at offset 0x%02x.\n", last_word);
goto free;
}
@@ -951,23 +1005,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) {
+ 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) {
+ 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) {
+ 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 +1056,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) {
+ 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) {
+ 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 +1138,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 +1154,29 @@ 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) {
+ 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) {
+ 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 +1256,20 @@ 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) {
+ 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 +1293,39 @@ 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) {
+ 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,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
+ 1, 1, &value);
+ if (ret) {
+ 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) {
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_LOW: %d",
+ ret);
+ return ret;
+ }
+
ledvalue |= value;
/* load internal ROM for defaule setting */
@@ -1213,11 +1347,21 @@ 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,
- GMII_LED_LINK, 2, &ledlink);
+ if (ret) {
+ 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) {
+ 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 +1439,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 +1448,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,
- ETH_ALEN, mac);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
+ ETH_ALEN, mac);
+
+ if (ret)
+ 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 +1721,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 +1731,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) {
+ netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return 0;
@@ -1590,11 +1746,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);
+
+ if (ret) {
+ 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);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_PHY_PHYSR, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
return 0;
@@ -1732,9 +1898,16 @@ static int ax88179_reset(struct usbnet *dev)
static int ax88179_stop(struct usbnet *dev)
{
u16 tmp16;
+ int ret;
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read STATUS_MODE: %d\n", ret);
+ return ret;
+ }
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
--
2.25.1
Hi David,
Almost perfect, please see one comment inline
On 4/16/22 10:48, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> gigabit ethernet adapter driver")
> Signed-off-by: David Kahurani <[email protected]>
> Reported-and-tested-by: [email protected]
> ---
[code snip]
>
> @@ -416,7 +441,7 @@ ax88179_phy_write_mmd_indirect(struct usbnet *dev, u16 prtad, u16 devad,
> ret = ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
> MII_MMD_DATA, 2, &data);
>
> - if (ret < 0)
> + if (ret)
> return ret;
>
I think, you didn't mean to do so. ax88179_write_cmd() returns number of
bytes written, so you are changing the logic here. And write call is not
related to uninit value bugs
With regards,
Pavel Skripkin
Hi David,
one more small comment
On 4/16/22 10:48, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> gigabit ethernet adapter driver")
> Signed-off-by: David Kahurani <[email protected]>
> Reported-and-tested-by: [email protected]
> ---
[code snip]
> @@ -1295,6 +1439,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 +1448,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,
> - ETH_ALEN, mac);
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> + ETH_ALEN, mac);
> +
> + if (ret)
> + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> +
> netif_dbg(dev, ifup, dev->net,
> "MAC address read from ASIX chip");
> }
This message sequence is confusing.
In case of ax88179_read_cmd() failure mac read from device actually
failed, but message says, that it was successfully finished.
With regards,
Pavel Skripkin
Hi David,
On 4/16/22 14:49, David Kahurani wrote:
>> > @@ -1303,8 +1448,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,
>> > - ETH_ALEN, mac);
>> > + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
>> > + ETH_ALEN, mac);
>> > +
>> > + if (ret)
>> > + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
>> > +
>> > netif_dbg(dev, ifup, dev->net,
>> > "MAC address read from ASIX chip");
>> > }
>>
>>
>> This message sequence is confusing.
>>
>> In case of ax88179_read_cmd() failure mac read from device actually
>> failed, but message says, that it was successfully finished.
>
> I suppose the code should return in case of an error that way the next
> message does not get executed.
>
No, this will break the driver. This function should set mac address in
netdev structure and if read from device fails code calls
eth_hw_addr_set(dev->net, mac);
which will generate random mac addr. I.e. read failure is not critical
in that function
So, no need to return with an error, just don't print confusing messages :)
With regards,
Pavel Skripkin
On 4/16/22 14:53, Pavel Skripkin wrote:
> No, this will break the driver. This function should set mac address in
> netdev structure and if read from device fails code calls
>
> eth_hw_addr_set(dev->net, mac);
>
Woops, I mean eth_hw_addr_random(dev->net) of course
I am sorry for confusion
With regards,
Pavel Skripkin
On Sat, Apr 16, 2022 at 2:10 PM Pavel Skripkin <[email protected]> wrote:
>
> Hi David,
Hi Pavel.
>
> one more small comment
>
> On 4/16/22 10:48, David Kahurani wrote:
> > Reads that are lesser than the requested size lead to uninit-value bugs.
> > In this particular case a variable which was supposed to be initialized
> > after a read is left uninitialized after a partial read.
> >
> > Qualify such reads as errors and handle them correctly and while at it
> > convert the reader functions to return zero on success for easier error
> > handling.
> >
> > Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> > gigabit ethernet adapter driver")
> > Signed-off-by: David Kahurani <[email protected]>
> > Reported-and-tested-by: [email protected]
> > ---
>
> [code snip]
>
> > @@ -1295,6 +1439,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 +1448,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,
> > - ETH_ALEN, mac);
> > + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> > + ETH_ALEN, mac);
> > +
> > + if (ret)
> > + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> > +
> > netif_dbg(dev, ifup, dev->net,
> > "MAC address read from ASIX chip");
> > }
>
>
> This message sequence is confusing.
>
> In case of ax88179_read_cmd() failure mac read from device actually
> failed, but message says, that it was successfully finished.
I suppose the code should return in case of an error that way the next
message does not get executed.
Thanks for the review! Will fix it and the other issue in the next version.
>
>
>
>
>
> With regards,
> Pavel Skripkin
On Sat, 2022-04-16 at 10:48 +0300, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> gigabit ethernet adapter driver")
In the next version, please additionally fix the above tag: it must use
a single line, even if that means exceeding the 72 chars length limit.
Thanks!
Paolo
Hi David,
On 5/14/22 16:32, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: David Kahurani <[email protected]>
> Reported-and-tested-by: [email protected]
> ---
<--- here (*)
> drivers/net/usb/ax88179_178a.c | 281 ++++++++++++++++++++++++++-------
> 1 file changed, 227 insertions(+), 54 deletions(-)
>
I don't see any error in that patch, but I had to find previous versions
of that patch in my inbox.
Usually new versions of single patches are linked in one thread and have
a version number in a title. You can generate patch with version using
-v option of git format-patch like:
$ git format-patch -v2 HEAD~
And you can send new version as reply using --in-reply= option of git
send-email. It helps a lot with finding previous version, since all
version are linked in one thread
And all updates from version to version should be put under --- (*),
since it's hard to remember why previous version was rejected.
> 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) {
> + 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);
I think, this change might be dangerous. Maybe it should be done in the
same way as in asix driver [1]. Code polls for some register after a
write and maybe non-fatal read error might occur here.
Just my thoughts, I don't know anything about that device :)
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> + ETH_ALEN, mac);
> +
> + if (ret)
> + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> + else
> + netif_dbg(dev, ifup, dev->net,
> + "MAC address read from ASIX chip");
Maybe also use `netif_dbg` here?... There should be a reason why it was
used here in the first place. Or should not :)
Anyway, if someone will say that bailing out from while loop on any
error is OK feel free to add
Reviewed-by: Pavel Skripkin <[email protected]>
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/asix_common.c#L78
With regards,
Pavel Skripkin
Hi Dan,
On 5/14/22 21:54, Dan Carpenter wrote:
> On Sat, May 14, 2022 at 07:51:12PM +0300, Pavel Skripkin wrote:
>> And you can send new version as reply using --in-reply= option of git
>> send-email. It helps a lot with finding previous version, since all version
>> are linked in one thread
>
> This is discouraged these days. It makes for long confusing threads.
> But more importantly, it apparently messes up patchwork.
>
Ok, but at least leaving a link to the previous version(s) seems
reasonable.
With regards,
Pavel Skripkin
On Sat, May 14, 2022 at 07:51:12PM +0300, Pavel Skripkin wrote:
> And you can send new version as reply using --in-reply= option of git
> send-email. It helps a lot with finding previous version, since all version
> are linked in one thread
This is discouraged these days. It makes for long confusing threads.
But more importantly, it apparently messes up patchwork.
regards,
dan carpenter