[PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN

[PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN

[Vladimir Oltean]

With Heiner's recent patch "b6163f194c69 net: phy: improve
genphy_read_status", the phydev->speed is now initialized by default to
SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad, since it
is not correct to call genphy_config_init() and genphy_read_status() for
a fixed PHY.

This dates back all the way to "39b0c705195e net: dsa: Allow
configuration of CPU & DSA port speeds/duplex" (discussion thread:
https://www.spinics.net/lists/netdev/msg340862.html).

I don't seem to understand why these calls were necessary back then, but
removing these calls seemingly has no impact now apart from preventing
the phydev->speed that was set in of_phy_register_fixed_link() from
getting overwritten.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/port.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 87769cb38c31..481412c892a7 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -485,9 +485,6 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
 		mode = PHY_INTERFACE_MODE_NA;
 	phydev->interface = mode;
 
-	genphy_config_init(phydev);
-	genphy_read_status(phydev);
-
 	if (ds->ops->adjust_link)
 		ds->ops->adjust_link(ds, port, phydev);
 
-- 
2.17.1

Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN

[Heiner Kallweit]

On 12.04.2019 01:01, Vladimir Oltean wrote:
> With Heiner's recent patch "b6163f194c69 net: phy: improve
> genphy_read_status", the phydev->speed is now initialized by default to
> SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad, since it
> is not correct to call genphy_config_init() and genphy_read_status() for
> a fixed PHY.
> 
What do you mean with "it is not correct"? Whether the calls are always
needed may be a valid question, but it's not forbidden to use these calls
with a fixed PHY. Actually in phylib polling mode genphy_read_status is
called every second also for a fixed PHY. swphy emulates all relevant
PHY registers.

> This dates back all the way to "39b0c705195e net: dsa: Allow
> configuration of CPU & DSA port speeds/duplex" (discussion thread:
> https://www.spinics.net/lists/netdev/msg340862.html).
> 
> I don't seem to understand why these calls were necessary back then, but
> removing these calls seemingly has no impact now apart from preventing
> the phydev->speed that was set in of_phy_register_fixed_link() from
> getting overwritten.
> 
I tend to agree with the change itself, but not with the justification.
For genphy_config_init I'd rather say:
genphy_config_init removes invalid modes based on the abilities read
from the chip. But as we emulate all registers anyway, this doesn't
provide any benefit for a swphy.

> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/port.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 87769cb38c31..481412c892a7 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -485,9 +485,6 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  		mode = PHY_INTERFACE_MODE_NA;
>  	phydev->interface = mode;
>  
> -	genphy_config_init(phydev);
> -	genphy_read_status(phydev);
> -
>  	if (ds->ops->adjust_link)
>  		ds->ops->adjust_link(ds, port, phydev);
>  
> 

Decoupling phy_device from net_device (was "Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN")

[Vladimir Oltean]

On 4/12/19 8:57 PM, Heiner Kallweit wrote:
> On 12.04.2019 01:01, Vladimir Oltean wrote:
>> With Heiner's recent patch "b6163f194c69 net: phy: improve
>> genphy_read_status", the phydev->speed is now initialized by default to
>> SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad, since it
>> is not correct to call genphy_config_init() and genphy_read_status() for
>> a fixed PHY.
>>
> What do you mean with "it is not correct"? Whether the calls are always
> needed may be a valid question, but it's not forbidden to use these calls
> with a fixed PHY. Actually in phylib polling mode genphy_read_status is
> called every second also for a fixed PHY. swphy emulates all relevant
> PHY registers.
> 
>> This dates back all the way to "39b0c705195e net: dsa: Allow
>> configuration of CPU & DSA port speeds/duplex" (discussion thread:
>> https://www.spinics.net/lists/netdev/msg340862.html).
>>
>> I don't seem to understand why these calls were necessary back then, but
>> removing these calls seemingly has no impact now apart from preventing
>> the phydev->speed that was set in of_phy_register_fixed_link() from
>> getting overwritten.
>>
> I tend to agree with the change itself, but not with the justification.
> For genphy_config_init I'd rather say:
> genphy_config_init removes invalid modes based on the abilities read
> from the chip. But as we emulate all registers anyway, this doesn't
> provide any benefit for a swphy.
> 
>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
>>   net/dsa/port.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/net/dsa/port.c b/net/dsa/port.c
>> index 87769cb38c31..481412c892a7 100644
>> --- a/net/dsa/port.c
>> +++ b/net/dsa/port.c
>> @@ -485,9 +485,6 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>>   		mode = PHY_INTERFACE_MODE_NA;
>>   	phydev->interface = mode;
>>   
>> -	genphy_config_init(phydev);
>> -	genphy_read_status(phydev);
>> -
>>   	if (ds->ops->adjust_link)
>>   		ds->ops->adjust_link(ds, port, phydev);
>>   
>>
> 

Hi,

I'd like to resend this, but I'm actually thinking of a nicer way to 
deal with the whole situation.
Would people be interested in making phylib/phylink decoupled from the 
phydev->attached_dev? The PHY state machine could be made to simply call 
a notifier block (similar to how switchdev works) registered by 
interested parties (MAC driver).
To keep API compatibility (phylink_create), this notifier block could be 
put inside the net_device structure and the PHY state machine would call 
it. But a new phylink_create_raw could be added, which passes only the 
notifier block with no net_device. The CPU port and DSA ports would be 
immediate and obvious users of this (since they don't register net 
devices), but there are some other out-of-tree Ethernet drivers out 
there that have strange workarounds (create a net device that they don't 
register) to have the PHY driver do its work.
Wondering what's your opinion on this and if it would be worth exploring.

Thanks,
-Vladimir

Re: Decoupling phy_device from net_device (was "Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN")

[Florian Fainelli]

On May 5, 2019 3:00:49 PM PDT, Vladimir Oltean <olteanv@gmail.com> wrote:
>On 4/12/19 8:57 PM, Heiner Kallweit wrote:
>> On 12.04.2019 01:01, Vladimir Oltean wrote:
>>> With Heiner's recent patch "b6163f194c69 net: phy: improve
>>> genphy_read_status", the phydev->speed is now initialized by default
>to
>>> SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad,
>since it
>>> is not correct to call genphy_config_init() and genphy_read_status()
>for
>>> a fixed PHY.
>>>
>> What do you mean with "it is not correct"? Whether the calls are
>always
>> needed may be a valid question, but it's not forbidden to use these
>calls
>> with a fixed PHY. Actually in phylib polling mode genphy_read_status
>is
>> called every second also for a fixed PHY. swphy emulates all relevant
>> PHY registers.
>> 
>>> This dates back all the way to "39b0c705195e net: dsa: Allow
>>> configuration of CPU & DSA port speeds/duplex" (discussion thread:
>>> https://www.spinics.net/lists/netdev/msg340862.html).
>>>
>>> I don't seem to understand why these calls were necessary back then,
>but
>>> removing these calls seemingly has no impact now apart from
>preventing
>>> the phydev->speed that was set in of_phy_register_fixed_link() from
>>> getting overwritten.
>>>
>> I tend to agree with the change itself, but not with the
>justification.
>> For genphy_config_init I'd rather say:
>> genphy_config_init removes invalid modes based on the abilities read
>> from the chip. But as we emulate all registers anyway, this doesn't
>> provide any benefit for a swphy.
>> 
>>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>>> ---
>>>   net/dsa/port.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/net/dsa/port.c b/net/dsa/port.c
>>> index 87769cb38c31..481412c892a7 100644
>>> --- a/net/dsa/port.c
>>> +++ b/net/dsa/port.c
>>> @@ -485,9 +485,6 @@ static int
>dsa_port_fixed_link_register_of(struct dsa_port *dp)
>>>   		mode = PHY_INTERFACE_MODE_NA;
>>>   	phydev->interface = mode;
>>>   
>>> -	genphy_config_init(phydev);
>>> -	genphy_read_status(phydev);
>>> -
>>>   	if (ds->ops->adjust_link)
>>>   		ds->ops->adjust_link(ds, port, phydev);
>>>   
>>>
>> 
>
>Hi,
>
>I'd like to resend this, but I'm actually thinking of a nicer way to 
>deal with the whole situation.
>Would people be interested in making phylib/phylink decoupled from the 
>phydev->attached_dev? The PHY state machine could be made to simply
>call 
>a notifier block (similar to how switchdev works) registered by 
>interested parties (MAC driver).
>To keep API compatibility (phylink_create), this notifier block could
>be 
>put inside the net_device structure and the PHY state machine would
>call 
>it. But a new phylink_create_raw could be added, which passes only the 
>notifier block with no net_device. The CPU port and DSA ports would be 
>immediate and obvious users of this (since they don't register net 
>devices), but there are some other out-of-tree Ethernet drivers out 
>there that have strange workarounds (create a net device that they
>don't 
>register) to have the PHY driver do its work.
>Wondering what's your opinion on this and if it would be worth
>exploring.

If you have patches for that idea, I would be glad to take a look. The current way of supporting DSA and CPU ports in DSA is now starting to show its limitations and we are not able to properly make use of PHYLINK at all for those ports. For PHYLIB (outside of PHYLINK), I would not want the decoupling to lead to e.g.: supporting Ethernet switches with only a single net_device instance and managing all the ports using the PHYLIB notifier, because that would bypass the model that DSA offers so we could miss opportunities to fix it, if needed.
-- 
Florian

Re: Decoupling phy_device from net_device (was "Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN")

[Andrew Lunn]

On Mon, May 06, 2019 at 01:00:49AM +0300, Vladimir Oltean wrote:
> On 4/12/19 8:57 PM, Heiner Kallweit wrote:
> >On 12.04.2019 01:01, Vladimir Oltean wrote:
> >>With Heiner's recent patch "b6163f194c69 net: phy: improve
> >>genphy_read_status", the phydev->speed is now initialized by default to
> >>SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad, since it
> >>is not correct to call genphy_config_init() and genphy_read_status() for
> >>a fixed PHY.
> >>
> >What do you mean with "it is not correct"? Whether the calls are always
> >needed may be a valid question, but it's not forbidden to use these calls
> >with a fixed PHY. Actually in phylib polling mode genphy_read_status is
> >called every second also for a fixed PHY. swphy emulates all relevant
> >PHY registers.
> >
> >>This dates back all the way to "39b0c705195e net: dsa: Allow
> >>configuration of CPU & DSA port speeds/duplex" (discussion thread:
> >>https://www.spinics.net/lists/netdev/msg340862.html).
> >>
> >>I don't seem to understand why these calls were necessary back then, but
> >>removing these calls seemingly has no impact now apart from preventing
> >>the phydev->speed that was set in of_phy_register_fixed_link() from
> >>getting overwritten.

As Florian said, if you have patches, please post them and we will
consider them.

But i think we also need to take a step back and consider the big
picture. There has been a lot of work recently to support multi-G
PHYs. It is clear we soon need to make changes to fixed-link. It only
supports up to 1G. But we have use cases where we need multi-G fixed
links.

We could also consider making the tie to the MAC much stronger. We
have been encouraging MAC driver writers to make use of the
ndev->phylib pointer. We could even enforce that, and use
container_of() to determine the MAC associated to a PHY.

	Andrew