Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
@ 2019-06-24  8:34 Rasmus Villemoes
  2019-06-24 17:26 ` Willem de Bruijn
  0 siblings, 1 reply; 7+ messages in thread
From: Rasmus Villemoes @ 2019-06-24  8:34 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller
  Cc: Rasmus Villemoes, linux-can, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
  2019-06-24  8:34 [PATCH net-next] can: dev: call netif_carrier_off() in register_candev() Rasmus Villemoes
@ 2019-06-24 17:26 ` Willem de Bruijn
  2019-06-26  9:31   ` Rasmus Villemoes
  0 siblings, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2019-06-24 17:26 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	Rasmus Villemoes, linux-can, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
  2019-06-24 17:26 ` Willem de Bruijn
@ 2019-06-26  9:31   ` Rasmus Villemoes
  2019-06-26 14:17     ` Willem de Bruijn
  2019-06-26 16:03     ` David Miller
  0 siblings, 2 replies; 7+ messages in thread
From: Rasmus Villemoes @ 2019-06-26  9:31 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	Rasmus Villemoes, linux-can, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
  2019-06-26  9:31   ` Rasmus Villemoes
@ 2019-06-26 14:17     ` Willem de Bruijn
  2019-06-26 21:19       ` Rasmus Villemoes
  2019-06-26 16:03     ` David Miller
  1 sibling, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2019-06-26 14:17 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	Rasmus Villemoes, linux-can, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
  2019-06-26  9:31   ` Rasmus Villemoes
  2019-06-26 14:17     ` Willem de Bruijn
@ 2019-06-26 16:03     ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2019-06-26 16:03 UTC (permalink / raw)
  To: rasmus.villemoes
  Cc: willemdebruijn.kernel, wg, mkl, Rasmus.Villemoes, linux-can,
	netdev, linux-kernel

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
  2019-06-26 14:17     ` Willem de Bruijn
@ 2019-06-26 21:19       ` Rasmus Villemoes
  2019-06-27  1:56         ` Willem de Bruijn
  0 siblings, 1 reply; 7+ messages in thread
From: Rasmus Villemoes @ 2019-06-26 21:19 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	linux-can, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] can: dev: call netif_carrier_off() in register_candev()
  2019-06-26 21:19       ` Rasmus Villemoes
@ 2019-06-27  1:56         ` Willem de Bruijn
  0 siblings, 0 replies; 7+ messages in thread
From: Willem de Bruijn @ 2019-06-27  1:56 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	linux-can, netdev, linux-kernel

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-24  8:34 [PATCH net-next] can: dev: call netif_carrier_off() in register_candev() Rasmus Villemoes
2019-06-24 17:26 ` Willem de Bruijn
2019-06-26  9:31   ` Rasmus Villemoes
2019-06-26 14:17     ` Willem de Bruijn
2019-06-26 21:19       ` Rasmus Villemoes
2019-06-27  1:56         ` Willem de Bruijn
2019-06-26 16:03     ` David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox