Hi all,
This patch series untangles the ethtool netlink dependency with PHYLIB
which exists because the cable test feature calls directly into PHY
library functions. The approach taken here is to utilize a new set of
net_device_ops function pointers which are automatically set to the PHY
library variants when a network device driver attaches to a PHY device.
Florian Fainelli (4):
net: Add cable test netdevice operations
net: phy: Change cable test arguments to net_device
net: phy: Automatically set-up cable test netdev_ops
net: ethtool: Remove PHYLIB dependency
drivers/net/phy/phy.c | 18 ++++++++++++++----
drivers/net/phy/phy_device.c | 32 ++++++++++++++++++++++++++++++++
include/linux/netdevice.h | 14 ++++++++++++++
include/linux/phy.h | 10 ++++++----
net/Kconfig | 1 -
net/ethtool/cabletest.c | 12 ++++++++----
6 files changed, 74 insertions(+), 13 deletions(-)
--
2.25.1
In order to untangle the ethtool/cabletest feature with the PHY library,
make the PHY library functions take a net_device argument and derive the
phy_device reference from there.
No functional changes introduced.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/phy.c | 18 ++++++++++++++----
include/linux/phy.h | 8 ++++----
net/ethtool/cabletest.c | 4 ++--
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 56cfae950472..fbb74f37b961 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -489,12 +489,17 @@ static void phy_abort_cable_test(struct phy_device *phydev)
phydev_err(phydev, "Error while aborting cable test");
}
-int phy_start_cable_test(struct phy_device *phydev,
+int phy_start_cable_test(struct net_device *dev,
struct netlink_ext_ack *extack)
{
- struct net_device *dev = phydev->attached_dev;
+ struct phy_device *phydev = dev->phydev;
int err = -ENOMEM;
+ if (!dev->phydev) {
+ NL_SET_ERR_MSG(extack, "Network device not attached to a PHY");
+ return -EOPNOTSUPP;
+ }
+
if (!(phydev->drv &&
phydev->drv->cable_test_start &&
phydev->drv->cable_test_get_status)) {
@@ -552,13 +557,18 @@ int phy_start_cable_test(struct phy_device *phydev,
}
EXPORT_SYMBOL(phy_start_cable_test);
-int phy_start_cable_test_tdr(struct phy_device *phydev,
+int phy_start_cable_test_tdr(struct net_device *dev,
struct netlink_ext_ack *extack,
const struct phy_tdr_config *config)
{
- struct net_device *dev = phydev->attached_dev;
+ struct phy_device *phydev = dev->phydev;
int err = -ENOMEM;
+ if (!dev->phydev) {
+ NL_SET_ERR_MSG(extack, "Network device not attached to a PHY");
+ return -EOPNOTSUPP;
+ }
+
if (!(phydev->drv &&
phydev->drv->cable_test_tdr_start &&
phydev->drv->cable_test_get_status)) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 101a48fa6750..53b95c52869d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1266,21 +1266,21 @@ int phy_restart_aneg(struct phy_device *phydev);
int phy_reset_after_clk_enable(struct phy_device *phydev);
#if IS_ENABLED(CONFIG_PHYLIB)
-int phy_start_cable_test(struct phy_device *phydev,
+int phy_start_cable_test(struct net_device *dev,
struct netlink_ext_ack *extack);
-int phy_start_cable_test_tdr(struct phy_device *phydev,
+int phy_start_cable_test_tdr(struct net_device *dev,
struct netlink_ext_ack *extack,
const struct phy_tdr_config *config);
#else
static inline
-int phy_start_cable_test(struct phy_device *phydev,
+int phy_start_cable_test(struct net_device *dev,
struct netlink_ext_ack *extack)
{
NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support");
return -EOPNOTSUPP;
}
static inline
-int phy_start_cable_test_tdr(struct phy_device *phydev,
+int phy_start_cable_test_tdr(struct net_device *dev,
struct netlink_ext_ack *extack,
const struct phy_tdr_config *config)
{
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 7194956aa09e..0d940a91493b 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -85,7 +85,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
if (ret < 0)
goto out_rtnl;
- ret = phy_start_cable_test(dev->phydev, info->extack);
+ ret = phy_start_cable_test(dev, info->extack);
ethnl_ops_complete(dev);
@@ -341,7 +341,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
if (ret < 0)
goto out_rtnl;
- ret = phy_start_cable_test_tdr(dev->phydev, info->extack, &cfg);
+ ret = phy_start_cable_test_tdr(dev, info->extack, &cfg);
ethnl_ops_complete(dev);
--
2.25.1
On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:
> Hi all,
>
> This patch series untangles the ethtool netlink dependency with PHYLIB
> which exists because the cable test feature calls directly into PHY
> library functions. The approach taken here is to utilize a new set of
> net_device_ops function pointers which are automatically set to the PHY
> library variants when a network device driver attaches to a PHY device.
I'm not sure about the idea of creating a copy of netdev_ops for each
device using phylib. First, there would be some overhead (just checked
my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
quite frequent pattern of comparing dev->netdev_ops against known
constants to check if a network device is of certain type; I can't say
for sure if it is also used with devices using phylib in existing code
but it feels risky.
As the two pointers are universal for all devices, couldn't we simply
use one global structure with them like we do for IPv6 (ipv6_stub) or
some netfilter modules (e.g. nf_ct_hook)?
Michal
> Florian Fainelli (4):
> net: Add cable test netdevice operations
> net: phy: Change cable test arguments to net_device
> net: phy: Automatically set-up cable test netdev_ops
> net: ethtool: Remove PHYLIB dependency
>
> drivers/net/phy/phy.c | 18 ++++++++++++++----
> drivers/net/phy/phy_device.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/netdevice.h | 14 ++++++++++++++
> include/linux/phy.h | 10 ++++++----
> net/Kconfig | 1 -
> net/ethtool/cabletest.c | 12 ++++++++----
> 6 files changed, 74 insertions(+), 13 deletions(-)
>
> --
> 2.25.1
>
On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote:
> On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:
> > Hi all,
> >
> > This patch series untangles the ethtool netlink dependency with PHYLIB
> > which exists because the cable test feature calls directly into PHY
> > library functions. The approach taken here is to utilize a new set of
> > net_device_ops function pointers which are automatically set to the PHY
> > library variants when a network device driver attaches to a PHY device.
>
> I'm not sure about the idea of creating a copy of netdev_ops for each
> device using phylib. First, there would be some overhead (just checked
> my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
> quite frequent pattern of comparing dev->netdev_ops against known
> constants to check if a network device is of certain type; I can't say
> for sure if it is also used with devices using phylib in existing code
> but it feels risky.
I agree with Michal here. I don't like this.
I think we need phylib to register a set of ops with ethtool when it
loads. It would also allow us to clean up phy_ethtool_get_strings(),
phy_ethtool_get_sset_count(), phy_ethtool_get_stats().
Andrew
On Thu, 2 Jul 2020 18:34:24 +0200 Andrew Lunn wrote:
> On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote:
> > On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:
> > > Hi all,
> > >
> > > This patch series untangles the ethtool netlink dependency with PHYLIB
> > > which exists because the cable test feature calls directly into PHY
> > > library functions. The approach taken here is to utilize a new set of
> > > net_device_ops function pointers which are automatically set to the PHY
> > > library variants when a network device driver attaches to a PHY device.
> >
> > I'm not sure about the idea of creating a copy of netdev_ops for each
> > device using phylib. First, there would be some overhead (just checked
> > my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
> > quite frequent pattern of comparing dev->netdev_ops against known
> > constants to check if a network device is of certain type; I can't say
> > for sure if it is also used with devices using phylib in existing code
> > but it feels risky.
>
> I agree with Michal here. I don't like this.
>
> I think we need phylib to register a set of ops with ethtool when it
> loads. It would also allow us to clean up phy_ethtool_get_strings(),
> phy_ethtool_get_sset_count(), phy_ethtool_get_stats().
+1
On 7/2/2020 9:35 AM, Jakub Kicinski wrote:
> On Thu, 2 Jul 2020 18:34:24 +0200 Andrew Lunn wrote:
>> On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote:
>>> On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:
>>>> Hi all,
>>>>
>>>> This patch series untangles the ethtool netlink dependency with PHYLIB
>>>> which exists because the cable test feature calls directly into PHY
>>>> library functions. The approach taken here is to utilize a new set of
>>>> net_device_ops function pointers which are automatically set to the PHY
>>>> library variants when a network device driver attaches to a PHY device.
>>>
>>> I'm not sure about the idea of creating a copy of netdev_ops for each
>>> device using phylib. First, there would be some overhead (just checked
>>> my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
>>> quite frequent pattern of comparing dev->netdev_ops against known
>>> constants to check if a network device is of certain type; I can't say
>>> for sure if it is also used with devices using phylib in existing code
>>> but it feels risky.
>>
>> I agree with Michal here. I don't like this.
>>
>> I think we need phylib to register a set of ops with ethtool when it
>> loads. It would also allow us to clean up phy_ethtool_get_strings(),
>> phy_ethtool_get_sset_count(), phy_ethtool_get_stats().
>
> +1
OK, that makes sense, I will work on that.
--
Florian