Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT

[Ulf Hansson <ulf.hansson@linaro.org>]

On Tue, 6 Oct 2020 at 17:33, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Brad,
>
> On Wed, Oct 07 2020 at 00:45, Brad Harper wrote:
> > I'm happy to test anything on a range of amlogic hardware with standard
> > / rt and  multiple mmc devices.  Ill test Jerome's patch in next 24
> > hours to report the results.
>
> please do not top-post and trim your replies.
>
> > On 6/10/2020 11:43 pm, Thomas Gleixner wrote:
> >>       We rather should make interrupts which need to have their primary
> >>       handler in hard interrupt context to set IRQF_NO_THREAD. That
> >>       should at the same time confirm that the primary handler is RT
> >>       safe.
> >>
> >>       Let me stare at the core code and the actual usage sites some more.
>
> So there are a few nasties in there and I faintly remember that there
> was an assumption that interrupts which are requested with both a
> primary and a secondary handler should quiesce the device interrupt in
> the primary handler if needed. OTOH, this also enforces that the primary
> handler is RT safe, which is after a quick scan of all the usage sites
> not a given and quite some of the users rely on IRQF_ONESHOT.
>
> The below untested patch should cure the problem and keep the interrupt
> line masked if requested with IRQF_ONESHOT.
>
> Thanks,
>
>         tglx

Thomas, thanks a lot for helping out and looking at this!

It looks like the testing of the patch below went well. Are you
intending to queue up the patch via your tip tree?

If you need any help, just tell us!

Kind regards
Uffe

> ---
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -967,8 +967,7 @@ static int irq_wait_for_interrupt(struct
>  static void irq_finalize_oneshot(struct irq_desc *desc,
>                                  struct irqaction *action)
>  {
> -       if (!(desc->istate & IRQS_ONESHOT) ||
> -           action->handler == irq_forced_secondary_handler)
> +       if (!(action->flags & IRQF_ONESHOT))
>                 return;
>  again:
>         chip_bus_lock(desc);
> @@ -1073,10 +1072,6 @@ irq_forced_thread_fn(struct irq_desc *de
>
>         local_bh_disable();
>         ret = action->thread_fn(action->irq, action->dev_id);
> -       if (ret == IRQ_HANDLED)
> -               atomic_inc(&desc->threads_handled);
> -
> -       irq_finalize_oneshot(desc, action);
>         local_bh_enable();
>         return ret;
>  }
> @@ -1089,14 +1084,7 @@ irq_forced_thread_fn(struct irq_desc *de
>  static irqreturn_t irq_thread_fn(struct irq_desc *desc,
>                 struct irqaction *action)
>  {
> -       irqreturn_t ret;
> -
> -       ret = action->thread_fn(action->irq, action->dev_id);
> -       if (ret == IRQ_HANDLED)
> -               atomic_inc(&desc->threads_handled);
> -
> -       irq_finalize_oneshot(desc, action);
> -       return ret;
> +       return action->thread_fn(action->irq, action->dev_id);
>  }
>
>  static void wake_threads_waitq(struct irq_desc *desc)
> @@ -1172,9 +1160,14 @@ static int irq_thread(void *data)
>                 irq_thread_check_affinity(desc, action);
>
>                 action_ret = handler_fn(desc, action);
> +               if (action_ret == IRQ_HANDLED)
> +                       atomic_inc(&desc->threads_handled);
> +
>                 if (action_ret == IRQ_WAKE_THREAD)
>                         irq_wake_secondary(desc, action);
>
> +               irq_finalize_oneshot(desc, action);
> +
>                 wake_threads_waitq(desc);
>         }
>
> @@ -1219,7 +1212,7 @@ static int irq_setup_forced_threading(st
>  {
>         if (!force_irqthreads)
>                 return 0;
> -       if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
> +       if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU))
>                 return 0;
>
>         /*
> @@ -1229,8 +1222,6 @@ static int irq_setup_forced_threading(st
>         if (new->handler == irq_default_primary_handler)
>                 return 0;
>
> -       new->flags |= IRQF_ONESHOT;
> -
>         /*
>          * Handle the case where we have a real primary handler and a
>          * thread handler. We force thread them as well by creating a
> @@ -1246,8 +1237,11 @@ static int irq_setup_forced_threading(st
>                 new->secondary->dev_id = new->dev_id;
>                 new->secondary->irq = new->irq;
>                 new->secondary->name = new->name;
> +               /* Preserve the requested IRQF_ONESHOT */
> +               new->secondary->flags = new->flags & IRQF_ONESHOT;
>         }
>         /* Deal with the primary handler */
> +       new->flags |= IRQF_ONESHOT;
>         set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
>         new->thread_fn = new->handler;
>         new->handler = irq_default_primary_handler;
> @@ -1341,6 +1335,38 @@ setup_irq_thread(struct irqaction *new,
>         return 0;
>  }
>
> +static unsigned long thread_mask_alloc(unsigned long thread_mask)
> +{
> +       /*
> +        * Unlikely to have 32 resp 64 irqs sharing one line,
> +        * but who knows.
> +        */
> +       if (thread_mask == ~0UL)
> +               return 0;
> +
> +       /*
> +        * The thread_mask for the action is or'ed to
> +        * desc->thread_active to indicate that the
> +        * IRQF_ONESHOT thread handler has been woken, but not
> +        * yet finished. The bit is cleared when a thread
> +        * completes. When all threads of a shared interrupt
> +        * line have completed desc->threads_active becomes
> +        * zero and the interrupt line is unmasked. See
> +        * handle.c:irq_wake_thread() for further information.
> +        *
> +        * If no thread is woken by primary (hard irq context)
> +        * interrupt handlers, then desc->threads_active is
> +        * also checked for zero to unmask the irq line in the
> +        * affected hard irq flow handlers
> +        * (handle_[fasteoi|level]_irq).
> +        *
> +        * The new action gets the first zero bit of
> +        * thread_mask assigned. See the loop above which or's
> +        * all existing action->thread_mask bits.
> +        */
> +       return 1UL << ffz(thread_mask);
> +}
> +
>  /*
>   * Internal function to register an irqaction - typically used to
>   * allocate special interrupts that are part of the architecture.
> @@ -1525,35 +1551,18 @@ static int
>          * conditional in irq_wake_thread().
>          */
>         if (new->flags & IRQF_ONESHOT) {
> -               /*
> -                * Unlikely to have 32 resp 64 irqs sharing one line,
> -                * but who knows.
> -                */
> -               if (thread_mask == ~0UL) {
> -                       ret = -EBUSY;
> +               ret = -EBUSY;
> +               new->thread_mask = thread_mask_alloc(thread_mask);
> +               if (!new->thread_mask)
>                         goto out_unlock;
> +
> +               if (new->secondary && (new->secondary->flags & IRQF_ONESHOT)) {
> +                       thread_mask |= new->thread_mask;
> +                       new->secondary->thread_mask =
> +                               thread_mask_alloc(thread_mask);
> +                       if (!new->secondary->thread_mask)
> +                               goto out_unlock;
>                 }
> -               /*
> -                * The thread_mask for the action is or'ed to
> -                * desc->thread_active to indicate that the
> -                * IRQF_ONESHOT thread handler has been woken, but not
> -                * yet finished. The bit is cleared when a thread
> -                * completes. When all threads of a shared interrupt
> -                * line have completed desc->threads_active becomes
> -                * zero and the interrupt line is unmasked. See
> -                * handle.c:irq_wake_thread() for further information.
> -                *
> -                * If no thread is woken by primary (hard irq context)
> -                * interrupt handlers, then desc->threads_active is
> -                * also checked for zero to unmask the irq line in the
> -                * affected hard irq flow handlers
> -                * (handle_[fasteoi|level]_irq).
> -                *
> -                * The new action gets the first zero bit of
> -                * thread_mask assigned. See the loop above which or's
> -                * all existing action->thread_mask bits.
> -                */
> -               new->thread_mask = 1UL << ffz(thread_mask);
>
>         } else if (new->handler == irq_default_primary_handler &&
>                    !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {
>
>