[PATCH] PCI: set flag IRQCHIP_ONESHOT_SAFE for PCI-MSI irqchip's

[PATCH] PCI: set flag IRQCHIP_ONESHOT_SAFE for PCI-MSI irqchip's

[Heiner Kallweit]

PCI-MSI is oneshot-safe, therefore set flag IRQCHIP_ONESHOT_SAFE to
avoid unneeded masking/unmasking. See also discussion here:
https://marc.info/?l=linux-pci&m=153332526101128&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/msi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 4d88afdf..f2ef8964 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1446,6 +1446,9 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
 	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
 		info->flags |= MSI_FLAG_MUST_REACTIVATE;
 
+	/* PCI-MSI is oneshot-safe */
+	info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
+
 	domain = msi_create_irq_domain(fwnode, info, parent);
 	if (!domain)
 		return NULL;
-- 
2.18.0

Re: [PATCH] PCI: set flag IRQCHIP_ONESHOT_SAFE for PCI-MSI irqchip's

[Thomas Gleixner]

On Fri, 3 Aug 2018, Heiner Kallweit wrote:

> PCI-MSI is oneshot-safe, therefore set flag IRQCHIP_ONESHOT_SAFE to
> avoid unneeded masking/unmasking. See also discussion here:
> https://marc.info/?l=linux-pci&m=153332526101128&w=2

This changelog really wants a bit more detailed information.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/msi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 4d88afdf..f2ef8964 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1446,6 +1446,9 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
>  		info->flags |= MSI_FLAG_MUST_REACTIVATE;
>  
> +	/* PCI-MSI is oneshot-safe */
> +	info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
> +
>  	domain = msi_create_irq_domain(fwnode, info, parent);
>  	if (!domain)
>  		return NULL;
> -- 
> 2.18.0
> 
> 

[PATCH v2] PCI: set flag IRQCHIP_ONESHOT_SAFE for PCI-MSI irqchip's

[Heiner Kallweit]

If flag IRQCHIP_ONESHOT_SAFE isn't set for an irqchip and we have a
threaded interrupt with no primary handler, then flag IRQF_ONESHOT
needs to be set for the interrupt, causing some overhead in the
threaded interrupt handler. For more detailed explanation also check
following comment in __setup_irq():

The interrupt was requested with handler = NULL, so we use the default
primary handler for it. But it does not have the oneshot flag set.
In combination with level interrupts this is deadly, because the
default primary handler just wakes the thread, then the irq lines is
reenabled, but the device still has the level irq asserted.
Rinse and repeat....
While this works for edge type interrupts, we play it safe and reject
unconditionally because we can't say for sure which type this interrupt
really has. The type flags are unreliable as the underlying chip
implementation can override them.

Another comment in __setup_irq() gives a hint already that this
overhead can be avoided for PCI-MSI:

Some irq chips like MSI based interrupts are per se one shot safe.
Check the chip flags, so we can avoid the unmask dance at the end
of the threaded handler for those.

Following this let's mark all PCI-MSI irqchip's as oneshot-safe.

See also discussion here:
https://marc.info/?l=linux-pci&m=153332526101128&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- extended commit message
---
 drivers/pci/msi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 4d88afdf..f2ef8964 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1446,6 +1446,9 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
 	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
 		info->flags |= MSI_FLAG_MUST_REACTIVATE;
 
+	/* PCI-MSI is oneshot-safe */
+	info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
+
 	domain = msi_create_irq_domain(fwnode, info, parent);
 	if (!domain)
 		return NULL;
-- 
2.18.0

Re: [PATCH v2] PCI: set flag IRQCHIP_ONESHOT_SAFE for PCI-MSI irqchip's

[Bjorn Helgaas]

On Sun, Aug 05, 2018 at 10:31:03PM +0200, Heiner Kallweit wrote:
> If flag IRQCHIP_ONESHOT_SAFE isn't set for an irqchip and we have a
> threaded interrupt with no primary handler, then flag IRQF_ONESHOT
> needs to be set for the interrupt, causing some overhead in the
> threaded interrupt handler. For more detailed explanation also check
> following comment in __setup_irq():
> 
> The interrupt was requested with handler = NULL, so we use the default
> primary handler for it. But it does not have the oneshot flag set.
> In combination with level interrupts this is deadly, because the
> default primary handler just wakes the thread, then the irq lines is
> reenabled, but the device still has the level irq asserted.
> Rinse and repeat....
> While this works for edge type interrupts, we play it safe and reject
> unconditionally because we can't say for sure which type this interrupt
> really has. The type flags are unreliable as the underlying chip
> implementation can override them.
> 
> Another comment in __setup_irq() gives a hint already that this
> overhead can be avoided for PCI-MSI:
> 
> Some irq chips like MSI based interrupts are per se one shot safe.
> Check the chip flags, so we can avoid the unmask dance at the end
> of the threaded handler for those.
> 
> Following this let's mark all PCI-MSI irqchip's as oneshot-safe.
> 
> See also discussion here:
> https://marc.info/?l=linux-pci&m=153332526101128&w=2
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to pci/msi for v4.19, thanks!

> ---
> v2:
> - extended commit message
> ---
>  drivers/pci/msi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 4d88afdf..f2ef8964 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1446,6 +1446,9 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
>  		info->flags |= MSI_FLAG_MUST_REACTIVATE;
>  
> +	/* PCI-MSI is oneshot-safe */
> +	info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
> +
>  	domain = msi_create_irq_domain(fwnode, info, parent);
>  	if (!domain)
>  		return NULL;
> -- 
> 2.18.0
> 
>