Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 12.07.2018 21:53, Florian Fainelli wrote: > > > On 07/12/2018 12:25 PM, Florian Fainelli wrote: >> >> >> On 07/12/2018 12:10 PM, Heiner Kallweit wrote: >>> On 12.07.2018 21:09, Andrew Lunn wrote: > Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation > to finish. Therefore, even though I share Andrew's concerns, there seem > to be chips where it's safe to not wait for the renegotiation to finish > (e.g. because device is in PCI D3 already and can't generate an > interrupt). > Having said that I'd keep the sync parameter for phy_speed_down so that > the driver can decide. Hi Heiner Please put a big fat comment about the dangers of sync=false in the function header. We want people to known it is dangerous by default, and should only be used in special conditions, when it is known to be safe. Andrew >>> OK .. >> >> What part do you find dangerous? Magic Packets are UDP packets and they >> are not routed (unless specifically taken care of) so there is already >> some "lossy" behavior involved with waking-up an Ethernet MAC, I don't >> think that is too bad to retry several times until the link comes up. > > I see the concern with the comment from v2, and indeed you could get an > interrupt signaling the PHY auto-negotiated the link before or at the > time we are suspending causing potentially an early wake-up. Not that > this should be a problem though since there is usually a point of not > return past which you can't do early wake-up anyway. > I think we should leave the comment in for the moment so that people think twice about the described scenario. If we should find out that the issue can't be triggered on all platforms then we still can remove the comment.
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 07/12/2018 12:25 PM, Florian Fainelli wrote: > > > On 07/12/2018 12:10 PM, Heiner Kallweit wrote: >> On 12.07.2018 21:09, Andrew Lunn wrote: Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation to finish. Therefore, even though I share Andrew's concerns, there seem to be chips where it's safe to not wait for the renegotiation to finish (e.g. because device is in PCI D3 already and can't generate an interrupt). Having said that I'd keep the sync parameter for phy_speed_down so that the driver can decide. >>> >>> Hi Heiner >>> >>> Please put a big fat comment about the dangers of sync=false in the >>> function header. We want people to known it is dangerous by default, >>> and should only be used in special conditions, when it is known to be >>> safe. >>> Andrew >>> >> OK .. > > What part do you find dangerous? Magic Packets are UDP packets and they > are not routed (unless specifically taken care of) so there is already > some "lossy" behavior involved with waking-up an Ethernet MAC, I don't > think that is too bad to retry several times until the link comes up. I see the concern with the comment from v2, and indeed you could get an interrupt signaling the PHY auto-negotiated the link before or at the time we are suspending causing potentially an early wake-up. Not that this should be a problem though since there is usually a point of not return past which you can't do early wake-up anyway. -- Florian
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 07/12/2018 12:10 PM, Heiner Kallweit wrote: > On 12.07.2018 21:09, Andrew Lunn wrote: >>> Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation >>> to finish. Therefore, even though I share Andrew's concerns, there seem >>> to be chips where it's safe to not wait for the renegotiation to finish >>> (e.g. because device is in PCI D3 already and can't generate an interrupt). >>> Having said that I'd keep the sync parameter for phy_speed_down so that >>> the driver can decide. >> >> Hi Heiner >> >> Please put a big fat comment about the dangers of sync=false in the >> function header. We want people to known it is dangerous by default, >> and should only be used in special conditions, when it is known to be >> safe. >> Andrew >> > OK .. What part do you find dangerous? Magic Packets are UDP packets and they are not routed (unless specifically taken care of) so there is already some "lossy" behavior involved with waking-up an Ethernet MAC, I don't think that is too bad to retry several times until the link comes up. -- Florian
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 12.07.2018 21:09, Andrew Lunn wrote: >> Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation >> to finish. Therefore, even though I share Andrew's concerns, there seem >> to be chips where it's safe to not wait for the renegotiation to finish >> (e.g. because device is in PCI D3 already and can't generate an interrupt). >> Having said that I'd keep the sync parameter for phy_speed_down so that >> the driver can decide. > > Hi Heiner > > Please put a big fat comment about the dangers of sync=false in the > function header. We want people to known it is dangerous by default, > and should only be used in special conditions, when it is known to be > safe. > Andrew > OK .. Heiner
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
> Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation > to finish. Therefore, even though I share Andrew's concerns, there seem > to be chips where it's safe to not wait for the renegotiation to finish > (e.g. because device is in PCI D3 already and can't generate an interrupt). > Having said that I'd keep the sync parameter for phy_speed_down so that > the driver can decide. Hi Heiner Please put a big fat comment about the dangers of sync=false in the function header. We want people to known it is dangerous by default, and should only be used in special conditions, when it is known to be safe. Andrew
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 11.07.2018 23:59, Heiner Kallweit wrote: > On 11.07.2018 23:33, Florian Fainelli wrote: >> >> >> On 07/11/2018 02:08 PM, Heiner Kallweit wrote: >>> 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. >> >> What I had in mind is that you would be able to register a callback that >> would tell you when auto-negotiation completes, and not register one if >> you did not want to have that information. >> >> As Andrew points out though, with PHY using interrupts, this might be a >> bit challenging to do because you will get an interrupt about "something >> has changed" and you would have to run the callback from the PHY state >> machine to determine this was indeed a result of triggering >> auto-negotiation. Maybe polling for auto-negotiation like you do here is >> good enough. >> > OK, then I would poll for autoneg finished in phy_speed_down and > remove the polling option from phy_speed_up. I will do some tests > with this before submitting a v2. > Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation to finish. Therefore, even though I share Andrew's concerns, there seem to be chips where it's safe to not wait for the renegotiation to finish (e.g. because device is in PCI D3 already and can't generate an interrupt). Having said that I'd keep the sync parameter for phy_speed_down so that the driver can decide. For phy_speed_up I'll remove the sync parameter as discussed. >> One nit, you might have to check for those functions that the PHY did >> have auto-negotiation enabled and was not forced. >> > This I'm doing already, or do you mean something different? >
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 11.07.2018 23:33, Florian Fainelli wrote: > > > On 07/11/2018 02:08 PM, Heiner Kallweit wrote: >> 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. > > What I had in mind is that you would be able to register a callback that > would tell you when auto-negotiation completes, and not register one if > you did not want to have that information. > > As Andrew points out though, with PHY using interrupts, this might be a > bit challenging to do because you will get an interrupt about "something > has changed" and you would have to run the callback from the PHY state > machine to determine this was indeed a result of triggering > auto-negotiation. Maybe polling for auto-negotiation like you do here is > good enough. > OK, then I would poll for autoneg finished in phy_speed_down and remove the polling option from phy_speed_up. I will do some tests with this before submitting a v2. > One nit, you might have to check for those functions that the PHY did > have auto-negotiation enabled and was not forced. > This I'm doing already, or do you mean something different?
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
On 07/11/2018 02:08 PM, Heiner Kallweit wrote: > 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. What I had in mind is that you would be able to register a callback that would tell you when auto-negotiation completes, and not register one if you did not want to have that information. As Andrew points out though, with PHY using interrupts, this might be a bit challenging to do because you will get an interrupt about "something has changed" and you would have to run the callback from the PHY state machine to determine this was indeed a result of triggering auto-negotiation. Maybe polling for auto-negotiation like you do here is good enough. One nit, you might have to check for those functions that the PHY did have auto-negotiation enabled and was not forced. -- Florian
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
Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up
> +/** > + * 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. Andrew
