diff mbox series

[v2,RFC] genirq/cpuhotplug, PCI/rcar-host: Silence set affinity failed warning

Message ID 20240714122405.27548-1-marek.vasut+renesas@mailbox.org (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [v2,RFC] genirq/cpuhotplug, PCI/rcar-host: Silence set affinity failed warning | expand

Commit Message

Marek Vasut July 14, 2024, 12:23 p.m. UTC
This is an RFC patch, I am looking for input on the approach taken here.
If the approach is sound, this patch would be split into proper patchset.

Various PCIe controllers that mux MSIs onto single IRQ line produce these
"IRQ%d: set affinity failed" warnings when entering suspend. This has been
discussed before [1] [2] and an example test case is included at the end
of this commit message.

Controller drivers which create MSI IRQ domain with MSI_FLAG_USE_DEF_CHIP_OPS
flag set and which do not override the .irq_set_affinity irqchip callback get
assigned default .irq_set_affinity = msi_domain_set_affinity() callback. That
is not desired on controllers where it is not possible to set affinity of each
MSI IRQ line to a specific CPU core due to hardware limitation.

Introduce flag MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF, which keeps .irq_set_affinity
unset in case the controller driver did not assign the callback. This way, the
migrate_one_irq() code in cpuhotplug.c can exit right away, without printing
the aforementioned warning. The .irq_set_affinity implementations which only
return -EINVAL can be removed from multiple controller drivers.

```
$ grep 25 /proc/interrupts
 25:   0 0 0 0 0 0 0 0   PCIe MSI   0   Edge   PCIe PME

$ echo core > /sys/power/pm_test ; echo mem > /sys/power/state
...
Disabling non-boot CPUs ...
IRQ25: set affinity failed(-22). <---------- This is being silenced here
psci: CPU7 killed (polled 4 ms)
...
```

[1] https://lore.kernel.org/all/d4a6eea3c5e33a3a4056885419df95a7@kernel.org/
[2] https://lore.kernel.org/all/5f4947b18bf381615a37aa81c2242477@kernel.org/

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Krzysztof WilczyƄski" <kw@linux.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Cc: Jon Hunter <jonathanh@nvidia.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
V2: - Introduce MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF to inhibit assignment
      of msi_domain_set_affinity()
---
 drivers/base/platform-msi.c             | 2 +-
 drivers/pci/controller/pcie-rcar-host.c | 8 +-------
 include/linux/msi.h                     | 2 ++
 kernel/irq/msi.c                        | 2 +-
 4 files changed, 5 insertions(+), 9 deletions(-)

Comments

Thomas Gleixner July 14, 2024, 7:14 p.m. UTC | #1
Marek!

On Sun, Jul 14 2024 at 14:23, Marek Vasut wrote:
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -100,7 +100,7 @@ static void platform_msi_update_chip_ops(struct msi_domain_info *info)
>  		chip->irq_unmask = irq_chip_unmask_parent;
>  	if (!chip->irq_eoi)
>  		chip->irq_eoi = irq_chip_eoi_parent;
> -	if (!chip->irq_set_affinity)
> +	if (!chip->irq_set_affinity && !(info->flags & MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF))
>  		chip->irq_set_affinity = msi_domain_set_affinity;

I'm not really a fan of this new flag.

I'd rather leave MSI_FLAG_USE_DEF_CHIP_OPS alone and introduce a
dedicated flag MSI_FLAG_NO_AFFINITY to tell the core code that these
interrupts cannot be steered. Other than that this approach looks sane.

Thanks,

        tglx
Marek Vasut July 15, 2024, 12:26 p.m. UTC | #2
On 7/14/24 9:14 PM, Thomas Gleixner wrote:
> Marek!

Hello Thomas,

> On Sun, Jul 14 2024 at 14:23, Marek Vasut wrote:
>> --- a/drivers/base/platform-msi.c
>> +++ b/drivers/base/platform-msi.c
>> @@ -100,7 +100,7 @@ static void platform_msi_update_chip_ops(struct msi_domain_info *info)
>>   		chip->irq_unmask = irq_chip_unmask_parent;
>>   	if (!chip->irq_eoi)
>>   		chip->irq_eoi = irq_chip_eoi_parent;
>> -	if (!chip->irq_set_affinity)
>> +	if (!chip->irq_set_affinity && !(info->flags & MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF))
>>   		chip->irq_set_affinity = msi_domain_set_affinity;
> 
> I'm not really a fan of this new flag.
> 
> I'd rather leave MSI_FLAG_USE_DEF_CHIP_OPS alone and introduce a
> dedicated flag MSI_FLAG_NO_AFFINITY to tell the core code that these
> interrupts cannot be steered. Other than that this approach looks sane.

Should be fixed in V3. I also converted Tegra so far and sent it 
alongside V3. There are many more controllers to convert that will follow.
diff mbox series

Patch

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 11f5fdf65b9ef..91e8a9e3d9610 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -100,7 +100,7 @@  static void platform_msi_update_chip_ops(struct msi_domain_info *info)
 		chip->irq_unmask = irq_chip_unmask_parent;
 	if (!chip->irq_eoi)
 		chip->irq_eoi = irq_chip_eoi_parent;
-	if (!chip->irq_set_affinity)
+	if (!chip->irq_set_affinity && !(info->flags & MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF))
 		chip->irq_set_affinity = msi_domain_set_affinity;
 	if (!chip->irq_write_msi_msg)
 		chip->irq_write_msi_msg = platform_msi_write_msg;
diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index c01efc6ea64f6..e898732159c04 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -658,11 +658,6 @@  static void rcar_msi_irq_unmask(struct irq_data *d)
 	spin_unlock_irqrestore(&msi->mask_lock, flags);
 }
 
