[v2,1/2] iio: Improve the kernel-doc of iio_trigger_poll

  Move the kernel-doc of the function to industrialio-trigger.c
Add a note on the context where the function is expected to be called.

Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v2:
- Changed the expected context of from interrupt to hard IRQ context

 drivers/iio/industrialio-trigger.c | 7 +++++++
 include/linux/iio/trigger.h        | 6 ------
 2 files changed, 7 insertions(+), 6 deletions(-)
  

On Thu, Mar 02, 2023 at 02:04:35PM +0100, Mehdi Djait wrote:
> Move the kernel-doc of the function to industrialio-trigger.c
> Add a note on the context where the function is expected to be called.

...

> v2:
> - Changed the expected context of from interrupt to hard IRQ context

Thank you for an update.

But it seems I messed up with this and my previous remark shouldn't be
taken into consideration.

The "relevant hardware interrupt handler" may be hard and threaded IRQ context,
which looks like your first version was correct.

Let's wait for Jonathan opinion on this as he is a native speaker.
  

Hello Andy,

On Thu, Mar 02, 2023 at 05:54:05PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 02, 2023 at 02:04:35PM +0100, Mehdi Djait wrote:
> > Move the kernel-doc of the function to industrialio-trigger.c
> > Add a note on the context where the function is expected to be called.
> 
> ...
> 
> > v2:
> > - Changed the expected context of from interrupt to hard IRQ context
> 
> Thank you for an update.
> 
> But it seems I messed up with this and my previous remark shouldn't be
> taken into consideration.
> 
> The "relevant hardware interrupt handler" may be hard and threaded IRQ context,
> which looks like your first version was correct.
> 
> Let's wait for Jonathan opinion on this as he is a native speaker.

If I understood the function correctly I think you were right. It should
be hard IRQ context

The relevant functions calls:
iio_trigger_poll --> generic_handle_irq --> handle_irq_desc

handle_irq_desc: returns Operation not permitted if !in_hardirq() && handle_enforce_irqctx 
and it is the reason why the sysfs trigger uses the irq_framework to call iio_trigger_poll 
from hard IRQ context [1][2]

[1] https://lwn.net/Articles/411605/ 
[2] https://lore.kernel.org/all/1346922337-17088-1-git-send-email-lars@metafoo.de/

--
Kind Regards
Mehdi Djait
  

On Thu, Mar 02, 2023 at 05:18:14PM +0100, Mehdi Djait wrote:
> On Thu, Mar 02, 2023 at 05:54:05PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 02, 2023 at 02:04:35PM +0100, Mehdi Djait wrote:
> > > Move the kernel-doc of the function to industrialio-trigger.c
> > > Add a note on the context where the function is expected to be called.

...

> > > v2:
> > > - Changed the expected context of from interrupt to hard IRQ context
> > 
> > Thank you for an update.
> > 
> > But it seems I messed up with this and my previous remark shouldn't be
> > taken into consideration.
> > 
> > The "relevant hardware interrupt handler" may be hard and threaded IRQ context,
> > which looks like your first version was correct.
> > 
> > Let's wait for Jonathan opinion on this as he is a native speaker.
> 
> If I understood the function correctly I think you were right. It should
> be hard IRQ context
> 
> The relevant functions calls:
> iio_trigger_poll --> generic_handle_irq --> handle_irq_desc
> 
> handle_irq_desc: returns Operation not permitted if !in_hardirq() && handle_enforce_irqctx 
> and it is the reason why the sysfs trigger uses the irq_framework to call iio_trigger_poll 
> from hard IRQ context [1][2]

Cool, thank you for elaboration!

In any case it's up to Jonathan now what to do. With your explanation it seems
correct to phrase as you did in v2. Hence,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> [1] https://lwn.net/Articles/411605/ 
> [2] https://lore.kernel.org/all/1346922337-17088-1-git-send-email-lars@metafoo.de/
  

On Thu, 2 Mar 2023 18:23:23 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Mar 02, 2023 at 05:18:14PM +0100, Mehdi Djait wrote:
> > On Thu, Mar 02, 2023 at 05:54:05PM +0200, Andy Shevchenko wrote:  
> > > On Thu, Mar 02, 2023 at 02:04:35PM +0100, Mehdi Djait wrote:  
> > > > Move the kernel-doc of the function to industrialio-trigger.c
> > > > Add a note on the context where the function is expected to be called.  
> 
> ...
> 
> > > > v2:
> > > > - Changed the expected context of from interrupt to hard IRQ context  
> > > 
> > > Thank you for an update.
> > > 
> > > But it seems I messed up with this and my previous remark shouldn't be
> > > taken into consideration.
> > > 
> > > The "relevant hardware interrupt handler" may be hard and threaded IRQ context,
> > > which looks like your first version was correct.
> > > 
> > > Let's wait for Jonathan opinion on this as he is a native speaker.  
> > 
> > If I understood the function correctly I think you were right. It should
> > be hard IRQ context
> > 
> > The relevant functions calls:
> > iio_trigger_poll --> generic_handle_irq --> handle_irq_desc
> > 
> > handle_irq_desc: returns Operation not permitted if !in_hardirq() && handle_enforce_irqctx 
> > and it is the reason why the sysfs trigger uses the irq_framework to call iio_trigger_poll 
> > from hard IRQ context [1][2]  
> 
> Cool, thank you for elaboration!
> 
> In any case it's up to Jonathan now what to do. With your explanation it seems
> correct to phrase as you did in v2. Hence,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

