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.
> 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.
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.
> > 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 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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
