Aw: Re: [RFC PATCH net-next v3 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

[Frank Wunderlich]

Hi netdev,

we are still stale in adding pcs driver here...can you please show us the way to go??

daniel posted another attempt but this seems also stale.
https://patchwork.kernel.org/project/netdevbpf/patch/ba4e359584a6b3bc4b3470822c42186d5b0856f9.1721910728.git.daniel@xxxxxxxxxxxxxx/

@Angelo/Matthias, can you help solving this?

left full quote below....

regards Frank

> Gesendet: Mittwoch, 10. Juli 2024 um 13:17 Uhr
> Von: "Frank Wunderlich" <frank-w@xxxxxxxxxxxxxxx>
> An: "Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>
> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>, "Eric Dumazet" <edumazet@xxxxxxxxxx>, "Jakub Kicinski" <kuba@xxxxxxxxxx>, "Paolo Abeni" <pabeni@xxxxxxxxxx>, "Rob Herring" <robh+dt@xxxxxxxxxx>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@xxxxxxxxxx>, "Conor Dooley" <conor+dt@xxxxxxxxxx>, "Chunfeng Yun" <chunfeng.yun@xxxxxxxxxxxx>, "Vinod Koul" <vkoul@xxxxxxxxxx>, "Kishon Vijay Abraham I" <kishon@xxxxxxxxxx>, "Felix Fietkau" <nbd@xxxxxxxx>, "John Crispin" <john@xxxxxxxxxxx>, "Sean Wang" <sean.wang@xxxxxxxxxxxx>, "Mark Lee" <Mark-MC.Lee@xxxxxxxxxxxx>, "Lorenzo Bianconi" <lorenzo@xxxxxxxxxx>, "Matthias Brugger" <matthias.bgg@xxxxxxxxx>, "AngeloGioacchino Del Regno" <angelogioacchino.delregno@xxxxxxxxxxxxx>, "Andrew Lunn" <andrew@xxxxxxx>, "Heiner Kallweit" <hkallweit1@xxxxxxxxx>, "Alexander Couzens" <lynxis@xxxxxxx>, "Qingfang Deng" <dqfext@xxxxxxxxx>, "SkyLake Huang" <SkyLake.Huang@xxxxxxxxxxxx>, "Philipp Zabel" <p.zabel@xxxxxxxxxxxxxx>, netdev@xxxxxxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-arm-kernel@xxxxxxxxxxxxxxxxxxx, linux-mediatek@xxxxxxxxxxxxxxxxxxx, linux-phy@xxxxxxxxxxxxxxxxxxx, "Daniel Golle" <daniel@xxxxxxxxxxxxxx>
> Betreff: Re: [RFC PATCH net-next v3 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988
>
> Hi Russel / netdev
> 
> how can we proceed here? development for mt7988/Bpi-R4 is stalled here 
> because it is unclear what's the right way to go...
> 
> We want to avoid much work adding a complete new framework for a "small" 
> change (imho this affects all SFP-like sub-devices and their linking to 
> phylink/pcs driver)...if this really the way to go then some boundary 
> conditions will be helpful.
> 
> regards Frank
> 
> Am 22.04.24 um 18:24 schrieb Daniel Golle:
> > Hi Russell,
> > Hi netdev crew,
> >
> > I haven't received any reply for this email and am still waiting for
> > clarification regarding how inter-driver dependencies should be modelled
> > in that case of mtk_eth_soc. My search for good examples for that in the
> > kernel code was quite frustrating and all I've found are supposedly bugs
> > of that exact cathegory.
> >
> > Please see my questions mentioned in the previous email I've sent and
> > also a summary of suggested solutions inline below:
> >
> > On Wed, Feb 07, 2024 at 01:29:18AM +0000, Daniel Golle wrote:
> >> Hi Russell,
> >>
> >> sorry for the extended time it took me to get back to this patch and
> >> the comments you made for it. Understanding the complete scope of the
> >> problem took a while (plus there were holidays and other fun things),
> >> and also brought up further questions which I hope you can help me
> >> find good answers for, see below:
> >>
> >> On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
> >>> On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> >>>> Introduce a proper platform MFD driver for the LynxI (H)SGMII PCS which
> >>>> is going to initially be used for the MT7988 SoC.
> >>>>
> >>>> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> >>> I made some specific suggestions about what I wanted to see for
> >>> "getting" PCS in the previous review, and I'm disappointed that this
> >>> patch set is still inventing its own solution.
> >>>
> >>>> +struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
> >>>> +{
> >>>> +	struct platform_device *pdev;
> >>>> +	struct mtk_pcs_lynxi *mpcs;
> >>>> +
> >>>> +	if (!np)
> >>>> +		return NULL;
> >>>> +
> >>>> +	if (!of_device_is_available(np))
> >>>> +		return ERR_PTR(-ENODEV);
> >>>> +
> >>>> +	if (!of_match_node(mtk_pcs_lynxi_of_match, np))
> >>>> +		return ERR_PTR(-EINVAL);
> >>>> +
> >>>> +	pdev = of_find_device_by_node(np);
> >>>> +	if (!pdev || !platform_get_drvdata(pdev)) {
> >>> This is racy - as I thought I described before, userspace can unbind
> >>> the device in one thread, while another thread is calling this
> >>> function. With just the right timing, this check succeeds, but...
> >>>
> >>>> +		if (pdev)
> >>>> +			put_device(&pdev->dev);
> >>>> +		return ERR_PTR(-EPROBE_DEFER);
> >>>> +	}
> >>>> +
> >>>> +	mpcs = platform_get_drvdata(pdev);
> >>> mpcs ends up being read as NULL here. Even if you did manage to get a
> >>> valid pointer, "mpcs" being devm-alloced could be freed from under
> >>> you at this point...
> >>>
> >>>> +	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> >>> resulting in this accessing memory which has been freed.
> >>>
> >>> The solution would be either to suppress the bind/unbind attributes
> >>> (provided the underlying struct device can't go away, which probably
> >>> also means ensuring the same of the MDIO bus. Aternatively, adding
> >>> a lock around the remove path and around the checking of
> >>> platform_get_drvdata() down to adding the device link would probably
> >>> solve it.
> >>>
> >>> However, I come back to my general point - this kind of stuff is
> >>> hairy. Do we want N different implementations of it in various drivers
> >>> with subtle bugs, or do we want _one_ implemenatation.
> >>>
> >>> If we go with the one implemenation approach, then we need to think
> >>> about whether we should be using device links or not. The problem
> >>> could be for network interfaces where one struct device is
> >>> associated with multiple network interfaces. Using device links has
> >>> the unfortunate side effect that if the PCS for one of those network
> >>> interfaces is removed, _all_ network interfaces disappear.
> >> I agree, esp. on this MT7988 removal of a PCS which may then not
> >> even be in use also impairs connectivity on the built-in gigE DSA
> >> switch. It would be nice to try to avoid this.
> >>
> >>> My original suggestion was to hook into phylink to cause that to
> >>> take the link down when an in-use PCS gets removed.
> >> I took a deep dive into how this could be done correctly and how
> >> similar things are done for other drivers. Apart from the PCS there
> >> often also is a muxing-PHY involved, eg. MediaTek's XFI T-PHY in this
> >> case (previously often called "pextp" for no apparent reason) or
> >> Marvell's comphy (mvebu-comphy).
> >>
> >> So let's try something simple on an already well-supported platform,
> >> I thought and grabbed Marvell Armada CN9131-DB running vanilla Linux,
> >> and it didn't even take some something racy, but plain:
> >>
> >> ip link set eth6 up
> >> cd /sys/bus/platform/drivers/mvebu-comphy
> >> echo f2120000.phy > unbind
> >> echo f4120000.phy > unbind
> >> echo f6120000.phy > unbind
> >> ip link set eth6 down
> >>
> >>
> >> That was enough. The result is a kernel crash, and the same should
> >> apply on popular platforms like the SolidRun MACCHIATOBin and other
> >> similar boards.
> >>
> >> So this gets me to think that there is a wider problem around
> >> non-phylink-managed resources which may disappear while in use by
> >> network drivers, and I guess that the same applies in many other
> >> places. I don't have a SATA drive connected to that Marvell board, but
> >> I can imagine what happens when suddenly removing the comphy instance
> >> in charge of the SATA link and then a subsequent SATA hotplug event
> >> happens or stuff like that...
> >>
> >> Anyway, back to network subsystem and phylink:
> >>
> >> Do you agree that we need a way to register (and unregister) PCS
> >> providers with phylink, so we don't need *_get_by_of_node implementations
> >> in each driver? If so, can you sketch out what the basic requirements
> >> for an acceptable solution would be?
> >>
> >> Also, do you agree that lack of handling of disappearing resources,
> >> such as PHYs (*not* network PHYs, but PHYs as in drivers/phy/*) or
> >> syscons, is already a problem right now which should be addressed?
> >>
> >> If you imagine phylink to take care of the life-cycle of all link-
> >> resources, what is vision about those things other than classic MDIO-
> >> connected PHYs?
> >>
> >> And well, of course, the easy fix for the example above would be:
> >> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> >> index b0dd133665986..9517c96319595 100644
> >> --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> >> +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> >> @@ -1099,6 +1099,7 @@ static const struct of_device_id mvebu_comphy_of_match_table[] = {
> >>   MODULE_DEVICE_TABLE(of, mvebu_comphy_of_match_table);
> >>   
> >>   static struct platform_driver mvebu_comphy_driver = {
> >> +	.suppress_bind_attrs = true,
> >>   	.probe	= mvebu_comphy_probe,
> >>   	.driver	= {
> >>   		.name = "mvebu-comphy",
> >>
> >> But that should then apply to every single driver in drivers/phy/...
> >>
> > My remaining questions are:
> >   - Is it ok to just use .suppress_bind_attrs = true for PCS and PHY
> >     drivers to avoid having to care out hot-removal?
> >     Those components are anyway built-into the SoC so removing them
> >     is physically not possible.
> >
> >   - In case of driver removal (rmmod -f) imho using a device-link would
> >     be sufficient to prevent the worst. However, we would have to live
> >     with the all-or-nothing nature of that approach, ie. once e.g. the
> >     USXGMII driver is being removed, *all* Ethernet interfaces would
> >     disappear with it (even those not actually using USXGMII).
> >
> > If the solutions mentioned above do not sound agreeable, please suggest
> > how a good solution would look like, ideally also share an example of
> > any driver in the kernel where this is done in the way you would like
> > to have things done.
> >
> >>>> +
> >>>> +	return &mpcs->pcs;
> >>>> +}
> >>>> +EXPORT_SYMBOL(mtk_pcs_lynxi_get);
> >>>> +
> >>>> +void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
> >>>> +{
> >>>> +	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
> >>>> +
> >>>> +	if (!pcs)
> >>>> +		return;
> >>>> +
> >>>> +	mutex_lock(&instance_mutex);
> >>>> +	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
> >>>> +		if (pcs == &cur->pcs) {
> >>>> +			mpcs = cur;
> >>>> +			break;
> >>>> +		}
> >>>> +	mutex_unlock(&instance_mutex);
> >>> I don't see what this loop gains us, other than checking that the "pcs"
> >>> is still on the list and hasn't already been removed. If that is all
> >>> that this is about, then I would suggest:
> >>>
> >>> 	bool found = false;
> >>>
> >>> 	if (!pcs)
> >>> 		return;
> >>>
> >>> 	mpcs = pcs_to_mtk_pcs_lynxi(pcs);
> >>> 	mutex_lock(&instance_mutex);
> >>> 	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
> >>> 		if (cur == mpcs) {
> >>> 			found = true;
> >>> 			break;
> >>> 		}
> >>> 	mutex_unlock(&instance_mutex);
> >>>
> >>> 	if (WARN_ON(!found))
> >>> 		return;
> >>>
> >>> which makes it more obvious why this exists.
> >> The idea was not only to make sure it hasn't been removed, but also
> >> to be sure that what ever the private data pointer points to has
> >> actually been created by that very driver.
> >>
> >> But yes, doing it in the way you suggest will work in the same way,
> >> just when having my idea in mind it looks more fishy to use
> >> pcs_to_mtk_pcs_lynxi() before we are 100% sure that what we dealing
> >> with has previously been created by this driver.
> >>
> >>
> >> Cheers
> >>
> >>
> >> Daniel
> >>
>