Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller

From: Marc Zyngier <maz@kernel.org>
To: Frank Li <frank.li@nxp.com>
Cc: "jdmason@kudzu.us" <jdmason@kudzu.us>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org" 
	<krzysztof.kozlowski+dt@linaro.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"kw@linux.com" <kw@linux.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"kernel@vger.kernel.org" <kernel@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Peng Fan <peng.fan@nxp.com>, Aisheng Dong <aisheng.dong@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	dl-linux-imx <linux-imx@nxp.com>, "kishon@ti.com" <kishon@ti.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"ntb@lists.linux.dev" <ntb@lists.linux.dev>
Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller
Date: Wed, 27 Jul 2022 09:02:41 +0100	[thread overview]
Message-ID: <877d3zx9su.wl-maz@kernel.org> (raw)
In-Reply-To: <PAXPR04MB918621013E6276D37B56C48488949@PAXPR04MB9186.eurprd04.prod.outlook.com>

On Tue, 26 Jul 2022 22:48:32 +0100,
Frank Li <frank.li@nxp.com> wrote:
> 
> > > > > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > > > > +{
> > > > > +     struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > > > > +     u32 status;
> > > > > +     int i;
> > > > > +
> > > > > +     status = imx_mu_read(msi_data, msi_data->cfg-
> > >xSR[IMX_MU_RSR]);
> > > > > +
> > > > > +     chained_irq_enter(irq_desc_get_chip(desc), desc);
> > > > > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > > > > +             if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
> > > > > +                     imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
> > > > > +                     generic_handle_domain_irq(msi_data->parent, i);
> > > >
> > > > Why the parent? You must start at the top of the hierarchy.
> 
> [Frank Li] Do you means that should be msi_data->msi_domain instead
> of msi_data->parent?

Indeed. you must *not* bypass the hierarchy, and the top level of the
hierarchy has to implement whatever is required by the interrupt flow.

> 
> > > >
> > > > > +             }
> > > > > +     }
> > > > > +     chained_irq_exit(irq_desc_get_chip(desc), desc);
> > > >
> > > > If your MSIs are a chained interrupt, why do you even provide an
> > > > affinity setting callback?
> > >
> > > [Frank Li]  it will be crash if no affinity setting callback.	
> > 
> > Then you have to fix your driver.
> 
> [Frank Li] After debug,  msi_domain_set_affinity() have not did null check for (parent->chip->irq_set_affinity). 
> I think impact by using dummy set_affinity is minimized.  
> 
> int msi_domain_set_affinity(struct irq_data *irq_data,	
> 			    const struct cpumask *mask, bool force)
> {
> 	struct irq_data *parent = irq_data->parent_data;
> 	struct msi_msg msg[2] = { [1] = { }, };
> 	int ret;
> 
> 	ret = parent->chip->irq_set_affinity(parent, mask, force);
> 	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> 		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
> 		msi_check_level(irq_data->domain, msg);
> 		irq_chip_write_msi_msg(irq_data, msg);
> 	}
> 
> 	return ret;
> }

No. Changing the affinity of an interrupt must not affect the affinity
of another. Given that this is a chained handler, you *cannot* satisfy
this requirement. So you can't change the affinity at all.

	N,

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