[PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

A number of network drivers has the same glue code to use phy_mii_ioctl
as ndo_do_ioctl handler. So let's add such a generic ndo_do_ioctl
handler to phylib. As first user convert r8169.

Heiner Kallweit (2):
  net: phy: add generic ndo_do_ioctl handler phy_do_ioctl
  r8169: use generic ndo_do_ioctl handler phy_do_ioctl

 drivers/net/ethernet/realtek/r8169_main.c | 12 +-----------
 drivers/net/phy/phy.c                     | 15 +++++++++++++++
 include/linux/phy.h                       |  1 +
 3 files changed, 17 insertions(+), 11 deletions(-)

-- 
2.25.0

[PATCH net-next 1/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

A number of network drivers has the same glue code to use phy_mii_ioctl
as ndo_do_ioctl handler. So let's add such a generic ndo_do_ioctl
handler to phylib.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 15 +++++++++++++++
 include/linux/phy.h   |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 541ed0149..da05b3480 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -432,6 +432,21 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 }
 EXPORT_SYMBOL(phy_mii_ioctl);
 
+/**
+ * phy_do_ioctl - generic ndo_do_ioctl implementation
+ * @dev: the net_device struct
+ * @ifr: &struct ifreq for socket ioctl's
+ * @cmd: ioctl cmd to execute
+ */
+int phy_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	if (!netif_running(dev) || !dev->phydev)
+		return -ENODEV;
+
+	return phy_mii_ioctl(dev->phydev, ifr, cmd);
+}
+EXPORT_SYMBOL(phy_do_ioctl);
+
 void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies)
 {
 	mod_delayed_work(system_power_efficient_wq, &phydev->state_queue,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2929d0bc3..996c4df11 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1231,6 +1231,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			      const struct ethtool_link_ksettings *cmd);
 int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
+int phy_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd);
 void phy_request_interrupt(struct phy_device *phydev);
 void phy_free_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
-- 
2.25.0

[PATCH net-next 2/2] r8169: use generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

Replace rtl8169_ioctl with new generic function phy_do_ioctl.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 6d699df7d..175f951b6 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2298,16 +2298,6 @@ static int rtl_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
-static int rtl8169_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-
-	if (!netif_running(dev))
-		return -ENODEV;
-
-	return phy_mii_ioctl(tp->phydev, ifr, cmd);
-}
-
 static void rtl_wol_suspend_quirk(struct rtl8169_private *tp)
 {
 	switch (tp->mac_version) {
@@ -5168,7 +5158,7 @@ static const struct net_device_ops rtl_netdev_ops = {
 	.ndo_fix_features	= rtl8169_fix_features,
 	.ndo_set_features	= rtl8169_set_features,
 	.ndo_set_mac_address	= rtl_set_mac_address,
-	.ndo_do_ioctl		= rtl8169_ioctl,
+	.ndo_do_ioctl		= phy_do_ioctl,
 	.ndo_set_rx_mode	= rtl_set_rx_mode,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= rtl8169_netpoll,
-- 
2.25.0

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Andrew Lunn]

On Sun, Jan 19, 2020 at 02:31:06PM +0100, Heiner Kallweit wrote:
> A number of network drivers has the same glue code to use phy_mii_ioctl
> as ndo_do_ioctl handler. So let's add such a generic ndo_do_ioctl
> handler to phylib. As first user convert r8169.

Hi Heiner

Looks sensible. 

Two questions:

Did you look at how many drivers don't make the running check? I know
there are some MAC drivers which allow PHY ioctls when the interface
is down.  So maybe we want to put _running_ into this helper name, and
add anther helper which does not check for running?

Do you plan to convert any more MAC drivers?

   Andrew

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

On 19.01.2020 17:12, Andrew Lunn wrote:
> On Sun, Jan 19, 2020 at 02:31:06PM +0100, Heiner Kallweit wrote:
>> A number of network drivers has the same glue code to use phy_mii_ioctl
>> as ndo_do_ioctl handler. So let's add such a generic ndo_do_ioctl
>> handler to phylib. As first user convert r8169.
> 
> Hi Heiner
> 
Hi Andrew,

> Looks sensible. 
> 
> Two questions:
> 
> Did you look at how many drivers don't make the running check? I know
> there are some MAC drivers which allow PHY ioctls when the interface
> is down.  So maybe we want to put _running_ into this helper name, and
> add anther helper which does not check for running?
> 
Almost all drivers have the running check. I found five that don't:

*ag71xx, fec_mpc52xx*
They don't have the running check but should, because the PHY is
attached in ndo_open only.

*agere, faraday, rdc*
They don't have the running check and attach the PHY in probe.

So yes, we could add a second helper w/o the running check, even if
it's just for three drivers. There may be more in the future.

> Do you plan to convert any more MAC drivers?
> 
Not yet ;) Question would be whether one patch would be sufficient
or whether we need one patch per driver that needs to be ACKed by
the respective maintainer.

>    Andrew
> 
Heiner

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Andrew Lunn]

