Hi,
I only recently browsed through the code, and had some queries regarding
the changes introduced by this commit.
On 21/11/18 3:43 pm, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@xxxxxxxxxxxx>
>
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@xxxxxxxxxxxx>
> Signed-off-by: Igor Russkikh <igor.russkikh@xxxxxxxxxxxx>
> ---
> drivers/net/usb/aqc111.c | 47 ++++++++++++++++++++++++++++++++++++++++
> drivers/net/usb/aqc111.h | 1 +
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index e33be16b506c..390ed6cbc3fd 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -11,6 +11,7 @@
> #include <linux/netdevice.h>
> #include <linux/mii.h>
> #include <linux/usb.h>
> +#include <linux/if_vlan.h>
> #include <linux/usb/cdc.h>
> #include <linux/usb/usbnet.h>
>
> @@ -204,11 +205,43 @@ static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed)
> aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg);
> }
>
> +static int aqc111_set_mac_addr(struct net_device *net, void *p)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + int ret = 0;
> +
> + ret = eth_mac_addr(net, p);
> + if (ret < 0)
> + return ret;
> +
When eth_mac_addr() fails, from what I can see, it returns either -EBUSY, or
-EADDRNOTAVAIL.
Wouldn't it be a better idea to set a random MAC address instead, when
-EADDRNOTAVAIL is returned, so that the interface still comes up and is
usable?
I'm only asking because this feels similar to the discussion that can be found here.
https://lkml.org/lkml/2020/10/2/305
> + /* Set the MAC address */
> + return aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
> + ETH_ALEN, net->dev_addr);
> +}
> +
> static const struct net_device_ops aqc111_netdev_ops = {
> .ndo_open = usbnet_open,
> .ndo_stop = usbnet_stop,
> + .ndo_set_mac_address = aqc111_set_mac_addr,
> + .ndo_validate_addr = eth_validate_addr,
> };
>
> +static int aqc111_read_perm_mac(struct usbnet *dev)
> +{
> + u8 buf[ETH_ALEN];
> + int ret;
> +
> + ret = aqc111_read_cmd(dev, AQ_FLASH_PARAMETERS, 0, 0, ETH_ALEN, buf);
> + if (ret < 0)
> + goto out;
> +
> + ether_addr_copy(dev->net->perm_addr, buf);
> +
> + return 0;
> +out:
> + return ret;
> +}
> +
> static void aqc111_read_fw_version(struct usbnet *dev,
> struct aqc111_data *aqc111_data)
> {
> @@ -251,6 +284,12 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
> /* store aqc111_data pointer in device data field */
> dev->driver_priv = aqc111_data;
>
> + /* Init the MAC address */
> + ret = aqc111_read_perm_mac(dev);
> + if (ret)
> + goto out;
> +
> + ether_addr_copy(dev->net->dev_addr, dev->net->perm_addr);
> dev->net->netdev_ops = &aqc111_netdev_ops;
>
> aqc111_read_fw_version(dev, aqc111_data);
> @@ -259,6 +298,10 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
> SPEED_5000 : SPEED_1000;
>
> return 0;
> +
> +out:
> + kfree(aqc111_data);
> + return ret;
> }
>
> static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
> @@ -467,6 +510,10 @@ static int aqc111_reset(struct usbnet *dev)
> aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0,
> &aqc111_data->phy_cfg);
>
> + /* Set the MAC address */
> + aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
> + ETH_ALEN, dev->net->dev_addr);
> +
There's a chance that aqc111_write_cmd() can end up writing only a
part of the required amount of data too.
Wouldn't it be a better idea to enforce a sanity check here, and set a
random mac address instead if this write fails?
> reg8 = 0xFF;
> aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_BM_INT_MASK, 1, 1, ®8);
>
> diff --git a/drivers/net/usb/aqc111.h b/drivers/net/usb/aqc111.h
> index f3b45d8ca4e3..0c8e1ee29893 100644
> --- a/drivers/net/usb/aqc111.h
> +++ b/drivers/net/usb/aqc111.h
> @@ -11,6 +11,7 @@
> #define __LINUX_USBNET_AQC111_H
>
> #define AQ_ACCESS_MAC 0x01
> +#define AQ_FLASH_PARAMETERS 0x20
> #define AQ_PHY_POWER 0x31
> #define AQ_PHY_OPS 0x61
>
If any of these changes sound like a good idea, I'd be happy to write a
patch implementing them. :)
Thanks,
Anant
