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

[Frank Li]

> -----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. 

> >
> > > > >
> > > > > > +             }
> > > > > > +     }
> > > > > > +     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. 

int irq_startup(struct irq_desc *desc, bool resend, bool force)
{
        struct irq_data *d = irq_desc_get_irq_data(desc);
        struct cpumask *aff = irq_data_get_affinity_mask(d);
        int ret = 0;

        desc->depth = 0;

        if (irqd_is_started(d)) {
                irq_enable(desc);
        } else {
                switch (__irq_startup_managed(desc, aff, force)) {
                case IRQ_STARTUP_NORMAL:
                        if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)
                                irq_setup_affinity(desc);
                        ret = __irq_startup(desc);
                        if (!(d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP))
                                irq_setup_affinity(desc);
                        break;
                case IRQ_STARTUP_MANAGED:
                        irq_do_set_affinity(d, aff, false);
                        ret = __irq_startup(desc);
                        break;
                case IRQ_STARTUP_ABORT:
                        irqd_set_managed_shutdown(d);
                        return 0;
                }
        }
        if (resend)
                check_irq_resend(desc, false);

        return ret;
}

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