Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 07 Mar 2022 11:45:44 +0000
Message-ID: <87cziy0yvr.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Shawn Guo <shawn.guo@linaro.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Maulik Shah <quic_mkshah@quicinc.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver
In-Reply-To: <20220306125710.GQ269879@dragon>
References: <20220302084028.GL269879@dragon>
        <877d9c3b2u.wl-maz@kernel.org>
        <20220302133441.GM269879@dragon>
        <875yow31a0.wl-maz@kernel.org>
        <20220303040229.GN269879@dragon>
        <87fsnytagc.wl-maz@kernel.org>
        <20220304082342.GO269879@dragon>
        <87lexp211g.wl-maz@kernel.org>
        <20220305092420.GP269879@dragon>
        <87czj0u0bg.wl-maz@kernel.org>
        <20220306125710.GQ269879@dragon>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 06 Mar 2022 12:57:10 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Sat, Mar 05, 2022 at 11:05:07AM +0000, Marc Zyngier wrote:
> > 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.
> 
> I tested with manually unbinding the USB driver and didn't run into any
> memory corruption.  If I read irq_domain code right, it seems that
> irq_domain_alloc_irq_data() will call into irq_domain_insert_irq_data()
> to allocate z irq_data in context of virq x and y respectively.  So x
> and y do not share a single parent (z) irq_data but have their own copy
> of z irq_data, no?

Which is just another bug you are relying on. Maybe you're OK with
that, but I'm not (and I intend to fix this bug).

I'm not taking this driver until you either:

- prevent a pin sharing a GIC interrupt from triggering an interrupt
  allocation in the driver

- or turn this driver into something that isn't a hierarchical setup

	M.

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