Re: [PATCH 1/2] net: phy: add ethtool_phy_{get|set}_link_ksettings
On May 7, 2016 3:56:34 PM PDT, Philippe Reynes wrote: >On 07/05/16 13:59, Ben Hutchings wrote: >> On Sat, 2016-05-07 at 01:18 +0200, Philippe Reynes wrote: >>> The callback {get|set}_link_ksettings are often defined >>> in a very close way. There are mainly two differences in >>> those callback: >>> - the name of the netdev private structure >>> - the name of the struct phydev in the private structure >>> >>> We add two defines ethtool_phy_{get|set}_link_ksettings >>> to avoid writing severals times almost the same function. >> [...] >> >> I don't think there's no need to access a private structure, as >there's >> a phydev pointer in struct net_device. If some drivers don't >maintain >> that pointer, they should be changed to do so. Then they can >> use generic implementations of {get,set}_link_ksettings provided by >> phylib. > >If we could use the phydev in the struct net_device, we could write a >generic function for {get|set}_link_ksettings. It's a good idea. > >But I've quickly looked and a lot of ethernet driver use the private >structure to store the phydev. If the ethernet driver may use the >struct net_device for phydev, do you know why so many drivers use >the private structure ? The introduction of a phy_device pointer in net_device came much later than the introduction and use of PHYLIB by most drivers so it is probably just an oversight. > >If everybody agree, I can send a new version with a generic >{get|set}_link_ksettings >and a update of fec to use the phydev store in the structure >net_device. Yes, that sounds very reasonable. It might be possible to cook a coccinelle patch which replaces the use of a private phy_device pointer and utilize the one from net_device. -- Florian
Re: [PATCH 1/2] net: phy: add ethtool_phy_{get|set}_link_ksettings
On Sun, 2016-05-08 at 00:56 +0200, Philippe Reynes wrote: > On 07/05/16 13:59, Ben Hutchings wrote: > > > > On Sat, 2016-05-07 at 01:18 +0200, Philippe Reynes wrote: > > > > > > The callback {get|set}_link_ksettings are often defined > > > in a very close way. There are mainly two differences in > > > those callback: > > > - the name of the netdev private structure > > > - the name of the struct phydev in the private structure > > > > > > We add two defines ethtool_phy_{get|set}_link_ksettings > > > to avoid writing severals times almost the same function. > > [...] > > > > I don't think there's no need to access a private structure, as there's > > a phydev pointer in struct net_device. If some drivers don't maintain > > that pointer, they should be changed to do so. Then they can > > use generic implementations of {get,set}_link_ksettings provided by > > phylib. > If we could use the phydev in the struct net_device, we could write a > generic function for {get|set}_link_ksettings. It's a good idea. > > But I've quickly looked and a lot of ethernet driver use the private > structure to store the phydev. If the ethernet driver may use the > struct net_device for phydev, do you know why so many drivers use > the private structure ? Maybe just because no-one bothered to update them after it was added to net_device. Ben. > If everybody agree, I can send a new version with a generic > {get|set}_link_ksettings > and a update of fec to use the phydev store in the structure net_device. -- Ben Hutchings I haven't lost my mind; it's backed up on tape somewhere. signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2] net: phy: add ethtool_phy_{get|set}_link_ksettings
On 07/05/16 13:59, Ben Hutchings wrote: On Sat, 2016-05-07 at 01:18 +0200, Philippe Reynes wrote: The callback {get|set}_link_ksettings are often defined in a very close way. There are mainly two differences in those callback: - the name of the netdev private structure - the name of the struct phydev in the private structure We add two defines ethtool_phy_{get|set}_link_ksettings to avoid writing severals times almost the same function. [...] I don't think there's no need to access a private structure, as there's a phydev pointer in struct net_device. If some drivers don't maintain that pointer, they should be changed to do so. Then they can use generic implementations of {get,set}_link_ksettings provided by phylib. If we could use the phydev in the struct net_device, we could write a generic function for {get|set}_link_ksettings. It's a good idea. But I've quickly looked and a lot of ethernet driver use the private structure to store the phydev. If the ethernet driver may use the struct net_device for phydev, do you know why so many drivers use the private structure ? If everybody agree, I can send a new version with a generic {get|set}_link_ksettings and a update of fec to use the phydev store in the structure net_device. Philippe
Re: [PATCH 1/2] net: phy: add ethtool_phy_{get|set}_link_ksettings
On Sat, 2016-05-07 at 01:18 +0200, Philippe Reynes wrote: > The callback {get|set}_link_ksettings are often defined > in a very close way. There are mainly two differences in > those callback: > - the name of the netdev private structure > - the name of the struct phydev in the private structure > > We add two defines ethtool_phy_{get|set}_link_ksettings > to avoid writing severals times almost the same function. [...] I don't think there's no need to access a private structure, as there's a phydev pointer in struct net_device. If some drivers don't maintain that pointer, they should be changed to do so. Then they can use generic implementations of {get,set}_link_ksettings provided by phylib. Ben. -- Ben Hutchings Editing code like this is akin to sticking plasters on the bleeding stump of a severed limb. - me, 29 June 1999 signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2] net: phy: add ethtool_phy_{get|set}_link_ksettings
On 06/05/16 16:18, Philippe Reynes wrote: > The callback {get|set}_link_ksettings are often defined > in a very close way. There are mainly two differences in > those callback: > - the name of the netdev private structure > - the name of the struct phydev in the private structure > > We add two defines ethtool_phy_{get|set}_link_ksettings > to avoid writing severals times almost the same function. This looks fine in principle, but then there is a whole ton of code that could become like that in the kernel, I do not have any strong opinion either way... -- Florian
[PATCH 1/2] net: phy: add ethtool_phy_{get|set}_link_ksettings
The callback {get|set}_link_ksettings are often defined in a very close way. There are mainly two differences in those callback: - the name of the netdev private structure - the name of the struct phydev in the private structure We add two defines ethtool_phy_{get|set}_link_ksettings to avoid writing severals times almost the same function. Signed-off-by: Philippe Reynes --- include/linux/phy.h | 46 ++ 1 files changed, 46 insertions(+), 0 deletions(-) diff --git a/include/linux/phy.h b/include/linux/phy.h index be3f83b..e4a79fa 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -830,6 +830,52 @@ int phy_ethtool_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol); void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol); +/** + * ethtool_phy_get_link_ksettings() - Helper macro for get_link_ksettings + * @name: name of the driver + * @private: name of the private structure in the net device + * @phy: name of the phydev variable in the private structure + * + * Helper macro for the callback get_link_ksettings which + * simply call phy_ethtool_ksettings_get. + */ +#define ethtool_phy_get_link_ksettings(name, private, phy) \ + static int \ + name##_get_link_ksettings(struct net_device *ndev, \ + struct ethtool_link_ksettings *cmd) \ + { \ + struct private *priv = netdev_priv(ndev); \ + struct phy_device *phydev = priv->phy; \ + \ + if (!phydev)\ + return -ENODEV; \ + \ + return phy_ethtool_ksettings_get(phydev, cmd); \ + } + +/** + * ethtool_phy_set_link_ksettings() - Helper macro for set_link_ksettings + * @name: name of the driver + * @private: name of the private structure in the net device + * @phy: name of the phydev variable in the private structure + * + * Helper macro for the callback set_link_ksettings which + * simply call phy_ethtool_ksettings_set. + */ +#define ethtool_phy_set_link_ksettings(name, private, phy) \ + static int \ + name##_set_link_ksettings(struct net_device *ndev, \ + const struct ethtool_link_ksettings *cmd) \ + { \ + struct private *priv = netdev_priv(ndev); \ + struct phy_device *phydev = priv->phy; \ + \ + if (!phydev)\ + return -ENODEV; \ + \ + return phy_ethtool_ksettings_set(phydev, cmd); \ + } + int __init mdio_bus_init(void); void mdio_bus_exit(void); -- 1.7.4.4