netdev - 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:   Mon, 6 May 2019 01:00:49 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>, f.fainelli@...il.com,
        andrew@...n.ch, davem@...emloft.net
Cc:     netdev@...r.kernel.org
Subject: Decoupling phy_device from net_device (was "Re: [PATCH] net: dsa:
 fixed-link interface is reporting SPEED_UNKNOWN")

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.

Thanks,
-Vladimir