On Wed, May 16, 2018 at 09:44:02AM +0200, Uwe Kleine-König wrote:
> On Wed, May 16, 2018 at 09:04:56AM +0200, Johan Hovold wrote:
> > On Mon, May 14, 2018 at 12:15:02PM +0200, Uwe Kleine-König wrote:
> > > As triggers are identified by name, two triggers must not share the same
> > > name. This is enforced at registration but must be done for renames,
> > > too.
> > >
> > > Fixes: a8df7b1ab70b ("leds: add led_trigger_rename function")
> >
> > Not sure a fixes tag is warranted for the current single user which
> > calls this functions with what (I assume is) a unique netdev name as
> > base. So this is currently a non-issue.
>
> Yeah, the netdev name is unique. Together with (the current state of)
> the tty trigger patch, it would result in a clash as soon as a netdev is
> renamed to say ttyS0.
Ah, that's right. But still not an issue with the current state of
things, or are there triggers named <dev>-rx currently?
> > And while not really your problem, having a fixes tag here may also cause
> > some bots to pick this up for stable. And especially as the impact of
> > this change is not made clear in the commit message.
>
> I didn't make this clear because I don't know what can happen. Maybe
> it's only that it becomes impossible to select one of the two twins as
> triggers.
Perhaps you could give it a try?
> Anyhow, mentioning a8df7b1ab70b seems sensible, and if we don't think it
> should be backported to stable, we can still veto it when the patch is
> nominated.
Yeah, that's a stable process issue.
> > > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > > index 431123b048a2..28e78697c63e 100644
> > > --- a/drivers/leds/led-triggers.c
> > > +++ b/drivers/leds/led-triggers.c
> > > @@ -177,10 +177,21 @@ EXPORT_SYMBOL_GPL(led_trigger_set_default);
> > >
> > > void led_trigger_rename_static(const char *name, struct led_trigger *trig)
> > > {
> > > + struct led_trigger *_trig;
> > > +
> > > /* new name must be on a temporary string to prevent races */
> > > BUG_ON(name == trig->name);
> > >
> > > down_write(&triggers_list_lock);
> > > +
> > > + list_for_each_entry(_trig, &trigger_list, next_trig) {
> > > + if (!strcmp(_trig->name, name)) {
> > > + up_write(&triggers_list_lock);
> > > + pr_warning("Cannot rename trigger, new name already exists\n");
> > > + return;
> >
> > Changing the API here makes some sense (i.e. to have core rather than
> > the caller guarantee uniqueness), but then I think you should return an
> > errno as well to allow the caller to detect and bail out on errors.
>
> This is done in the next patch.
I noticed after I sent this.
> I expect that the users of the can triggers can just use the netdev
> trigger without loss of functionallity. So the can-trigger and with it
> led_trigger_rename_static can go away. Maybe it's not worth to discuss
> this patch at all if we don't backport it to stable.
Less code is always better. ;)
Thanks,
Johan
