Re: [PATCH can-next v6] can: add tx/rx LED trigger support

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

Hello Kurt,

On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
> On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> > On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > > Hello,
> > > 
> > > I find the CAN led triggers an interesting thing.
> > > 
> > > And then, this scenario fell crossed my mind:
> > > Imagine I do:
> > > [insert CAN device: can0]
> > > $ ip link set can0 name helga
> > > [insert another CAN device: again 'can0']
> > > 
> > > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > > since that led trigger name is already reserved for CAN device 'helga'.
> > Good point.

Yep, thanks for pointing that out!

Interface renaming was something I considered when I first wrote the
code and I had the mac80211-led driver in mind, as that driver uses the
phy name and not the netdev one for its triggers.

The reason why I did not care that much in the end is that on SoC based
systems trigger-led association is made at probe time, based on data
either from platform_data or devicetree, so I imagined that once the
kernel is ported to the board and default triggers are set correctly at
boot time, the userspace is free to rename CAN interfaces and nobody
should notice... :^)

The thing I did not consider are hot-plug interfaces mixed with
renaming, such as in the case you pointed out - it's probably not really
common but still possible.

> > > I'm not sure how to fix such.
> > > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > > Just wild thinking ...
> > 
> > I think the device's name (not netdev) is unique in the system and
> > cannot be changed.
>
> but may contain several netdev's ...

Ouch.

> 
> > 
> > On my device tree enabled mx28 I'm talking about the "80032000.can" in:
> 
> You idea triggered another thougt: since control is put in device drivers,
> why putting the name in the generic can_dev struct?

Why not?  That makes the API easy.

> A more flexible approach to assign names is the key to success here.
> The correct 'works in all conditions' approach is not yet in my sight :-(

Agreed.

What about using a combination of device name + an optional port index
specified in devm_can_led_init()? (something like to platform_device names)
Of course that would require changing the API for libraries like
register_sja1000dev(), to add a port index.

Fabio