On Wed, 27 Jul 2022 16:23:26 +0100, Frank Li <frank.li@xxxxxxx> wrote: > > > > > -----Original Message----- > > From: Marc Zyngier <maz@xxxxxxxxxx> > > Sent: Wednesday, July 27, 2022 3:03 AM > > To: Frank Li <frank.li@xxxxxxx> > > Cc: jdmason@xxxxxxxx; tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > > krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; > > s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx; > > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm- > > kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan > > <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>; > > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx <linux- > > imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx; > > ntb@xxxxxxxxxxxxxxx > > 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@xxxxxxx> 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). M. -- Without deviation from the norm, progress is not possible.