[PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[Rasmus Villemoes]

CONFIG_CAN_LEDS is deprecated. When trying to use the generic netdev
trigger as suggested, there's a small inconsistency with the link
property: The LED is on initially, stays on when the device is brought
up, and then turns off (as expected) when the device is brought down.

Make sure the LED always reflects the state of the CAN device.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/can/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c05e4d50d43d..fad27ace6248 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -1260,6 +1260,7 @@ int register_candev(struct net_device *dev)
 		return -EINVAL;
 
 	dev->rtnl_link_ops = &can_link_ops;
+	netif_carrier_off(dev);
 	return register_netdev(dev);
 }
 EXPORT_SYMBOL_GPL(register_candev);
-- 
2.20.1

Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[Willem de Bruijn]

On Mon, Jun 24, 2019 at 4:34 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> CONFIG_CAN_LEDS is deprecated. When trying to use the generic netdev
> trigger as suggested, there's a small inconsistency with the link
> property: The LED is on initially, stays on when the device is brought
> up, and then turns off (as expected) when the device is brought down.
>
> Make sure the LED always reflects the state of the CAN device.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Should this target net? Regardless of CONFIG_CAN_LEDS deprecation,
this is already not initialized properly if that CONFIG is disabled
and a can_led_event call at device probe is a noop.

Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[Rasmus Villemoes]

On 24/06/2019 19.26, Willem de Bruijn wrote:
> On Mon, Jun 24, 2019 at 4:34 AM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> CONFIG_CAN_LEDS is deprecated. When trying to use the generic netdev
>> trigger as suggested, there's a small inconsistency with the link
>> property: The LED is on initially, stays on when the device is brought
>> up, and then turns off (as expected) when the device is brought down.
>>
>> Make sure the LED always reflects the state of the CAN device.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 
> Should this target net?

No, I think this should go through the CAN tree. Perhaps I've
misunderstood when to use the net-next prefix - is that only for things
that should be applied directly to the net-next tree? If so, sorry.

> Regardless of CONFIG_CAN_LEDS deprecation,
> this is already not initialized properly if that CONFIG is disabled
> and a can_led_event call at device probe is a noop.

I'm not sure I understand this part. The CONFIG_CAN_LEDS support for
showing the state of the interface is implemented via hooking into the
ndo_open/ndo_stop callbacks, and does not look at or touch the
__LINK_STATE_NOCARRIER bit at all.

Other than via the netdev LED trigger I don't think one can even observe
the slightly odd initial state of the __LINK_STATE_NOCARRIER bit for CAN
devices, which is why I framed this as a fix purely to allow the netdev
trigger to be a closer drop-in replacement for CONFIG_CAN_LEDS.

Rasmus

Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[Willem de Bruijn]

On Wed, Jun 26, 2019 at 5:31 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 24/06/2019 19.26, Willem de Bruijn wrote:
> > On Mon, Jun 24, 2019 at 4:34 AM Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> CONFIG_CAN_LEDS is deprecated. When trying to use the generic netdev
> >> trigger as suggested, there's a small inconsistency with the link
> >> property: The LED is on initially, stays on when the device is brought
> >> up, and then turns off (as expected) when the device is brought down.
> >>
> >> Make sure the LED always reflects the state of the CAN device.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >
> > Should this target net?
>
> No, I think this should go through the CAN tree. Perhaps I've
> misunderstood when to use the net-next prefix - is that only for things
> that should be applied directly to the net-next tree? If so, sorry.

I don't see consistent behavior on the list, so this is probably fine.
It would probably help to target can (for fixes) or can-next (for new
features).

Let me reframe the question: should this target can, instead of can-next?

> > Regardless of CONFIG_CAN_LEDS deprecation,
> > this is already not initialized properly if that CONFIG is disabled
> > and a can_led_event call at device probe is a noop.
>
> I'm not sure I understand this part. The CONFIG_CAN_LEDS support for
> showing the state of the interface is implemented via hooking into the
> ndo_open/ndo_stop callbacks, and does not look at or touch the
> __LINK_STATE_NOCARRIER bit at all.
>
> Other than via the netdev LED trigger I don't think one can even observe
> the slightly odd initial state of the __LINK_STATE_NOCARRIER bit for CAN
> devices,

