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

2017-09-22 Thread Vivien Didelot 
Hi Florian,

Florian Fainelli  writes:

> 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.

OK, since this is a sensible topic, I will respin a v3 without this
patch, so that a future patchset can address your comments below and
also gives you time to test this one patch alone.

>>  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...

Haaa, good to read that! I wasn't sure about this, but with this patch
the slave phy ethtool functions seemed indeed quite generic...


Thanks,

Vivien

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

2017-09-22 Thread Andrew Lunn 
On Fri, Sep 22, 2017 at 03:40:43PM -0400, 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/.
> 
> While at it, return -ENODEV when it is NULL instead of -EOPNOTSUPP.

I just did a quick poll for calling phy_mii_ioctl(). ENODEV seems the
most popular, second to EINVAL. Marvell drivers all use EOPNOTSUPP.

>  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);
>  }

It looks like this can now be replaced with phy_ethtool_nway_reset().

It could be there are other phy_ethtool_ helpers which can be used,
now that we have phydev in ndev.

Andrew

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

2017-09-22 Thread Florian Fainelli 
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 
> ---

>  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

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

2017-09-22 Thread Vivien Didelot 
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/.

While at it, return -ENODEV when it is NULL instead of -EOPNOTSUPP.

Signed-off-by: Vivien Didelot 
---
 net/dsa/slave.c | 126 ++--
 1 file changed, 58 insertions(+), 68 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 02ace7d462c4..3760472bf41d 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -99,15 +99,15 @@ static int dsa_slave_open(struct net_device *dev)
}
 
if (ds->ops->port_enable) {
-   err = ds->ops->port_enable(ds, p->dp->index, p->phy);
+   err = ds->ops->port_enable(ds, p->dp->index, dev->phydev);
if (err)
goto clear_promisc;
}
 
dsa_port_set_state_now(p->dp, stp_state);
 
-   if (p->phy)
-   phy_start(p->phy);
+   if (dev->phydev)
+   phy_start(dev->phydev);
 
return 0;
 
@@ -130,8 +130,8 @@ static int dsa_slave_close(struct net_device *dev)
struct net_device *master = dsa_master_netdev(p);
struct dsa_switch *ds = p->dp->ds;
 
-   if (p->phy)
-   phy_stop(p->phy);
+   if (dev->phydev)
+   phy_stop(dev->phydev);
 
dev_mc_unsync(master, dev);
dev_uc_unsync(master, dev);
@@ -144,7 +144,7 @@ static int dsa_slave_close(struct net_device *dev)
dev_uc_del(master, dev->dev_addr);
 
if (ds->ops->port_disable)
-   ds->ops->port_disable(ds, p->dp->index, p->phy);
+   ds->ops->port_disable(ds, p->dp->index, dev->phydev);
 
dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
 
@@ -273,12 +273,10 @@ dsa_slave_fdb_dump(struct sk_buff *skb, struct 
netlink_callback *cb,
 
 static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
-   struct dsa_slave_priv *p = netdev_priv(dev);
+   if (!dev->phydev)
+   return -ENODEV;
 
-   if (p->phy != NULL)
-   return phy_mii_ioctl(p->phy, ifr, cmd);
-
-   return -EOPNOTSUPP;
+   return phy_mii_ioctl(dev->phydev, ifr, cmd);
 }
 
 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);
 
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);
 }
 
 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);
 }
 
 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;
 }
 
 static int dsa_slave_get_eeprom_len(struct net_device *dev)
@@ -640,7 +630,7 @@ static int dsa_slave_set_eee(struct net_device *dev, struct 
ethtool_eee *e)
int ret;
 
/* Port's PHY and MAC both need to be EEE capable */
-   if (!p->phy)
+   if (!dev->phydev)
return -ENODEV;
 
if (!ds->ops->set_mac_eee)
@@ -651,12 +641,12 @@ static int dsa_slave_set_eee(struct net_device *dev, 
struct ethtool_eee *e)
return ret;
 
if (e->eee_enabled) {
-   ret = phy_init_eee(p->phy, 0);
+   ret = phy_init_eee(dev->phydev, 0);
if (ret)
return ret;
}
 
-   return phy_ethtool_set_eee(p->phy, e);
+   return phy_ethtool_set_eee(dev->phydev, e);
 }
 
 static int dsa_slave_get_eee(struct net_device *dev, struct ethtool_eee *e)
@@