Cutting the link on ndo_stop - phy_stop or phy_disconnect?

Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

Hi,

I've been wondering what is the correct approach to cut the Ethernet 
link when the user requests it to be administratively down (aka ip link 
set dev eth0 down).
Most of the Ethernet drivers simply call phy_stop or the phylink 
equivalent. This leaves an Ethernet link between the PHY and its link 
partner.
The Freescale gianfar driver (authored by Andy Fleming who also authored 
the phylib) does a phy_disconnect here. It may seem a bit overkill, but 
of the extra things it does, it calls phy_suspend where most PHY drivers 
set the BMCR_PDOWN bit. Only this achieves the intended purpose of also 
cutting the link partner's link on 'ip link set dev eth0 down'.
What is the general consensus here?
I see the ability to be able to put the PHY link administratively down a 
desirable feat. If it's left to negotiate/receive traffic etc while the 
MAC driver isn't completely set up and ready, in theory a lot of 
processing can happen outside of the operating system's control.

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Andrew Lunn]

On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
> Hi,
> 
> I've been wondering what is the correct approach to cut the Ethernet link
> when the user requests it to be administratively down (aka ip link set dev
> eth0 down).
> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
> This leaves an Ethernet link between the PHY and its link partner.
> The Freescale gianfar driver (authored by Andy Fleming who also authored the
> phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
> extra things it does, it calls phy_suspend where most PHY drivers set the
> BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
> link partner's link on 'ip link set dev eth0 down'.

Hi Vladimir

Heiner knows the state machine better than i. But when we transition
to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().

   Andrew

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Tue, 4 Jun 2019 at 23:07, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
> > Hi,
> >
> > I've been wondering what is the correct approach to cut the Ethernet link
> > when the user requests it to be administratively down (aka ip link set dev
> > eth0 down).
> > Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
> > This leaves an Ethernet link between the PHY and its link partner.
> > The Freescale gianfar driver (authored by Andy Fleming who also authored the
> > phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
> > extra things it does, it calls phy_suspend where most PHY drivers set the
> > BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
> > link partner's link on 'ip link set dev eth0 down'.
>
> Hi Vladimir
>
> Heiner knows the state machine better than i. But when we transition
> to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().
>
>    Andrew

Hi Andrew, Florian,

Thanks for giving me the PHY_HALTED hint!
Indeed it looks like I conflated two things - the Ehernet port that
uses phy_disconnect also happens to be connected to a PHY that has
phy_suspend implemented. Whereas the one that only does phy_stop is
connected to a PHY that doesn't have that... I thought that in absence
of .suspend, the PHY library automatically calls genphy_suspend. Oh
well, looks like it doesn't. So of course, phy_stop calls phy_suspend
too.
But now the second question: between a phy_connect and a phy_start,
shouldn't the PHY be suspended too? Experimentally it looks like it
still isn't.
By the way, Florian, yes, PHY drivers that use WOL still set
BMCR_ISOLATE, which cuts the MII-side, so that's ok. However that's
not the case here - no WOL.

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Heiner Kallweit]

On 04.06.2019 22:42, Vladimir Oltean wrote:
> On Tue, 4 Jun 2019 at 23:07, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
>>> Hi,
>>>
>>> I've been wondering what is the correct approach to cut the Ethernet link
>>> when the user requests it to be administratively down (aka ip link set dev
>>> eth0 down).
>>> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
>>> This leaves an Ethernet link between the PHY and its link partner.
>>> The Freescale gianfar driver (authored by Andy Fleming who also authored the
>>> phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
>>> extra things it does, it calls phy_suspend where most PHY drivers set the
>>> BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
>>> link partner's link on 'ip link set dev eth0 down'.
>>
>> Hi Vladimir
>>
>> Heiner knows the state machine better than i. But when we transition
>> to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().
>>
>>    Andrew
> 
> Hi Andrew, Florian,
> 
> Thanks for giving me the PHY_HALTED hint!
> Indeed it looks like I conflated two things - the Ehernet port that
> uses phy_disconnect also happens to be connected to a PHY that has
> phy_suspend implemented. Whereas the one that only does phy_stop is
> connected to a PHY that doesn't have that... I thought that in absence
> of .suspend, the PHY library automatically calls genphy_suspend. Oh
> well, looks like it doesn't. So of course, phy_stop calls phy_suspend
> too.
> But now the second question: between a phy_connect and a phy_start,
> shouldn't the PHY be suspended too? Experimentally it looks like it
> still isn't.
> By the way, Florian, yes, PHY drivers that use WOL still set
> BMCR_ISOLATE, which cuts the MII-side, so that's ok. However that's
> not the case here - no WOL.
> 
Right, some PHY driver callbacks fall back to the generic functionality,
for the suspend/resume callbacks that's not the case.
phy_connect() eventually calls phy_attach_direct() that has a call to
phy_resume(). So your observation is correct, phy_connect() wakes the
PHY. I'm not 100% sure whether this is needed because also phy_start()
resumes the PHY.

BMCR_ISOLATE isn't set by any phylib function. We just have few
calls where BMCR_ISOLATE is cleared as part of the functionality.

> Regards,
> -Vladimir
> 
Heiner

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Tue, 4 Jun 2019 at 23:55, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 04.06.2019 22:42, Vladimir Oltean wrote:
> > On Tue, 4 Jun 2019 at 23:07, Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >> On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
> >>> Hi,
> >>>
> >>> I've been wondering what is the correct approach to cut the Ethernet link
> >>> when the user requests it to be administratively down (aka ip link set dev
> >>> eth0 down).
> >>> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
> >>> This leaves an Ethernet link between the PHY and its link partner.
> >>> The Freescale gianfar driver (authored by Andy Fleming who also authored the
> >>> phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
> >>> extra things it does, it calls phy_suspend where most PHY drivers set the
> >>> BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
> >>> link partner's link on 'ip link set dev eth0 down'.
> >>
> >> Hi Vladimir
> >>
> >> Heiner knows the state machine better than i. But when we transition
> >> to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().
> >>
> >>    Andrew
> >
> > Hi Andrew, Florian,
> >
> > Thanks for giving me the PHY_HALTED hint!
> > Indeed it looks like I conflated two things - the Ehernet port that
> > uses phy_disconnect also happens to be connected to a PHY that has
> > phy_suspend implemented. Whereas the one that only does phy_stop is
> > connected to a PHY that doesn't have that... I thought that in absence
> > of .suspend, the PHY library automatically calls genphy_suspend. Oh
> > well, looks like it doesn't. So of course, phy_stop calls phy_suspend
> > too.
> > But now the second question: between a phy_connect and a phy_start,
> > shouldn't the PHY be suspended too? Experimentally it looks like it
> > still isn't.
> > By the way, Florian, yes, PHY drivers that use WOL still set
> > BMCR_ISOLATE, which cuts the MII-side, so that's ok. However that's
> > not the case here - no WOL.
> >
> Right, some PHY driver callbacks fall back to the generic functionality,
> for the suspend/resume callbacks that's not the case.
> phy_connect() eventually calls phy_attach_direct() that has a call to
> phy_resume(). So your observation is correct, phy_connect() wakes the
> PHY. I'm not 100% sure whether this is needed because also phy_start()
> resumes the PHY.
>

Thanks Heiner!
Looks like replacing the phy_resume() from phy_attach_direct with
phy_suspend() does what I want it to.

> BMCR_ISOLATE isn't set by any phylib function. We just have few
> calls where BMCR_ISOLATE is cleared as part of the functionality.
>
> > Regards,
> > -Vladimir
> >
> Heiner

-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Florian Fainelli]

On 6/4/2019 1:42 PM, Vladimir Oltean wrote:
> On Tue, 4 Jun 2019 at 23:07, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
>>> Hi,
>>>
>>> I've been wondering what is the correct approach to cut the Ethernet link
>>> when the user requests it to be administratively down (aka ip link set dev
>>> eth0 down).
>>> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
>>> This leaves an Ethernet link between the PHY and its link partner.
>>> The Freescale gianfar driver (authored by Andy Fleming who also authored the
>>> phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
>>> extra things it does, it calls phy_suspend where most PHY drivers set the
>>> BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
>>> link partner's link on 'ip link set dev eth0 down'.
>>
>> Hi Vladimir
>>
>> Heiner knows the state machine better than i. But when we transition
>> to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().
>>
>>    Andrew
> 
> Hi Andrew, Florian,
> 
> Thanks for giving me the PHY_HALTED hint!
> Indeed it looks like I conflated two things - the Ehernet port that
> uses phy_disconnect also happens to be connected to a PHY that has
> phy_suspend implemented. Whereas the one that only does phy_stop is
> connected to a PHY that doesn't have that... I thought that in absence
> of .suspend, the PHY library automatically calls genphy_suspend.

It does not fallback to genphy_suspend(), maybe we should change that,
setting BMCR.PDOWN is a good power saving in itself, if the PHY can do
more, you have to implement a .suspend() callback to get the additional
power savings.

> Oh well, looks like it doesn't. So of course, phy_stop calls phy_suspend
> too.
> But now the second question: between a phy_connect and a phy_start,
> shouldn't the PHY be suspended too? Experimentally it looks like it
> still isn't.
> By the way, Florian, yes, PHY drivers that use WOL still set
> BMCR_ISOLATE, which cuts the MII-side, so that's ok. However that's
> not the case here - no WOL.

I was just responding about what is IMHO sensible to do. There is an
additional caveat/use case to possibly consider which I am sure some
drivers intentionally support (or not), which is that bringing down the
interface does stop the PHY state machine and then you are free to issue
whatever SIO{S,G}MIIREG for diagnostics etc. Whether the MAC or PHYLIB
is responsible for taking the PHY out of power down mode, or if it is up
to the diagnostics software to do that is up for debate, I would go with
the latter, which would always work regardless of what you are trying to do.
-- 
Florian

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Tue, 4 Jun 2019 at 23:57, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/4/2019 1:42 PM, Vladimir Oltean wrote:
> > On Tue, 4 Jun 2019 at 23:07, Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >> On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
> >>> Hi,
> >>>
> >>> I've been wondering what is the correct approach to cut the Ethernet link
> >>> when the user requests it to be administratively down (aka ip link set dev
> >>> eth0 down).
> >>> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
> >>> This leaves an Ethernet link between the PHY and its link partner.
> >>> The Freescale gianfar driver (authored by Andy Fleming who also authored the
> >>> phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
> >>> extra things it does, it calls phy_suspend where most PHY drivers set the
> >>> BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
> >>> link partner's link on 'ip link set dev eth0 down'.
> >>
> >> Hi Vladimir
> >>
> >> Heiner knows the state machine better than i. But when we transition
> >> to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().
> >>
> >>    Andrew
> >
> > Hi Andrew, Florian,
> >
> > Thanks for giving me the PHY_HALTED hint!
> > Indeed it looks like I conflated two things - the Ehernet port that
> > uses phy_disconnect also happens to be connected to a PHY that has
> > phy_suspend implemented. Whereas the one that only does phy_stop is
> > connected to a PHY that doesn't have that... I thought that in absence
> > of .suspend, the PHY library automatically calls genphy_suspend.
>
> It does not fallback to genphy_suspend(), maybe we should change that,
> setting BMCR.PDOWN is a good power saving in itself, if the PHY can do
> more, you have to implement a .suspend() callback to get the additional
> power savings.
>
> > Oh well, looks like it doesn't. So of course, phy_stop calls phy_suspend
> > too.
> > But now the second question: between a phy_connect and a phy_start,
> > shouldn't the PHY be suspended too? Experimentally it looks like it
> > still isn't.
> > By the way, Florian, yes, PHY drivers that use WOL still set
> > BMCR_ISOLATE, which cuts the MII-side, so that's ok. However that's
> > not the case here - no WOL.
>
> I was just responding about what is IMHO sensible to do. There is an
> additional caveat/use case to possibly consider which I am sure some
> drivers intentionally support (or not), which is that bringing down the
> interface does stop the PHY state machine and then you are free to issue
> whatever SIO{S,G}MIIREG for diagnostics etc. Whether the MAC or PHYLIB
> is responsible for taking the PHY out of power down mode, or if it is up
> to the diagnostics software to do that is up for debate, I would go with
> the latter, which would always work regardless of what you are trying to do.

Well in that case the "diagnostics software" (we're probably all
looking at phytool) should just unset the ISOLATE/PDOWN bits...
The netdev is still going to have carrier off either way...

> --
> Florian

-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Andrew Lunn]

> But now the second question: between a phy_connect and a phy_start,
> shouldn't the PHY be suspended too? Experimentally it looks like it
> still isn't.

This is not always clear cut. Doing auto-neg is slow. Some systems
want to get networking going as fast as possible. The PHY can be
strapped so that on power up it starts autoneg. That can be finished
by the time linux takes control of the PHY, and it can take over the
results, rather than triggering another auto-neg, which will add
another 3 seconds before the network is up.

If we power the PHY down, between connect and start, we loose all
this.

I don't remember anybody submitting patches because the PHY passed
frames to the MAC too early. So i don't think there is much danger
there.

	Andrew

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 00:12, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > But now the second question: between a phy_connect and a phy_start,
> > shouldn't the PHY be suspended too? Experimentally it looks like it
> > still isn't.
>
> This is not always clear cut. Doing auto-neg is slow. Some systems
> want to get networking going as fast as possible. The PHY can be
> strapped so that on power up it starts autoneg. That can be finished
> by the time linux takes control of the PHY, and it can take over the
> results, rather than triggering another auto-neg, which will add
> another 3 seconds before the network is up.
>
> If we power the PHY down, between connect and start, we loose all
> this.
>
> I don't remember anybody submitting patches because the PHY passed
> frames to the MAC too early. So i don't think there is much danger
> there.
>
>         Andrew

Hi Andrew,

Call me paranoid, but I think the assumption you're making is that
every time you have an Ethernet link, you want it.
Consider the case where you have an Ethernet switch brought up by
U-boot (where it does dumb switching, with no STP, nothing) and the
system power-cycles in a network with loops.
If the operating system has no way to control whether the Ethernet
ports are administratively up, anything can happen... I don't think
it's a bad idea to err on the safe side here. Even in the case of a
regular NIC, packets can go up quite a bit in the MAC, possibly even
triggering interrupts on the cores, when the interface should have
been otherwise "down".

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Florian Fainelli]

On 6/4/2019 2:29 PM, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 00:12, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>>> But now the second question: between a phy_connect and a phy_start,
>>> shouldn't the PHY be suspended too? Experimentally it looks like it
>>> still isn't.
>>
>> This is not always clear cut. Doing auto-neg is slow. Some systems
>> want to get networking going as fast as possible. The PHY can be
>> strapped so that on power up it starts autoneg. That can be finished
>> by the time linux takes control of the PHY, and it can take over the
>> results, rather than triggering another auto-neg, which will add
>> another 3 seconds before the network is up.
>>
>> If we power the PHY down, between connect and start, we loose all
>> this.
>>
>> I don't remember anybody submitting patches because the PHY passed
>> frames to the MAC too early. So i don't think there is much danger
>> there.
>>
>>         Andrew
> 
> Hi Andrew,
> 
> Call me paranoid, but I think the assumption you're making is that
> every time you have an Ethernet link, you want it.
> Consider the case where you have an Ethernet switch brought up by
> U-boot (where it does dumb switching, with no STP, nothing) and the
> system power-cycles in a network with loops.
> If the operating system has no way to control whether the Ethernet
> ports are administratively up, anything can happen... I don't think
> it's a bad idea to err on the safe side here. Even in the case of a
> regular NIC, packets can go up quite a bit in the MAC, possibly even
> triggering interrupts on the cores, when the interface should have
> been otherwise "down".

The firmware/boot loader transition to a full fledged OS with a switch
is a tricky one to answer though, and there are no perfect answers
AFAICT. If your SW is totally hosed, you might want the switch to
forward traffic between all LAN ports (excluding WAN, otherwise you
expose your home devices to the outside world, whoops).

If your SW is fully operational, then the questions are:

- do you want a DSA like behavior in your boot loader, in that all ports
are separated but fully usable or do you want a dumb switch model where
any port can forward to the CPU/management port, without any tags or
anything (unmanaged mode)

- what happens during bootloader to OS handover, should the switch be
held in reset so as to avoid any possible indirect DMA into main memory
as much as power saving? Should nothing happen and let the OS wipe out
clean the setting left by the boot loader?

All of these are in the realm of policy and trade offs as far as
initializing/disruption goes, so there are no hard and fast answers.
-- 
Florian

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Tue, Jun 04, 2019 at 02:37:31PM -0700, Florian Fainelli wrote:
> The firmware/boot loader transition to a full fledged OS with a switch
> is a tricky one to answer though, and there are no perfect answers
> AFAICT. If your SW is totally hosed, you might want the switch to
> forward traffic between all LAN ports (excluding WAN, otherwise you
> expose your home devices to the outside world, whoops).
> 
> If your SW is fully operational, then the questions are:
> 
> - do you want a DSA like behavior in your boot loader, in that all ports
> are separated but fully usable or do you want a dumb switch model where
> any port can forward to the CPU/management port, without any tags or
> anything (unmanaged mode)
> 
> - what happens during bootloader to OS handover, should the switch be
> held in reset so as to avoid any possible indirect DMA into main memory
> as much as power saving? Should nothing happen and let the OS wipe out
> clean the setting left by the boot loader?
> 
> All of these are in the realm of policy and trade offs as far as
> initializing/disruption goes, so there are no hard and fast answers.

For a switch, there are four stages, not two:

1. The out-of-reset state, which from what I've seen seems to be to
   behave like a dumb switch.

2. The boot loader state, which is generally the same as the
   out-of-reset state.

3. The OS-booting state, which for a DSA switch in Linux isolates each
   port from each other.

4. The OS-booted state, which depends on the system configuration.

If you are setting up a switch in a STP environment, you _have_ to be
aware of all of those states, and plan your network accordingly.
While it's possible to argue that the boot loader should isolate the
ports, it may be that the system gets hosed to the point that the boot
loader is unable to run - then you have a switch operating in a STP
environment acting as a dumb switch.

The same actually goes for many switches - consider your typical DSL
router integrated with a four port switch.  By default, that switch
forwards traffic between each port.  If you've setup the ports to be
isolated, each time the router is rebooted (e.g., due to a
configuration change) it will forward traffic between all ports until
the routers OS has finished booting and applied the switch
configuration.

What I'm basically saying is that if you're going to the point of
using such hardware in a STP environment, you _must_ pay attention
to the behaviour of the hardware through all phases of its operation
and consider the consequences should it fail in any of those phases.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Andrew Lunn]

> For a switch, there are four stages, not two:

Agreed.
 
> 1. The out-of-reset state, which from what I've seen seems to be to
>    behave like a dumb switch.

For the Marvell Switches, there is a pin you can strap to control
this, NO_CPU. If you say there is no CPU, it will power up in dumb
switch mode, and potentially bring your network down with a broadcast
storm because of loops.

So yes, you need to think about this at all four stages, and make sure
your hardware engineer is also on board.

     Andrew

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 00:48, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Tue, Jun 04, 2019 at 02:37:31PM -0700, Florian Fainelli wrote:
> > The firmware/boot loader transition to a full fledged OS with a switch
> > is a tricky one to answer though, and there are no perfect answers
> > AFAICT. If your SW is totally hosed, you might want the switch to
> > forward traffic between all LAN ports (excluding WAN, otherwise you
> > expose your home devices to the outside world, whoops).
> >
> > If your SW is fully operational, then the questions are:
> >
> > - do you want a DSA like behavior in your boot loader, in that all ports
> > are separated but fully usable or do you want a dumb switch model where
> > any port can forward to the CPU/management port, without any tags or
> > anything (unmanaged mode)
> >
> > - what happens during bootloader to OS handover, should the switch be
> > held in reset so as to avoid any possible indirect DMA into main memory
> > as much as power saving? Should nothing happen and let the OS wipe out
> > clean the setting left by the boot loader?
> >
> > All of these are in the realm of policy and trade offs as far as
> > initializing/disruption goes, so there are no hard and fast answers.
>
> For a switch, there are four stages, not two:
>
> 1. The out-of-reset state, which from what I've seen seems to be to
>    behave like a dumb switch.
>
> 2. The boot loader state, which is generally the same as the
>    out-of-reset state.
>
> 3. The OS-booting state, which for a DSA switch in Linux isolates each
>    port from each other.
>
> 4. The OS-booted state, which depends on the system configuration.
>
> If you are setting up a switch in a STP environment, you _have_ to be
> aware of all of those states, and plan your network accordingly.
> While it's possible to argue that the boot loader should isolate the
> ports, it may be that the system gets hosed to the point that the boot
> loader is unable to run - then you have a switch operating in a STP
> environment acting as a dumb switch.
>
> The same actually goes for many switches - consider your typical DSL
> router integrated with a four port switch.  By default, that switch
> forwards traffic between each port.  If you've setup the ports to be
> isolated, each time the router is rebooted (e.g., due to a
> configuration change) it will forward traffic between all ports until
> the routers OS has finished booting and applied the switch
> configuration.
>
> What I'm basically saying is that if you're going to the point of
> using such hardware in a STP environment, you _must_ pay attention
> to the behaviour of the hardware through all phases of its operation
> and consider the consequences should it fail in any of those phases.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Hi Russell,

The dumb switch was just an example. The absolute same thing (unwanted
PHY connection) applies to regular NICs. I am not aware of any setting
that makes the MAC ignore frames as long as they observe the
appropriate MII spec. And the hardware will go on to process those
frames, potentially calling into the operating system and making it
susceptible to denial of service. That is unless you set up your
buffer rings/queues/whatever in the ndo_open/ndo_close callbacks.
The point is that having a way to make a link dead/have it dead by
default is the easiest way to prevent a lot of problems. Call that
policy, whatever, but I think at the very least the Linux kernel
should have a way to operate in this mode (if not even be the default
one, unless the user knows better).

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 01:03:27AM +0300, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 00:48, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Tue, Jun 04, 2019 at 02:37:31PM -0700, Florian Fainelli wrote:
> > > The firmware/boot loader transition to a full fledged OS with a switch
> > > is a tricky one to answer though, and there are no perfect answers
> > > AFAICT. If your SW is totally hosed, you might want the switch to
> > > forward traffic between all LAN ports (excluding WAN, otherwise you
> > > expose your home devices to the outside world, whoops).
> > >
> > > If your SW is fully operational, then the questions are:
> > >
> > > - do you want a DSA like behavior in your boot loader, in that all ports
> > > are separated but fully usable or do you want a dumb switch model where
> > > any port can forward to the CPU/management port, without any tags or
> > > anything (unmanaged mode)
> > >
> > > - what happens during bootloader to OS handover, should the switch be
> > > held in reset so as to avoid any possible indirect DMA into main memory
> > > as much as power saving? Should nothing happen and let the OS wipe out
> > > clean the setting left by the boot loader?
> > >
> > > All of these are in the realm of policy and trade offs as far as
> > > initializing/disruption goes, so there are no hard and fast answers.
> >
> > For a switch, there are four stages, not two:
> >
> > 1. The out-of-reset state, which from what I've seen seems to be to
> >    behave like a dumb switch.
> >
> > 2. The boot loader state, which is generally the same as the
> >    out-of-reset state.
> >
> > 3. The OS-booting state, which for a DSA switch in Linux isolates each
> >    port from each other.
> >
> > 4. The OS-booted state, which depends on the system configuration.
> >
> > If you are setting up a switch in a STP environment, you _have_ to be
> > aware of all of those states, and plan your network accordingly.
> > While it's possible to argue that the boot loader should isolate the
> > ports, it may be that the system gets hosed to the point that the boot
> > loader is unable to run - then you have a switch operating in a STP
> > environment acting as a dumb switch.
> >
> > The same actually goes for many switches - consider your typical DSL
> > router integrated with a four port switch.  By default, that switch
> > forwards traffic between each port.  If you've setup the ports to be
> > isolated, each time the router is rebooted (e.g., due to a
> > configuration change) it will forward traffic between all ports until
> > the routers OS has finished booting and applied the switch
> > configuration.
> >
> > What I'm basically saying is that if you're going to the point of
> > using such hardware in a STP environment, you _must_ pay attention
> > to the behaviour of the hardware through all phases of its operation
> > and consider the consequences should it fail in any of those phases.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > According to speedtest.net: 11.9Mbps down 500kbps up
> 
> Hi Russell,
> 
> The dumb switch was just an example. The absolute same thing (unwanted
> PHY connection) applies to regular NICs. I am not aware of any setting
> that makes the MAC ignore frames as long as they observe the
> appropriate MII spec. And the hardware will go on to process those
> frames, potentially calling into the operating system and making it
> susceptible to denial of service.

Having authored several network device drivers, and worked on several
others, I think you have a misunderstanding somewhere.

It is not expected that the MAC will be "alive" while the network
interface is down - it is expected that the MAC will be disabled, and
memory necessary for buffer rings etc will not be claimed.

> That is unless you set up your
> buffer rings/queues/whatever in the ndo_open/ndo_close callbacks.

So yes, that is what is expected - so that when the interface is down,
the memory needed for the buffer rings and packet buffers is given
back to the system for other uses.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 01:16, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 05, 2019 at 01:03:27AM +0300, Vladimir Oltean wrote:
> > On Wed, 5 Jun 2019 at 00:48, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Tue, Jun 04, 2019 at 02:37:31PM -0700, Florian Fainelli wrote:
> > > > The firmware/boot loader transition to a full fledged OS with a switch
> > > > is a tricky one to answer though, and there are no perfect answers
> > > > AFAICT. If your SW is totally hosed, you might want the switch to
> > > > forward traffic between all LAN ports (excluding WAN, otherwise you
> > > > expose your home devices to the outside world, whoops).
> > > >
> > > > If your SW is fully operational, then the questions are:
> > > >
> > > > - do you want a DSA like behavior in your boot loader, in that all ports
> > > > are separated but fully usable or do you want a dumb switch model where
> > > > any port can forward to the CPU/management port, without any tags or
> > > > anything (unmanaged mode)
> > > >
> > > > - what happens during bootloader to OS handover, should the switch be
> > > > held in reset so as to avoid any possible indirect DMA into main memory
> > > > as much as power saving? Should nothing happen and let the OS wipe out
> > > > clean the setting left by the boot loader?
> > > >
> > > > All of these are in the realm of policy and trade offs as far as
> > > > initializing/disruption goes, so there are no hard and fast answers.
> > >
> > > For a switch, there are four stages, not two:
> > >
> > > 1. The out-of-reset state, which from what I've seen seems to be to
> > >    behave like a dumb switch.
> > >
> > > 2. The boot loader state, which is generally the same as the
> > >    out-of-reset state.
> > >
> > > 3. The OS-booting state, which for a DSA switch in Linux isolates each
> > >    port from each other.
> > >
> > > 4. The OS-booted state, which depends on the system configuration.
> > >
> > > If you are setting up a switch in a STP environment, you _have_ to be
> > > aware of all of those states, and plan your network accordingly.
> > > While it's possible to argue that the boot loader should isolate the
> > > ports, it may be that the system gets hosed to the point that the boot
> > > loader is unable to run - then you have a switch operating in a STP
> > > environment acting as a dumb switch.
> > >
> > > The same actually goes for many switches - consider your typical DSL
> > > router integrated with a four port switch.  By default, that switch
> > > forwards traffic between each port.  If you've setup the ports to be
> > > isolated, each time the router is rebooted (e.g., due to a
> > > configuration change) it will forward traffic between all ports until
> > > the routers OS has finished booting and applied the switch
> > > configuration.
> > >
> > > What I'm basically saying is that if you're going to the point of
> > > using such hardware in a STP environment, you _must_ pay attention
> > > to the behaviour of the hardware through all phases of its operation
> > > and consider the consequences should it fail in any of those phases.
> > >
> > > --
> > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > > According to speedtest.net: 11.9Mbps down 500kbps up
> >
> > Hi Russell,
> >
> > The dumb switch was just an example. The absolute same thing (unwanted
> > PHY connection) applies to regular NICs. I am not aware of any setting
> > that makes the MAC ignore frames as long as they observe the
> > appropriate MII spec. And the hardware will go on to process those
> > frames, potentially calling into the operating system and making it
> > susceptible to denial of service.
>
> Having authored several network device drivers, and worked on several
> others, I think you have a misunderstanding somewhere.
>

You caught me.

But even ignoring the NIC case, isn't the PHY state machine
inconsistent with itself? It is ok with callink phy_suspend upon
ndo_stop, but it won't call phy_suspend after phy_connect, when the
netdev is implicitly stopped?

> It is not expected that the MAC will be "alive" while the network
> interface is down - it is expected that the MAC will be disabled, and
> memory necessary for buffer rings etc will not be claimed.
>
> > That is unless you set up your
> > buffer rings/queues/whatever in the ndo_open/ndo_close callbacks.
>
> So yes, that is what is expected - so that when the interface is down,
> the memory needed for the buffer rings and packet buffers is given
> back to the system for other uses.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> You caught me.
> 
> But even ignoring the NIC case, isn't the PHY state machine
> inconsistent with itself? It is ok with callink phy_suspend upon
> ndo_stop, but it won't call phy_suspend after phy_connect, when the
> netdev is implicitly stopped?

The PHY state machine isn't inconsistent with itself, but it does
have strange behaviour.

When the PHY is attached, the PHY is resumed and the state machine
is in PHY_READY state.  If it goes through a start/stop cycle, the
state machine transitions to PHY_HALTED and attempts to place the
PHY into a low power state.  So the PHY state is consistent with
the state machine state (we don't end up in the same state but with
the PHY in a different state.)

What we do have is a difference between the PHY state (and state
machine state) between the boot scenario, and the interface up/down
scenario, the latter behaviour having been introduced by a commit
back in 2013:

    net: phy: suspend phydev when going to HALTED

    When phydev is going to HALTED state, we can try to suspend it to
    safe more power. phy_suspend helper will check if PHY can be suspended,
    so just call it when entering HALTED state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> > You caught me.
> >
> > But even ignoring the NIC case, isn't the PHY state machine
> > inconsistent with itself? It is ok with callink phy_suspend upon
> > ndo_stop, but it won't call phy_suspend after phy_connect, when the
> > netdev is implicitly stopped?
>
> The PHY state machine isn't inconsistent with itself, but it does
> have strange behaviour.
>
> When the PHY is attached, the PHY is resumed and the state machine
> is in PHY_READY state.  If it goes through a start/stop cycle, the
> state machine transitions to PHY_HALTED and attempts to place the
> PHY into a low power state.  So the PHY state is consistent with
> the state machine state (we don't end up in the same state but with
> the PHY in a different state.)
>
> What we do have is a difference between the PHY state (and state
> machine state) between the boot scenario, and the interface up/down
> scenario, the latter behaviour having been introduced by a commit
> back in 2013:
>
>     net: phy: suspend phydev when going to HALTED
>
>     When phydev is going to HALTED state, we can try to suspend it to
>     safe more power. phy_suspend helper will check if PHY can be suspended,
>     so just call it when entering HALTED state.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

I am really not into the PHYLIB internals, but basically what you're
telling me is that running "ip link set dev eth0 down" is a
stronger/more imperative condition than not running "ip link set dev
eth0 up"... Does it also suspend the PHY if I put the interface down
while it was already down?

-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> > > You caught me.
> > >
> > > But even ignoring the NIC case, isn't the PHY state machine
> > > inconsistent with itself? It is ok with callink phy_suspend upon
> > > ndo_stop, but it won't call phy_suspend after phy_connect, when the
> > > netdev is implicitly stopped?
> >
> > The PHY state machine isn't inconsistent with itself, but it does
> > have strange behaviour.
> >
> > When the PHY is attached, the PHY is resumed and the state machine
> > is in PHY_READY state.  If it goes through a start/stop cycle, the
> > state machine transitions to PHY_HALTED and attempts to place the
> > PHY into a low power state.  So the PHY state is consistent with
> > the state machine state (we don't end up in the same state but with
> > the PHY in a different state.)
> >
> > What we do have is a difference between the PHY state (and state
> > machine state) between the boot scenario, and the interface up/down
> > scenario, the latter behaviour having been introduced by a commit
> > back in 2013:
> >
> >     net: phy: suspend phydev when going to HALTED
> >
> >     When phydev is going to HALTED state, we can try to suspend it to
> >     safe more power. phy_suspend helper will check if PHY can be suspended,
> >     so just call it when entering HALTED state.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > According to speedtest.net: 11.9Mbps down 500kbps up
> 
> I am really not into the PHYLIB internals, but basically what you're
> telling me is that running "ip link set dev eth0 down" is a
> stronger/more imperative condition than not running "ip link set dev
> eth0 up"... Does it also suspend the PHY if I put the interface down
> while it was already down?

No - but that has nothing to do with phylib internals, more to do with
the higher levels of networking.  ndo_stop() will not be called unless
ndo_open() has already been called.  In other words, setting an already
down device down via "ip link set dev eth0 down" is a no-op.

So, let's a common scenario.  You power up a board.  The PHY comes up
and establishes a link.  The boot loader runs, loads the kernel, which
then boots.  Your network driver is a module, and hasn't been loaded
yet.  The link is still up.

The modular network driver gets loaded, and initialises.  Userspace
does not bring the network device up, and the network driver does not
attach or connect to the PHY (which is actually quite common).  So,
the link is still up.

The modular PHY driver gets loaded, and binds to the PHY.  The link
is still up.

Userspace configures the network interface, which causes the PHY
device to be attached to the network device, and phy_start() to be
called on it - the negotiation advertisement is configured, and
negotiation restarted if necessary.

When userspace deconfigures the network interface, phy_stop() will
be called, and, as the network driver attached the PHY in its
ndo_open() function, the network driver will detach the PHY from
the network interface to reverse those effects.  The PHY will be
suspended, but more so than that, if there is a reset line, the
reset line will be activated to the PHY.

The above is an illustration of one sequence that /can/ happen.
Other sequences are also possible.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
> > On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> > > > You caught me.
> > > >
> > > > But even ignoring the NIC case, isn't the PHY state machine
> > > > inconsistent with itself? It is ok with callink phy_suspend upon
> > > > ndo_stop, but it won't call phy_suspend after phy_connect, when the
> > > > netdev is implicitly stopped?
> > >
> > > The PHY state machine isn't inconsistent with itself, but it does
> > > have strange behaviour.
> > >
> > > When the PHY is attached, the PHY is resumed and the state machine
> > > is in PHY_READY state.  If it goes through a start/stop cycle, the
> > > state machine transitions to PHY_HALTED and attempts to place the
> > > PHY into a low power state.  So the PHY state is consistent with
> > > the state machine state (we don't end up in the same state but with
> > > the PHY in a different state.)
> > >
> > > What we do have is a difference between the PHY state (and state
> > > machine state) between the boot scenario, and the interface up/down
> > > scenario, the latter behaviour having been introduced by a commit
> > > back in 2013:
> > >
> > >     net: phy: suspend phydev when going to HALTED
> > >
> > >     When phydev is going to HALTED state, we can try to suspend it to
> > >     safe more power. phy_suspend helper will check if PHY can be suspended,
> > >     so just call it when entering HALTED state.
> > >
> > > --
> > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > > According to speedtest.net: 11.9Mbps down 500kbps up
> >
> > I am really not into the PHYLIB internals, but basically what you're
> > telling me is that running "ip link set dev eth0 down" is a
> > stronger/more imperative condition than not running "ip link set dev
> > eth0 up"... Does it also suspend the PHY if I put the interface down
> > while it was already down?
>
> No - but that has nothing to do with phylib internals, more to do with
> the higher levels of networking.  ndo_stop() will not be called unless
> ndo_open() has already been called.  In other words, setting an already
> down device down via "ip link set dev eth0 down" is a no-op.
>
> So, let's a common scenario.  You power up a board.  The PHY comes up
> and establishes a link.  The boot loader runs, loads the kernel, which

This may or may not be the case. As you pointed out a few emails back,
this is a system-level issue that requires a system-level solution -
so cutting the link in U-boot is not out of the question.

> then boots.  Your network driver is a module, and hasn't been loaded
> yet.  The link is still up.
>
> The modular network driver gets loaded, and initialises.  Userspace
> does not bring the network device up, and the network driver does not
> attach or connect to the PHY (which is actually quite common).  So,
> the link is still up.
>
> The modular PHY driver gets loaded, and binds to the PHY.  The link
> is still up.

I would rather say, 'even if the link is not up, Linux brings it up
(possibly prematurely) via phy_resume'.
But let's consider the case where the link *was* up. The general idea
is 'implement your workarounds in whatever other way, that link is
welcome!'.

>
> Userspace configures the network interface, which causes the PHY
> device to be attached to the network device, and phy_start() to be
> called on it - the negotiation advertisement is configured, and
> negotiation restarted if necessary.

In general (the typical driver that isn't bothered by the presence of
an unrequested Ethernet link during initialization), U-boot may
negociate a mode without flow control, and Linux one with. So in
practice, AN is going to restart anyway, which makes the argument for
preserving that link established by the predecessors less strong.

>
> When userspace deconfigures the network interface, phy_stop() will
> be called, and, as the network driver attached the PHY in its
> ndo_open() function, the network driver will detach the PHY from
> the network interface to reverse those effects.  The PHY will be
> suspended, but more so than that, if there is a reset line, the
> reset line will be activated to the PHY.
>
> The above is an illustration of one sequence that /can/ happen.
> Other sequences are also possible.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 02:46:13AM +0300, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
> > > On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> > > > > You caught me.
> > > > >
> > > > > But even ignoring the NIC case, isn't the PHY state machine
> > > > > inconsistent with itself? It is ok with callink phy_suspend upon
> > > > > ndo_stop, but it won't call phy_suspend after phy_connect, when the
> > > > > netdev is implicitly stopped?
> > > >
> > > > The PHY state machine isn't inconsistent with itself, but it does
> > > > have strange behaviour.
> > > >
> > > > When the PHY is attached, the PHY is resumed and the state machine
> > > > is in PHY_READY state.  If it goes through a start/stop cycle, the
> > > > state machine transitions to PHY_HALTED and attempts to place the
> > > > PHY into a low power state.  So the PHY state is consistent with
> > > > the state machine state (we don't end up in the same state but with
> > > > the PHY in a different state.)
> > > >
> > > > What we do have is a difference between the PHY state (and state
> > > > machine state) between the boot scenario, and the interface up/down
> > > > scenario, the latter behaviour having been introduced by a commit
> > > > back in 2013:
> > > >
> > > >     net: phy: suspend phydev when going to HALTED
> > > >
> > > >     When phydev is going to HALTED state, we can try to suspend it to
> > > >     safe more power. phy_suspend helper will check if PHY can be suspended,
> > > >     so just call it when entering HALTED state.
> > > >
> > > > --
> > > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > > > According to speedtest.net: 11.9Mbps down 500kbps up
> > >
> > > I am really not into the PHYLIB internals, but basically what you're
> > > telling me is that running "ip link set dev eth0 down" is a
> > > stronger/more imperative condition than not running "ip link set dev
> > > eth0 up"... Does it also suspend the PHY if I put the interface down
> > > while it was already down?
> >
> > No - but that has nothing to do with phylib internals, more to do with
> > the higher levels of networking.  ndo_stop() will not be called unless
> > ndo_open() has already been called.  In other words, setting an already
> > down device down via "ip link set dev eth0 down" is a no-op.
> >
> > So, let's a common scenario.  You power up a board.  The PHY comes up
> > and establishes a link.  The boot loader runs, loads the kernel, which
> 
> This may or may not be the case. As you pointed out a few emails back,
> this is a system-level issue that requires a system-level solution -
> so cutting the link in U-boot is not out of the question.

"Let's take a common scenario" means "I'm about to describe one example
scenario".  So yes, of course it may or may not be the case, and I'm in
no way contradicting what I've said earlier.  I'm merely giving an
example that seems - in my experience - to be very common.

> > then boots.  Your network driver is a module, and hasn't been loaded
> > yet.  The link is still up.
> >
> > The modular network driver gets loaded, and initialises.  Userspace
> > does not bring the network device up, and the network driver does not
> > attach or connect to the PHY (which is actually quite common).  So,
> > the link is still up.
> >
> > The modular PHY driver gets loaded, and binds to the PHY.  The link
> > is still up.
> 
> I would rather say, 'even if the link is not up, Linux brings it up
> (possibly prematurely) via phy_resume'.
> But let's consider the case where the link *was* up. The general idea
> is 'implement your workarounds in whatever other way, that link is
> welcome!'.
> 
> >
> > Userspace configures the network interface, which causes the PHY
> > device to be attached to the network device, and phy_start() to be
> > called on it - the negotiation advertisement is configured, and
> > negotiation restarted if necessary.
> 
> In general (the typical driver that isn't bothered by the presence of
> an unrequested Ethernet link during initialization), U-boot may
> negociate a mode without flow control, and Linux one with. So in
> practice, AN is going to restart anyway, which makes the argument for
> preserving that link established by the predecessors less strong.
> 
> >
> > When userspace deconfigures the network interface, phy_stop() will
> > be called, and, as the network driver attached the PHY in its
> > ndo_open() function, the network driver will detach the PHY from
> > the network interface to reverse those effects.  The PHY will be
> > suspended, but more so than that, if there is a reset line, the
> > reset line will be activated to the PHY.
> >
> > The above is an illustration of one sequence that /can/ happen.
> > Other sequences are also possible.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > According to speedtest.net: 11.9Mbps down 500kbps up
> 
> Regards,
> -Vladimir
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Florian Fainelli]

