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 <[email protected]> --- Cc: Yoshihiro Shimoda <[email protected]> Cc: Michael Dege <[email protected]> Cc: Andrew Lunn <[email protected]> Cc: "David S. Miller" <[email protected]> Cc: Eric Dumazet <[email protected]> Cc: Jakub Kicinski <[email protected]> Cc: Paolo Abeni <[email protected]> Cc: Geert Uytterhoeven <[email protected]> Cc: "Russell King (Oracle)" <[email protected]> --- drivers/net/ethernet/renesas/rswitch_main.c | 1 + 1 file changed, 1 insertion(+) 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" -- 2.43.0
