[PATCH] leds: triggers: netdev: add a check, whether device is up

[PATCH] leds: triggers: netdev: add a check, whether device is up

[Klaus Kudielka]

Some net devices do not report NO-CARRIER, if they haven't been brought
up. In that case, the netdev trigger results in a wrong link state being
displayed. Fix this, by adding a check, whether the device is up.

Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index e358e77e4b..bd5e21d0f0 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -195,7 +195,8 @@ static void get_device_state(struct led_netdev_data *trigger_data)
 {
 	struct ethtool_link_ksettings cmd;
 
-	trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
+	trigger_data->carrier_link_up = netif_running(trigger_data->net_dev) &&
+		netif_carrier_ok(trigger_data->net_dev);
 	if (!trigger_data->carrier_link_up)
 		return;
 
-- 
2.42.0

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Andrew Lunn]

On Sat, Nov 04, 2023 at 01:58:40PM +0100, Klaus Kudielka wrote:
> Some net devices do not report NO-CARRIER, if they haven't been brought
> up.

Hi Klaus

We should probably fix the driver. What device is it?

> In that case, the netdev trigger results in a wrong link state being
> displayed. Fix this, by adding a check, whether the device is up.

Is it wrong? Maybe the carrier really is up, even if the interface is
admin down. Broadcast packets are being received by the
hardware. Maybe there is a BMC sharing the link and it is active?

It is not a clear cut wrong to me. And its a way to find broken
drivers. We might want to discuss this some more.

	Andrew

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Klaus Kudielka]

On Sat, 2023-11-04 at 15:29 +0100, Andrew Lunn wrote:
> On Sat, Nov 04, 2023 at 01:58:40PM +0100, Klaus Kudielka wrote:
> > Some net devices do not report NO-CARRIER, if they haven't been brought
> > up.
> 
> Hi Klaus
> 
> We should probably fix the driver. What device is it?
> 
> > In that case, the netdev trigger results in a wrong link state being
> > displayed. Fix this, by adding a check, whether the device is up.
> 
> Is it wrong? Maybe the carrier really is up, even if the interface is
> admin down. Broadcast packets are being received by the
> hardware. Maybe there is a BMC sharing the link and it is active?

My particular example is Turris Omnia, eth2 (WAN), connected to SFP.
SFP module is inserted, but no fiber connected, so definitely no carrier. 

*Without* the patch:

After booting, the device is down, but netdev trigger reports "link" active.
This looks wrong to me.

I can then "ip link set eth2 up", and the "link" goes away - as I
would have expected it to be from the beginning.

> It is not a clear cut wrong to me. And its a way to find broken
> drivers. We might want to discuss this some more.

Maybe an initialization issue.
Just a guess, I'm really not an expert here:

phylink_start() is the first one that does netif_carrier_off() and thus
sets the NOCARRIER bit, but that only happens when bringing the device up.

Before that, I would not know who cares about setting the NOCARRIER bit.


Anyway, it's only a cosmetic issue, but it has been bugging me for too
long :)


Best regards, Klaus

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Klaus Kudielka]

On Sat, 2023-11-04 at 16:27 +0100, Klaus Kudielka wrote:
> 
> phylink_start() is the first one that does netif_carrier_off() and thus
> sets the NOCARRIER bit, but that only happens when bringing the device up.
> 
> Before that, I would not know who cares about setting the NOCARRIER bit.

A different, driver-specific solution could be like this (tested and working):

--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5690,6 +5690,7 @@ static int mvneta_probe(struct platform_device *pdev)
        /* 9676 == 9700 - 20 and rounding to 8 */
        dev->max_mtu = 9676;
 
+       netif_carrier_off(dev);
        err = register_netdev(dev);
        if (err < 0) {
                dev_err(&pdev->dev, "failed to register\n");


Would that be the "correct" approach?

Regards, Klaus

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Andrew Lunn]

