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

Commit Message

Daniel Golle Dec. 12, 2023, 3:47 a.m. UTC

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 <[e-mail]>
---
 drivers/net/pcs/pcs-mtk-lynxi.c   | 226 ++++++++++++++++++++++++++++--
 include/linux/pcs/pcs-mtk-lynxi.h |  11 ++
 2 files changed, 226 insertions(+), 11 deletions(-)

Comments

Russell King (Oracle) Dec. 13, 2023, 4:04 p.m. UTC | #1

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 <[e-mail]>

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.

My original suggestion was to hook into phylink to cause that to
take the link down when an in-use PCS gets removed.

> +
> +	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.

Daniel Golle Feb. 7, 2024, 1:29 a.m. UTC | #2

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 <[e-mail]>
> 
> 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/...


> 
> > +
> > +	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

Daniel Golle April 22, 2024, 4:23 p.m. UTC | #3

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 <[e-mail]>
> > 
> > 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
>

Frank Wunderlich July 10, 2024, 11:17 a.m. UTC | #4

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 <[e-mail]>
>>> 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
>>

Frank Wunderlich Oct. 4, 2024, 2:17 p.m. UTC | #5

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/[e-mail]/

@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" <[e-mail]>
> An: "Russell King (Oracle)" <[e-mail]>
> Cc: "David S. Miller" <[e-mail]>, "Eric Dumazet" <[e-mail]>, "Jakub Kicinski" <[e-mail]>, "Paolo Abeni" <[e-mail]>, "Rob Herring" <[e-mail]>, "Krzysztof Kozlowski" <[e-mail]>, "Conor Dooley" <[e-mail]>, "Chunfeng Yun" <[e-mail]>, "Vinod Koul" <[e-mail]>, "Kishon Vijay Abraham I" <[e-mail]>, "Felix Fietkau" <[e-mail]>, "John Crispin" <[e-mail]>, "Sean Wang" <[e-mail]>, "Mark Lee" <[e-mail]>, "Lorenzo Bianconi" <[e-mail]>, "Matthias Brugger" <[e-mail]>, "AngeloGioacchino Del Regno" <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Heiner Kallweit" <[e-mail]>, "Alexander Couzens" <[e-mail]>, "Qingfang Deng" <[e-mail]>, "SkyLake Huang" <[e-mail]>, "Philipp Zabel" <[e-mail]>, [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], "Daniel Golle" <[e-mail]>
> 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 <[e-mail]>
> >>> 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
> >>
>

Russell King (Oracle) Oct. 4, 2024, 2:35 p.m. UTC | #6

Hi Frank,

Sorry, but I've not been able to look at this, and I've completely lost
all context now. I was diverted onto a high priority work issue for a
while (was it from April to end of June) so didn't have much time
available for mainline work. I then had a much needed holiday (three
weeks) in July. I then had a clear week where I did look at mainline.
Since then, I've had two cataract operations that have made being on the
computer somewhat difficult, and it is only recently that I'm
effectively "back" after what is approximately six months of not having
a lot of bandwidth. I've seen the cataract consultant this morning, and
just found out that my optometrist appointment for Tuesday is too soon
after the cataract operation, and needs to be moved two weeks. The
optometrist doesn't have availability then, so it's going to be another
four weeks. FFS... I wish I'd known, then I could've made an
arrangement with the optometrist months ago for the correct date.

Now, XPCS has introduced a hack in a similar way to what you're trying
to do, but I wasn't able to review it, so it went in. We're heading
towards the situation where every PCS driver is going to have its own
way to look up a PCS registered as a device. This is not going to scale.

We need something better than this - and at the moment that's all I can
say because I haven't given it any more thought beyond that so far.