Hi Heiner

> Almost all drivers have the running check. I found five that don't:
> 
> *ag71xx, fec_mpc52xx*
> They don't have the running check but should, because the PHY is
> attached in ndo_open only.

So long an ndo_close() sets the phydev pointer to NULL, it should be
safe. But do the drivers do this?

> *agere, faraday, rdc*
> They don't have the running check and attach the PHY in probe.
> 
> So yes, we could add a second helper w/o the running check, even if
> it's just for three drivers. There may be more in the future.
> 
> > Do you plan to convert any more MAC drivers?
> > 
> Not yet ;) Question would be whether one patch would be sufficient
> or whether we need one patch per driver that needs to be ACKed by
> the respective maintainer.

For this sort of mechanical change, i would do one patch for all
without running, and another with running. If any driver needs more
than a mechanical change, then do a patch per driver, and get the
maintainer involved.

    Andrew

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

On 19.01.2020 18:51, Andrew Lunn wrote:
> Hi Heiner
> 
>> Almost all drivers have the running check. I found five that don't:
>>
>> *ag71xx, fec_mpc52xx*
>> They don't have the running check but should, because the PHY is
>> attached in ndo_open only.
> 
> So long an ndo_close() sets the phydev pointer to NULL, it should be
> safe. But do the drivers do this?
> 
Setting ndev->phydev to NULL is done by phy_detach(), typically
called from phy_disconnect(). Both drivers do this.
In mpc52xx_fec_close() there might be a small chance for a race
because the chip is stopped before calling phy_diconnect().
Stopping the chip may make the MDIO bus unaccessible.

Actually I was wondering why we need to check at all whether
net_device is running. Typically the PHY is connected / disconnected
in ndo_open / ndo_close, and if both are implemented sanely then
checking for ndev->phydev should be sufficient.

Also it seems we don't consider situations like runtime PM yet.
Then the MDIO bus may not be accessible, but ndev is running
and PHY is attached. Maybe we should add a check for ndev being
present? Because typically netif_device_detach() is called
when (runtime-)suspending.

>> *agere, faraday, rdc*
>> They don't have the running check and attach the PHY in probe.
>>
>> So yes, we could add a second helper w/o the running check, even if
>> it's just for three drivers. There may be more in the future.
>>
>>> Do you plan to convert any more MAC drivers?
>>>
>> Not yet ;) Question would be whether one patch would be sufficient
>> or whether we need one patch per driver that needs to be ACKed by
>> the respective maintainer.
> 
> For this sort of mechanical change, i would do one patch for all
> without running, and another with running. If any driver needs more
> than a mechanical change, then do a patch per driver, and get the
> maintainer involved.
> 
>     Andrew
> 

Heiner

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Andrew Lunn]

> Also it seems we don't consider situations like runtime PM yet.
> Then the MDIO bus may not be accessible, but ndev is running
> and PHY is attached.

I don't think this can happen. If the device is running, the MDIO bus
has to work, or phylib is broken.

I have had issue with the FEC, runtime PM and MDIO, but that was when
the interface was not running, but an Ethernet switch was using the
MDIO bus. MDIO transactions would time out, until i made the MDIO
operations PM aware.

But in general, we should keep the running test, just to avoid
breakage of assumptions we don't know about.

	 Andrew

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

On 19.01.2020 19:50, Andrew Lunn wrote:
>> Also it seems we don't consider situations like runtime PM yet.
>> Then the MDIO bus may not be accessible, but ndev is running
>> and PHY is attached.
> 
> I don't think this can happen. If the device is running, the MDIO bus
> has to work, or phylib is broken.
> 

netif_running() checks flag __LINK_STATE_START. Once the network has
been brought up, this flag is set. Also the comment in the code says:
"Test if the device has been brought up."
A driver has no means to clear this flag when runtime-suspending.
netif_device_detach() however clears flag __LINK_STATE_PRESENT.
I think it's not completely consistent if a device can be running
that is not present. IMO naming of netif_running() is misleading.
It better should be named netif_if_up(), or similar.

So far very few network drivers use runtime PM, this may be the
reason why such questions didn't come up before.

Speaking for r8169:
If interface is up and cable detached, then it runtime-suspends
and goes into PCI D3 (chip and MDIO bus not accessible).
But ndev is "running" and PHY is attached.

