[PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Benjamin Herrenschmidt]

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 = {

[PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Benjamin Herrenschmidt]

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 = {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Benjamin Herrenschmidt]

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.

Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Benjamin Herrenschmidt]

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.




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Marc Zyngier]

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.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Marc Zyngier]

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.

-- 
Jazz is not dead, it just smells funny.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Benjamin Herrenschmidt]

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.

Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Benjamin Herrenschmidt]

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.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel