Re: [PATCH net-next v2] net: dsa: mv88e6060: add phylink_get_caps implementation

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Aug 10, 2023 at 06:38:29PM +0100, Russell King (Oracle) wrote:
> What I meant is that there are no in-tree users of the Marvell 88E6060
> DSA driver. It looks like it was contributed in 2008. Whether it had
> users between the date that it was contributed and today I don't know.
> 
> All that I can see is that the only users of it are out-of-tree users,
> which means we have the maintenance burden from the driver but no
> apparent platforms that make use of it, and no way to test it (other
> than if one of those out-of-tree users pops up, such as like last
> month.)
> 
> I know that Arnd tends to strip out code that a platform uses when the
> platform is removed, was there a reason that this got left behind,
> assuming that it was used by a board?

I also have reasons to believe that this driver is seeing very little use,
based on the number of reports relative to the severity and age of the
bugs/performance limitations found.

But, since it hasn't really bothered the general DSA maintenance all that
much up until now, we just kept it, not knowing what state it is in.

If you're saying that's starting to change, I suppose we can start
issuing ultimatums, and make a list of what's being blocked because of
the lack of activity. But as long as Sergei is responding, maybe the
mv88e6060 can live another day.

> > Maybe if we don't want to introduce PHY_INTERFACE_MODE_SNI for fear of a
> > lack of real users, we could at least detect PortMode=0, and not
> > populate supported_interfaces, leading to an intentional validation
> > failure and a comment above that check, stating that phy-mode = "sni" is
> > not yet implemented?
> 
> It would probably be better for mv88e6060_phylink_get_caps() to detect
> it and print the warning, leaving supported_interfaces empty - which
> will then cause phylink_create() to fail. Maybe that's what you meant,
> but I interpreted it as modifying the check in phylink_create().

Yes, this is what I meant. I didn't say anything about phylink_create().

diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index 0e776be5e941..23cc6b01a1c4 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -263,15 +263,12 @@ static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port,
 		return;
 	}

-	if (!(ret & PORT_STATUS_PORTMODE)) {
-		/* Port configured in SNI mode (acts as a 10Mbps PHY) */
-		config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE;
-		/* I don't think SNI is SMII - SMII has a sync signal, and
-		 * SNI doesn't.
-		 */
-		__set_bit(PHY_INTERFACE_MODE_SMII, interfaces);
+	/* If the port is configured in SNI mode (acts as a 10Mbps PHY),
+	 * it should have phy-mode = "sni", but that doesn't yet exist, so
+	 * forcibly fail validation until the need arises to introduce it.
+	 */
+	if (!(ret & PORT_STATUS_PORTMODE))
 		return;
-	}

 	config->mac_capabilities = MAC_100 | MAC_10 | MAC_SYM_PAUSE;