On 6/4/2019 4:46 PM, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
>>
>> On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
>>> On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
>>> <linux@armlinux.org.uk> wrote:
>>>>
>>>> On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
>>>>> You caught me.
>>>>>
>>>>> But even ignoring the NIC case, isn't the PHY state machine
>>>>> inconsistent with itself? It is ok with callink phy_suspend upon
>>>>> ndo_stop, but it won't call phy_suspend after phy_connect, when the
>>>>> netdev is implicitly stopped?
>>>>
>>>> The PHY state machine isn't inconsistent with itself, but it does
>>>> have strange behaviour.
>>>>
>>>> When the PHY is attached, the PHY is resumed and the state machine
>>>> is in PHY_READY state.  If it goes through a start/stop cycle, the
>>>> state machine transitions to PHY_HALTED and attempts to place the
>>>> PHY into a low power state.  So the PHY state is consistent with
>>>> the state machine state (we don't end up in the same state but with
>>>> the PHY in a different state.)
>>>>
>>>> What we do have is a difference between the PHY state (and state
>>>> machine state) between the boot scenario, and the interface up/down
>>>> scenario, the latter behaviour having been introduced by a commit
>>>> back in 2013:
>>>>
>>>>     net: phy: suspend phydev when going to HALTED
>>>>
>>>>     When phydev is going to HALTED state, we can try to suspend it to
>>>>     safe more power. phy_suspend helper will check if PHY can be suspended,
>>>>     so just call it when entering HALTED state.
>>>>
>>>> --
>>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>>>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>>>> According to speedtest.net: 11.9Mbps down 500kbps up
>>>
>>> I am really not into the PHYLIB internals, but basically what you're
>>> telling me is that running "ip link set dev eth0 down" is a
>>> stronger/more imperative condition than not running "ip link set dev
>>> eth0 up"... Does it also suspend the PHY if I put the interface down
>>> while it was already down?
>>
>> No - but that has nothing to do with phylib internals, more to do with
>> the higher levels of networking.  ndo_stop() will not be called unless
>> ndo_open() has already been called.  In other words, setting an already
>> down device down via "ip link set dev eth0 down" is a no-op.
>>
>> So, let's a common scenario.  You power up a board.  The PHY comes up
>> and establishes a link.  The boot loader runs, loads the kernel, which
> 
> This may or may not be the case. As you pointed out a few emails back,
> this is a system-level issue that requires a system-level solution -
> so cutting the link in U-boot is not out of the question.
> 
>> then boots.  Your network driver is a module, and hasn't been loaded
>> yet.  The link is still up.
>>
>> The modular network driver gets loaded, and initialises.  Userspace
>> does not bring the network device up, and the network driver does not
>> attach or connect to the PHY (which is actually quite common).  So,
>> the link is still up.
>>
>> The modular PHY driver gets loaded, and binds to the PHY.  The link
>> is still up.
> 
> I would rather say, 'even if the link is not up, Linux brings it up
> (possibly prematurely) via phy_resume'.
> But let's consider the case where the link *was* up. The general idea
> is 'implement your workarounds in whatever other way, that link is
> welcome!'.