It is indeed hard interrupt context that matters here so I'm fine with this version.
Though I'm going to tweak it to drop the empty line at the end of the comment block
whilst applying it.

Thanks,

Jonathan

> 
> > [1] https://lwn.net/Articles/411605/ 
> > [2] https://lore.kernel.org/all/1346922337-17088-1-git-send-email-lars@metafoo.de/  
>
  

On Thu,  2 Mar 2023 14:04:35 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> Move the kernel-doc of the function to industrialio-trigger.c
> Add a note on the context where the function is expected to be called.
> 
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> ---
> v2:
> - Changed the expected context of from interrupt to hard IRQ context
> 
>  drivers/iio/industrialio-trigger.c | 7 +++++++
>  include/linux/iio/trigger.h        | 6 ------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index a2f3cc2f65ef..bb0a44df8740 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -192,6 +192,13 @@ static void iio_trigger_notify_done_atomic(struct iio_trigger *trig)
>  		schedule_work(&trig->reenable_work);
>  }
>  
> +/**
> + * iio_trigger_poll() - Call the IRQ trigger handler of the consumers
> + * @trig: trigger which occurred
> + *
> + * This function needs to be called from a hard IRQ context.
I tweaked this to drop the line below and to say
This function should only be called from a hard IRQ context.

Fun subtlety of English that reminds me of the sign on the London underground
escalators that said "Guide dogs must be carried" thus apparently limiting
their use to people who could find a guide dog to carry.

Here we don't want to oblige all code to call the function :)

> + *
> + */
>  void iio_trigger_poll(struct iio_trigger *trig)
>  {
>  	int i;
> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
> index f6360d9a492d..42da55dc3aa7 100644
> --- a/include/linux/iio/trigger.h
> +++ b/include/linux/iio/trigger.h
> @@ -151,12 +151,6 @@ void iio_trigger_unregister(struct iio_trigger *trig_info);
>   **/
>  int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *trig);
>  
> -/**
> - * iio_trigger_poll() - called on a trigger occurring
> - * @trig:	trigger which occurred
> - *
> - * Typically called in relevant hardware interrupt handler.
> - **/
>  void iio_trigger_poll(struct iio_trigger *trig);
>  void iio_trigger_poll_chained(struct iio_trigger *trig);
>
  

On Sat, Mar 04, 2023 at 04:46:01PM +0000, Jonathan Cameron wrote:
> On Thu,  2 Mar 2023 14:04:35 +0100
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> 
> > Move the kernel-doc of the function to industrialio-trigger.c
> > Add a note on the context where the function is expected to be called.
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> > ---
> > v2:
> > - Changed the expected context of from interrupt to hard IRQ context
> > 
> >  drivers/iio/industrialio-trigger.c | 7 +++++++
> >  include/linux/iio/trigger.h        | 6 ------
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> > index a2f3cc2f65ef..bb0a44df8740 100644
> > --- a/drivers/iio/industrialio-trigger.c
> > +++ b/drivers/iio/industrialio-trigger.c
> > @@ -192,6 +192,13 @@ static void iio_trigger_notify_done_atomic(struct iio_trigger *trig)
> >  		schedule_work(&trig->reenable_work);
> >  }
> >  
> > +/**
> > + * iio_trigger_poll() - Call the IRQ trigger handler of the consumers
> > + * @trig: trigger which occurred
> > + *
> > + * This function needs to be called from a hard IRQ context.
> I tweaked this to drop the line below and to say
> This function should only be called from a hard IRQ context.
> 
> Fun subtlety of English that reminds me of the sign on the London underground
> escalators that said "Guide dogs must be carried" thus apparently limiting
> their use to people who could find a guide dog to carry.
> 
> Here we don't want to oblige all code to call the function :)

I was even going for "must be called from hard IRQ context" first :) but
then decided that I need more accepted patches before instructing others on 
what must be done in the kernel :)

--
Kind Regards
Mehdi Djait