> I have had issue with the FEC, runtime PM and MDIO, but that was when
> the interface was not running, but an Ethernet switch was using the
> MDIO bus. MDIO transactions would time out, until i made the MDIO
> operations PM aware.
> 
> But in general, we should keep the running test, just to avoid
> breakage of assumptions we don't know about.
> 
> 	 Andrew
> 

Heiner

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Andrew Lunn]

> Speaking for r8169:
> If interface is up and cable detached, then it runtime-suspends
> and goes into PCI D3 (chip and MDIO bus not accessible).
> But ndev is "running" and PHY is attached.

Hi Heiner

And how does it get out of this state? I assume the PHY interrupts
when the link is established. Is phylib handling this interrupt? If
so, when phylib accesses the MDIO bus, the bus needs to be runtime PM
aware. And if the bus is runtime PM aware, the IOCTL handler should
work, when the device is runtime suspended.  If the MAC is handling
this interrupt, and it is the MAC interrupt handler which is run-time
unsuspending, then the ioctl handler is not going to work unless it
also runtime unsuspends.

	Andrew

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

On 19.01.2020 21:50, Andrew Lunn wrote:
>> Speaking for r8169:
>> If interface is up and cable detached, then it runtime-suspends
>> and goes into PCI D3 (chip and MDIO bus not accessible).
>> But ndev is "running" and PHY is attached.
> 
> Hi Heiner
> 
Hi Andrew,

> And how does it get out of this state? I assume the PHY interrupts
> when the link is established. Is phylib handling this interrupt? If
> so, when phylib accesses the MDIO bus, the bus needs to be runtime PM
> aware. And if the bus is runtime PM aware, the IOCTL handler should
> work, when the device is runtime suspended.  If the MAC is handling
> this interrupt, and it is the MAC interrupt handler which is run-time
> unsuspending, then the ioctl handler is not going to work unless it
> also runtime unsuspends.
> 
The chip (wherever the magic happens) generates a PCI PME if WoL
config includes detection of physical link-up. The PCI core then
runtime-resumes the device.
I'd prefer the ioctl to return an error here instead of e.g.
0xffff for a register read.

> 	Andrew
> 
Heiner

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit]

On 19.01.2020 23:28, Heiner Kallweit wrote:
> On 19.01.2020 21:50, Andrew Lunn wrote:
>>> Speaking for r8169:
>>> If interface is up and cable detached, then it runtime-suspends
>>> and goes into PCI D3 (chip and MDIO bus not accessible).
>>> But ndev is "running" and PHY is attached.
>>
>> Hi Heiner
>>
> Hi Andrew,
> 
Hi Andrew

>> And how does it get out of this state? I assume the PHY interrupts
>> when the link is established. Is phylib handling this interrupt? If
>> so, when phylib accesses the MDIO bus, the bus needs to be runtime PM
>> aware. And if the bus is runtime PM aware, the IOCTL handler should
>> work, when the device is runtime suspended.  If the MAC is handling
>> this interrupt, and it is the MAC interrupt handler which is run-time
>> unsuspending, then the ioctl handler is not going to work unless it
>> also runtime unsuspends.
>>
> The chip (wherever the magic happens) generates a PCI PME if WoL
> config includes detection of physical link-up. The PCI core then
> runtime-resumes the device.
> I'd prefer the ioctl to return an error here instead of e.g.
> 0xffff for a register read.
> 
I checked a little further and indeed there's a need to check whether
net_device is present, but net core (dev_ifsioc) already does this
check before calling the ndo_do_ioctl handler. So we don't have to
do it in the handler again.

I saw that David applied my series already. So I'll send a follow-up
series that:
- renames phy_do_ioctl to phy_do_ioctl_running
- adds phy_do_ioctl (w/o check that netdev is running)
- converts users to phy_do_ioctl

>> 	Andrew
>>
> Heiner
> 
Heiner

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Sun, 19 Jan 2020 18:51:09 +0100

> Hi Heiner
> 
>> Not yet ;) Question would be whether one patch would be sufficient
>> or whether we need one patch per driver that needs to be ACKed by
>> the respective maintainer.
> 
> For this sort of mechanical change, i would do one patch for all
> without running, and another with running. If any driver needs more
> than a mechanical change, then do a patch per driver, and get the
> maintainer involved.

Agreed.

Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 19 Jan 2020 14:31:06 +0100

> A number of network drivers has the same glue code to use phy_mii_ioctl
> as ndo_do_ioctl handler. So let's add such a generic ndo_do_ioctl
> handler to phylib. As first user convert r8169.

Series applied, thanks Heiner.