With the systems that I work with, we have enforced the following
behavior to happen: the boot loader and kernel only turn on what they
needs, at the time they need it, and nothing more, once done, they put
the blocks back into lowest power mode (clock and power gated if
available). So yes, there are multiple link re-negotiations throughput
the boot process, but when there is no device bound to a driver the
system conserves power by default which is deemed a higher goal than
speed. Your mileage may vary of course.

There is not exactly a simple way of enforcing that kind (or another
kind for that matter) of policy kernel wide, so it's unfortunately up to
the driver writer to propose something that is deemed sensible.

We could however, extend existing tools like iproute2 to offer the
ability to control whether the PHY should be completely powered off, in
a low power state allowing WoL, or remain UP when the network device is
brought down. That would not cover the case that Russell explained, but
it would be another monkey wrench you can throw at the system.
-- 
Florian

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 06:06, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/4/2019 4:46 PM, Vladimir Oltean wrote:
> > On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> >>
> >> On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
> >>> On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
> >>> <linux@armlinux.org.uk> wrote:
> >>>>
> >>>> On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> >>>>> You caught me.
> >>>>>
> >>>>> But even ignoring the NIC case, isn't the PHY state machine
> >>>>> inconsistent with itself? It is ok with callink phy_suspend upon
> >>>>> ndo_stop, but it won't call phy_suspend after phy_connect, when the
> >>>>> netdev is implicitly stopped?
> >>>>
> >>>> The PHY state machine isn't inconsistent with itself, but it does
> >>>> have strange behaviour.
> >>>>
> >>>> When the PHY is attached, the PHY is resumed and the state machine
> >>>> is in PHY_READY state.  If it goes through a start/stop cycle, the
> >>>> state machine transitions to PHY_HALTED and attempts to place the
> >>>> PHY into a low power state.  So the PHY state is consistent with
> >>>> the state machine state (we don't end up in the same state but with
> >>>> the PHY in a different state.)
> >>>>
> >>>> What we do have is a difference between the PHY state (and state
> >>>> machine state) between the boot scenario, and the interface up/down
> >>>> scenario, the latter behaviour having been introduced by a commit
> >>>> back in 2013:
> >>>>
> >>>>     net: phy: suspend phydev when going to HALTED
> >>>>
> >>>>     When phydev is going to HALTED state, we can try to suspend it to
> >>>>     safe more power. phy_suspend helper will check if PHY can be suspended,
> >>>>     so just call it when entering HALTED state.
> >>>>
> >>>> --
> >>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> >>>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> >>>> According to speedtest.net: 11.9Mbps down 500kbps up
> >>>
> >>> I am really not into the PHYLIB internals, but basically what you're
> >>> telling me is that running "ip link set dev eth0 down" is a
> >>> stronger/more imperative condition than not running "ip link set dev
> >>> eth0 up"... Does it also suspend the PHY if I put the interface down
> >>> while it was already down?
> >>
> >> No - but that has nothing to do with phylib internals, more to do with
> >> the higher levels of networking.  ndo_stop() will not be called unless
> >> ndo_open() has already been called.  In other words, setting an already
> >> down device down via "ip link set dev eth0 down" is a no-op.
> >>
> >> So, let's a common scenario.  You power up a board.  The PHY comes up
> >> and establishes a link.  The boot loader runs, loads the kernel, which
> >
> > This may or may not be the case. As you pointed out a few emails back,
> > this is a system-level issue that requires a system-level solution -
> > so cutting the link in U-boot is not out of the question.
> >
> >> then boots.  Your network driver is a module, and hasn't been loaded
> >> yet.  The link is still up.
> >>
> >> The modular network driver gets loaded, and initialises.  Userspace
> >> does not bring the network device up, and the network driver does not
> >> attach or connect to the PHY (which is actually quite common).  So,
> >> the link is still up.
> >>
> >> The modular PHY driver gets loaded, and binds to the PHY.  The link
> >> is still up.
> >
> > I would rather say, 'even if the link is not up, Linux brings it up
> > (possibly prematurely) via phy_resume'.
> > But let's consider the case where the link *was* up. The general idea
> > is 'implement your workarounds in whatever other way, that link is
> > welcome!'.
>
> With the systems that I work with, we have enforced the following
> behavior to happen: the boot loader and kernel only turn on what they
> needs, at the time they need it, and nothing more, once done, they put
> the blocks back into lowest power mode (clock and power gated if
> available). So yes, there are multiple link re-negotiations throughput
> the boot process, but when there is no device bound to a driver the
> system conserves power by default which is deemed a higher goal than
> speed. Your mileage may vary of course.
>
> There is not exactly a simple way of enforcing that kind (or another
> kind for that matter) of policy kernel wide, so it's unfortunately up to
> the driver writer to propose something that is deemed sensible.
>
> We could however, extend existing tools like iproute2 to offer the
> ability to control whether the PHY should be completely powered off, in
> a low power state allowing WoL, or remain UP when the network device is
> brought down. That would not cover the case that Russell explained, but
> it would be another monkey wrench you can throw at the system.
> --
> Florian

