On Wed, 4 Mar 2026 at 19:00, Vladimir Oltean <[email protected]> wrote: > 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]>
Reviewed-by: Geert Uytterhoeven <[email protected]> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
