Date:   Fri, 20 May 2022 15:08:32 -0700
From:   Saeed Mahameed <saeedm@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux@...linux.org.uk, olteanv@...il.com, hkallweit1@...il.com,
        f.fainelli@...il.com, michael.chan@...adcom.com
Subject: Re: [RFC net-next] net: track locally triggered link loss

On 20 May 20:48, Andrew Lunn wrote:
>> I was looking at bnxt because it's relatively standard for DC NICs and
>> doesn't have 10M lines of code.. then again I could be misinterpreting
>> the code, I haven't tested this theory:
>>
>> In bnxt_set_pauseparam() for example the driver will send a request to
>> the FW which will result in the link coming down and back up with
>> different settings (e.g. when pause autoneg was changed). Since the
>> driver doesn't call netif_carrier_off() explicitly as part of sending
>> the FW message but the link down gets reported thru the usual interrupt
>> (as if someone yanked the cable out) - we need to wrap the FW call with
>> the __LINK_STATE_NOCARRIER_LOCAL
>
>I'm not sure this is a good example. If the PHY is doing an autoneg,
>the link really is down for around a second. The link peer will also
>so the link go down and come back up. So this seems like a legitimate
>time to set the carrier off and then back on again.
>
+1

also looks very racy, what happens if a real phy link event happens in
between carrier_change_start() and carrier_change_end() ?

I think you shouldn't treat configuration flows where the driver actually
toggles the phy link as a special case, they should be counted as a real
link flap.. because they are.

It's impossible from the driver level to know if a FW link event is
due to configuration causes or external forces !

the new 3 APIs are going to be a heavy burden on drivers to maintain. if
you agree with the above and treat all phy link events as one, then we end
up with one new API drivers has to maintain "net_if_carrier_admin_off()"
which is manageable.

But what about SW netdevices, should all of them change to use the "admin"
version ?   

We should keep current carrier logic as is and add new state/counter
to count real phy link state.

netif_phy_link_down(netdev) {
    set_bit(__LINK_STATE_NOPHYLINK, &dev->state);
    atomic_inc(netdev->phy_link_down);
    netif_carrier_off(ndetdev);
}

netif_phy_link_up(netdev) {...}

such API should be maintained by real HW device drivers. 

>> > The driver has a few netif_carrier_off() calls changed to
>> > netif_carrier_admin_off(). It is then unclear looking at the code
>> > which of the calls to netif_carrier_on() match the off.
>>
>> Right, for bnxt again the carrier_off in bnxt_tx_disable() would become
>> an admin_carrier_off, since it's basically part of closing the netdev.
>
>> > Maybe include a driver which makes use of phylib, which should be
>> > doing control of the carrier based on the actual link status.
>>
>> For phylib I was thinking of modifying phy_stop()... but I can't
>> grep out where carrier_off gets called. I'll take a closer look.
>
>If the driver is calling phy_stop() the link will go down. So again, i
>would say setting the carrier off is correct. If the driver calls
>phy_start() an auto neg is likely to happen and 1 second later the
>link will come up.
>
>Maybe i'm not understanding what you are trying to count here. If the
>MAC driver needs to stop the MAC in order to reallocate buffers with
>different MTU, or more rings etc, then i can understand not wanting to
>count that as a carrier off, because the carrier does not actually go
>off. But if it is in fact marking the carrier off, it sounds like a
>MAC driver bug, or a firmware bug.
>
>    Andrew

Powered by blists - more mailing lists