On Fri, Oct 04, 2024 at 04:17:37PM +0200, Frank Wunderlich wrote:
> 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/[e-mail]/
> 
> @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" <[e-mail]>
> > An: "Russell King (Oracle)" <[e-mail]>
> > Cc: "David S. Miller" <[e-mail]>, "Eric Dumazet" <[e-mail]>, "Jakub Kicinski" <[e-mail]>, "Paolo Abeni" <[e-mail]>, "Rob Herring" <[e-mail]>, "Krzysztof Kozlowski" <[e-mail]>, "Conor Dooley" <[e-mail]>, "Chunfeng Yun" <[e-mail]>, "Vinod Koul" <[e-mail]>, "Kishon Vijay Abraham I" <[e-mail]>, "Felix Fietkau" <[e-mail]>, "John Crispin" <[e-mail]>, "Sean Wang" <[e-mail]>, "Mark Lee" <[e-mail]>, "Lorenzo Bianconi" <[e-mail]>, "Matthias Brugger" <[e-mail]>, "AngeloGioacchino Del Regno" <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Heiner Kallweit" <[e-mail]>, "Alexander Couzens" <[e-mail]>, "Qingfang Deng" <[e-mail]>, "SkyLake Huang" <[e-mail]>, "Philipp Zabel" <[e-mail]>, [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], "Daniel Golle" <[e-mail]>
> > 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 <[e-mail]>
> > >>> 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
> > >>
> >
>

Patch

diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
index 8501dd365279b..e06dd7db66144 100644
--- a/drivers/net/pcs/pcs-mtk-lynxi.c
+++ b/drivers/net/pcs/pcs-mtk-lynxi.c
@@ -1,6 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2018-2019 MediaTek Inc.
-/* A library for MediaTek SGMII circuit
+/* A library and platform driver for the MediaTek LynxI SGMII circuit
  *
  * Author: Sean Wang <[e-mail]>
  * Author: Alexander Couzens <[e-mail]>
@@ -8,11 +8,17 @@ 
  *
  */
 
+#include <linux/clk.h>
 #include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/pcs/pcs-mtk-lynxi.h>
 #include <linux/phylink.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 
 /* SGMII subsystem config registers */
 /* BMCR (low 16) BMSR (high 16) */
@@ -65,6 +71,8 @@ 
 #define SGMII_PN_SWAP_MASK		GENMASK(1, 0)
 #define SGMII_PN_SWAP_TX_RX		(BIT(0) | BIT(1))
 
+#define MTK_NETSYS_V3_AMA_RGC3		0x128
+
 /* struct mtk_pcs_lynxi -  This structure holds each sgmii regmap andassociated
  *                         data
  * @regmap:                The register map pointing at the range used to setup
@@ -74,15 +82,29 @@ 
  * @interface:             Currently configured interface mode
  * @pcs:                   Phylink PCS structure
  * @flags:                 Flags indicating hardware properties
+ * @rstc:                  Reset controller
+ * @sgmii_sel:             SGMII Register Clock
+ * @sgmii_rx:              SGMII RX Clock
+ * @sgmii_tx:              SGMII TX Clock
+ * @node:                  List node
  */
 struct mtk_pcs_lynxi {
 	struct regmap		*regmap;
+	struct device		*dev;
 	u32			ana_rgc3;
 	phy_interface_t		interface;
 	struct			phylink_pcs pcs;
 	u32			flags;
+	struct reset_control	*rstc;
+	struct clk		*sgmii_sel;
+	struct clk		*sgmii_rx;
+	struct clk		*sgmii_tx;
+	struct list_head	node;
 };
 
+static LIST_HEAD(mtk_pcs_lynxi_instances);
+static DEFINE_MUTEX(instance_mutex);
+
 static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs)
 {
 	return container_of(pcs, struct mtk_pcs_lynxi, pcs);
@@ -102,6 +124,17 @@  static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs,
 					 FIELD_GET(SGMII_LPA, adv));
 }
 
