From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up Date: Wed, 11 Jul 2018 23:59:26 +0200 Message-ID: References: <0d031081-4a7f-ddde-87c0-2c1c6be543c3@gmail.com> <407ed2cd-db27-f179-8b98-0d1e61513e07@gmail.com> <20180711205518.GJ21430@lunn.ch> <7dfcb4d5-2a0c-9244-53e4-564014b16b58@gmail.com> <28ea392b-6d3a-0efe-a0ed-ebe82fe14099@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: David Miller , "netdev@vger.kernel.org" To: Florian Fainelli , Andrew Lunn Return-path: Received: from mail-wr1-f66.google.com ([209.85.221.66]:37165 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1733011AbeGKWF5 (ORCPT ); Wed, 11 Jul 2018 18:05:57 -0400 Received: by mail-wr1-f66.google.com with SMTP id q10-v6so19625392wrd.4 for ; Wed, 11 Jul 2018 14:59:34 -0700 (PDT) In-Reply-To: <28ea392b-6d3a-0efe-a0ed-ebe82fe14099@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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?