| Message ID | e4c7b434452775d00b6621012ad5e263076b3fcf.camel@kernel.crashing.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment | expand |
On Wed, 2019-06-12 at 15:16 +1000, Benjamin Herrenschmidt wrote: > pci_msi_create_irq_domain -> pci_msi_domain_update_chip_ops will > set those two already since the driver sets MSI_FLAG_USE_DEF_CHIP_OPS > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > > [UNTESTED] > > Just something I noticed while browsing through those drivers in > search of ways to factor some of the code. > > That leads to a question here: > > Some MSI drivers such as this one (or any using the defaults > mask/unmask > provided by drivers/pci/msi.c) only call the PCI MSI mask/unmask > functions. > > Some other drivers call those PCI function but *also* call the parent > mask/unmask (giv-v2m for example) which generally is the inner domain > which just itself forwards to its own parent. .../... So I looked at x86 and it also only uses pci_msi_unmask_irq, it doesn't mask at the parent level. And it also specifies those explicitly which isn't necessary so the same trivial cleanup patch could be done (happy to do it unless I missed something here). Question: If that's indeed the rule we want to establish, should we consider making all MSI controllers just use the PCI masking and remove the forwarding to the parent ? The ones that do the parent, at least in drivers/irqchip/* and drivers/pci/controller/* (ther are more in arch code) are all the GIC ones (v2m, v3-its, v3-mbi), alpine which was copied on GIC I think, tango and dwc. The other approach would be to make the generic ops setup by pci_msi_domain_update_chip_ops call the parent as well .. if there is one and it has corresponding mask/unmask callbacks. That means things like armada_370 would be unaffected since their "middle" irqdomain chip doesn't have them, at least until somebody decides that masking at the parent level as well is a good thing. I *think* it would also work for x86 since the parent in that case is x86_vector_domain which also doesn't have mask and unmask callbacks, so it would be a nop change. Let me know what you think. Cheers, Ben.
Hi Ben, On Wed, 12 Jun 2019 06:16:05 +0100, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > > pci_msi_create_irq_domain -> pci_msi_domain_update_chip_ops will > set those two already since the driver sets MSI_FLAG_USE_DEF_CHIP_OPS > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > > [UNTESTED] > > Just something I noticed while browsing through those drivers in > search of ways to factor some of the code. > > That leads to a question here: > > Some MSI drivers such as this one (or any using the defaults mask/unmask > provided by drivers/pci/msi.c) only call the PCI MSI mask/unmask functions. > > Some other drivers call those PCI function but *also* call the parent > mask/unmask (giv-v2m for example) which generally is the inner domain > which just itself forwards to its own parent. > > Is there any preference for doing it one way or the other ? I can see > that in cases where the device doesn't support MSI masking, calling the > parent could be useful but we don't know that at the moment in the > corresponding code. > > It feels like something we should consolidate (and remove code from > drivers). For example, the defaults in drivers/pci/msi.c could always > call the parent if it exists and has a mask/unmask callback. > > Opinions ? I'm happy to produce patches once we agree... > > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c > index c9bdc5221b82..911230f28e2d 100644 > --- a/drivers/irqchip/irq-armada-370-xp.c > +++ b/drivers/irqchip/irq-armada-370-xp.c > @@ -197,8 +197,6 @@ static void armada_370_xp_irq_unmask(struct irq_data *d) > > static struct irq_chip armada_370_xp_msi_irq_chip = { > .name = "MPIC MSI", > - .irq_mask = pci_msi_mask_irq, > - .irq_unmask = pci_msi_unmask_irq, > }; > > static struct msi_domain_info armada_370_xp_msi_domain_info = { > It looks to me that masking at the PCI level is rather superfluous as long as the MSI controller HW has the capability to mask the interrupt on a per MSI basis. After all, most non MSI-X endpoint lack support for masking of individual vectors, so I think that we should just mask things at the irqchip level. This is also consistent with what you'd have to do for non-PCI MSI, where nothing standardises the MSI masking. I think this is in effect a split in responsibilities: - the end-point driver should (directly or indirectly) control the interrupt generation at the end-point level, - the MSI controller driver should control the signalling of the MSI to the CPU. The only case where we should rely on masking interrupts at the end-point level is when the MSI controller doesn't provide a method to do so (hopefully a rare exception). To take the example of the gicv2m driver that you mention above, I'd suggest the following: diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c index 3c77ab676e54..2ce801207acd 100644 --- a/drivers/irqchip/irq-gic-v2m.c +++ b/drivers/irqchip/irq-gic-v2m.c @@ -72,22 +72,10 @@ struct v2m_data { u32 flags; /* v2m flags for specific implementation */ }; -static void gicv2m_mask_msi_irq(struct irq_data *d) -{ - pci_msi_mask_irq(d); - irq_chip_mask_parent(d); -} - -static void gicv2m_unmask_msi_irq(struct irq_data *d) -{ - pci_msi_unmask_irq(d); - irq_chip_unmask_parent(d); -} - static struct irq_chip gicv2m_msi_irq_chip = { .name = "MSI", - .irq_mask = gicv2m_mask_msi_irq, - .irq_unmask = gicv2m_unmask_msi_irq, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, .irq_eoi = irq_chip_eoi_parent, .irq_write_msi_msg = pci_msi_domain_write_msg, }; The same should be applied to a number of drivers in the tree, which seem to have cargo-culted the wrong idiom (and I take responsibility for that). Thanks, M.
On Thu, 2019-06-13 at 10:22 +0100, Marc Zyngier wrote: > > It looks to me that masking at the PCI level is rather superfluous as > long as the MSI controller HW has the capability to mask the interrupt > on a per MSI basis. After all, most non MSI-X endpoint lack support > for masking of individual vectors, so I think that we should just mask > things at the irqchip level. This is also consistent with what you'd > have to do for non-PCI MSI, where nothing standardises the MSI > masking. > > I think this is in effect a split in responsibilities: > > - the end-point driver should (directly or indirectly) control the > interrupt generation at the end-point level, > > - the MSI controller driver should control the signalling of the MSI > to the CPU. > > The only case where we should rely on masking interrupts at the > end-point level is when the MSI controller doesn't provide a method to > do so (hopefully a rare exception). While I would tend to agree, I'm also wary of standardizing on something which isn't what x86 does today :-) You know what happens when we break them... interestingly enough they (like quite a few other drivers) don't even bother trying to mask at the APIC level unless I misread the code. That means that for endpoints that don't support masking, they just get those MSIs and "ignore" them... But I'll look into it, see what the patch looks like. I've also looked at trying to make the "inner domain" more generic but that's looking a tad trickier... not giving up yet though :-) Cheers, Ben.
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c index c9bdc5221b82..911230f28e2d 100644 --- a/drivers/irqchip/irq-armada-370-xp.c +++ b/drivers/irqchip/irq-armada-370-xp.c @@ -197,8 +197,6 @@ static void armada_370_xp_irq_unmask(struct irq_data *d) static struct irq_chip armada_370_xp_msi_irq_chip = { .name = "MPIC MSI", - .irq_mask = pci_msi_mask_irq, - .irq_unmask = pci_msi_unmask_irq, }; static struct msi_domain_info armada_370_xp_msi_domain_info = {
pci_msi_create_irq_domain -> pci_msi_domain_update_chip_ops will set those two already since the driver sets MSI_FLAG_USE_DEF_CHIP_OPS Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- [UNTESTED] Just something I noticed while browsing through those drivers in search of ways to factor some of the code. That leads to a question here: Some MSI drivers such as this one (or any using the defaults mask/unmask provided by drivers/pci/msi.c) only call the PCI MSI mask/unmask functions. Some other drivers call those PCI function but *also* call the parent mask/unmask (giv-v2m for example) which generally is the inner domain which just itself forwards to its own parent. Is there any preference for doing it one way or the other ? I can see that in cases where the device doesn't support MSI masking, calling the parent could be useful but we don't know that at the moment in the corresponding code. It feels like something we should consolidate (and remove code from drivers). For example, the defaults in drivers/pci/msi.c could always call the parent if it exists and has a mask/unmask callback. Opinions ? I'm happy to produce patches once we agree...