Hi Florian,

By going to HALTED on phy_stop, the system already achieves what I am
looking after - although maybe it is an unintended consequence for
you.
I'm only trying to make an argument for removing the phy_resume from
phy_attach_direct now. If there was a link, and it doesn't need
re-negociation, fine, use it in phy_start, but at most leave U-boot to
put that link down and don't force it up prior to the netdev's
ndo_open.

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 11:27:59AM +0300, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 06:06, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> >
> >
> > On 6/4/2019 4:46 PM, Vladimir Oltean wrote:
> > > On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > >>
> > >> On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
> > >>> On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
> > >>> <linux@armlinux.org.uk> wrote:
> > >>>>
> > >>>> On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> > >>>>> You caught me.
> > >>>>>
> > >>>>> But even ignoring the NIC case, isn't the PHY state machine
> > >>>>> inconsistent with itself? It is ok with callink phy_suspend upon
> > >>>>> ndo_stop, but it won't call phy_suspend after phy_connect, when the
> > >>>>> netdev is implicitly stopped?
> > >>>>
> > >>>> The PHY state machine isn't inconsistent with itself, but it does
> > >>>> have strange behaviour.
> > >>>>
> > >>>> When the PHY is attached, the PHY is resumed and the state machine
> > >>>> is in PHY_READY state.  If it goes through a start/stop cycle, the
> > >>>> state machine transitions to PHY_HALTED and attempts to place the
> > >>>> PHY into a low power state.  So the PHY state is consistent with
> > >>>> the state machine state (we don't end up in the same state but with
> > >>>> the PHY in a different state.)
> > >>>>
> > >>>> What we do have is a difference between the PHY state (and state
> > >>>> machine state) between the boot scenario, and the interface up/down
> > >>>> scenario, the latter behaviour having been introduced by a commit
> > >>>> back in 2013:
> > >>>>
> > >>>>     net: phy: suspend phydev when going to HALTED
> > >>>>
> > >>>>     When phydev is going to HALTED state, we can try to suspend it to
> > >>>>     safe more power. phy_suspend helper will check if PHY can be suspended,
> > >>>>     so just call it when entering HALTED state.
> > >>>>
> > >>>> --
> > >>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > >>>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > >>>> According to speedtest.net: 11.9Mbps down 500kbps up
> > >>>
> > >>> I am really not into the PHYLIB internals, but basically what you're
> > >>> telling me is that running "ip link set dev eth0 down" is a
> > >>> stronger/more imperative condition than not running "ip link set dev
> > >>> eth0 up"... Does it also suspend the PHY if I put the interface down
> > >>> while it was already down?
> > >>
> > >> No - but that has nothing to do with phylib internals, more to do with
> > >> the higher levels of networking.  ndo_stop() will not be called unless
> > >> ndo_open() has already been called.  In other words, setting an already
> > >> down device down via "ip link set dev eth0 down" is a no-op.
> > >>
> > >> So, let's a common scenario.  You power up a board.  The PHY comes up
> > >> and establishes a link.  The boot loader runs, loads the kernel, which
> > >
> > > This may or may not be the case. As you pointed out a few emails back,
> > > this is a system-level issue that requires a system-level solution -
> > > so cutting the link in U-boot is not out of the question.
> > >
> > >> then boots.  Your network driver is a module, and hasn't been loaded
> > >> yet.  The link is still up.
> > >>
> > >> The modular network driver gets loaded, and initialises.  Userspace
> > >> does not bring the network device up, and the network driver does not
> > >> attach or connect to the PHY (which is actually quite common).  So,
> > >> the link is still up.
> > >>
> > >> The modular PHY driver gets loaded, and binds to the PHY.  The link
> > >> is still up.
> > >
> > > I would rather say, 'even if the link is not up, Linux brings it up
> > > (possibly prematurely) via phy_resume'.
> > > But let's consider the case where the link *was* up. The general idea
> > > is 'implement your workarounds in whatever other way, that link is
> > > welcome!'.
> >
> > With the systems that I work with, we have enforced the following
> > behavior to happen: the boot loader and kernel only turn on what they
> > needs, at the time they need it, and nothing more, once done, they put
> > the blocks back into lowest power mode (clock and power gated if
> > available). So yes, there are multiple link re-negotiations throughput
> > the boot process, but when there is no device bound to a driver the
> > system conserves power by default which is deemed a higher goal than
> > speed. Your mileage may vary of course.
> >
> > There is not exactly a simple way of enforcing that kind (or another
> > kind for that matter) of policy kernel wide, so it's unfortunately up to
> > the driver writer to propose something that is deemed sensible.
> >
> > We could however, extend existing tools like iproute2 to offer the
> > ability to control whether the PHY should be completely powered off, in
> > a low power state allowing WoL, or remain UP when the network device is
> > brought down. That would not cover the case that Russell explained, but
> > it would be another monkey wrench you can throw at the system.
> > --
> > Florian
> 
> Hi Florian,
> 
> By going to HALTED on phy_stop, the system already achieves what I am
> looking after - although maybe it is an unintended consequence for
> you.
> I'm only trying to make an argument for removing the phy_resume from
> phy_attach_direct now.