it's still incorrect, though I guess that's moot in practice.

> which is why I framed this as a fix purely to allow the netdev
> trigger to be a closer drop-in replacement for CONFIG_CAN_LEDS.

So the entire CONFIG_CAN_LEDS code is to be removed? What exactly is
this netdev trigger replacement, if not can_led_event? Sorry, I
probably miss some context.

Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[David Miller]

From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Date: Wed, 26 Jun 2019 09:31:39 +0000

> Perhaps I've misunderstood when to use the net-next prefix - is that
> only for things that should be applied directly to the net-next
> tree?

Yes, it is.

Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[Rasmus Villemoes]

On 26/06/2019 16.17, Willem de Bruijn wrote:
> On Wed, Jun 26, 2019 at 5:31 AM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 24/06/2019 19.26, Willem de Bruijn wrote:
>>> On Mon, Jun 24, 2019 at 4:34 AM Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
>>>> Make sure the LED always reflects the state of the CAN device.
>>>
>>> Should this target net?
>>
>> No, I think this should go through the CAN tree. Perhaps I've
>> misunderstood when to use the net-next prefix - is that only for things
>> that should be applied directly to the net-next tree? If so, sorry.
> 
> I don't see consistent behavior on the list, so this is probably fine.
> It would probably help to target can (for fixes) or can-next (for new
> features).
> 
> Let me reframe the question: should this target can, instead of can-next?

Ah, now I see what you meant, but at least I learned when to use
net/net-next.

I think can-next is fine, especially this late in the rc cycle. But I'll
leave it to the CAN maintainer(s).

>>> Regardless of CONFIG_CAN_LEDS deprecation,
>>> this is already not initialized properly if that CONFIG is disabled
>>> and a can_led_event call at device probe is a noop.
>>
>> I'm not sure I understand this part. The CONFIG_CAN_LEDS support for
>> showing the state of the interface is implemented via hooking into the
>> ndo_open/ndo_stop callbacks, and does not look at or touch the
>> __LINK_STATE_NOCARRIER bit at all.
>>
>> Other than via the netdev LED trigger I don't think one can even observe
>> the slightly odd initial state of the __LINK_STATE_NOCARRIER bit for CAN
>> devices,
> 
> it's still incorrect, though I guess that's moot in practice.
Exactly.

>> which is why I framed this as a fix purely to allow the netdev
>> trigger to be a closer drop-in replacement for CONFIG_CAN_LEDS.
> 
> So the entire CONFIG_CAN_LEDS code is to be removed? What exactly is
> this netdev trigger replacement, if not can_led_event? Sorry, I
> probably miss some context.

drivers/net/can/Kconfig contains these comments

        # The netdev trigger (LEDS_TRIGGER_NETDEV) should be able to do
        # everything that this driver is doing. This is marked as broken
        # because it uses stuff that is intended to be changed or removed.
        # Please consider switching to the netdev trigger and confirm it
        # fulfills your needs instead of fixing this driver.

introduced by the commit 30f3b42147ba6 which also marked CONFIG_CAN_LEDS
as (depends on) BROKEN. So while a .dts for using the CAN led trigger
might be

                cana {
                        label = "cana:green:activity";
                        gpios = <&gpio0 10 0>;
                        default-state = "off";
                        linux,default-trigger = "can0-rxtx";
                };

one can achieve mostly the same thing with CAN_LEDS=n,
LEDS_TRIGGER_NETDEV=y setting linux,default-trigger = "netdev" plus a
small init script (or udev rule or whatever works) that does

d=/sys/class/leds/cana:green:activity
echo can0 > $d/device_name
echo 1 > $d/link
echo 1 > $d/rx
echo 1 > $d/tx

to tie the cana LED to the can0 device, plus configure it to show "link"
state as well as blink on rx and tx.

