Re: [PATCH net-next v1 3/3] net: dsa: microchip: Fix PHY loopback configuration for KSZ8794 and KSZ8873

[Vladimir Oltean]

On Wed, Dec 06, 2023 at 09:55:20AM +0100, Oleksij Rempel wrote:
> On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote:
> > Hi,
> > 
> > On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote:
> > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
> > > driver. Previously, the code erroneously used Bit 7 of port register 0xD
> > > for both chip variants, which is actually for LED configuration. This
> > > update ensures the correct registers and bits are used for the PHY
> > > loopback feature:
> > > 
> > > - For KSZ8794: Use 0xF / Bit 7.
> > > - For KSZ8873: Use 0xD / Bit 0.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > 
> > This looks like a bugfix, so possibly worth a Fixes tag? Given the
> > dependency on the previous refactor, I think we can take it via net-
> > next.
> > 
> > @Andrew, Florian, Vladimir: do you have any specific preference here?
> 
> I do not think any one cares about supporting this switch variant in
> stable :)
> 
> Regards,
> Oleksij

Sorry, this simply fell through the cracks.

How is PHY loopback even supposed to be triggered? User space flips
NETIF_F_LOOPBACK on the netdev, driver ndo_set_features() catches it and
calls phy_loopback() and this calls into phylib's phydev->drv->set_loopback()
or the generic genphy_loopback()?

I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK.
The PHY is integrated, so DSA is the only relevant netdev driver. Is
there any other way to test this functionality?

If not, I think it's a case of "tree falling in the woods and nobody
hearing it". Not "stable" material. But it definitely has nothing to do
with not caring about the switch variant.

If my analysis is correct, then I actually have a suggestion for you,
Oleksij. Using the F word ("fix") can work against you, if you don't
have enough proof that you're really fixing something which has a user
visible impact. So either do a thorough analysis of the impact in the
commit message, or don't use the F word.