* [RFC] net: phy: add shutdown hook to struct phy_driver @ 2020-09-30 9:47 Jisheng Zhang 2020-09-30 19:09 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Jisheng Zhang @ 2020-09-30 9:47 UTC (permalink / raw) To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller Cc: netdev, linux-kernel Hi, A GE phy supports pad isolation which can save power in WOL mode. But once the isolation is enabled, the MAC can't send/receive pkts to/from the phy because the phy is "isolated". To make the PHY work normally, I need to move the enabling isolation to suspend hook, so far so good. But the isolation isn't enabled in system shutdown case, to support this, I want to add shutdown hook to net phy_driver, then also enable the isolation in the shutdown hook. Is there any elegant solution? Or we can break the assumption: ethernet can still send/receive pkts after enabling WoL, no? Thanks in advance, Jisheng ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-09-30 9:47 [RFC] net: phy: add shutdown hook to struct phy_driver Jisheng Zhang @ 2020-09-30 19:09 ` Andrew Lunn 2020-09-30 20:07 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2020-09-30 19:09 UTC (permalink / raw) To: Jisheng Zhang Cc: Heiner Kallweit, Russell King, David S. Miller, netdev, linux-kernel On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: > Hi, > > A GE phy supports pad isolation which can save power in WOL mode. But once the > isolation is enabled, the MAC can't send/receive pkts to/from the phy because > the phy is "isolated". To make the PHY work normally, I need to move the > enabling isolation to suspend hook, so far so good. But the isolation isn't > enabled in system shutdown case, to support this, I want to add shutdown hook > to net phy_driver, then also enable the isolation in the shutdown hook. Is > there any elegant solution? > Or we can break the assumption: ethernet can still send/receive pkts after > enabling WoL, no? That is not an easy assumption to break. The MAC might be doing WOL, so it needs to be able to receive packets. What you might be able to assume is, if this PHY device has had WOL enabled, it can assume the MAC does not need to send/receive after suspend. The problem is, phy_suspend() will not call into the driver is WOL is enabled, so you have no idea when you can isolate the MAC from the PHY. So adding a shutdown in mdio_driver_register() seems reasonable. But you need to watch out for ordering. Is the MDIO bus driver still running? Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-09-30 19:09 ` Andrew Lunn @ 2020-09-30 20:07 ` Florian Fainelli 2020-09-30 20:11 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2020-09-30 20:07 UTC (permalink / raw) To: Andrew Lunn, Jisheng Zhang Cc: Heiner Kallweit, Russell King, David S. Miller, netdev, linux-kernel On 9/30/2020 12:09 PM, Andrew Lunn wrote: > On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: >> Hi, >> >> A GE phy supports pad isolation which can save power in WOL mode. But once the >> isolation is enabled, the MAC can't send/receive pkts to/from the phy because >> the phy is "isolated". To make the PHY work normally, I need to move the >> enabling isolation to suspend hook, so far so good. But the isolation isn't >> enabled in system shutdown case, to support this, I want to add shutdown hook >> to net phy_driver, then also enable the isolation in the shutdown hook. Is >> there any elegant solution? > >> Or we can break the assumption: ethernet can still send/receive pkts after >> enabling WoL, no? > > That is not an easy assumption to break. The MAC might be doing WOL, > so it needs to be able to receive packets. > > What you might be able to assume is, if this PHY device has had WOL > enabled, it can assume the MAC does not need to send/receive after > suspend. The problem is, phy_suspend() will not call into the driver > is WOL is enabled, so you have no idea when you can isolate the MAC > from the PHY. > > So adding a shutdown in mdio_driver_register() seems reasonable. But > you need to watch out for ordering. Is the MDIO bus driver still > running? If your Ethernet MAC controller implements a shutdown callback and that callback takes care of unregistering the network device which should also ensure that phy_disconnect() gets called, then your PHY's suspend function will be called. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-09-30 20:07 ` Florian Fainelli @ 2020-09-30 20:11 ` Andrew Lunn 2020-09-30 20:23 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2020-09-30 20:11 UTC (permalink / raw) To: Florian Fainelli Cc: Jisheng Zhang, Heiner Kallweit, Russell King, David S. Miller, netdev, linux-kernel On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: > > > On 9/30/2020 12:09 PM, Andrew Lunn wrote: > > On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: > > > Hi, > > > > > > A GE phy supports pad isolation which can save power in WOL mode. But once the > > > isolation is enabled, the MAC can't send/receive pkts to/from the phy because > > > the phy is "isolated". To make the PHY work normally, I need to move the > > > enabling isolation to suspend hook, so far so good. But the isolation isn't > > > enabled in system shutdown case, to support this, I want to add shutdown hook > > > to net phy_driver, then also enable the isolation in the shutdown hook. Is > > > there any elegant solution? > > > > > Or we can break the assumption: ethernet can still send/receive pkts after > > > enabling WoL, no? > > > > That is not an easy assumption to break. The MAC might be doing WOL, > > so it needs to be able to receive packets. > > > > What you might be able to assume is, if this PHY device has had WOL > > enabled, it can assume the MAC does not need to send/receive after > > suspend. The problem is, phy_suspend() will not call into the driver > > is WOL is enabled, so you have no idea when you can isolate the MAC > > from the PHY. > > > > So adding a shutdown in mdio_driver_register() seems reasonable. But > > you need to watch out for ordering. Is the MDIO bus driver still > > running? > > If your Ethernet MAC controller implements a shutdown callback and that > callback takes care of unregistering the network device which should also > ensure that phy_disconnect() gets called, then your PHY's suspend function > will be called. Hi Florian I could be missing something here, but: phy_suspend does not call into the PHY driver if WOL is enabled. So Jisheng needs a way to tell the PHY it should isolate itself from the MAC, and suspend is not that. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-09-30 20:11 ` Andrew Lunn @ 2020-09-30 20:23 ` Florian Fainelli 2020-10-05 8:53 ` Jisheng Zhang 0 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2020-09-30 20:23 UTC (permalink / raw) To: Andrew Lunn Cc: Jisheng Zhang, Heiner Kallweit, Russell King, David S. Miller, netdev, linux-kernel On 9/30/2020 1:11 PM, Andrew Lunn wrote: > On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: >> >> >> On 9/30/2020 12:09 PM, Andrew Lunn wrote: >>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: >>>> Hi, >>>> >>>> A GE phy supports pad isolation which can save power in WOL mode. But once the >>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because >>>> the phy is "isolated". To make the PHY work normally, I need to move the >>>> enabling isolation to suspend hook, so far so good. But the isolation isn't >>>> enabled in system shutdown case, to support this, I want to add shutdown hook >>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is >>>> there any elegant solution? >>> >>>> Or we can break the assumption: ethernet can still send/receive pkts after >>>> enabling WoL, no? >>> >>> That is not an easy assumption to break. The MAC might be doing WOL, >>> so it needs to be able to receive packets. >>> >>> What you might be able to assume is, if this PHY device has had WOL >>> enabled, it can assume the MAC does not need to send/receive after >>> suspend. The problem is, phy_suspend() will not call into the driver >>> is WOL is enabled, so you have no idea when you can isolate the MAC >>> from the PHY. >>> >>> So adding a shutdown in mdio_driver_register() seems reasonable. But >>> you need to watch out for ordering. Is the MDIO bus driver still >>> running? >> >> If your Ethernet MAC controller implements a shutdown callback and that >> callback takes care of unregistering the network device which should also >> ensure that phy_disconnect() gets called, then your PHY's suspend function >> will be called. > > Hi Florian > > I could be missing something here, but: > > phy_suspend does not call into the PHY driver if WOL is enabled. So > Jisheng needs a way to tell the PHY it should isolate itself from the > MAC, and suspend is not that. I missed that part, that's right if WoL is enabled at the PHY level then the suspend callback is not called, how about we change that and we always call the PHY's suspend callback? This would require that we audit every driver that defines both .suspend and .set_wol but there are not that many. Adding an additional callback to the PHY driver does not really scale and it would require us to be extra careful and also plumb the MDIO bus shutdown. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-09-30 20:23 ` Florian Fainelli @ 2020-10-05 8:53 ` Jisheng Zhang 2020-10-05 15:41 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Jisheng Zhang @ 2020-10-05 8:53 UTC (permalink / raw) To: Florian Fainelli Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller, netdev, linux-kernel On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote: > > On 9/30/2020 1:11 PM, Andrew Lunn wrote: > > On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: > >> > >> > >> On 9/30/2020 12:09 PM, Andrew Lunn wrote: > >>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: > >>>> Hi, > >>>> > >>>> A GE phy supports pad isolation which can save power in WOL mode. But once the > >>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because > >>>> the phy is "isolated". To make the PHY work normally, I need to move the > >>>> enabling isolation to suspend hook, so far so good. But the isolation isn't > >>>> enabled in system shutdown case, to support this, I want to add shutdown hook > >>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is > >>>> there any elegant solution? > >>> > >>>> Or we can break the assumption: ethernet can still send/receive pkts after > >>>> enabling WoL, no? > >>> > >>> That is not an easy assumption to break. The MAC might be doing WOL, > >>> so it needs to be able to receive packets. > >>> > >>> What you might be able to assume is, if this PHY device has had WOL > >>> enabled, it can assume the MAC does not need to send/receive after > >>> suspend. The problem is, phy_suspend() will not call into the driver > >>> is WOL is enabled, so you have no idea when you can isolate the MAC > >>> from the PHY. > >>> > >>> So adding a shutdown in mdio_driver_register() seems reasonable. But > >>> you need to watch out for ordering. Is the MDIO bus driver still > >>> running? > >> > >> If your Ethernet MAC controller implements a shutdown callback and that > >> callback takes care of unregistering the network device which should also > >> ensure that phy_disconnect() gets called, then your PHY's suspend function > >> will be called. > > > > Hi Florian > > > > I could be missing something here, but: > > > > phy_suspend does not call into the PHY driver if WOL is enabled. So > > Jisheng needs a way to tell the PHY it should isolate itself from the > > MAC, and suspend is not that. > > I missed that part, that's right if WoL is enabled at the PHY level then > the suspend callback is not called, how about we change that and we > always call the PHY's suspend callback? This would require that we audit Hi all, The PHY's suspend callback usually calls genphy_suspend() which will set BMCR_PDOWN bit, this may break WoL. I think this is one the reason why we ignore the phydrv->suspend() when WoL is enabled. If we goes to this directly, it looks like we need to change each phy's suspend implementation, I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; If WoL is disabled, keep the code path as is. So compared with the shutdown hook, which direction is better? Thanks in advance, Jisheng > every driver that defines both .suspend and .set_wol but there are not > that many. > > Adding an additional callback to the PHY driver does not really scale > and it would require us to be extra careful and also plumb the MDIO bus > shutdown. > -- > Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-10-05 8:53 ` Jisheng Zhang @ 2020-10-05 15:41 ` Florian Fainelli 2020-10-05 15:54 ` Heiner Kallweit 0 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2020-10-05 15:41 UTC (permalink / raw) To: Jisheng Zhang Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller, netdev, linux-kernel On 10/5/2020 1:53 AM, Jisheng Zhang wrote: > On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote: > > >> >> On 9/30/2020 1:11 PM, Andrew Lunn wrote: >>> On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: >>>> >>>> >>>> On 9/30/2020 12:09 PM, Andrew Lunn wrote: >>>>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: >>>>>> Hi, >>>>>> >>>>>> A GE phy supports pad isolation which can save power in WOL mode. But once the >>>>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because >>>>>> the phy is "isolated". To make the PHY work normally, I need to move the >>>>>> enabling isolation to suspend hook, so far so good. But the isolation isn't >>>>>> enabled in system shutdown case, to support this, I want to add shutdown hook >>>>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is >>>>>> there any elegant solution? >>>>> >>>>>> Or we can break the assumption: ethernet can still send/receive pkts after >>>>>> enabling WoL, no? >>>>> >>>>> That is not an easy assumption to break. The MAC might be doing WOL, >>>>> so it needs to be able to receive packets. >>>>> >>>>> What you might be able to assume is, if this PHY device has had WOL >>>>> enabled, it can assume the MAC does not need to send/receive after >>>>> suspend. The problem is, phy_suspend() will not call into the driver >>>>> is WOL is enabled, so you have no idea when you can isolate the MAC >>>>> from the PHY. >>>>> >>>>> So adding a shutdown in mdio_driver_register() seems reasonable. But >>>>> you need to watch out for ordering. Is the MDIO bus driver still >>>>> running? >>>> >>>> If your Ethernet MAC controller implements a shutdown callback and that >>>> callback takes care of unregistering the network device which should also >>>> ensure that phy_disconnect() gets called, then your PHY's suspend function >>>> will be called. >>> >>> Hi Florian >>> >>> I could be missing something here, but: >>> >>> phy_suspend does not call into the PHY driver if WOL is enabled. So >>> Jisheng needs a way to tell the PHY it should isolate itself from the >>> MAC, and suspend is not that. >> >> I missed that part, that's right if WoL is enabled at the PHY level then >> the suspend callback is not called, how about we change that and we >> always call the PHY's suspend callback? This would require that we audit > > Hi all, > > The PHY's suspend callback usually calls genphy_suspend() which will set > BMCR_PDOWN bit, this may break WoL. I think this is one the reason why > we ignore the phydrv->suspend() when WoL is enabled. If we goes to this > directly, it looks like we need to change each phy's suspend implementation, > I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; > If WoL is disabled, keep the code path as is. > > So compared with the shutdown hook, which direction is better? I believe you will have an easier time to add an argument to the PHY driver suspend's function to indicate the WoL status, or to move down the check for WoL being enabled/supported compared to adding support for shutdown into the MDIO bus layer, and then PHY drivers. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-10-05 15:41 ` Florian Fainelli @ 2020-10-05 15:54 ` Heiner Kallweit 2020-10-05 16:00 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Heiner Kallweit @ 2020-10-05 15:54 UTC (permalink / raw) To: Florian Fainelli, Jisheng Zhang Cc: Andrew Lunn, Russell King, David S. Miller, netdev, linux-kernel On 05.10.2020 17:41, Florian Fainelli wrote: > > > On 10/5/2020 1:53 AM, Jisheng Zhang wrote: >> On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote: >> >> >>> >>> On 9/30/2020 1:11 PM, Andrew Lunn wrote: >>>> On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: >>>>> >>>>> >>>>> On 9/30/2020 12:09 PM, Andrew Lunn wrote: >>>>>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: >>>>>>> Hi, >>>>>>> >>>>>>> A GE phy supports pad isolation which can save power in WOL mode. But once the >>>>>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because >>>>>>> the phy is "isolated". To make the PHY work normally, I need to move the >>>>>>> enabling isolation to suspend hook, so far so good. But the isolation isn't >>>>>>> enabled in system shutdown case, to support this, I want to add shutdown hook >>>>>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is >>>>>>> there any elegant solution? >>>>>> >>>>>>> Or we can break the assumption: ethernet can still send/receive pkts after >>>>>>> enabling WoL, no? >>>>>> >>>>>> That is not an easy assumption to break. The MAC might be doing WOL, >>>>>> so it needs to be able to receive packets. >>>>>> >>>>>> What you might be able to assume is, if this PHY device has had WOL >>>>>> enabled, it can assume the MAC does not need to send/receive after >>>>>> suspend. The problem is, phy_suspend() will not call into the driver >>>>>> is WOL is enabled, so you have no idea when you can isolate the MAC >>>>>> from the PHY. >>>>>> >>>>>> So adding a shutdown in mdio_driver_register() seems reasonable. But >>>>>> you need to watch out for ordering. Is the MDIO bus driver still >>>>>> running? >>>>> >>>>> If your Ethernet MAC controller implements a shutdown callback and that >>>>> callback takes care of unregistering the network device which should also >>>>> ensure that phy_disconnect() gets called, then your PHY's suspend function >>>>> will be called. >>>> >>>> Hi Florian >>>> >>>> I could be missing something here, but: >>>> >>>> phy_suspend does not call into the PHY driver if WOL is enabled. So >>>> Jisheng needs a way to tell the PHY it should isolate itself from the >>>> MAC, and suspend is not that. >>> >>> I missed that part, that's right if WoL is enabled at the PHY level then >>> the suspend callback is not called, how about we change that and we >>> always call the PHY's suspend callback? This would require that we audit >> >> Hi all, >> >> The PHY's suspend callback usually calls genphy_suspend() which will set >> BMCR_PDOWN bit, this may break WoL. I think this is one the reason why >> we ignore the phydrv->suspend() when WoL is enabled. If we goes to this >> directly, it looks like we need to change each phy's suspend implementation, >> I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; >> If WoL is disabled, keep the code path as is. >> >> So compared with the shutdown hook, which direction is better? > > I believe you will have an easier time to add an argument to the PHY driver suspend's function to indicate the WoL status, or to move down the check for WoL being enabled/supported compared to adding support for shutdown into the MDIO bus layer, and then PHY drivers. Maybe the shutdown callback of mdio_bus_type could be implemented. It could iterate over all PHY's on the bus, check for WoL (similar to mdio_bus_phy_may_suspend) and do whatever is needed. Seems to me to be the most generic way. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-10-05 15:54 ` Heiner Kallweit @ 2020-10-05 16:00 ` Florian Fainelli 2020-10-06 5:45 ` Heiner Kallweit 0 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2020-10-05 16:00 UTC (permalink / raw) To: Heiner Kallweit, Jisheng Zhang Cc: Andrew Lunn, Russell King, David S. Miller, netdev, linux-kernel On 10/5/2020 8:54 AM, Heiner Kallweit wrote: > On 05.10.2020 17:41, Florian Fainelli wrote: >> >> >> On 10/5/2020 1:53 AM, Jisheng Zhang wrote: >>> On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote: >>> >>> >>>> >>>> On 9/30/2020 1:11 PM, Andrew Lunn wrote: >>>>> On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: >>>>>> >>>>>> >>>>>> On 9/30/2020 12:09 PM, Andrew Lunn wrote: >>>>>>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> A GE phy supports pad isolation which can save power in WOL mode. But once the >>>>>>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because >>>>>>>> the phy is "isolated". To make the PHY work normally, I need to move the >>>>>>>> enabling isolation to suspend hook, so far so good. But the isolation isn't >>>>>>>> enabled in system shutdown case, to support this, I want to add shutdown hook >>>>>>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is >>>>>>>> there any elegant solution? >>>>>>> >>>>>>>> Or we can break the assumption: ethernet can still send/receive pkts after >>>>>>>> enabling WoL, no? >>>>>>> >>>>>>> That is not an easy assumption to break. The MAC might be doing WOL, >>>>>>> so it needs to be able to receive packets. >>>>>>> >>>>>>> What you might be able to assume is, if this PHY device has had WOL >>>>>>> enabled, it can assume the MAC does not need to send/receive after >>>>>>> suspend. The problem is, phy_suspend() will not call into the driver >>>>>>> is WOL is enabled, so you have no idea when you can isolate the MAC >>>>>>> from the PHY. >>>>>>> >>>>>>> So adding a shutdown in mdio_driver_register() seems reasonable. But >>>>>>> you need to watch out for ordering. Is the MDIO bus driver still >>>>>>> running? >>>>>> >>>>>> If your Ethernet MAC controller implements a shutdown callback and that >>>>>> callback takes care of unregistering the network device which should also >>>>>> ensure that phy_disconnect() gets called, then your PHY's suspend function >>>>>> will be called. >>>>> >>>>> Hi Florian >>>>> >>>>> I could be missing something here, but: >>>>> >>>>> phy_suspend does not call into the PHY driver if WOL is enabled. So >>>>> Jisheng needs a way to tell the PHY it should isolate itself from the >>>>> MAC, and suspend is not that. >>>> >>>> I missed that part, that's right if WoL is enabled at the PHY level then >>>> the suspend callback is not called, how about we change that and we >>>> always call the PHY's suspend callback? This would require that we audit >>> >>> Hi all, >>> >>> The PHY's suspend callback usually calls genphy_suspend() which will set >>> BMCR_PDOWN bit, this may break WoL. I think this is one the reason why >>> we ignore the phydrv->suspend() when WoL is enabled. If we goes to this >>> directly, it looks like we need to change each phy's suspend implementation, >>> I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; >>> If WoL is disabled, keep the code path as is. >>> >>> So compared with the shutdown hook, which direction is better? >> >> I believe you will have an easier time to add an argument to the PHY driver suspend's function to indicate the WoL status, or to move down the check for WoL being enabled/supported compared to adding support for shutdown into the MDIO bus layer, and then PHY drivers. > > Maybe the shutdown callback of mdio_bus_type could be implemented. > It could iterate over all PHY's on the bus, check for WoL (similar to > mdio_bus_phy_may_suspend) and do whatever is needed. > Seems to me to be the most generic way. OK and we optionally call into a PHY device's shutdown function if defined so it can perform PHY specific work? That would work for me. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-10-05 16:00 ` Florian Fainelli @ 2020-10-06 5:45 ` Heiner Kallweit 2020-10-06 7:17 ` Jisheng Zhang 0 siblings, 1 reply; 11+ messages in thread From: Heiner Kallweit @ 2020-10-06 5:45 UTC (permalink / raw) To: Florian Fainelli, Jisheng Zhang Cc: Andrew Lunn, Russell King, David S. Miller, netdev, linux-kernel On 05.10.2020 18:00, Florian Fainelli wrote: > > > On 10/5/2020 8:54 AM, Heiner Kallweit wrote: >> On 05.10.2020 17:41, Florian Fainelli wrote: >>> >>> >>> On 10/5/2020 1:53 AM, Jisheng Zhang wrote: >>>> On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote: >>>> >>>> >>>>> >>>>> On 9/30/2020 1:11 PM, Andrew Lunn wrote: >>>>>> On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: >>>>>>> >>>>>>> >>>>>>> On 9/30/2020 12:09 PM, Andrew Lunn wrote: >>>>>>>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> A GE phy supports pad isolation which can save power in WOL mode. But once the >>>>>>>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because >>>>>>>>> the phy is "isolated". To make the PHY work normally, I need to move the >>>>>>>>> enabling isolation to suspend hook, so far so good. But the isolation isn't >>>>>>>>> enabled in system shutdown case, to support this, I want to add shutdown hook >>>>>>>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is >>>>>>>>> there any elegant solution? >>>>>>>> >>>>>>>>> Or we can break the assumption: ethernet can still send/receive pkts after >>>>>>>>> enabling WoL, no? >>>>>>>> >>>>>>>> That is not an easy assumption to break. The MAC might be doing WOL, >>>>>>>> so it needs to be able to receive packets. >>>>>>>> >>>>>>>> What you might be able to assume is, if this PHY device has had WOL >>>>>>>> enabled, it can assume the MAC does not need to send/receive after >>>>>>>> suspend. The problem is, phy_suspend() will not call into the driver >>>>>>>> is WOL is enabled, so you have no idea when you can isolate the MAC >>>>>>>> from the PHY. >>>>>>>> >>>>>>>> So adding a shutdown in mdio_driver_register() seems reasonable. But >>>>>>>> you need to watch out for ordering. Is the MDIO bus driver still >>>>>>>> running? >>>>>>> >>>>>>> If your Ethernet MAC controller implements a shutdown callback and that >>>>>>> callback takes care of unregistering the network device which should also >>>>>>> ensure that phy_disconnect() gets called, then your PHY's suspend function >>>>>>> will be called. >>>>>> >>>>>> Hi Florian >>>>>> >>>>>> I could be missing something here, but: >>>>>> >>>>>> phy_suspend does not call into the PHY driver if WOL is enabled. So >>>>>> Jisheng needs a way to tell the PHY it should isolate itself from the >>>>>> MAC, and suspend is not that. >>>>> >>>>> I missed that part, that's right if WoL is enabled at the PHY level then >>>>> the suspend callback is not called, how about we change that and we >>>>> always call the PHY's suspend callback? This would require that we audit >>>> >>>> Hi all, >>>> >>>> The PHY's suspend callback usually calls genphy_suspend() which will set >>>> BMCR_PDOWN bit, this may break WoL. I think this is one the reason why >>>> we ignore the phydrv->suspend() when WoL is enabled. If we goes to this >>>> directly, it looks like we need to change each phy's suspend implementation, >>>> I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; >>>> If WoL is disabled, keep the code path as is. >>>> >>>> So compared with the shutdown hook, which direction is better? >>> >>> I believe you will have an easier time to add an argument to the PHY driver suspend's function to indicate the WoL status, or to move down the check for WoL being enabled/supported compared to adding support for shutdown into the MDIO bus layer, and then PHY drivers. >> >> Maybe the shutdown callback of mdio_bus_type could be implemented. >> It could iterate over all PHY's on the bus, check for WoL (similar to >> mdio_bus_phy_may_suspend) and do whatever is needed. >> Seems to me to be the most generic way. > > OK and we optionally call into a PHY device's shutdown function if defined so it can perform PHY specific work? That would work for me. If suspend and shutdown procedure are the same for the PHY, then we may not need a shutdown hook in phy_driver. This seems to be the case here. I just wonder what the actual use case is, because typically MAC drivers call phy_stop (that calls phy_suspend) in their shutdown hook. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] net: phy: add shutdown hook to struct phy_driver 2020-10-06 5:45 ` Heiner Kallweit @ 2020-10-06 7:17 ` Jisheng Zhang 0 siblings, 0 replies; 11+ messages in thread From: Jisheng Zhang @ 2020-10-06 7:17 UTC (permalink / raw) To: Heiner Kallweit Cc: Florian Fainelli, Andrew Lunn, Russell King, David S. Miller, netdev, linux-kernel On Tue, 6 Oct 2020 07:45:10 +0200 Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 05.10.2020 18:00, Florian Fainelli wrote: > > > > > > On 10/5/2020 8:54 AM, Heiner Kallweit wrote: > >> On 05.10.2020 17:41, Florian Fainelli wrote: > >>> > >>> > >>> On 10/5/2020 1:53 AM, Jisheng Zhang wrote: > >>>> On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote: > >>>> > >>>> > >>>>> > >>>>> On 9/30/2020 1:11 PM, Andrew Lunn wrote: > >>>>>> On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 9/30/2020 12:09 PM, Andrew Lunn wrote: > >>>>>>>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> A GE phy supports pad isolation which can save power in WOL mode. But once the > >>>>>>>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy because > >>>>>>>>> the phy is "isolated". To make the PHY work normally, I need to move the > >>>>>>>>> enabling isolation to suspend hook, so far so good. But the isolation isn't > >>>>>>>>> enabled in system shutdown case, to support this, I want to add shutdown hook > >>>>>>>>> to net phy_driver, then also enable the isolation in the shutdown hook. Is > >>>>>>>>> there any elegant solution? > >>>>>>>> > >>>>>>>>> Or we can break the assumption: ethernet can still send/receive pkts after > >>>>>>>>> enabling WoL, no? > >>>>>>>> > >>>>>>>> That is not an easy assumption to break. The MAC might be doing WOL, > >>>>>>>> so it needs to be able to receive packets. > >>>>>>>> > >>>>>>>> What you might be able to assume is, if this PHY device has had WOL > >>>>>>>> enabled, it can assume the MAC does not need to send/receive after > >>>>>>>> suspend. The problem is, phy_suspend() will not call into the driver > >>>>>>>> is WOL is enabled, so you have no idea when you can isolate the MAC > >>>>>>>> from the PHY. > >>>>>>>> > >>>>>>>> So adding a shutdown in mdio_driver_register() seems reasonable. But > >>>>>>>> you need to watch out for ordering. Is the MDIO bus driver still > >>>>>>>> running? > >>>>>>> > >>>>>>> If your Ethernet MAC controller implements a shutdown callback and that > >>>>>>> callback takes care of unregistering the network device which should also > >>>>>>> ensure that phy_disconnect() gets called, then your PHY's suspend function > >>>>>>> will be called. > >>>>>> > >>>>>> Hi Florian > >>>>>> > >>>>>> I could be missing something here, but: > >>>>>> > >>>>>> phy_suspend does not call into the PHY driver if WOL is enabled. So > >>>>>> Jisheng needs a way to tell the PHY it should isolate itself from the > >>>>>> MAC, and suspend is not that. > >>>>> > >>>>> I missed that part, that's right if WoL is enabled at the PHY level then > >>>>> the suspend callback is not called, how about we change that and we > >>>>> always call the PHY's suspend callback? This would require that we audit > >>>> > >>>> Hi all, > >>>> > >>>> The PHY's suspend callback usually calls genphy_suspend() which will set > >>>> BMCR_PDOWN bit, this may break WoL. I think this is one the reason why > >>>> we ignore the phydrv->suspend() when WoL is enabled. If we goes to this > >>>> directly, it looks like we need to change each phy's suspend implementation, > >>>> I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; > >>>> If WoL is disabled, keep the code path as is. > >>>> > >>>> So compared with the shutdown hook, which direction is better? > >>> > >>> I believe you will have an easier time to add an argument to the PHY driver suspend's function to indicate the WoL status, or to move down the check for WoL being enabled/supported compared to adding support for shutdown into the MDIO bus layer, and then PHY drivers. > >> > >> Maybe the shutdown callback of mdio_bus_type could be implemented. > >> It could iterate over all PHY's on the bus, check for WoL (similar to > >> mdio_bus_phy_may_suspend) and do whatever is needed. > >> Seems to me to be the most generic way. > > > > OK and we optionally call into a PHY device's shutdown function if defined so it can perform PHY specific work? That would work for me. > > If suspend and shutdown procedure are the same for the PHY, then we may not > need a shutdown hook in phy_driver. This seems to be the case here. > I just wonder what the actual use case is, because typically MAC drivers > call phy_stop (that calls phy_suspend) in their shutdown hook. Thank you all, I will follow this direction and send out an RFC for review. Thanks a lot, Jisheng ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-06 7:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-30 9:47 [RFC] net: phy: add shutdown hook to struct phy_driver Jisheng Zhang 2020-09-30 19:09 ` Andrew Lunn 2020-09-30 20:07 ` Florian Fainelli 2020-09-30 20:11 ` Andrew Lunn 2020-09-30 20:23 ` Florian Fainelli 2020-10-05 8:53 ` Jisheng Zhang 2020-10-05 15:41 ` Florian Fainelli 2020-10-05 15:54 ` Heiner Kallweit 2020-10-05 16:00 ` Florian Fainelli 2020-10-06 5:45 ` Heiner Kallweit 2020-10-06 7:17 ` Jisheng Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).