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/[email protected]/
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 <[email protected]>
---
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 Jonathan,
I notice the patchset from
https://lore.kernel.org/all/[email protected]/
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/[email protected]/
>
> 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 <[email protected]>
> ---
> 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
--
Thank you,
Dmitry
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/[email protected]/
>
> 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 <[email protected]>
> ---
> 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
--
Thank you,
Dmitry
On Thu, 16 Jun 2022 09:13:00 +0000
Dmitry Rokosov <[email protected]> wrote:
> Hello Jonathan,
>
> I notice the patchset from
> https://lore.kernel.org/all/[email protected]/
> 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/[email protected]/
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> > Hello Jonathan,
> >
> > I notice the patchset from
> > https://lore.kernel.org/all/[email protected]/
> > 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.
--
Thank you,
Dmitry