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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 05 May 2019 20:43:38 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>, andrew@...n.ch,
        davem@...emloft.net
CC:     netdev@...r.kernel.org
Subject: Re: Decoupling phy_device from net_device (was "Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN")



On May 5, 2019 3:00:49 PM PDT, Vladimir Oltean <olteanv@...il.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@...il.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