Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier <maz@kernel.org>]

On Sat, 05 Mar 2022 09:24:20 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Fri, Mar 04, 2022 at 03:24:43PM +0000, Marc Zyngier wrote:
> > On Fri, 04 Mar 2022 08:23:42 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > 
> > > On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> > > > On Thu, 03 Mar 2022 04:02:29 +0000,
> > > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > 
> > > > > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > > > > This code actually makes me ask more questions. Why is it programming
> > > > > > 2 'pins' for each IRQ?
> > > > > 
> > > > > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > > > > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > > > > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > > > > 
> > > > > 	qcom,mpm-pin-map = <2 275>,     /* tsens0_tsens_upper_lower_int */
> > > > > 			   <5 296>,     /* lpass_irq_out_sdc */
> > > > > 			   <12 422>,    /* b3_lfps_rxterm_irq */
> > > > > 			   <24 79>,     /* bi_px_lpi_1_aoss_mx */
> > > > > 			   <86 183>,    /* mpm_wake,spmi_m */
> > > > > 			   <90 260>,    /* eud_p0_dpse_int_mx */
> > > > > 			   <91 260>;    /* eud_p0_dmse_int_mx */
> > > > > 
> > > > > 
> > > > > The downstream uses a DT bindings that specifies GIC hwirq number in
> > > > > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > > > > number, and the driver will need to query mapping table, find out the
> > > > > possible 2 MPM pins, and set them up.
> > > > > 
> > > > > The patches I'm posting here use a different bindings that specifies MPM
> > > > > pin instead in client device nodes.  Thus the driver can simply get the
> > > > > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> > > > 
> > > > It still remains that there is no 1:1 mapping between input and
> > > > output, which is the rule #1 to be able to use a hierarchical setup.
> > > 
> > > For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
> > > given MPM pin, there is only one GIC interrupt.  And that's the
> > > mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
> > > a strict 1:1 mapping.
> > 
> > Then this isn't a 1:1 mapping *AT ALL*. The hierarchical setup
> > mandates that the mapping is a bijective function, and that's exactly
> > what 1:1 means. There is no such thing a 1:1 in a single
> > direction. When you take an interrupt, all you see is the GIC
> > interrupt. How do you know which of the *two* pins interrupted you? Oh
> > wait, you *can't* know. You end-up never servicing one of the two
> > interrupts
> 
> Yes, you are right!  But that might be a problem only in theory.  I
> checked all the Qualcomm platforms I know built on MPM, and found that
> the only 2:1 case is USB DP & DM sensing pins.  Since these two pins
> will be handled by USB driver with a single interrupt handler, it should
> not cause any problem in practice.  That said, the 2:1 mapping is just
> a special case specific to USB, and MPM driver can be implemented as if
> it's just a 1:1 mapping.
>
> Shawn
> 
> > (and I suspect this results in memory corruption if you
> > tear a hierarchy down).

Key point here ^^^^^^^^^^

You can't have *any* interrupt that fits this 2:1 model if the irqchip
implements 1:1. Think about the data structures for a second:

Pins x and y and routed to GIC interrupt z. This results in the
following irq_data structures:

   MPM-x ---\
             GIC-z
   MPM-y ---/

Now, the driver using these interrupts is being removed, and the
hierarchies is being freed. Tearing down the interrupt with pin x will
result in z being also freed. And then you'll process pin y, which
will just explode.

So either you make sure these mappings can never be created, or you
change the driver to not be hierarchical. You absolutely cannot have
it both ways.

	M.

-- 
Without deviation from the norm, progress is not possible.