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.
[1] https://lore.kernel.org/all/CAE-0n5100eGC0c09oq4B3M=aHtKW5+wGLGsS1jM91SCyZ5wffQ@xxxxxxxxxxxxxx/