Merely doing that will create problems.  You may remember a few emails
ago, we discussed whether the physical PHY state was consistent with
the PHY state machine state.  By making that change, the PHY state
machine vs the physical PHY state becomes inconsistent.

Removing phy_resume() from phy_attach_direct() means that the PHY may
not be in a resumed state at this point, yet we set the PHY state
machine to PHY_READY.  When phy_start() is called, the state will be
transitioned to PHY_UP rather than PHY_RESUMING, and we will try to
update the advertisement into the PHY (as I've previously described.)

If the PHY is powered down, but the state machine transitions from
PHY_READY to PHY_UP state, programming the advertisement will have no
effect, and the PHY will remain in power-down mode.

So, the PHY state machine state needs to also be set according to the
PHY mode.  The same is true for phy_probe(), which also assumes that
the PHY is not powered down.

> If there was a link, and it doesn't need
> re-negociation, fine, use it in phy_start, but at most leave U-boot to
> put that link down and don't force it up prior to the netdev's
> ndo_open.
> 
> Regards,
> -Vladimir
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Vladimir Oltean]

On Wed, 5 Jun 2019 at 12:31, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 05, 2019 at 11:27:59AM +0300, Vladimir Oltean wrote:
> > On Wed, 5 Jun 2019 at 06:06, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > >
> > >
> > >
> > > On 6/4/2019 4:46 PM, Vladimir Oltean wrote:
> > > > On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> wrote:
> > > >>
> > > >> On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote:
> > > >>> On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin
> > > >>> <linux@armlinux.org.uk> wrote:
> > > >>>>
> > > >>>> On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote:
> > > >>>>> You caught me.
> > > >>>>>
> > > >>>>> But even ignoring the NIC case, isn't the PHY state machine
> > > >>>>> inconsistent with itself? It is ok with callink phy_suspend upon
> > > >>>>> ndo_stop, but it won't call phy_suspend after phy_connect, when the
> > > >>>>> netdev is implicitly stopped?
> > > >>>>
> > > >>>> The PHY state machine isn't inconsistent with itself, but it does
> > > >>>> have strange behaviour.
> > > >>>>
> > > >>>> When the PHY is attached, the PHY is resumed and the state machine
> > > >>>> is in PHY_READY state.  If it goes through a start/stop cycle, the
> > > >>>> state machine transitions to PHY_HALTED and attempts to place the
> > > >>>> PHY into a low power state.  So the PHY state is consistent with
> > > >>>> the state machine state (we don't end up in the same state but with
> > > >>>> the PHY in a different state.)
> > > >>>>
> > > >>>> What we do have is a difference between the PHY state (and state
> > > >>>> machine state) between the boot scenario, and the interface up/down
> > > >>>> scenario, the latter behaviour having been introduced by a commit
> > > >>>> back in 2013:
> > > >>>>
> > > >>>>     net: phy: suspend phydev when going to HALTED
> > > >>>>
> > > >>>>     When phydev is going to HALTED state, we can try to suspend it to
> > > >>>>     safe more power. phy_suspend helper will check if PHY can be suspended,
> > > >>>>     so just call it when entering HALTED state.
> > > >>>>
> > > >>>> --
> > > >>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > >>>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > > >>>> According to speedtest.net: 11.9Mbps down 500kbps up
> > > >>>
> > > >>> I am really not into the PHYLIB internals, but basically what you're
> > > >>> telling me is that running "ip link set dev eth0 down" is a
> > > >>> stronger/more imperative condition than not running "ip link set dev
> > > >>> eth0 up"... Does it also suspend the PHY if I put the interface down
> > > >>> while it was already down?
> > > >>
> > > >> No - but that has nothing to do with phylib internals, more to do with
> > > >> the higher levels of networking.  ndo_stop() will not be called unless
> > > >> ndo_open() has already been called.  In other words, setting an already
> > > >> down device down via "ip link set dev eth0 down" is a no-op.
> > > >>
> > > >> So, let's a common scenario.  You power up a board.  The PHY comes up
> > > >> and establishes a link.  The boot loader runs, loads the kernel, which
> > > >
> > > > This may or may not be the case. As you pointed out a few emails back,
> > > > this is a system-level issue that requires a system-level solution -
> > > > so cutting the link in U-boot is not out of the question.
> > > >
> > > >> then boots.  Your network driver is a module, and hasn't been loaded
> > > >> yet.  The link is still up.
> > > >>
> > > >> The modular network driver gets loaded, and initialises.  Userspace
> > > >> does not bring the network device up, and the network driver does not
> > > >> attach or connect to the PHY (which is actually quite common).  So,
> > > >> the link is still up.
> > > >>
> > > >> The modular PHY driver gets loaded, and binds to the PHY.  The link
> > > >> is still up.
> > > >
> > > > I would rather say, 'even if the link is not up, Linux brings it up
> > > > (possibly prematurely) via phy_resume'.
> > > > But let's consider the case where the link *was* up. The general idea
> > > > is 'implement your workarounds in whatever other way, that link is
> > > > welcome!'.
> > >
> > > With the systems that I work with, we have enforced the following
> > > behavior to happen: the boot loader and kernel only turn on what they
> > > needs, at the time they need it, and nothing more, once done, they put
> > > the blocks back into lowest power mode (clock and power gated if
> > > available). So yes, there are multiple link re-negotiations throughput
> > > the boot process, but when there is no device bound to a driver the
> > > system conserves power by default which is deemed a higher goal than
> > > speed. Your mileage may vary of course.
> > >
> > > There is not exactly a simple way of enforcing that kind (or another
> > > kind for that matter) of policy kernel wide, so it's unfortunately up to
> > > the driver writer to propose something that is deemed sensible.
> > >
> > > We could however, extend existing tools like iproute2 to offer the
> > > ability to control whether the PHY should be completely powered off, in
> > > a low power state allowing WoL, or remain UP when the network device is
> > > brought down. That would not cover the case that Russell explained, but
> > > it would be another monkey wrench you can throw at the system.
> > > --
> > > Florian
> >
> > Hi Florian,
> >
> > By going to HALTED on phy_stop, the system already achieves what I am
> > looking after - although maybe it is an unintended consequence for
> > you.
> > I'm only trying to make an argument for removing the phy_resume from
> > phy_attach_direct now.
>
> Merely doing that will create problems.  You may remember a few emails
> ago, we discussed whether the physical PHY state was consistent with
> the PHY state machine state.  By making that change, the PHY state
> machine vs the physical PHY state becomes inconsistent.
>
> Removing phy_resume() from phy_attach_direct() means that the PHY may
> not be in a resumed state at this point, yet we set the PHY state
> machine to PHY_READY.  When phy_start() is called, the state will be
> transitioned to PHY_UP rather than PHY_RESUMING, and we will try to
> update the advertisement into the PHY (as I've previously described.)
>
> If the PHY is powered down, but the state machine transitions from
> PHY_READY to PHY_UP state, programming the advertisement will have no
> effect, and the PHY will remain in power-down mode.
>
> So, the PHY state machine state needs to also be set according to the
> PHY mode.  The same is true for phy_probe(), which also assumes that
> the PHY is not powered down.
>
> > If there was a link, and it doesn't need
> > re-negociation, fine, use it in phy_start, but at most leave U-boot to
> > put that link down and don't force it up prior to the netdev's
> > ndo_open.
> >
> > Regards,
> > -Vladimir
> >
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Thanks for explaining this to me in a neutral way. I need to take some
time to figure whether changing the state machine has any value for
anyone.

Regards,
-Vladimir

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 02:46:13AM +0300, Vladimir Oltean wrote:
> On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > No - but that has nothing to do with phylib internals, more to do with
> > the higher levels of networking.  ndo_stop() will not be called unless
> > ndo_open() has already been called.  In other words, setting an already
> > down device down via "ip link set dev eth0 down" is a no-op.
> >
> > So, let's a common scenario.  You power up a board.  The PHY comes up
> > and establishes a link.  The boot loader runs, loads the kernel, which
> > then boots.  Your network driver is a module, and hasn't been loaded
> > yet.  The link is still up.
> >
> > The modular network driver gets loaded, and initialises.  Userspace
> > does not bring the network device up, and the network driver does not
> > attach or connect to the PHY (which is actually quite common).  So,
> > the link is still up.
> >
> > The modular PHY driver gets loaded, and binds to the PHY.  The link
> > is still up.
> 
> I would rather say, 'even if the link is not up, Linux brings it up
> (possibly prematurely) via phy_resume'.
> But let's consider the case where the link *was* up. The general idea
> is 'implement your workarounds in whatever other way, that link is
> welcome!'.

I think you've missed some of the nuances about my example scenario.

If your MAC driver expects the MII pins to be silent after it probes,
this will not be the case in the scenario that I've given you.  The
PHY won't be silenced here, even with your proposed changes.

> > Userspace configures the network interface, which causes the PHY
> > device to be attached to the network device, and phy_start() to be
> > called on it - the negotiation advertisement is configured, and
> > negotiation restarted if necessary.

This is where your suggested modifications first take effect.

What I'm stating is that if you write your network driver to require
that the PHY link is down after the network driver is probed but before
ndo_open is called, in the above exact scenario, that will not be the
case and your network driver may malfunction.

Having the kernel rely on a certain boot loader behaviour is very bad.

You also have to consider that the previous context to the kernel
booting may _not_ be the boot loader - for example, if the kernel
supports crash dump kexec, then the previous context to the crash
kernel is the kernel which crashed, which may well have established a
link on the network interface.

So, relying on the state of the hardware from the boot loader is a
recipe for a buggy driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 01:16:31PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jun 05, 2019 at 02:46:13AM +0300, Vladimir Oltean wrote:
> > On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > No - but that has nothing to do with phylib internals, more to do with
> > > the higher levels of networking.  ndo_stop() will not be called unless
> > > ndo_open() has already been called.  In other words, setting an already
> > > down device down via "ip link set dev eth0 down" is a no-op.
> > >
> > > So, let's a common scenario.  You power up a board.  The PHY comes up
> > > and establishes a link.  The boot loader runs, loads the kernel, which
> > > then boots.  Your network driver is a module, and hasn't been loaded
> > > yet.  The link is still up.
> > >
> > > The modular network driver gets loaded, and initialises.  Userspace
> > > does not bring the network device up, and the network driver does not
> > > attach or connect to the PHY (which is actually quite common).  So,
> > > the link is still up.
> > >
> > > The modular PHY driver gets loaded, and binds to the PHY.  The link
> > > is still up.
> > 
> > I would rather say, 'even if the link is not up, Linux brings it up
> > (possibly prematurely) via phy_resume'.
> > But let's consider the case where the link *was* up. The general idea
> > is 'implement your workarounds in whatever other way, that link is
> > welcome!'.
> 
> I think you've missed some of the nuances about my example scenario.
> 
> If your MAC driver expects the MII pins to be silent after it probes,
> this will not be the case in the scenario that I've given you.  The
> PHY won't be silenced here, even with your proposed changes.
> 
> > > Userspace configures the network interface, which causes the PHY
> > > device to be attached to the network device, and phy_start() to be
> > > called on it - the negotiation advertisement is configured, and
> > > negotiation restarted if necessary.
> 
> This is where your suggested modifications first take effect.
> 
> What I'm stating is that if you write your network driver to require
> that the PHY link is down after the network driver is probed but before
> ndo_open is called, in the above exact scenario, that will not be the
> case and your network driver may malfunction.
> 
> Having the kernel rely on a certain boot loader behaviour is very bad.
> 
> You also have to consider that the previous context to the kernel
> booting may _not_ be the boot loader - for example, if the kernel
> supports crash dump kexec, then the previous context to the crash
> kernel is the kernel which crashed, which may well have established a
> link on the network interface.
> 
> So, relying on the state of the hardware from the boot loader is a
> recipe for a buggy driver.

There is another reason to avoid having the MAC active while the PHY
is in low power mode.  From 802.3-2015 clause 45.2.1.1.2 Low power
(1.0.11):

"The behavior of the PMA/PMD in transition to and from the low-power
mode is implementation specific and any interface signals should not
be relied upon."

which is different to clause 22.2.4.1.5 Power down:

"During the transition to the power-down state and while in the
power-down state, the PHY shall not generate spurious signals on the
MII or GMII."

Since Clause 45 PHYs are supported by phylib, and can respond to
Clause 22 management frames, this is another reason why the MAC should
ignore any signals it receives from the PHY while the network interface
is down.

Of course, your specific situation, you may have a PHY that is compliant
with 22.2.4.1.5 rather than 45.2.1.1.2, but if you're writing a network
driver, you can't make that assumption, especially if you're going to be
submitting it to mainline.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Florian Fainelli]

