| Message ID | 20220607183907.20017-1-ddrokosov@sberdevices.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] iio: trigger: warn about non-registered iio trigger getting attempt | expand |
Hello Jonathan, I notice the patchset from https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ is not merged to stable yet. I think if this WARN() patch is okay for you, maybe it's better to merge it together with the previous one. It will notify developers about this problem as you suggested before, and the previous patchset resolves the issue in the all IIO drivers. What do you think about it? On Tue, Jun 07, 2022 at 06:39:18PM +0000, Dmitry Rokosov wrote: > As a part of patch series about wrong trigger register() and get() > calls order in the some IIO drivers trigger initialization path: > > https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ > > runtime WARN_ONCE() is added to alarm IIO driver authors who make such > a mistake. > > When an IIO driver allocates a new IIO trigger, it should register it > before calling the get() operation. In other words, each IIO driver > must abide by IIO trigger alloc()/register()/get() calls order. > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > Changes: > v1 -> v2: totally reworked the patch, used trig->list entry instead of > trig->owner as driver registration indicator. > It works perfectly for both builtin and built as a module > drivers. > > v2 -> v3: changed WARN() call to WARN_ONCE() to avoid warn spamming > during deferred probe() as Andy suggested. > --- > drivers/iio/industrialio-trigger.c | 2 ++ > include/linux/iio/trigger.h | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c > index f504ed351b3e..d6277e72d515 100644 > --- a/drivers/iio/industrialio-trigger.c > +++ b/drivers/iio/industrialio-trigger.c > @@ -581,6 +581,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent, > if (trig->name == NULL) > goto free_descs; > > + INIT_LIST_HEAD(&trig->list); > + > trig->subirq_chip.name = trig->name; > trig->subirq_chip.irq_mask = &iio_trig_subirqmask; > trig->subirq_chip.irq_unmask = &iio_trig_subirqunmask; > diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h > index 4c69b144677b..03b1d6863436 100644 > --- a/include/linux/iio/trigger.h > +++ b/include/linux/iio/trigger.h > @@ -93,6 +93,11 @@ static inline void iio_trigger_put(struct iio_trigger *trig) > static inline struct iio_trigger *iio_trigger_get(struct iio_trigger *trig) > { > get_device(&trig->dev); > + > + WARN_ONCE(list_empty(&trig->list), > + "Getting non-registered iio trigger %s is prohibited\n", > + trig->name); > + > __module_get(trig->owner); > > return trig; > -- > 2.36.0
Hello Andy, Could you please review this patch version if possible? I've changed WARN() to WARN_ONCE() and fixed commit msg as you suggested in the v2. On Tue, Jun 07, 2022 at 06:39:18PM +0000, Dmitry Rokosov wrote: > As a part of patch series about wrong trigger register() and get() > calls order in the some IIO drivers trigger initialization path: > > https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ > > runtime WARN_ONCE() is added to alarm IIO driver authors who make such > a mistake. > > When an IIO driver allocates a new IIO trigger, it should register it > before calling the get() operation. In other words, each IIO driver > must abide by IIO trigger alloc()/register()/get() calls order. > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > Changes: > v1 -> v2: totally reworked the patch, used trig->list entry instead of > trig->owner as driver registration indicator. > It works perfectly for both builtin and built as a module > drivers. > > v2 -> v3: changed WARN() call to WARN_ONCE() to avoid warn spamming > during deferred probe() as Andy suggested. > --- > drivers/iio/industrialio-trigger.c | 2 ++ > include/linux/iio/trigger.h | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c > index f504ed351b3e..d6277e72d515 100644 > --- a/drivers/iio/industrialio-trigger.c > +++ b/drivers/iio/industrialio-trigger.c > @@ -581,6 +581,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent, > if (trig->name == NULL) > goto free_descs; > > + INIT_LIST_HEAD(&trig->list); > + > trig->subirq_chip.name = trig->name; > trig->subirq_chip.irq_mask = &iio_trig_subirqmask; > trig->subirq_chip.irq_unmask = &iio_trig_subirqunmask; > diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h > index 4c69b144677b..03b1d6863436 100644 > --- a/include/linux/iio/trigger.h > +++ b/include/linux/iio/trigger.h > @@ -93,6 +93,11 @@ static inline void iio_trigger_put(struct iio_trigger *trig) > static inline struct iio_trigger *iio_trigger_get(struct iio_trigger *trig) > { > get_device(&trig->dev); > + > + WARN_ONCE(list_empty(&trig->list), > + "Getting non-registered iio trigger %s is prohibited\n", > + trig->name); > + > __module_get(trig->owner); > > return trig; > -- > 2.36.0
On Thu, 16 Jun 2022 09:13:00 +0000 Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > Hello Jonathan, > > I notice the patchset from > https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ > is not merged to stable yet. > I think if this WARN() patch is okay for you, maybe it's better to merge > it together with the previous one. It will notify developers about this > problem as you suggested before, and the previous patchset resolves the issue > in the all IIO drivers. > > What do you think about it? It would be a stretch to take a defensive measure like this into stable, so I'll just queue this up for the next merge window. We might have some exciting intermediate times where anyone actually using the togreg branch directly will get drivers that will spit out the warning. That should only be people active on the list though who will find this quickly enough and understand what is gong on. I'm fine with this and it's been on list long enough for anyone else to comment. It'll be in a branch I'm happy to rebase for at few days anyway if there are any last minute comments or tags. Applied to the togreg branch of iio.git and pushed out as testing. Hopefully I'll get a pull request out for the fixes-togreg branch sometime this weekend. Thanks for adding this protection btw. Jonathan > > On Tue, Jun 07, 2022 at 06:39:18PM +0000, Dmitry Rokosov wrote: > > As a part of patch series about wrong trigger register() and get() > > calls order in the some IIO drivers trigger initialization path: > > > > https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ > > > > runtime WARN_ONCE() is added to alarm IIO driver authors who make such > > a mistake. > > > > When an IIO driver allocates a new IIO trigger, it should register it > > before calling the get() operation. In other words, each IIO driver > > must abide by IIO trigger alloc()/register()/get() calls order. > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > > --- > > Changes: > > v1 -> v2: totally reworked the patch, used trig->list entry instead of > > trig->owner as driver registration indicator. > > It works perfectly for both builtin and built as a module > > drivers. > > > > v2 -> v3: changed WARN() call to WARN_ONCE() to avoid warn spamming > > during deferred probe() as Andy suggested. > > --- > > drivers/iio/industrialio-trigger.c | 2 ++ > > include/linux/iio/trigger.h | 5 +++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c > > index f504ed351b3e..d6277e72d515 100644 > > --- a/drivers/iio/industrialio-trigger.c > > +++ b/drivers/iio/industrialio-trigger.c > > @@ -581,6 +581,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent, > > if (trig->name == NULL) > > goto free_descs; > > > > + INIT_LIST_HEAD(&trig->list); > > + > > trig->subirq_chip.name = trig->name; > > trig->subirq_chip.irq_mask = &iio_trig_subirqmask; > > trig->subirq_chip.irq_unmask = &iio_trig_subirqunmask; > > diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h > > index 4c69b144677b..03b1d6863436 100644 > > --- a/include/linux/iio/trigger.h > > +++ b/include/linux/iio/trigger.h > > @@ -93,6 +93,11 @@ static inline void iio_trigger_put(struct iio_trigger *trig) > > static inline struct iio_trigger *iio_trigger_get(struct iio_trigger *trig) > > { > > get_device(&trig->dev); > > + > > + WARN_ONCE(list_empty(&trig->list), > > + "Getting non-registered iio trigger %s is prohibited\n", > > + trig->name); > > + > > __module_get(trig->owner); > > > > return trig; > > -- > > 2.36.0 >
Jonathan, On Sat, Jun 18, 2022 at 02:27:03PM +0100, Jonathan Cameron wrote: > On Thu, 16 Jun 2022 09:13:00 +0000 > Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > > > Hello Jonathan, > > > > I notice the patchset from > > https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ > > is not merged to stable yet. > > I think if this WARN() patch is okay for you, maybe it's better to merge > > it together with the previous one. It will notify developers about this > > problem as you suggested before, and the previous patchset resolves the issue > > in the all IIO drivers. > > > > What do you think about it? > > It would be a stretch to take a defensive measure like this into stable, > so I'll just queue this up for the next merge window. We might have > some exciting intermediate times where anyone actually using the togreg > branch directly will get drivers that will spit out the warning. > That should only be people active on the list though who will find > this quickly enough and understand what is gong on. > > I'm fine with this and it's been on list long enough for anyone else to comment. > It'll be in a branch I'm happy to rebase for at few days anyway if there > are any last minute comments or tags. > > Applied to the togreg branch of iio.git and pushed out as testing. > > Hopefully I'll get a pull request out for the fixes-togreg branch > sometime this weekend. Thanks a lot for such detailed clarification! I'm seeing this warn patchset in the linux-next already.
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index f504ed351b3e..d6277e72d515 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -581,6 +581,8 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent, if (trig->name == NULL) goto free_descs; + INIT_LIST_HEAD(&trig->list); + trig->subirq_chip.name = trig->name; trig->subirq_chip.irq_mask = &iio_trig_subirqmask; trig->subirq_chip.irq_unmask = &iio_trig_subirqunmask; diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h index 4c69b144677b..03b1d6863436 100644 --- a/include/linux/iio/trigger.h +++ b/include/linux/iio/trigger.h @@ -93,6 +93,11 @@ static inline void iio_trigger_put(struct iio_trigger *trig) static inline struct iio_trigger *iio_trigger_get(struct iio_trigger *trig) { get_device(&trig->dev); + + WARN_ONCE(list_empty(&trig->list), + "Getting non-registered iio trigger %s is prohibited\n", + trig->name); + __module_get(trig->owner); return trig;
As a part of patch series about wrong trigger register() and get() calls order in the some IIO drivers trigger initialization path: https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ runtime WARN_ONCE() is added to alarm IIO driver authors who make such a mistake. When an IIO driver allocates a new IIO trigger, it should register it before calling the get() operation. In other words, each IIO driver must abide by IIO trigger alloc()/register()/get() calls order. Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> --- Changes: v1 -> v2: totally reworked the patch, used trig->list entry instead of trig->owner as driver registration indicator. It works perfectly for both builtin and built as a module drivers. v2 -> v3: changed WARN() call to WARN_ONCE() to avoid warn spamming during deferred probe() as Andy suggested. --- drivers/iio/industrialio-trigger.c | 2 ++ include/linux/iio/trigger.h | 5 +++++ 2 files changed, 7 insertions(+)