This works just fine, except for the initial state of the LED. AFAIU,
the netdev trigger doesn't need cooperation from each device driver
since it simply works of a timer that periodically checks for changes in
dev_get_stats().

Rasmus

Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()

[Willem de Bruijn]

On Wed, Jun 26, 2019 at 5:19 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 26/06/2019 16.17, Willem de Bruijn wrote:
> > On Wed, Jun 26, 2019 at 5:31 AM Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> On 24/06/2019 19.26, Willem de Bruijn wrote:
> >>> On Mon, Jun 24, 2019 at 4:34 AM Rasmus Villemoes
> >>> <rasmus.villemoes@prevas.dk> wrote:
> >>>>
> >>>> Make sure the LED always reflects the state of the CAN device.
> >>>
> >>> Should this target net?
> >>
> >> No, I think this should go through the CAN tree. Perhaps I've
> >> misunderstood when to use the net-next prefix - is that only for things
> >> that should be applied directly to the net-next tree? If so, sorry.
> >
> > I don't see consistent behavior on the list, so this is probably fine.
> > It would probably help to target can (for fixes) or can-next (for new
> > features).
> >
> > Let me reframe the question: should this target can, instead of can-next?
>
> Ah, now I see what you meant, but at least I learned when to use
> net/net-next.
>
> I think can-next is fine, especially this late in the rc cycle. But I'll
> leave it to the CAN maintainer(s).
>
> >>> Regardless of CONFIG_CAN_LEDS deprecation,
> >>> this is already not initialized properly if that CONFIG is disabled
> >>> and a can_led_event call at device probe is a noop.
> >>
> >> I'm not sure I understand this part. The CONFIG_CAN_LEDS support for
> >> showing the state of the interface is implemented via hooking into the
> >> ndo_open/ndo_stop callbacks, and does not look at or touch the
> >> __LINK_STATE_NOCARRIER bit at all.
> >>
> >> Other than via the netdev LED trigger I don't think one can even observe
> >> the slightly odd initial state of the __LINK_STATE_NOCARRIER bit for CAN
> >> devices,
> >
> > it's still incorrect, though I guess that's moot in practice.
> Exactly.
>
> >> which is why I framed this as a fix purely to allow the netdev
> >> trigger to be a closer drop-in replacement for CONFIG_CAN_LEDS.
> >
> > So the entire CONFIG_CAN_LEDS code is to be removed? What exactly is
> > this netdev trigger replacement, if not can_led_event? Sorry, I
> > probably miss some context.
>
> drivers/net/can/Kconfig contains these comments
>
>         # The netdev trigger (LEDS_TRIGGER_NETDEV) should be able to do
>         # everything that this driver is doing. This is marked as broken
>         # because it uses stuff that is intended to be changed or removed.
>         # Please consider switching to the netdev trigger and confirm it
>         # fulfills your needs instead of fixing this driver.
>
> introduced by the commit 30f3b42147ba6 which also marked CONFIG_CAN_LEDS
> as (depends on) BROKEN. So while a .dts for using the CAN led trigger
> might be
>
>                 cana {
>                         label = "cana:green:activity";
>                         gpios = <&gpio0 10 0>;
>                         default-state = "off";
>                         linux,default-trigger = "can0-rxtx";
>                 };
>
> one can achieve mostly the same thing with CAN_LEDS=n,
> LEDS_TRIGGER_NETDEV=y setting linux,default-trigger = "netdev" plus a
> small init script (or udev rule or whatever works) that does
>
> d=/sys/class/leds/cana:green:activity
> echo can0 > $d/device_name
> echo 1 > $d/link
> echo 1 > $d/rx
> echo 1 > $d/tx
>
> to tie the cana LED to the can0 device, plus configure it to show "link"
> state as well as blink on rx and tx.
>
> This works just fine, except for the initial state of the LED. AFAIU,
> the netdev trigger doesn't need cooperation from each device driver
> since it simply works of a timer that periodically checks for changes in
> dev_get_stats().

Thanks, I had to read up on that code. Makes sense.

Acked-by: Willem de Bruijn <willemb@google.com>