[EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller
Frank Li
frank.li at nxp.com
Wed Jul 27 11:58:33 PDT 2022
> -----Original Message-----
> From: Frank Li
> Sent: Wednesday, July 27, 2022 1:30 PM
> To: Marc Zyngier <maz at kernel.org>
> Cc: jdmason at kudzu.us; tglx at linutronix.de; robh+dt at kernel.org;
> krzysztof.kozlowski+dt at linaro.org; shawnguo at kernel.org;
> s.hauer at pengutronix.de; kw at linux.com; bhelgaas at google.com;
> kernel at vger.kernel.org; devicetree at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linux-pci at vger.kernel.org; Peng Fan
> <peng.fan at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>;
> kernel at pengutronix.de; festevam at gmail.com; dl-linux-imx <linux-
> imx at nxp.com>; kishon at ti.com; lorenzo.pieralisi at arm.com;
> ntb at lists.linux.dev
> Subject: RE: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> controller
>
>
>
> > -----Original Message-----
> > From: Marc Zyngier <maz at kernel.org>
> > Sent: Wednesday, July 27, 2022 10:35 AM
> > To: Frank Li <frank.li at nxp.com>
> > Cc: jdmason at kudzu.us; tglx at linutronix.de; robh+dt at kernel.org;
> > krzysztof.kozlowski+dt at linaro.org; shawnguo at kernel.org;
> > s.hauer at pengutronix.de; kw at linux.com; bhelgaas at google.com;
> > kernel at vger.kernel.org; devicetree at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org; linux-pci at vger.kernel.org; Peng Fan
> > <peng.fan at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>;
> > kernel at pengutronix.de; festevam at gmail.com; dl-linux-imx <linux-
> > imx at nxp.com>; kishon at ti.com; lorenzo.pieralisi at arm.com;
> > ntb at lists.linux.dev
> > Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> > controller
> >
> > Caution: EXT Email
> >
> > On Wed, 27 Jul 2022 16:23:26 +0100,
> > Frank Li <frank.li at nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marc Zyngier <maz at kernel.org>
> > > > Sent: Wednesday, July 27, 2022 3:03 AM
> > > > To: Frank Li <frank.li at nxp.com>
> > > > Cc: jdmason at kudzu.us; tglx at linutronix.de; robh+dt at kernel.org;
> > > > krzysztof.kozlowski+dt at linaro.org; shawnguo at kernel.org;
> > > > s.hauer at pengutronix.de; kw at linux.com; bhelgaas at google.com;
> > > > kernel at vger.kernel.org; devicetree at vger.kernel.org; linux-arm-
> > > > kernel at lists.infradead.org; linux-pci at vger.kernel.org; Peng Fan
> > > > <peng.fan at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>;
> > > > kernel at pengutronix.de; festevam at gmail.com; dl-linux-imx <linux-
> > > > imx at nxp.com>; kishon at ti.com; lorenzo.pieralisi at arm.com;
> > > > ntb at lists.linux.dev
> > > > Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> > > > controller
> > > >
> > > > Caution: EXT Email
> > > >
> > > > On Tue, 26 Jul 2022 22:48:32 +0100,
> > > > Frank Li <frank.li at 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.
> > > >
> > >
> > > [Frank Li] I see, just want to confirm msi_data->msi_domain should
> > > be correct here? It should be leaf of irq hierarchy tree.
> >
> > Yes.
> >
> > >
> > > > >
> > > > > > > >
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > + 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.
> > > >
> > >
> > > [Frank Li] I understand affinity can't be changed.
> > > But system use set affinity to write msi msg.
> > >
> > > The call stack as
> > > [ 25.508229] epf_ntb_write_msi_msg+0x78/0x90
> > > [ 25.512512] platform_msi_write_msg+0x2c/0x38
> > > [ 25.516882] msi_domain_set_affinity+0xb0/0xc0
> > > [ 25.521330] irq_do_set_affinity+0x174/0x220
> > > [ 25.525604] irq_setup_affinity+0xe0/0x188
> > > [ 25.529713] irq_startup+0x88/0x160
> > > [ 25.533214] __setup_irq+0x6c8/0x768
> > >
> > > I have not found good place to hook a function to write msi msg.
> >
> > It is called at MSI activation time (msi_domain_activate).
>
> Another issue: platform_msi_write_msg() is static function at platform-msi.c.
> It access a local structure struct platform_msi_priv_data.
>
> If I use MSI_FLAG_USE_DEF_CHIP_OPS flags, both msi_domain_set_affinity
> and msi_domain_set_affinity.
> will be set at chip. So it will NULL point error happen if I don't set affinity
> function.
>
[Frank Li] look like imx_mu_msi_irq_chip.irq_set_affinity = NULL; after call
platform_msi_create_irq_domain() can resolve this problem.
But it looks hack method. I think imx_mu_msi_irq_chip should keep unchanged
after call create platform_msi_create_irq_domain().
Do you have better solution or this way should be fine?
> >
> > M.
> >
> > --
> > Without deviation from the norm, progress is not possible.
More information about the linux-arm-kernel
mailing list