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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2262f266-6fb9-cce4-f83a-933dd41b9062@gmail.com>
Date:   Fri, 22 Sep 2017 13:00:32 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, kernel@...oirfairelinux.com,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v2 1/3] net: dsa: use slave device phydev

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@...oirfairelinux.com>
> ---

>  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