Re: [PATCH net-next v2 1/3] net: dsa: use slave device phydev

From: Florian Fainelli
Date: Fri Sep 22 2017 - 16:00:44 EST


On 09/22/2017 12:40 PM, Vivien Didelot wrote:
> There is no need to store a phy_device in dsa_slave_priv since
> net_device already provides one. Simply s/p->phy/dev->phydev/.

You can therefore remove the phy_device from dsa_slave_priv, see below
for more comments. I will have to regress test the heck out of this,
this should take a few hours.

>
> While at it, return -ENODEV when it is NULL instead of -EOPNOTSUPP.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> ---

> static int dsa_slave_port_attr_set(struct net_device *dev,
> @@ -435,12 +433,10 @@ static int
> dsa_slave_get_link_ksettings(struct net_device *dev,
> struct ethtool_link_ksettings *cmd)
> {
> - struct dsa_slave_priv *p = netdev_priv(dev);
> + if (!dev->phydev)
> + return -ENODEV;
>
> - if (!p->phy)
> - return -EOPNOTSUPP;
> -
> - phy_ethtool_ksettings_get(p->phy, cmd);
> + phy_ethtool_ksettings_get(dev->phydev, cmd);

This can be replaced by phy_ethtool_get_link_ksettings()

>
> return 0;
> }
> @@ -449,12 +445,10 @@ static int
> dsa_slave_set_link_ksettings(struct net_device *dev,
> const struct ethtool_link_ksettings *cmd)
> {
> - struct dsa_slave_priv *p = netdev_priv(dev);
> + if (!dev->phydev)
> + return -ENODEV;
>
> - if (p->phy != NULL)
> - return phy_ethtool_ksettings_set(p->phy, cmd);
> -
> - return -EOPNOTSUPP;
> + return phy_ethtool_ksettings_set(dev->phydev, cmd);
> }

This can disappear and you can assign this ethtool operation to
phy_ethtool_set_link_ksettings()

>
> static void dsa_slave_get_drvinfo(struct net_device *dev,
> @@ -488,24 +482,20 @@ dsa_slave_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *_p)
>
> static int dsa_slave_nway_reset(struct net_device *dev)
> {
> - struct dsa_slave_priv *p = netdev_priv(dev);
> + if (!dev->phydev)
> + return -ENODEV;
>
> - if (p->phy != NULL)
> - return genphy_restart_aneg(p->phy);
> -
> - return -EOPNOTSUPP;
> + return genphy_restart_aneg(dev->phydev);
> }

This can now disappear and you can use phy_ethtool_nway_reset() directly
in ethtool_ops

>
> static u32 dsa_slave_get_link(struct net_device *dev)
> {
> - struct dsa_slave_priv *p = netdev_priv(dev);
> + if (!dev->phydev)
> + return -ENODEV;
>
> - if (p->phy != NULL) {
> - genphy_update_link(p->phy);
> - return p->phy->link;
> - }
> + genphy_update_link(dev->phydev);
>
> - return -EOPNOTSUPP;
> + return dev->phydev->link;
> }

This should certainly be just ethtool_op_get_link(), not sure why we
kept that around here...
--
Florian