On 6/4/2019 12:58 PM, Vladimir Oltean wrote:
> Hi,
> 
> I've been wondering what is the correct approach to cut the Ethernet
> link when the user requests it to be administratively down (aka ip link
> set dev eth0 down).
> Most of the Ethernet drivers simply call phy_stop or the phylink
> equivalent. This leaves an Ethernet link between the PHY and its link
> partner.
> The Freescale gianfar driver (authored by Andy Fleming who also authored
> the phylib) does a phy_disconnect here. It may seem a bit overkill, but
> of the extra things it does, it calls phy_suspend where most PHY drivers
> set the BMCR_PDOWN bit. Only this achieves the intended purpose of also
> cutting the link partner's link on 'ip link set dev eth0 down'.
> What is the general consensus here?
> I see the ability to be able to put the PHY link administratively down a
> desirable feat. If it's left to negotiate/receive traffic etc while the
> MAC driver isn't completely set up and ready, in theory a lot of
> processing can happen outside of the operating system's control.

What would seem sensible from my perspective is the following: upon
ndo_stop() the PHY is brought into the lowest power mode available,
unless the device is used for Wake-on-LAN, in which case, an appropriate
low power mode (e.g.: 10Mbps/half receive only) allowing WoL to function
can be selected.
-- 
Florian

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
> Hi,
> 
> I've been wondering what is the correct approach to cut the Ethernet link
> when the user requests it to be administratively down (aka ip link set dev
> eth0 down).
> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.

