diff mbox series

[v7,phy-next,21/27] net: renesas: rswitch: include PHY provider header

Message ID 20260430110652.558622-22-vladimir.oltean@nxp.com (mailing list archive)
State New
Headers
Series Split Generic PHY consumer and provider |

Commit Message

Vladimir Oltean April 30, 2026, 11:06 a.m. UTC 
As a PHY consumer driver, the Renesas rswitch dereferences internal
fields of struct phy, something which shouldn't be done, as that is
going to be made an opaque pointer.

It is quite clearly visible that the driver is tightly coupled with the
drivers/phy/renesas/r8a779f0-ether-serdes.c, which puts heavy pressure
on the Generic PHY subsystem.

This was discussed before here:
https://lore.kernel.org/linux-phy/20260211194541.cdmibrpfn6ej6e74@skbuf/

but to summarize, it is generally expected that when a Generic PHY
function is called, it takes effect immediately. When this doesn't
happen, the PHY provider driver must change its implementation rather
than the consumer be made to work around it. PHY providers which rely on
a hardcoded call sequence in the consumer are just lazy and wrong.

The most obvious example is commit 5cb630925b49 ("net: renesas: rswitch:
Add phy_power_{on,off}() calling"). Problem description:
- Ethernet PHYs may change phydev->interface. When this happens, the
  SerDes must learn of the new phydev->interface using phy_set_mode_ext().
- drivers/phy/renesas/r8a779f0-ether-serdes.c implements phy_set_mode_ext(),
  but this only caches the mode and submode into channel->phy_interface
  and applies this to hardware during phy_power_on().

The commit author decided to work around this at the consumer site, by
power cycling the PHY for the configuration to take effect.

This had a worse implication from an API perspective in subsequent
commit 053f13f67be6 ("rswitch: Fix imbalance phy_power_off() calling").
It was observed that phy_power_on() and phy_power_off() calls need to be
balanced, and so, the consumer decided to start looking at the struct
phy :: power_count (the technical reason why I'm making this change).

This is also wrong from an API perspective because
- a consumer should only care about its own vote on the PHY power state.
  If this is a multi-port submode like QSGMII, a single phy_power_off()
  call will not actually turn the PHY off (nor should it).
- the power_count is written under the &phy->mutex, but read unlocked
  here.

The rswitch and r8a779f0-ether-serdes drivers both need to be completely
rethought in terms of Generic PHY API call sequence. There is no quick
fix to apply. Just include the PHY provider API along with the consumer
one, to keep working as before when struct phy will be made an opaque
pointer to normal PHY consumers. But this is a bad offender (and it's
not even a provider) so add a FIXME.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Michael Dege <michael.dege@renesas.com>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>

v2->v7: none
v1->v2: collect tag
---
 drivers/net/ethernet/renesas/rswitch_main.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Vladimir Oltean May 5, 2026, 9:24 a.m. UTC | #1 
Sashiko says:
> commit 4e15b6920cd2b2dfce685835fa09c66664407579
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> net: renesas: rswitch: include PHY provider header
> 
> This commit includes the PHY provider header in the rswitch driver as a
> workaround to avoid breaking the build when struct phy becomes opaque.
> It allows the driver to continue accessing internal PHY fields while
> acknowledging that the driver's PHY API call sequence needs a complete
> redesign.
> 
> > diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> > index 6fe964816322a..132be5f15073c 100644
> > --- a/drivers/net/ethernet/renesas/rswitch_main.c
> > +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/sys_soc.h>
> >
> > +#include "../../../phy/phy-provider.h" /* FIXME */
> 
> Does preserving the ability to check the provider's global reference counts
> (power_count and init_count) via this internal header leave the driver
> vulnerable to data races?
> 
> Since these fields are protected by the PHY provider's internal phy->mutex,
> reading them locklessly can create a time-of-check to time-of-use
> vulnerability. The count could be altered by a concurrent operation before
> the mutex is acquired inside phy_power_off() or phy_exit().
> 
> Additionally, if the PHY is shared across multiple ports (like in QSGMII
> mode), would checking the global reference count instead of tracking the
> consumer's local vote cause a port to observe a non-zero count from
> another port's vote? This could lead to an incorrect phy_power_off()
> call, prematurely decrementing the shared refcount and powering down the
> PHY while it is still in active use by other ports.
> 
> While the commit message notes this is a known issue preserved as a
> workaround until a complete redesign, could retaining these unlocked
> reads cause a regression in power vote tracking in the interim?
> 
> >  #include "rswitch.h"
> >  #include "rswitch_l2.h"

I will ignore this comment. Inaction will leave the driver in exactly
the same state as before. Not great, true, but I don't see how not
changing anything could cause a regression.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
index 6fe964816322..132be5f15073 100644
--- a/drivers/net/ethernet/renesas/rswitch_main.c
+++ b/drivers/net/ethernet/renesas/rswitch_main.c
@@ -27,6 +27,7 @@ 
 #include <linux/spinlock.h>
 #include <linux/sys_soc.h>
 
+#include "../../../phy/phy-provider.h" /* FIXME */
 #include "rswitch.h"
 #include "rswitch_l2.h"