-static int rcar_msi_set_affinity(struct irq_data *d, const struct cpumask *mask, bool force)
-{
-	return -EINVAL;
-}
-
 static void rcar_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct rcar_msi *msi = irq_data_get_irq_chip_data(data);
@@ -678,7 +673,6 @@  static struct irq_chip rcar_msi_bottom_chip = {
 	.irq_ack		= rcar_msi_irq_ack,
 	.irq_mask		= rcar_msi_irq_mask,
 	.irq_unmask		= rcar_msi_irq_unmask,
-	.irq_set_affinity 	= rcar_msi_set_affinity,
 	.irq_compose_msi_msg	= rcar_compose_msi_msg,
 };
 
@@ -725,7 +719,7 @@  static const struct irq_domain_ops rcar_msi_domain_ops = {
 };
 
 static struct msi_domain_info rcar_msi_info = {
-	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF |
 		   MSI_FLAG_MULTI_PCI_MSI),
 	.chip	= &rcar_msi_top_chip,
 };
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 26588da88bdd8..ab79d772fd43d 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -573,6 +573,8 @@  enum {
 	MSI_FLAG_MSIX_CONTIGUOUS	= (1 << 19),
 	/* PCI/MSI-X vectors can be dynamically allocated/freed post MSI-X enable */
 	MSI_FLAG_PCI_MSIX_ALLOC_DYN	= (1 << 20),
+	/* Same as MSI_FLAG_USE_DEF_CHIP_OPS , but does not set .irq_set_affinity */
+	MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF	= ((1 << 21) | MSI_FLAG_USE_DEF_CHIP_OPS),
 };
 
 /**
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 2024f89baea4b..877365eeab262 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -816,7 +816,7 @@  static void msi_domain_update_chip_ops(struct msi_domain_info *info)
 	struct irq_chip *chip = info->chip;
 
 	BUG_ON(!chip || !chip->irq_mask || !chip->irq_unmask);
-	if (!chip->irq_set_affinity)
+	if (!chip->irq_set_affinity && !(info->flags & MSI_FLAG_USE_DEF_CHIP_OPS_NOAFF))
 		chip->irq_set_affinity = msi_domain_set_affinity;
 }