+static void mtk_sgmii_reset(struct mtk_pcs_lynxi *mpcs)
+{
+	if (!mpcs->rstc)
+		return;
+
+	reset_control_assert(mpcs->rstc);
+	udelay(100);
+	reset_control_deassert(mpcs->rstc);
+	mdelay(1);
+}
+
 static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				phy_interface_t interface,
 				const unsigned long *advertising,
@@ -147,6 +180,7 @@  static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				SGMII_PHYA_PWD);
 
 		/* Reset SGMII PCS state */
+		mtk_sgmii_reset(mpcs);
 		regmap_set_bits(mpcs->regmap, SGMSYS_RESERVED_0,
 				SGMII_SW_RESET);
 
@@ -233,10 +267,29 @@  static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs,
 	}
 }
 
+static int mtk_pcs_lynxi_enable(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_prepare_enable(mpcs->sgmii_rx);
+		clk_prepare_enable(mpcs->sgmii_tx);
+	}
+
+	return 0;
+}
+
 static void mtk_pcs_lynxi_disable(struct phylink_pcs *pcs)
 {
 	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
 
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_disable_unprepare(mpcs->sgmii_tx);
+		clk_disable_unprepare(mpcs->sgmii_rx);
+	}
+
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 }
 
@@ -246,11 +299,12 @@  static const struct phylink_pcs_ops mtk_pcs_lynxi_ops = {
 	.pcs_an_restart = mtk_pcs_lynxi_restart_an,
 	.pcs_link_up = mtk_pcs_lynxi_link_up,
 	.pcs_disable = mtk_pcs_lynxi_disable,
+	.pcs_enable = mtk_pcs_lynxi_enable,
 };
 
-struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
-					 struct regmap *regmap, u32 ana_rgc3,
-					 u32 flags)
+static struct phylink_pcs *mtk_pcs_lynxi_init(struct device *dev, struct regmap *regmap,
+					      u32 ana_rgc3, u32 flags,
+					      struct mtk_pcs_lynxi *prealloc)
 {
 	struct mtk_pcs_lynxi *mpcs;
 	u32 id, ver;
@@ -258,29 +312,33 @@  struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 
 	ret = regmap_read(regmap, SGMSYS_PCS_DEVICE_ID, &id);
 	if (ret < 0)
-		return NULL;
+		return ERR_PTR(ret);
 
 	if (id != SGMII_LYNXI_DEV_ID) {
 		dev_err(dev, "unknown PCS device id %08x\n", id);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	ret = regmap_read(regmap, SGMSYS_PCS_SCRATCH, &ver);
 	if (ret < 0)
-		return NULL;
+		return ERR_PTR(ret);
 
 	ver = FIELD_GET(SGMII_DEV_VERSION, ver);
 	if (ver != 0x1) {
 		dev_err(dev, "unknown PCS device version %04x\n", ver);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	dev_dbg(dev, "MediaTek LynxI SGMII PCS (id 0x%08x, ver 0x%04x)\n", id,
 		ver);
 
-	mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
-	if (!mpcs)
-		return NULL;
+	if (prealloc) {
+		mpcs = prealloc;
+	} else {
+		mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
+		if (!mpcs)
+			return ERR_PTR(-ENOMEM);
+	};
 
 	mpcs->ana_rgc3 = ana_rgc3;
 	mpcs->regmap = regmap;
@@ -291,6 +349,13 @@  struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 
 	return &mpcs->pcs;
+};
+
+struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
+					 struct regmap *regmap, u32 ana_rgc3,
+					 u32 flags)
+{
+	return mtk_pcs_lynxi_init(dev, regmap, ana_rgc3, flags, NULL);
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_create);
 
@@ -303,4 +368,143 @@  void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs)
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_destroy);
 
