| 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
| ||
|
Message-ID: <20180402212526.2a9b709a@why.wild-wind.fr.eu.org>
Date: Mon, 2 Apr 2018 21:25:26 +0100
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>
Subject: Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code
On Mon, 2 Apr 2018 07:43:49 +0000
Alexander Kurz <akurz@...la.de> 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 | 137 ++++++++++-------------------------------
> 1 file changed, 31 insertions(+), 106 deletions(-)
The good news is that this patch doesn't seem to break anything this
time. A few remarks below:
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index a6ef75907ae9..fea4c7b877cc 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_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);
> -
So you move this to "bind"...
> tmp16 = (u16 *)buf;
> tmp = (u8 *)buf;
>
> - memset(ax179_data, 0, sizeof(*ax179_data));
> + if (!do_reset)
> + memset(ax179_data, 0, sizeof(*ax179_data));
... but not that. Why? They both are exclusive to the bind operation.
>
> /* 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);
> +
> 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_bind_or_reset(dev, false);
> +}
I find the whole construct extremely messy.
You shouldn't need to "bind or reset" the adapter. You first reset it
(that's the HW part), and you then bind it (that's the SW part). I
understand that the code is quite messy already, and that you're trying
to improve it. I'm just not convinced that having a single function
containing everything and the kitchen sink is the most sensible
approach.
Instead, you probably want to extract stuff that needs to be done at
reset time from what can be done at bind time, and keep the two quite
separate. The fact that bind is mostly a superset of reset is a bit of
an odd thing, and it'd be good to get to the bottom of that (I fully
admit that my understanding of USB networking is close to zero).
I came up with the below hack on top of your patch, and things seem to
still behave.
Thanks,
M.
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index fea4c7b877cc..aed98d400d7a 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_or_reset(struct usbnet *dev, bool do_reset)
+static int ax88179_reset(struct usbnet *dev)
{
u8 buf[5];
u16 *tmp16;
@@ -1234,9 +1234,6 @@ static int ax88179_bind_or_reset(struct usbnet *dev, bool do_reset)
tmp16 = (u16 *)buf;
tmp = (u8 *)buf;
- if (!do_reset)
- memset(ax179_data, 0, sizeof(*ax179_data));
-
/* Power up ethernet PHY */
*tmp16 = 0;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
@@ -1248,13 +1245,10 @@ static int ax88179_bind_or_reset(struct usbnet *dev, bool do_reset)
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
msleep(100);
- if (do_reset)
- ax88179_auto_detach(dev, 0);
+ ax88179_auto_detach(dev, 0);
ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
ETH_ALEN, dev->net->dev_addr);
- if (!do_reset)
- memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
/* RX bulk configuration */
memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
@@ -1269,28 +1263,6 @@ static int ax88179_bind_or_reset(struct usbnet *dev, bool do_reset)
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
1, 1, tmp);
- 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;
-
- dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
- NETIF_F_RXCSUM;
-
/* Enable checksum offload */
*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
@@ -1337,9 +1309,36 @@ static int ax88179_bind_or_reset(struct usbnet *dev, bool do_reset)
static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
{
+ struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
+
usbnet_get_endpoints(dev, intf);
- return ax88179_bind_or_reset(dev, false);
+ memset(ax179_data, 0, sizeof(*ax179_data));
+
+ dev->net->netdev_ops = &ax88179_netdev_ops;
+ dev->net->ethtool_ops = &ax88179_ethtool_ops;
+ dev->net->needed_headroom = 8;
+ dev->net->max_mtu = 4088;
+
+ dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+ NETIF_F_RXCSUM;
+
+ dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+ NETIF_F_RXCSUM;
+
+ /* 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;
+
+ ax88179_reset(dev);
+ memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
+
+ return 0;
}
static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf)
@@ -1540,11 +1539,6 @@ static int ax88179_link_reset(struct usbnet *dev)
return 0;
}
-static int ax88179_reset(struct usbnet *dev)
-{
- return ax88179_bind_or_reset(dev, true);
-}
-
static int ax88179_stop(struct usbnet *dev)
{
u16 tmp16;
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists