Re: [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr

[Doug Anderson]

Hi,

On Wed, Jan 18, 2023 at 2:34 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Doug Anderson (2023-01-18 10:29:59)
> > Hi,
> >
> > On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
> > > +
> > >         if (isr & DP_INTR_AUX_ERROR) {
> > >                 aux->aux_error_num = DP_AUX_ERR_PHY;
> > >                 dp_catalog_aux_clear_hw_interrupts(aux->catalog);
> > > +               ret = IRQ_HANDLED;
> > >         }
> >
> > The end result of the above is a weird mix of "if" and "else if" for
> > no apparent reason. All except one of them just updates the exact same
> > variable so doing more than one is mostly useless. If you made it
> > consistently with "else" then the whole thing could be much easier,
> > like this (untested):
>
> Totally agreed. I even asked that when I posted the RFC[1]!
>
> "Can we also simplify the aux handlers to be a big pile of
> if-else-if conditions that don't overwrite the 'aux_error_num'? That
> would simplify the patch below."
>
> > > @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
> > >
> > >         /* no interrupts pending, return immediately */
> > >         if (!isr)
> > > -               return;
> > > +               return IRQ_NONE;
> > >
> > >         if (!aux->cmd_busy)
> > > -               return;
> > > +               return IRQ_NONE;
> > >
> > >         if (aux->native)
> > > -               dp_aux_native_handler(aux, isr);
> > > +               return dp_aux_native_handler(aux, isr);
> > >         else
> > > -               dp_aux_i2c_handler(aux, isr);
> > > -
> > > -       complete(&aux->comp);
> > > +               return dp_aux_i2c_handler(aux, isr);
> >
> > Personally, I wouldn't have done it this way. I guess that means I
> > disagree with Stephen. I'm not dead-set against this way and it's fine
> > if you want to continue with it. If I were doing it, though, then I
> > would always return IRQ_HANDLED IF dp_catalog_aux_get_irq() returned
> > anything non-zero. Why? Officially if dp_catalog_aux_get_irq() returns
> > something non-zero then you know for sure that there was an interrupt
> > for this device and officially you have "handled" it by acking it,
> > since dp_catalog_aux_get_irq() acks all the bits that it returns. That
> > means that even if dp_aux_native_handler() or dp_aux_i2c_handler()
> > didn't do anything with the interrupt you at least know that it was
> > for us (so if the IRQ is shared we properly report back to the IRQ
> > subsystem) and that it won't keep firing over and over (because we
> > acked it).
>
> I'm primarily concerned with irq storms taking down the system. Can that
> happen here? If not, then returning IRQ_NONE is not really useful. The
> overall IRQ for DP looks to be level, because the driver requests the
> IRQ that way. The aux interrupt status bits look to be edge style
> interrupts though, because the driver acks them in the handler. I guess
> that means the edges come in and latch into the interrupt status
> register so the driver has to ack all of them to drop the IRQ level for
> the overall DP interrupt? If the driver only acked the bits it looked at
> instead of all interrupt bits in the register, then the level would
> never go down for the IRQ if an unhandled interrupt bit was present like
> 'DP_INTR_PLL_UNLOCKED'. That would mean we would hit spurious IRQ
> handling very quickly if that interrupt bit was ever seen.
>
> But the driver is acking all interrupts, so probably trying to work
> IRQ_NONE into this code is not very useful? The only thing it would
> catch is DP_INTR_PLL_UNLOCKED being set over and over again, which seems
> unlikely. Of course, why is this driver unmasking interrupt bits it
> doesn't care about? That may be leading to useless interrupt handling in
> this driver if some interrupt bit is unmasked but never looked at. Can
> that be fixed in another patch?
>
> >
> > NOTE: I still like having the complete() call in
> > dp_aux_native_handler() and dp_aux_i2c_handler() and, to me, that part
> > of this patch is worthwhile. That makes it more obvious that the code
> > is truly expecting that complete to be called for all error cases as
> > well as transfer finished.
> >
>
> I think it may be required. We don't want to allow DP_INTR_PLL_UNLOCKED
> to complete() the transfer.

OK, I've tried to code up what I think is the right solution. I'd
appreciate review and testing.

https://lore.kernel.org/r/20230119145248.1.I90ffed3ddd21e818ae534f820cb4d6d8638859ab@changeid

-Doug