+static int mtk_pcs_lynxi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mtk_pcs_lynxi *mpcs;
+	struct phylink_pcs *pcs;
+	struct regmap *regmap;
+	u32 flags = 0;
+
+	mpcs = devm_kzalloc(dev, sizeof(*mpcs), GFP_KERNEL);
+	if (!mpcs)
+		return -ENOMEM;
+
+	mpcs->dev = dev;
+	regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	if (of_property_read_bool(np->parent, "mediatek,pnswap"))
+		flags |= MTK_SGMII_FLAG_PN_SWAP;
+
+	mpcs->rstc = of_reset_control_get_shared(np->parent, NULL);
+	if (IS_ERR(mpcs->rstc))
+		return PTR_ERR(mpcs->rstc);
+
+	reset_control_deassert(mpcs->rstc);
+	mpcs->sgmii_sel = devm_clk_get_enabled(dev, "sgmii_sel");
+	if (IS_ERR(mpcs->sgmii_sel))
+		return PTR_ERR(mpcs->sgmii_sel);
+
+	mpcs->sgmii_rx = devm_clk_get(dev, "sgmii_rx");
+	if (IS_ERR(mpcs->sgmii_rx))
+		return PTR_ERR(mpcs->sgmii_rx);
+
+	mpcs->sgmii_tx = devm_clk_get(dev, "sgmii_tx");
+	if (IS_ERR(mpcs->sgmii_tx))
+		return PTR_ERR(mpcs->sgmii_tx);
+
+	pcs = mtk_pcs_lynxi_init(dev, regmap, (uintptr_t)of_device_get_match_data(dev),
+				 flags, mpcs);
+	if (IS_ERR(pcs))
+		return PTR_ERR(pcs);
+
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	platform_set_drvdata(pdev, mpcs);
+
+	mutex_lock(&instance_mutex);
+	list_add_tail(&mpcs->node, &mtk_pcs_lynxi_instances);
+	mutex_unlock(&instance_mutex);
+
+	return 0;
+}
+
+static int mtk_pcs_lynxi_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_pcs_lynxi *cur, *tmp;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry_safe(cur, tmp, &mtk_pcs_lynxi_instances, node)
+		if (cur->dev == dev) {
+			list_del(&cur->node);
+			kfree(cur);
+			break;
+		}
+	mutex_unlock(&instance_mutex);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_pcs_lynxi_of_match[] = {
+	{ .compatible = "mediatek,mt7988-sgmii", .data = (void *)MTK_NETSYS_V3_AMA_RGC3 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_pcs_lynxi_of_match);
+
+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)) {
+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+
+	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);
+
+	if (WARN_ON(!mpcs))
+		return;
+
+	put_device(mpcs->dev);
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_put);
+
+static struct platform_driver mtk_pcs_lynxi_driver = {
+	.driver = {
+		.name = "mtk-pcs-lynxi",
+		.of_match_table = mtk_pcs_lynxi_of_match,
+	},
+	.probe = mtk_pcs_lynxi_probe,
+	.remove = mtk_pcs_lynxi_remove,
+};
+module_platform_driver(mtk_pcs_lynxi_driver);
+
+MODULE_AUTHOR("Daniel Golle <[e-mail]>");
+MODULE_DESCRIPTION("MediaTek LynxI HSGMII PCS");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs/pcs-mtk-lynxi.h b/include/linux/pcs/pcs-mtk-lynxi.h
index be3b4ab32f4a7..2d44e951229c7 100644
--- a/include/linux/pcs/pcs-mtk-lynxi.h
+++ b/include/linux/pcs/pcs-mtk-lynxi.h
@@ -10,4 +10,15 @@  struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 					 struct regmap *regmap,
 					 u32 ana_rgc3, u32 flags);
 void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs);
+
+#if IS_ENABLED(CONFIG_PCS_MTK_LYNXI)
+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np);
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs);
+#else
+static inline struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	return NULL;
+}
+static inline void mtk_pcs_lynxi_put(struct phylink_pcs *pcs) { }
+#endif /* IS_ENABLED(CONFIG_PCS_MTK_LYNXI) */
 #endif