Message-ID: <ZqfwogNXxmAL1WB2@makrotopia.org>
Date: Mon, 29 Jul 2024 20:42:26 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	John Crispin <john@...ozen.org>, Felix Fietkau <nbd@....name>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Sean Wang <sean.wang@...iatek.com>,
	Mark Lee <Mark-MC.Lee@...iatek.com>,
	Bc-bocun Chen <bc-bocun.chen@...iatek.com>,
	Sam Shih <Sam.Shih@...iatek.com>,
	Weijie Gao <Weijie.Gao@...iatek.com>,
	Steven Liu <steven.liu@...iatek.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH RFC net-next] net: pcs: add helper module for standalone
 drivers

Hi Russell,

thank you for commenting on this patch. Please help me understand which
direction I should work towards to see support for the MT7988 SoC
Ethernet in future kernels, see my questions below.

On Mon, Jul 29, 2024 at 12:28:15PM +0100, Russell King (Oracle) wrote:
> On Thu, Jul 25, 2024 at 01:44:49PM +0100, Daniel Golle wrote:
> > +static void devm_pcs_provider_release(struct device *dev, void *res)
> > +{
> > +	struct pcs_standalone *pcssa = (struct pcs_standalone *)res;
> > +
> > +	mutex_lock(&pcs_mutex);
> > +	list_del(&pcssa->list);
> > +	mutex_unlock(&pcs_mutex);
> 
> This needs to do notify phylink if the PCS has gone away, but the
> locking for this would be somewhat difficult (because pcs->phylink
> could change if the PCS changes.) That would need to be solved
> somehow.

>From my understanding the only way the PCS would "go away" is by
rmmod, which is prevented by the usage counter if still in use by the
Ethernet driver (removal of instances by using unbind in sysfs is
prevented by .suppress_bind_attrs).

I understand that Ethernet MAC and PCS both being built-into the SoC may
not be the only case we may want to support in the long run, but it is
the case for the MT7988 SoC which I'd like to see supported in upstream
Linux.

So imho this is something quite hypothetical which can be prevented by
setting .suppress_bind_attrs and bumping the module usage counter, as
those are not really dedicated devices on some kind of hotplug-able bus
what-so-ever, but all just components built-into the SoC itself. They
won't just go away. At least in case of the SoC I'm looking at.

If you have other use-cases in mind which this infrastructure should be
suitable for, it'd be helpful if you would spell them out.

If your criticism was meant to be directed towards the whole idea of
using standlone drivers for the PCS units of the SoC then the easiest
would of course be to just not do that and instead keep handling the
PCS as part of the Ethernet driver.

The main reason why I like the idea of the PCS driver being separate is
because it is not even needed on older platforms, and those are quite
resource constraint so it would be a waste to carry all the USXGMII
logic, let's say, on devices with MT7621 or even MT7628.

However, there are of course other ways to achieve nearly the same, such
as Kconfig symbols which select parts of the driver to be included or
not.

Hence my question: Do you think it is worth going down this road and
introducing standalone PCS drivers, given that the infrastructure
requirements include graceful removal of any PCS instance?

Also note that the same situation (things which may "go away") applies
to PHYs (as in: drivers/phy, not drivers/net/phy) as well, and I don't
see this being addressed for any of the in-SoC Ethernet controllers
supported by the kernel.

I was hoping for clarification regarding this but never received a
reply, see https://lkml.org/lkml/2024/2/7/101

And what about used instances of drivers/pinctrl, drivers/reset,
drivers/clk, ...? Should a SoC Ethernet driver be built in a way that
all those may gracefully "go away" as well?

I'm totally up to work on improving the overall situation there, but
it'd be good to know which direction I should be aiming for.
(as in: pre-removal call-back functions? just setting
.suppress_bind_attrs for all drivers/phy/ and such by default? extending
phylink itself to handle drivers/phy instances and their disappearance,
as well as potentially more than one PCS instance per net_device? ...)

> > [...]
> > +	device_link_add(dev, pcssa->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> 
> This is really not a nice solution when one has a network device that
> has multiple interfaces. This will cause all interfaces on that device
> to be purged from the system when a PCS for one of the interfaces
> goes away. If the system is using NFS-root, that could result in the
> rootfs being lost. We should handle this more gracefully.

"DL_FLAG_AUTOREMOVE_CONSUMER causes the device link to be automatically
purged when the consumer fails to probe or later unbinds."
(from Documentation/driver-api/device_link.rst)

The consumer is the Ethernet driver in this case. Hence the automatic
purge is only applied in case the Ethernet device goes away, and meanwhile
it would prevent the PCS driver from being rmmod'ed (which in my case is
the only way for the PCS to "disappear").

Also note that the same flag is used in pcs-rzn1-miic.c as well.


Thank you for your patiente and helping me to understand how to proceed.


Cheers


Daniel

Powered by blists - more mailing lists