The first requirement is that phylink_start() is required to be called
in the ndo_open callback, and phylink_stop() is required to be called
in the ndo_stop callback - this is because when a SFP is used, it has
other effects beyond managing the PHY.

phylink_start() and phylink_stop() call the appropriately phylib
functions, and what happens to the PHY is up to phylib.

Whether a PHY has its link brought down or not is not set in stone:
some network cards, particularly those that do not use phylib, do not
bring the link down when the interface is brought down - doing so,
allows for (eg) a faster boot, and of course bringing an interface
up is faster if the link is already established.

Then there's the question about when the PHY is attached to the
network device.  Some drivers attach the PHY in their probe
function, others attach the PHY in their ndo_open method.  If the
PHY is attached in the ndo_open method, then the PHY must be
detached(disconnected) in the ndo_stop method - basically, ndo_stop
must reverse everything that ndo_open did.

> I see the ability to be able to put the PHY link administratively down a
> desirable feat. If it's left to negotiate/receive traffic etc while the MAC
> driver isn't completely set up and ready, in theory a lot of processing can
> happen outside of the operating system's control.

Normally the PHY receives traffic, and passes it to the MAC which
just ignores the signals it receives from the PHY, so no processing
beyond the PHY receiving the traffic happens.

Ultimately, whether you want the PHY to stay linked or not linked
is, imho, a policy that should be set by the administrator (consider
where a system needs to become available quickly after boot vs a
system where power saving is important.)  We don't, however, have
a facility to specify that policy though.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Florian Fainelli]

