* [v3,2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 7:43 ` Alexander Kurz
0 siblings, 0 replies; 14+ messages in thread
From: Alexander Kurz @ 2018-04-02 7:43 UTC (permalink / raw)
To: David S . Miller
Cc: Andrew F . Davis, Marc Zyngier, linux-usb, netdev, Freddy Xin,
Alexander Kurz
Remove the duplicated code for asix88179_178a bind and reset methods.
Signed-off-by: Alexander Kurz <akurz@blala.de>
---
drivers/net/usb/ax88179_178a.c | 137 ++++++++++-------------------------------
1 file changed, 31 insertions(+), 106 deletions(-)
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);
-
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);
+
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);
+}
+
static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf)
{
u16 tmp16;
@@ -1530,94 +1542,7 @@ static int ax88179_link_reset(struct usbnet *dev)
static int ax88179_reset(struct usbnet *dev)
{
- u8 buf[5];
- u16 *tmp16;
- u8 *tmp;
- struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
- struct ethtool_eee eee_data;
-
- tmp16 = (u16 *)buf;
- tmp = (u8 *)buf;
-
- /* Power up ethernet PHY */
- *tmp16 = 0;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
-
- *tmp16 = AX_PHYPWR_RSTCTL_IPRL;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
- msleep(200);
-
- *tmp = AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
- msleep(100);
-
- /* Ethernet PHY Auto Detach*/
- ax88179_auto_detach(dev, 0);
-
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
- dev->net->dev_addr);
-
- /* RX bulk configuration */
- memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_BULKIN_QCTRL, 5, 5, tmp);
-
- dev->rx_urb_size = 1024 * 20;
-
- *tmp = 0x34;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);
-
- *tmp = 0x52;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
- 1, 1, tmp);
-
- 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;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, tmp);
-
- *tmp = AX_TXCOE_IP | AX_TXCOE_TCP | AX_TXCOE_UDP |
- AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, tmp);
-
- /* Configure RX control register => start operation */
- *tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
- AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16);
-
- *tmp = AX_MONITOR_MODE_PMETYPE | AX_MONITOR_MODE_PMEPOL |
- AX_MONITOR_MODE_RWMP;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, tmp);
-
- /* Configure default medium type => giga */
- *tmp16 = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
- AX_MEDIUM_RXFLOW_CTRLEN | AX_MEDIUM_FULL_DUPLEX |
- AX_MEDIUM_GIGAMODE;
- ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, tmp16);
-
- ax88179_led_setting(dev);
-
- ax179_data->eee_enabled = 0;
- ax179_data->eee_active = 0;
-
- ax88179_disable_eee(dev);
-
- ax88179_ethtool_get_eee(dev, &eee_data);
- eee_data.advertised = 0;
- ax88179_ethtool_set_eee(dev, &eee_data);
-
- /* Restart autoneg */
- mii_nway_restart(&dev->mii);
-
- usbnet_link_change(dev, 0, 0);
-
- return 0;
+ return ax88179_bind_or_reset(dev, true);
}
static int ax88179_stop(struct usbnet *dev)
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 9:45 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2018-04-02 9:45 UTC (permalink / raw)
To: Alexander Kurz
Cc: David S . Miller, Andrew F . Davis, linux-usb, netdev, Freddy Xin
On Mon, 02 Apr 2018 08:43:49 +0100,
Alexander Kurz wrote:
Alexander,
>
> Remove the duplicated code for asix88179_178a bind and reset methods.
>
> Signed-off-by: Alexander Kurz <akurz@blala.de>
> ---
> drivers/net/usb/ax88179_178a.c | 137 ++++++++++-------------------------------
> 1 file changed, 31 insertions(+), 106 deletions(-)
What has changed between this patch and the previous one? Having a bit
of a change-log would certainly help. Also, I would have appreciated a
reply to the questions I had on v2 before you posted a third version.
>
> 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);
> -
> 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);
> +
> 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);
> +}
> +
> static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf)
> {
> u16 tmp16;
> @@ -1530,94 +1542,7 @@ static int ax88179_link_reset(struct usbnet *dev)
>
> static int ax88179_reset(struct usbnet *dev)
> {
> - u8 buf[5];
> - u16 *tmp16;
> - u8 *tmp;
> - struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
> - struct ethtool_eee eee_data;
> -
> - tmp16 = (u16 *)buf;
> - tmp = (u8 *)buf;
> -
> - /* Power up ethernet PHY */
> - *tmp16 = 0;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> -
> - *tmp16 = AX_PHYPWR_RSTCTL_IPRL;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> - msleep(200);
> -
> - *tmp = AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
> - msleep(100);
> -
> - /* Ethernet PHY Auto Detach*/
> - ax88179_auto_detach(dev, 0);
> -
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
> - dev->net->dev_addr);
> -
> - /* RX bulk configuration */
> - memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_BULKIN_QCTRL, 5, 5, tmp);
> -
> - dev->rx_urb_size = 1024 * 20;
> -
> - *tmp = 0x34;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);
> -
> - *tmp = 0x52;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
> - 1, 1, tmp);
> -
> - 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;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, tmp);
> -
> - *tmp = AX_TXCOE_IP | AX_TXCOE_TCP | AX_TXCOE_UDP |
> - AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, tmp);
> -
> - /* Configure RX control register => start operation */
> - *tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
> - AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16);
> -
> - *tmp = AX_MONITOR_MODE_PMETYPE | AX_MONITOR_MODE_PMEPOL |
> - AX_MONITOR_MODE_RWMP;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, tmp);
> -
> - /* Configure default medium type => giga */
> - *tmp16 = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
> - AX_MEDIUM_RXFLOW_CTRLEN | AX_MEDIUM_FULL_DUPLEX |
> - AX_MEDIUM_GIGAMODE;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> - 2, 2, tmp16);
> -
> - ax88179_led_setting(dev);
> -
> - ax179_data->eee_enabled = 0;
> - ax179_data->eee_active = 0;
> -
> - ax88179_disable_eee(dev);
> -
> - ax88179_ethtool_get_eee(dev, &eee_data);
> - eee_data.advertised = 0;
> - ax88179_ethtool_set_eee(dev, &eee_data);
> -
> - /* Restart autoneg */
> - mii_nway_restart(&dev->mii);
> -
> - usbnet_link_change(dev, 0, 0);
> -
> - return 0;
> + return ax88179_bind_or_reset(dev, true);
> }
>
> static int ax88179_stop(struct usbnet *dev)
Overall, this patch makes much more sense than the previous one (I can
actually see duplicated code being removed). I'll give it a go later
today.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
^ permalink raw reply [flat|nested] 14+ messages in thread* [v3,2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 9:45 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2018-04-02 9:45 UTC (permalink / raw)
To: Alexander Kurz
Cc: David S . Miller, Andrew F . Davis, linux-usb, netdev, Freddy Xin
On Mon, 02 Apr 2018 08:43:49 +0100,
Alexander Kurz wrote:
Alexander,
>
> Remove the duplicated code for asix88179_178a bind and reset methods.
>
> Signed-off-by: Alexander Kurz <akurz@blala.de>
> ---
> drivers/net/usb/ax88179_178a.c | 137 ++++++++++-------------------------------
> 1 file changed, 31 insertions(+), 106 deletions(-)
What has changed between this patch and the previous one? Having a bit
of a change-log would certainly help. Also, I would have appreciated a
reply to the questions I had on v2 before you posted a third version.
>
> 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);
> -
> 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);
> +
> 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);
> +}
> +
> static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf)
> {
> u16 tmp16;
> @@ -1530,94 +1542,7 @@ static int ax88179_link_reset(struct usbnet *dev)
>
> static int ax88179_reset(struct usbnet *dev)
> {
> - u8 buf[5];
> - u16 *tmp16;
> - u8 *tmp;
> - struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
> - struct ethtool_eee eee_data;
> -
> - tmp16 = (u16 *)buf;
> - tmp = (u8 *)buf;
> -
> - /* Power up ethernet PHY */
> - *tmp16 = 0;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> -
> - *tmp16 = AX_PHYPWR_RSTCTL_IPRL;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> - msleep(200);
> -
> - *tmp = AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
> - msleep(100);
> -
> - /* Ethernet PHY Auto Detach*/
> - ax88179_auto_detach(dev, 0);
> -
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
> - dev->net->dev_addr);
> -
> - /* RX bulk configuration */
> - memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_BULKIN_QCTRL, 5, 5, tmp);
> -
> - dev->rx_urb_size = 1024 * 20;
> -
> - *tmp = 0x34;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);
> -
> - *tmp = 0x52;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
> - 1, 1, tmp);
> -
> - 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;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, tmp);
> -
> - *tmp = AX_TXCOE_IP | AX_TXCOE_TCP | AX_TXCOE_UDP |
> - AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, tmp);
> -
> - /* Configure RX control register => start operation */
> - *tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
> - AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16);
> -
> - *tmp = AX_MONITOR_MODE_PMETYPE | AX_MONITOR_MODE_PMEPOL |
> - AX_MONITOR_MODE_RWMP;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, tmp);
> -
> - /* Configure default medium type => giga */
> - *tmp16 = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
> - AX_MEDIUM_RXFLOW_CTRLEN | AX_MEDIUM_FULL_DUPLEX |
> - AX_MEDIUM_GIGAMODE;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> - 2, 2, tmp16);
> -
> - ax88179_led_setting(dev);
> -
> - ax179_data->eee_enabled = 0;
> - ax179_data->eee_active = 0;
> -
> - ax88179_disable_eee(dev);
> -
> - ax88179_ethtool_get_eee(dev, &eee_data);
> - eee_data.advertised = 0;
> - ax88179_ethtool_set_eee(dev, &eee_data);
> -
> - /* Restart autoneg */
> - mii_nway_restart(&dev->mii);
> -
> - usbnet_link_change(dev, 0, 0);
> -
> - return 0;
> + return ax88179_bind_or_reset(dev, true);
> }
>
> static int ax88179_stop(struct usbnet *dev)
Overall, this patch makes much more sense than the previous one (I can
actually see duplicated code being removed). I'll give it a go later
today.
Thanks,
M.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 14:14 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2018-04-02 14:14 UTC (permalink / raw)
To: marc.zyngier; +Cc: akurz, afd, linux-usb, netdev, freddy
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 02 Apr 2018 10:45:40 +0100
> What has changed between this patch and the previous one? Having a bit
> of a change-log would certainly help. Also, I would have appreciated a
> reply to the questions I had on v2 before you posted a third version.
Agreed, and I'm not applying these patches until this is sorted out
and explained properly.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [v3,2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 14:14 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2018-04-02 14:14 UTC (permalink / raw)
To: marc.zyngier; +Cc: akurz, afd, linux-usb, netdev, freddy
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 02 Apr 2018 10:45:40 +0100
> What has changed between this patch and the previous one? Having a bit
> of a change-log would certainly help. Also, I would have appreciated a
> reply to the questions I had on v2 before you posted a third version.
Agreed, and I'm not applying these patches until this is sorted out
and explained properly.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 15:21 ` Alexander Kurz
0 siblings, 0 replies; 14+ messages in thread
From: Alexander Kurz @ 2018-04-02 15:21 UTC (permalink / raw)
To: David Miller; +Cc: marc.zyngier, afd, linux-usb, netdev, freddy
Hi Marc, David,
with the v2 patch ("net: usb: asix88179_178a: de-duplicate code")
I made an embarrasly stupid mistake of removing the wrong function.
The v2 patch accidentially changed ax88179_link_reset() instead of
ax88179_reset(). Hunk 6 of v2 ("net: usb: asix88179_178a: de-duplicate
code") is just utterly wrong.
ax88179_bind() and ax88179_reset() were the correct targets to be
de-duplicated, as done in the v3 patch.
Sorry for this, Alexander
On Mon, 2 Apr 2018, David Miller wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> Date: Mon, 02 Apr 2018 10:45:40 +0100
>
> > What has changed between this patch and the previous one? Having a bit
> > of a change-log would certainly help. Also, I would have appreciated a
> > reply to the questions I had on v2 before you posted a third version.
>
> Agreed, and I'm not applying these patches until this is sorted out
> and explained properly.
>
^ permalink raw reply [flat|nested] 14+ messages in thread* [v3,2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 15:21 ` Alexander Kurz
0 siblings, 0 replies; 14+ messages in thread
From: Alexander Kurz @ 2018-04-02 15:21 UTC (permalink / raw)
To: David Miller; +Cc: marc.zyngier, afd, linux-usb, netdev, freddy
Hi Marc, David,
with the v2 patch ("net: usb: asix88179_178a: de-duplicate code")
I made an embarrasly stupid mistake of removing the wrong function.
The v2 patch accidentially changed ax88179_link_reset() instead of
ax88179_reset(). Hunk 6 of v2 ("net: usb: asix88179_178a: de-duplicate
code") is just utterly wrong.
ax88179_bind() and ax88179_reset() were the correct targets to be
de-duplicated, as done in the v3 patch.
Sorry for this, Alexander
On Mon, 2 Apr 2018, David Miller wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> Date: Mon, 02 Apr 2018 10:45:40 +0100
>
> > What has changed between this patch and the previous one? Having a bit
> > of a change-log would certainly help. Also, I would have appreciated a
> > reply to the questions I had on v2 before you posted a third version.
>
> Agreed, and I'm not applying these patches until this is sorted out
> and explained properly.
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 19:06 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2018-04-02 19:06 UTC (permalink / raw)
To: Alexander Kurz; +Cc: David Miller, afd, linux-usb, netdev, freddy
[dropping Freddy as I'm getting bounces from asix.com.tw]
On Mon, 2 Apr 2018 15:21:08 +0000 Alexander Kurz <akurz@blala.de> wrote:
Alexander,
> Hi Marc, David,
> with the v2 patch ("net: usb: asix88179_178a: de-duplicate code")
> I made an embarrasly stupid mistake of removing the wrong function.
> The v2 patch accidentially changed ax88179_link_reset() instead of
> ax88179_reset(). Hunk 6 of v2 ("net: usb: asix88179_178a: de-duplicate
> code") is just utterly wrong.
OK, that explains what I saw here.
> ax88179_bind() and ax88179_reset() were the correct targets to be
> de-duplicated, as done in the v3 patch.
>
> Sorry for this, Alexander
We all make mistakes, so let's try to learn from them.
You can improve your process by testing what you're about to send (a
very basic requirement), and documenting the changes you make on each
version you send (a cover letter is a good place to put it).
Answering reviewer questions helps building trust between the
contributor and the maintainer on the receiving end of the patch, and
you probably want to answer them before sending a new version so that a
proper discussion can take place, specially if the reviewer doesn't
quite see what you're aiming for.
I'll comment on the patch separately.
Hope this helps,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 14+ messages in thread* [v3,2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 19:06 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2018-04-02 19:06 UTC (permalink / raw)
To: Alexander Kurz; +Cc: David Miller, afd, linux-usb, netdev, freddy
[dropping Freddy as I'm getting bounces from asix.com.tw]
On Mon, 2 Apr 2018 15:21:08 +0000 Alexander Kurz <akurz@blala.de> wrote:
Alexander,
> Hi Marc, David,
> with the v2 patch ("net: usb: asix88179_178a: de-duplicate code")
> I made an embarrasly stupid mistake of removing the wrong function.
> The v2 patch accidentially changed ax88179_link_reset() instead of
> ax88179_reset(). Hunk 6 of v2 ("net: usb: asix88179_178a: de-duplicate
> code") is just utterly wrong.
OK, that explains what I saw here.
> ax88179_bind() and ax88179_reset() were the correct targets to be
> de-duplicated, as done in the v3 patch.
>
> Sorry for this, Alexander
We all make mistakes, so let's try to learn from them.
You can improve your process by testing what you're about to send (a
very basic requirement), and documenting the changes you make on each
version you send (a cover letter is a good place to put it).
Answering reviewer questions helps building trust between the
contributor and the maintainer on the receiving end of the patch, and
you probably want to answer them before sending a new version so that a
proper discussion can take place, specially if the reviewer doesn't
quite see what you're aiming for.
I'll comment on the patch separately.
Hope this helps,
M.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 20:25 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2018-04-02 20:25 UTC (permalink / raw)
To: Alexander Kurz; +Cc: David S . Miller, Andrew F . Davis, linux-usb, netdev
On Mon, 2 Apr 2018 07:43:49 +0000
Alexander Kurz <akurz@blala.de> wrote:
> Remove the duplicated code for asix88179_178a bind and reset methods.
>
> Signed-off-by: Alexander Kurz <akurz@blala.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.
^ permalink raw reply related [flat|nested] 14+ messages in thread* [v3,2/2] net: usb: asix88179_178a: de-duplicate code
@ 2018-04-02 20:25 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2018-04-02 20:25 UTC (permalink / raw)
To: Alexander Kurz; +Cc: David S . Miller, Andrew F . Davis, linux-usb, netdev
On Mon, 2 Apr 2018 07:43:49 +0000
Alexander Kurz <akurz@blala.de> wrote:
> Remove the duplicated code for asix88179_178a bind and reset methods.
>
> Signed-off-by: Alexander Kurz <akurz@blala.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;
^ permalink raw reply related [flat|nested] 14+ messages in thread