netdev - Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_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]

Message-ID: <7dfcb4d5-2a0c-9244-53e4-564014b16b58@gmail.com>
Date:   Wed, 11 Jul 2018 23:08:52 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and
 phy_speed_up

On 11.07.2018 22:55, Andrew Lunn wrote:
>> +/**
>> + * phy_speed_down - set speed to lowest speed supported by both link partners
>> + * @phydev: the phy_device struct
>> + * @sync: perform action synchronously
>> + *
>> + * Description: Typically used to save energy when waiting for a WoL packet
>> + */
>> +int phy_speed_down(struct phy_device *phydev, bool sync)
> 
> This sync parameter needs some more thought. I'm not sure it is safe.
> 
> How does a PHY trigger a WoL wake up? I guess some use the interrupt
> pin. How does a PHY indicate auto-neg has completed? It triggers an
> interrupt. So it seems like there is a danger here we suspend, and
> then wake up 2 seconds later when auto-neg has completed.
> 
> I'm not sure we can safely suspend until auto-neg has completed.
> 
>> +/**
>> + * phy_speed_up - (re)set advertised speeds to all supported speeds
>> + * @phydev: the phy_device struct
>> + * @sync: perform action synchronously
>> + *
>> + * Description: Used to revert the effect of phy_speed_down
>> + */
>> +int phy_speed_up(struct phy_device *phydev, bool sync)
> 
> And here, i'm thinking the opposite. A MAC driver needs to be ready
> for the PHY state to change at any time. So why do we need to wait?
> Just let the normal mechanisms inform the MAC when the link is up.
> 
I see your points, thanks for the feedback. In my case WoL triggers
a PCI PME and the code works as expected, but I agree this may be
different in other setups (external PHY).

The sync parameter was inspired by following comment from Florian:
"One thing that bothers me a bit is that this should ideally be
offered as both blocking and non-blocking options"
So let's see which comments he may have before preparing a v2.

>      Andrew
> 
Heiner