On 6/4/2019 2:36 PM, Russell King - ARM Linux admin wrote:
> Normally the PHY receives traffic, and passes it to the MAC which
> just ignores the signals it receives from the PHY, so no processing
> beyond the PHY receiving the traffic happens.
> 
> Ultimately, whether you want the PHY to stay linked or not linked
> is, imho, a policy that should be set by the administrator (consider
> where a system needs to become available quickly after boot vs a
> system where power saving is important.)  We don't, however, have
> a facility to specify that policy though.

Maybe that's what we need, something like:

ip link set dev eth0 phy [on|off|wake]

or whatever we deem appropriate such that people willing to maintain the
PHY on can do that.
-- 
Florian

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Russell King - ARM Linux admin]

On Tue, Jun 04, 2019 at 07:25:46PM -0700, Florian Fainelli wrote:
> On 6/4/2019 2:36 PM, Russell King - ARM Linux admin wrote:
> > Normally the PHY receives traffic, and passes it to the MAC which
> > just ignores the signals it receives from the PHY, so no processing
> > beyond the PHY receiving the traffic happens.
> > 
> > Ultimately, whether you want the PHY to stay linked or not linked
> > is, imho, a policy that should be set by the administrator (consider
> > where a system needs to become available quickly after boot vs a
> > system where power saving is important.)  We don't, however, have
> > a facility to specify that policy though.
> 
> Maybe that's what we need, something like:
> 
> ip link set dev eth0 phy [on|off|wake]
> 
> or whatever we deem appropriate such that people willing to maintain the
> PHY on can do that.

How would that work when the PHY isn't bound to the network device until
the network device is brought up?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

[Florian Fainelli]

On 6/5/2019 1:45 AM, Russell King - ARM Linux admin wrote:
> On Tue, Jun 04, 2019 at 07:25:46PM -0700, Florian Fainelli wrote:
>> On 6/4/2019 2:36 PM, Russell King - ARM Linux admin wrote:
>>> Normally the PHY receives traffic, and passes it to the MAC which
>>> just ignores the signals it receives from the PHY, so no processing
>>> beyond the PHY receiving the traffic happens.
>>>
>>> Ultimately, whether you want the PHY to stay linked or not linked
>>> is, imho, a policy that should be set by the administrator (consider
>>> where a system needs to become available quickly after boot vs a
>>> system where power saving is important.)  We don't, however, have
>>> a facility to specify that policy though.
>>
>> Maybe that's what we need, something like:
>>
>> ip link set dev eth0 phy [on|off|wake]
>>
>> or whatever we deem appropriate such that people willing to maintain the
>> PHY on can do that.
> 
> How would that work when the PHY isn't bound to the network device until
> the network device is brought up?

There was supposed to be a down somewhere, something like:

ip link set dev eth0 down phy [on|off|wake]

such that we have been guaranteed to be connected to the PHY at some
point. Maybe this is not such a great idea after all, since not all
drivers would be able to support the optional "phy" argument.
-- 
Florian