| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 31 Mar 2018 12:16:05 +0000
From: Marc Zyngier <marc.zyngier@....com>
To: Alexander Kurz <akurz@...la.de>
Cc: "David S . Miller" <davem@...emloft.net>,
"Andrew F . Davis" <afd@...com>, <linux-usb@...r.kernel.org>,
<netdev@...r.kernel.org>, Freddy Xin <freddy@...x.com.tw>
Subject: Re: [PATCH v2 2/2] net: usb: asix88179_178a: de-duplicate code
Alexander, David,
On 2018-03-08 11:19, Alexander Kurz wrote:
> Remove the duplicated code for asix88179_178a bind and reset methods.
>
> Signed-off-by: Alexander Kurz <akurz@...la.de>
> ---
> drivers/net/usb/ax88179_178a.c | 117
> +++++++++++------------------------------
> 1 file changed, 31 insertions(+), 86 deletions(-)
This patch (which is now in -next) causes an infinite stream of:
[ 14.922412] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 14.954411] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 18.474372] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 18.506371] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 22.026327] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 22.058328] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 25.578285] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[ 25.610286] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link
status is: 1
[...]
and never comes really up on my arm64 box. Reverting it immediately
fixes the issue.
Looking more closely at the patch, it raises a couple of questions:
>
> diff --git a/drivers/net/usb/ax88179_178a.c
> b/drivers/net/usb/ax88179_178a.c
> index a6ef75907ae9..fb1b78d4b9ef 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1223,7 +1223,7 @@ static int ax88179_led_setting(struct usbnet
> *dev)
> return 0;
> }
>
> -static int ax88179_bind(struct usbnet *dev, struct usb_interface
> *intf)
> +static int ax88179_link_bind_or_reset(struct usbnet *dev, bool
> do_reset)
> {
> u8 buf[5];
> u16 *tmp16;
> @@ -1231,12 +1231,11 @@ static int ax88179_bind(struct usbnet *dev,
> struct usb_interface *intf)
> struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
> struct ethtool_eee eee_data;
>
> - usbnet_get_endpoints(dev, intf);
> -
> tmp16 = (u16 *)buf;
> tmp = (u8 *)buf;
>
> - memset(ax179_data, 0, sizeof(*ax179_data));
> + if (!do_reset)
> + memset(ax179_data, 0, sizeof(*ax179_data));
>
> /* Power up ethernet PHY */
> *tmp16 = 0;
> @@ -1249,9 +1248,13 @@ static int ax88179_bind(struct usbnet *dev,
> struct usb_interface *intf)
> ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
> msleep(100);
>
> + if (do_reset)
> + ax88179_auto_detach(dev, 0);
This wasn't initially present. As such, it doesn't really match the
"de-duplication" goal of that patch.
> +
> ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> ETH_ALEN, dev->net->dev_addr);
> - memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> + if (!do_reset)
> + memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
>
> /* RX bulk configuration */
> memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
> @@ -1266,19 +1269,21 @@ static int ax88179_bind(struct usbnet *dev,
> struct usb_interface *intf)
> ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
> 1, 1, tmp);
>
> - dev->net->netdev_ops = &ax88179_netdev_ops;
> - dev->net->ethtool_ops = &ax88179_ethtool_ops;
> - dev->net->needed_headroom = 8;
> - dev->net->max_mtu = 4088;
> -
> - /* Initialize MII structure */
> - dev->mii.dev = dev->net;
> - dev->mii.mdio_read = ax88179_mdio_read;
> - dev->mii.mdio_write = ax88179_mdio_write;
> - dev->mii.phy_id_mask = 0xff;
> - dev->mii.reg_num_mask = 0xff;
> - dev->mii.phy_id = 0x03;
> - dev->mii.supports_gmii = 1;
> + if (!do_reset) {
> + dev->net->netdev_ops = &ax88179_netdev_ops;
> + dev->net->ethtool_ops = &ax88179_ethtool_ops;
> + dev->net->needed_headroom = 8;
> + dev->net->max_mtu = 4088;
> +
> + /* Initialize MII structure */
> + dev->mii.dev = dev->net;
> + dev->mii.mdio_read = ax88179_mdio_read;
> + dev->mii.mdio_write = ax88179_mdio_write;
> + dev->mii.phy_id_mask = 0xff;
> + dev->mii.reg_num_mask = 0xff;
> + dev->mii.phy_id = 0x03;
> + dev->mii.supports_gmii = 1;
> + }
>
> dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> NETIF_F_RXCSUM;
> @@ -1330,6 +1335,13 @@ static int ax88179_bind(struct usbnet *dev,
> struct usb_interface *intf)
> return 0;
> }
>
> +static int ax88179_bind(struct usbnet *dev, struct usb_interface
> *intf)
> +{
> + usbnet_get_endpoints(dev, intf);
> +
> + return ax88179_link_bind_or_reset(dev, false);
> +}
> +
> static void ax88179_unbind(struct usbnet *dev, struct usb_interface
> *intf)
> {
> u16 tmp16;
> @@ -1458,74 +1470,7 @@ ax88179_tx_fixup(struct usbnet *dev, struct
> sk_buff *skb, gfp_t flags)
>
> static int ax88179_link_reset(struct usbnet *dev)
> {
> - struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
> - u8 tmp[5], link_sts;
> - u16 mode, tmp16, delay = HZ / 10;
> - u32 tmp32 = 0x40000000;
> - unsigned long jtimeout;
> -
> - jtimeout = jiffies + delay;
> - while (tmp32 & 0x40000000) {
> - mode = 0;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, &mode);
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2,
> - &ax179_data->rxctl);
> -
> - /*link up, check the usb device control TX FIFO full or empty*/
> - ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> -
> - if (time_after(jiffies, jtimeout))
> - return 0;
> - }
> -
> - 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);
> -
> - ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
> - GMII_PHY_PHYSR, 2, &tmp16);
> -
> - if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
> - return 0;
> - } else if (GMII_PHY_PHYSR_GIGA == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
> - mode |= AX_MEDIUM_GIGAMODE | AX_MEDIUM_EN_125MHZ;
> - if (dev->net->mtu > 1500)
> - mode |= AX_MEDIUM_JUMBO_EN;
> -
> - if (link_sts & AX_USB_SS)
> - memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
> - else if (link_sts & AX_USB_HS)
> - memcpy(tmp, &AX88179_BULKIN_SIZE[1], 5);
> - else
> - memcpy(tmp, &AX88179_BULKIN_SIZE[3], 5);
> - } else if (GMII_PHY_PHYSR_100 == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
> - mode |= AX_MEDIUM_PS;
> -
> - if (link_sts & (AX_USB_SS | AX_USB_HS))
> - memcpy(tmp, &AX88179_BULKIN_SIZE[2], 5);
> - else
> - memcpy(tmp, &AX88179_BULKIN_SIZE[3], 5);
> - } else {
> - memcpy(tmp, &AX88179_BULKIN_SIZE[3], 5);
> - }
We seem to have lost most of this logic. What is replacing it?
> -
> - /* RX bulk configuration */
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_BULKIN_QCTRL, 5, 5,
> tmp);
> -
> - dev->rx_urb_size = (1024 * (tmp[3] + 2));
> -
> - if (tmp16 & GMII_PHY_PHYSR_FULL)
> - mode |= AX_MEDIUM_FULL_DUPLEX;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> - 2, 2, &mode);
> -
> - ax179_data->eee_enabled = ax88179_chk_eee(dev);
> -
> - netif_carrier_on(dev->net);
> -
> - return 0;
> + return ax88179_link_bind_or_reset(dev, true);
> }
>
> static int ax88179_reset(struct usbnet *dev)
I'd suggest that this patch is reverted until we understand what is
actually going on here.
Thanks,
M.
--
Fast, cheap, reliable. Pick two.
Powered by blists - more mailing lists