Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri <fabio.baltieri@gmail.com>]

Hello Wolfgang,

On Wed, Apr 11, 2012 at 4:45 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> this is a try to add generic tx/rx LED triggers for canbus interfaces, somthing
>> I think is very useful in embedded systems where canbus devices often have
>> status leds associated with them, maybe on the bus connector itself, like
>
> Please define "often"? So far only a few CAN devices need LED control by
> software. Actually, I only remember the PEAK PCMCIA card.

I'm actually referring to SoC-based boards with led-class supported
LEDs, which can be linked to kernel events (like
wifi-association/activity etc...).

>> ethernet interfaces. I saw a couple of hardware implementation to drive status
>> leds from tx/rx lines but these were not as effective as software ones.
>
> Why? LEDs are ususally controlled by software only if the hardware does
> not do it (properly?). But your LED support cannot be compared with the
> LEDs on the CAN devices itself.

I agree for "smart" CAN interfaces - like USB ones. What I had in mind
are embedded canbus controllers, which does not provide any activity
indication signal (as ethernet PHYs does). In that case I saw some
hardware implementation which try to drive LEDs from TX/RX signals of
the single-ended bus between controller and transceiver, and in that
case LEDs barely turns on for single packets and you can't have
individual tx/rx indication as the RX driver in the transceiver is
always active.

>> The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
>> of inspiration and some code from it.
>>
>> In this case, however, tx and rx events are trapped from the af_can source file
>> as there are no generic tx/rx functions in can/dev.c. This also required an
>> additional counter to discard rx events for looped frames.
>
> Well, controlling the LEDs from the protocol level seems wrong to me. If
> at all, it should be handled by the CAN device interface.

Agreed, that was a try to keep it driver-independent. I'll rework the
code as suggested by Oliver.

>> Also, as all the support data are in the can_priv structure, this only supports
>> can-dev based drivers. All the others are ignored by checking the
>> netdev->rtnl_link_ops->kind string. This actually excludes only vcan and slcan
>> drivers.
>>
>> The implementation should be quite unintrusive on existing code and can be disabled
>> altogether with a config option.
>
> I find it rather intrusive, especially when #ifdef's should be avoided.

I only used one in "struct can_priv" and then redefined trap functions
to inline-noop. Is that a better way to do that?

> Furthermore it introduces overhead in the RX and TX path and uses a lot
> of space in the "struct can_priv".

Ok for tx/rx path overhead, I did not like the solution either.

>> This has been tested tested on x86 and a powerpc with a custom USB-CAN
>> interface (which I hope to publish as open-hardware soon BTW) and i2c-based
>> leds.
>>
>> Any thoughts? Do you think this can be merged?
>
> LED interface might be useful, especially for debugging. Why not
> enabling it just if "CONFIG_CAN_DEBUG_DEVICES=y". Also LEDs for error
> states would make sense.

I see that more useful for an installer of the system to see that the
bus is working than for debugging the driver, also because what I had
in mind was some kind of device with front-panel connector and status
leds.

Agreed for error state. Of course that have to be trapped in the driver too.

> Some more comments inline...
>
>>  include/linux/can/dev.h |    9 +++
>>  net/can/Kconfig         |   10 +++
>>  net/can/Makefile        |    2 +
>>  net/can/af_can.c        |   14 ++++-
>>  net/can/led.c           |  154 +++++++++++++++++++++++++++++++++++++++++++++++
>>  net/can/led.h           |   29 +++++++++
>>  6 files changed, 217 insertions(+), 1 deletions(-)
>>  create mode 100644 net/can/led.c
>>  create mode 100644 net/can/led.h
>>
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index 5d2efe7..76eb70c 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -16,6 +16,8 @@
>>  #include <linux/can.h>
>>  #include <linux/can/netlink.h>
>>  #include <linux/can/error.h>
>> +#include <linux/timer.h>
>> +#include <linux/leds.h>
>>
>>  /*
>>   * CAN mode
>> @@ -52,6 +54,13 @@ struct can_priv {
>>
>>       unsigned int echo_skb_max;
>>       struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> +     struct timer_list tx_off_timer, rx_off_timer;
>> +     struct led_trigger *tx_led, *rx_led;
>> +     char tx_led_name[32], rx_led_name[32];
>> +     atomic_t led_discard_count;
>> +#endif
>
> This requires a lot of space. Please add just *one* pointer here and
> allocate the required space when needed.

How would that help?

>>  };
>>
>>  /*
>> diff --git a/net/can/Kconfig b/net/can/Kconfig
>> index 0320069..55894b7 100644
>> --- a/net/can/Kconfig
>> +++ b/net/can/Kconfig
>> @@ -52,4 +52,14 @@ config CAN_GW
>>         They can be modified with AND/OR/XOR/SET operations as configured
>>         by the netlink configuration interface known e.g. from iptables.
>>
>> +config CAN_LEDS
>> +     bool "Enable LED triggers for Netlink based drivers"
>> +     depends on CAN
>> +     depends on CAN_DEV
>> +     depends on LEDS_CLASS
>> +     select LEDS_TRIGGERS
>> +     ---help---
>> +       This option enables two LED triggers for packet receive and transmit
>> +       events on each CAN device based on the can-dev framework.
>> +
>>  source "drivers/net/can/Kconfig"
>> diff --git a/net/can/Makefile b/net/can/Makefile
>> index cef49eb..fc6b286 100644
>> --- a/net/can/Makefile
>> +++ b/net/can/Makefile
>> @@ -5,6 +5,8 @@
>>  obj-$(CONFIG_CAN)    += can.o
>>  can-y                        := af_can.o proc.o
>>
>> +can-$(CONFIG_CAN_LEDS)       += led.o
>> +
>>  obj-$(CONFIG_CAN_RAW)        += can-raw.o
>>  can-raw-y            := raw.o
>
> In this file you only find things for CAN protocol support. LEDs should
> be handled by drivers/net/can/dev.c.

Agreed.

Thanks for the feedback.

Regards.

-- 
Fabio Baltieri