[Changes the Cc: list. Dropping LED people, adding a few netdev
people]

On Sat, Nov 04, 2023 at 04:27:45PM +0100, Klaus Kudielka wrote:
> On Sat, 2023-11-04 at 15:29 +0100, Andrew Lunn wrote:
> > On Sat, Nov 04, 2023 at 01:58:40PM +0100, Klaus Kudielka wrote:
> > > Some net devices do not report NO-CARRIER, if they haven't been brought
> > > up.
> > 
> > Hi Klaus
> > 
> > We should probably fix the driver. What device is it?
> > 
> > > In that case, the netdev trigger results in a wrong link state being
> > > displayed. Fix this, by adding a check, whether the device is up.
> > 
> > Is it wrong? Maybe the carrier really is up, even if the interface is
> > admin down. Broadcast packets are being received by the
> > hardware. Maybe there is a BMC sharing the link and it is active?
> 
> My particular example is Turris Omnia, eth2 (WAN), connected to SFP.
> SFP module is inserted, but no fiber connected, so definitely no carrier. 
> 
> *Without* the patch:
> 
> After booting, the device is down, but netdev trigger reports "link" active.
> This looks wrong to me.
> 
> I can then "ip link set eth2 up", and the "link" goes away - as I
> would have expected it to be from the beginning.

Thanks for the details.

A brain dump...

You do see a lot of MAC drivers doing a netif_carrier_off() in there
probe function. That suggests the carrier is on by default. I doubt we
can change that, we would break all the drivers which assume the
carrier is on by default, probably virtual devices and some real
devices.

Often the MAC and PHY are connected in the open() callback, when using
phylib. So that is too late.  phylink_create() is however mostly used
in the probe function. So it could set the carrier to off by default.
Russell, what do you think?

Turris Omnia uses mvneta. That does not turn the carrier off in its
probe function. So a quick fix would be to add it. However, that then
becomes pointless if we make phylink_create() disable the carrier.

	Andrew

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Andrew Lunn]

On Sat, Nov 04, 2023 at 05:32:19PM +0100, Klaus Kudielka wrote:
> On Sat, 2023-11-04 at 16:27 +0100, Klaus Kudielka wrote:
> > 
> > phylink_start() is the first one that does netif_carrier_off() and thus
> > sets the NOCARRIER bit, but that only happens when bringing the device up.
> > 
> > Before that, I would not know who cares about setting the NOCARRIER bit.
> 
> A different, driver-specific solution could be like this (tested and working):
> 
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5690,6 +5690,7 @@ static int mvneta_probe(struct platform_device *pdev)
>         /* 9676 == 9700 - 20 and rounding to 8 */
>         dev->max_mtu = 9676;
>  
> +       netif_carrier_off(dev);
>         err = register_netdev(dev);
>         if (err < 0) {
>                 dev_err(&pdev->dev, "failed to register\n");
> 
> 
> Would that be the "correct" approach?

Crossing emails.

Its a better approach. But it fixes just one driver. If we can do this
in phylink_create(), we fix it in a lot of drivers with a single
change...

	Andrew

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Russell King (Oracle)]

On Sat, Nov 04, 2023 at 05:41:54PM +0100, Andrew Lunn wrote:
> [Changes the Cc: list. Dropping LED people, adding a few netdev
> people]
> 
> On Sat, Nov 04, 2023 at 04:27:45PM +0100, Klaus Kudielka wrote:
> > After booting, the device is down, but netdev trigger reports "link" active.
> > This looks wrong to me.
> > 
> > I can then "ip link set eth2 up", and the "link" goes away - as I
> > would have expected it to be from the beginning.
> 
> Thanks for the details.
> 
> A brain dump...
> 
> You do see a lot of MAC drivers doing a netif_carrier_off() in there
> probe function. That suggests the carrier is on by default. I doubt we
> can change that, we would break all the drivers which assume the
> carrier is on by default, probably virtual devices and some real
> devices.

Note that one of the things that phylink will do is call
netif_carrier_off() when phylink_start() is called to ensure that the
netdev state matches its internal state.

> Often the MAC and PHY are connected in the open() callback, when using
> phylib. So that is too late.  phylink_create() is however mostly used
> in the probe function. So it could set the carrier to off by default.
> Russell, what do you think?

I was going to ask whether that would be a good idea - since it means
that the carrier is in the right state at probe time as well as after
phylink_start() has been called.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Russell King (Oracle)]

On Sat, Nov 04, 2023 at 05:46:44PM +0100, Andrew Lunn wrote:
> On Sat, Nov 04, 2023 at 05:32:19PM +0100, Klaus Kudielka wrote:
> > On Sat, 2023-11-04 at 16:27 +0100, Klaus Kudielka wrote:
> > > 
> > > phylink_start() is the first one that does netif_carrier_off() and thus
> > > sets the NOCARRIER bit, but that only happens when bringing the device up.
> > > 
> > > Before that, I would not know who cares about setting the NOCARRIER bit.
> > 
> > A different, driver-specific solution could be like this (tested and working):
> > 
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -5690,6 +5690,7 @@ static int mvneta_probe(struct platform_device *pdev)
> >         /* 9676 == 9700 - 20 and rounding to 8 */
> >         dev->max_mtu = 9676;
> >  
> > +       netif_carrier_off(dev);
> >         err = register_netdev(dev);
> >         if (err < 0) {
> >                 dev_err(&pdev->dev, "failed to register\n");
> > 
> > 
> > Would that be the "correct" approach?
> 
> Crossing emails.
> 
> Its a better approach. But it fixes just one driver. If we can do this
> in phylink_create(), we fix it in a lot of drivers with a single
> change...

... and I think we should.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Klaus Kudielka]

On Sat, 2023-11-04 at 17:42 +0000, Russell King (Oracle) wrote:
> On Sat, Nov 04, 2023 at 05:46:44PM +0100, Andrew Lunn wrote:
> > On Sat, Nov 04, 2023 at 05:32:19PM +0100, Klaus Kudielka wrote:
> > > On Sat, 2023-11-04 at 16:27 +0100, Klaus Kudielka wrote:
> > > > 
> > > > phylink_start() is the first one that does netif_carrier_off() and thus
> > > > sets the NOCARRIER bit, but that only happens when bringing the device up.
> > > > 
> > > > Before that, I would not know who cares about setting the NOCARRIER bit.
> > > 
> > > A different, driver-specific solution could be like this (tested and working):
> > > 
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -5690,6 +5690,7 @@ static int mvneta_probe(struct platform_device *pdev)
> > >         /* 9676 == 9700 - 20 and rounding to 8 */
> > >         dev->max_mtu = 9676;
> > >  
> > > +       netif_carrier_off(dev);
> > >         err = register_netdev(dev);
> > >         if (err < 0) {
> > >                 dev_err(&pdev->dev, "failed to register\n");
> > > 
> > > 
> > > Would that be the "correct" approach?
> > 
> > Crossing emails.
> > 
> > Its a better approach. But it fixes just one driver. If we can do this
> > in phylink_create(), we fix it in a lot of drivers with a single
> > change...
> 
> ... and I think we should.
> 

So, the patch below also results in correct behaviour of the netdev trigger
with eth2 down, but I'm not so sure whether it really covers all desired
cases. Any advice?

--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1616,6 +1616,7 @@ struct phylink *phylink_create(struct phylink_config *config,
        pl->config = config;
        if (config->type == PHYLINK_NETDEV) {
                pl->netdev = to_net_dev(config->dev);
+               netif_carrier_off(pl->netdev);
        } else if (config->type == PHYLINK_DEV) {
                pl->dev = config->dev;
        } else {


Best regards, Klaus