All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-17 13:14 Thomas Gleixner
  2020-07-17 15:25 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-17 13:14 UTC (permalink / raw)
  To: LKML; +Cc: x86, Ben Herrenschmidt, Ali Saidi, Marc Zyngier, linux-arm-kernel

Setting interrupt affinity on inactive interrupts is inconsistent when
hierarchical irq domains are enabled. The core code should just store the
affinity and not call into the irq chip driver for inactive interrupts
because the chip drivers may not be in a state to handle such requests.

X86 has a hacky workaround for that but all other irq chips have not which
causes problems e.g. on GIC V3 ITS.

Instead of adding more ugly hacks all over the place, solve the problem in
the core code. If the affinity is set on an inactive interrupt then:

    - Store it in the irq descriptors affinity mask
    - Update the effective affinity to reflect that so user space has
      a consistent view
    - Don't call into the irq chip driver

This is the core equivalent of the X86 workaround and works correctly
because the affinity setting is established in the irq chip when the
interrupt is activated later on.

Note, that this is only effective when hierarchical irq domains are enabled
by the architecture. Doing it unconditionally would break legacy irq chip
implementations.

For hierarchial irq domains this works correctly as none of the drivers can
have a dependency on affinity setting in inactive state by design.

Remove the X86 workaround as it is not longer required.

Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive interrupts")
Reported-by: Ali Saidi <alisaidi@amazon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20200529015501.15771-1-alisaidi@amazon.com
---
---
 arch/x86/kernel/apic/vector.c |   22 +++++-----------------
 kernel/irq/manage.c           |   37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 19 deletions(-)

--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
 	trace_vector_activate(irqd->irq, apicd->is_managed,
 			      apicd->can_reserve, reserve);
 
-	/* Nothing to do for fixed assigned vectors */
-	if (!apicd->can_reserve && !apicd->is_managed)
-		return 0;
-
 	raw_spin_lock_irqsave(&vector_lock, flags);
-	if (reserve || irqd_is_managed_and_shutdown(irqd))
+	if (!apicd->can_reserve && !apicd->is_managed)
+		assign_irq_vector_any_locked(irqd);
+	else if (reserve || irqd_is_managed_and_shutdown(irqd))
 		vector_assign_managed_shutdown(irqd);
 	else if (apicd->is_managed)
 		ret = activate_managed(irqd);
@@ -775,20 +773,10 @@ void lapic_offline(void)
 static int apic_set_affinity(struct irq_data *irqd,
 			     const struct cpumask *dest, bool force)
 {
-	struct apic_chip_data *apicd = apic_chip_data(irqd);
 	int err;
 
-	/*
-	 * Core code can call here for inactive interrupts. For inactive
-	 * interrupts which use managed or reservation mode there is no
-	 * point in going through the vector assignment right now as the
-	 * activation will assign a vector which fits the destination
-	 * cpumask. Let the core code store the destination mask and be
-	 * done with it.
-	 */
-	if (!irqd_is_activated(irqd) &&
-	    (apicd->is_managed || apicd->can_reserve))
-		return IRQ_SET_MASK_OK;
+	if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
+		return -EIO;
 
 	raw_spin_lock(&vector_lock);
 	cpumask_and(vector_searchmask, dest, cpu_online_mask);
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -195,9 +195,9 @@ void irq_set_thread_affinity(struct irq_
 			set_bit(IRQTF_AFFINITY, &action->thread_flags);
 }
 
+#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 static void irq_validate_effective_affinity(struct irq_data *data)
 {
-#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	const struct cpumask *m = irq_data_get_effective_affinity_mask(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 
@@ -205,9 +205,19 @@ static void irq_validate_effective_affin
 		return;
 	pr_warn_once("irq_chip %s did not update eff. affinity mask of irq %u\n",
 		     chip->name, data->irq);
-#endif
 }
 
+static inline void irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask)
+{
+	cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
+}
+#else
+static inline void irq_validate_effective_affinity(struct irq_data *data) { }
+static inline boot irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask) { }
+#endif
+
 int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 			bool force)
 {
@@ -304,6 +314,26 @@ static int irq_try_set_affinity(struct i
 	return ret;
 }
 
+static bool irq_set_affinity_deactivated(struct irq_data *data,
+					 const struct cpumask *mask, bool force)
+{
+	struct irq_desc *desc = irq_data_to_desc(data);
+
+	/*
+	 * If the interrupt is not yet activated, just store the affinity
+	 * mask and do not call the chip driver at all. On activation the
+	 * driver has to make sure anyway that the interrupt is in a
+	 * useable state so startup works.
+	 */
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+		return false;
+
+	cpumask_copy(desc->irq_common_data.affinity, mask);
+	irq_init_effective_affinity(data, mask);
+	irqd_set(data, IRQD_AFFINITY_SET);
+	return true;
+}
+
 int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 			    bool force)
 {
@@ -314,6 +344,9 @@ int irq_set_affinity_locked(struct irq_d
 	if (!chip || !chip->irq_set_affinity)
 		return -EINVAL;
 
+	if (irq_set_affinity_deactivated(data, mask, force))
+		return 0;
+
 	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
 		ret = irq_try_set_affinity(data, mask, force);
 	} else {

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-17 13:14 Thomas Gleixner
  2020-07-17 15:25 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-17 13:14 UTC (permalink / raw)
  To: LKML; +Cc: Ben Herrenschmidt, Ali Saidi, x86, linux-arm-kernel, Marc Zyngier

Setting interrupt affinity on inactive interrupts is inconsistent when
hierarchical irq domains are enabled. The core code should just store the
affinity and not call into the irq chip driver for inactive interrupts
because the chip drivers may not be in a state to handle such requests.

X86 has a hacky workaround for that but all other irq chips have not which
causes problems e.g. on GIC V3 ITS.

Instead of adding more ugly hacks all over the place, solve the problem in
the core code. If the affinity is set on an inactive interrupt then:

    - Store it in the irq descriptors affinity mask
    - Update the effective affinity to reflect that so user space has
      a consistent view
    - Don't call into the irq chip driver

This is the core equivalent of the X86 workaround and works correctly
because the affinity setting is established in the irq chip when the
interrupt is activated later on.

Note, that this is only effective when hierarchical irq domains are enabled
by the architecture. Doing it unconditionally would break legacy irq chip
implementations.

For hierarchial irq domains this works correctly as none of the drivers can
have a dependency on affinity setting in inactive state by design.

Remove the X86 workaround as it is not longer required.

Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive interrupts")
Reported-by: Ali Saidi <alisaidi@amazon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20200529015501.15771-1-alisaidi@amazon.com
---
---
 arch/x86/kernel/apic/vector.c |   22 +++++-----------------
 kernel/irq/manage.c           |   37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 19 deletions(-)

--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
 	trace_vector_activate(irqd->irq, apicd->is_managed,
 			      apicd->can_reserve, reserve);
 
-	/* Nothing to do for fixed assigned vectors */
-	if (!apicd->can_reserve && !apicd->is_managed)
-		return 0;
-
 	raw_spin_lock_irqsave(&vector_lock, flags);
-	if (reserve || irqd_is_managed_and_shutdown(irqd))
+	if (!apicd->can_reserve && !apicd->is_managed)
+		assign_irq_vector_any_locked(irqd);
+	else if (reserve || irqd_is_managed_and_shutdown(irqd))
 		vector_assign_managed_shutdown(irqd);
 	else if (apicd->is_managed)
 		ret = activate_managed(irqd);
@@ -775,20 +773,10 @@ void lapic_offline(void)
 static int apic_set_affinity(struct irq_data *irqd,
 			     const struct cpumask *dest, bool force)
 {
-	struct apic_chip_data *apicd = apic_chip_data(irqd);
 	int err;
 
-	/*
-	 * Core code can call here for inactive interrupts. For inactive
-	 * interrupts which use managed or reservation mode there is no
-	 * point in going through the vector assignment right now as the
-	 * activation will assign a vector which fits the destination
-	 * cpumask. Let the core code store the destination mask and be
-	 * done with it.
-	 */
-	if (!irqd_is_activated(irqd) &&
-	    (apicd->is_managed || apicd->can_reserve))
-		return IRQ_SET_MASK_OK;
+	if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
+		return -EIO;
 
 	raw_spin_lock(&vector_lock);
 	cpumask_and(vector_searchmask, dest, cpu_online_mask);
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -195,9 +195,9 @@ void irq_set_thread_affinity(struct irq_
 			set_bit(IRQTF_AFFINITY, &action->thread_flags);
 }
 
+#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 static void irq_validate_effective_affinity(struct irq_data *data)
 {
-#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	const struct cpumask *m = irq_data_get_effective_affinity_mask(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 
@@ -205,9 +205,19 @@ static void irq_validate_effective_affin
 		return;
 	pr_warn_once("irq_chip %s did not update eff. affinity mask of irq %u\n",
 		     chip->name, data->irq);
-#endif
 }
 
+static inline void irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask)
+{
+	cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
+}
+#else
+static inline void irq_validate_effective_affinity(struct irq_data *data) { }
+static inline boot irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask) { }
+#endif
+
 int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 			bool force)
 {
@@ -304,6 +314,26 @@ static int irq_try_set_affinity(struct i
 	return ret;
 }
 
+static bool irq_set_affinity_deactivated(struct irq_data *data,
+					 const struct cpumask *mask, bool force)
+{
+	struct irq_desc *desc = irq_data_to_desc(data);
+
+	/*
+	 * If the interrupt is not yet activated, just store the affinity
+	 * mask and do not call the chip driver at all. On activation the
+	 * driver has to make sure anyway that the interrupt is in a
+	 * useable state so startup works.
+	 */
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+		return false;
+
+	cpumask_copy(desc->irq_common_data.affinity, mask);
+	irq_init_effective_affinity(data, mask);
+	irqd_set(data, IRQD_AFFINITY_SET);
+	return true;
+}
+
 int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 			    bool force)
 {
@@ -314,6 +344,9 @@ int irq_set_affinity_locked(struct irq_d
 	if (!chip || !chip->irq_set_affinity)
 		return -EINVAL;
 
+	if (irq_set_affinity_deactivated(data, mask, force))
+		return 0;
+
 	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
 		ret = irq_try_set_affinity(data, mask, force);
 	} else {

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-17 13:14 [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly Thomas Gleixner
@ 2020-07-17 15:25 ` kernel test robot
  2020-07-17 16:00 ` [PATCH V2] " Thomas Gleixner
  2020-07-17 18:04 ` [PATCH] " kernel test robot
  2 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-07-17 15:25 UTC (permalink / raw)
  To: Thomas Gleixner, LKML
  Cc: kbuild-all, x86, Ben Herrenschmidt, Ali Saidi, Marc Zyngier,
	linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on tip/x86/core tip/irq/core linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Thomas-Gleixner/genirq-affinity-Handle-affinity-setting-on-inactive-interrupts-correctly/20200717-211602
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68
config: parisc-defconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> kernel/irq/manage.c:217:15: error: unknown type name 'boot'
     217 | static inline boot irq_init_effective_affinity(struct irq_data *data,
         |               ^~~~
   kernel/irq/manage.c: In function 'irq_init_effective_affinity':
>> kernel/irq/manage.c:218:26: warning: no return statement in function returning non-void [-Wreturn-type]
     218 |             const struct cpumask *mask) { }
         |                          ^~~~~~~

vim +/boot +217 kernel/irq/manage.c

   209	
   210	static inline void irq_init_effective_affinity(struct irq_data *data,
   211						       const struct cpumask *mask)
   212	{
   213		cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
   214	}
   215	#else
   216	static inline void irq_validate_effective_affinity(struct irq_data *data) { }
 > 217	static inline boot irq_init_effective_affinity(struct irq_data *data,
 > 218						       const struct cpumask *mask) { }
   219	#endif
   220	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18612 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-17 15:25 ` kernel test robot
  0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-07-17 15:25 UTC (permalink / raw)
  To: Thomas Gleixner, LKML
  Cc: Ben Herrenschmidt, Ali Saidi, kbuild-all, Marc Zyngier, x86,
	linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on tip/x86/core tip/irq/core linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Thomas-Gleixner/genirq-affinity-Handle-affinity-setting-on-inactive-interrupts-correctly/20200717-211602
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68
config: parisc-defconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> kernel/irq/manage.c:217:15: error: unknown type name 'boot'
     217 | static inline boot irq_init_effective_affinity(struct irq_data *data,
         |               ^~~~
   kernel/irq/manage.c: In function 'irq_init_effective_affinity':
>> kernel/irq/manage.c:218:26: warning: no return statement in function returning non-void [-Wreturn-type]
     218 |             const struct cpumask *mask) { }
         |                          ^~~~~~~

vim +/boot +217 kernel/irq/manage.c

   209	
   210	static inline void irq_init_effective_affinity(struct irq_data *data,
   211						       const struct cpumask *mask)
   212	{
   213		cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
   214	}
   215	#else
   216	static inline void irq_validate_effective_affinity(struct irq_data *data) { }
 > 217	static inline boot irq_init_effective_affinity(struct irq_data *data,
 > 218						       const struct cpumask *mask) { }
   219	#endif
   220	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18612 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-17 13:14 [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly Thomas Gleixner
  2020-07-17 15:25 ` kernel test robot
@ 2020-07-17 16:00 ` Thomas Gleixner
  2020-07-24 17:24   ` John Keeping
  2020-07-17 18:04 ` [PATCH] " kernel test robot
  2 siblings, 1 reply; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-17 16:00 UTC (permalink / raw)
  To: LKML; +Cc: x86, Ben Herrenschmidt, Ali Saidi, Marc Zyngier, linux-arm-kernel

Setting interrupt affinity on inactive interrupts is inconsistent when
hierarchical irq domains are enabled. The core code should just store the
affinity and not call into the irq chip driver for inactive interrupts
because the chip drivers may not be in a state to handle such requests.

X86 has a hacky workaround for that but all other irq chips have not which
causes problems e.g. on GIC V3 ITS.

Instead of adding more ugly hacks all over the place, solve the problem in
the core code. If the affinity is set on an inactive interrupt then:

    - Store it in the irq descriptors affinity mask
    - Update the effective affinity to reflect that so user space has
      a consistent view
    - Don't call into the irq chip driver

This is the core equivalent of the X86 workaround and works correctly
because the affinity setting is established in the irq chip when the
interrupt is activated later on.

Note, that this is only effective when hierarchical irq domains are enabled
by the architecture. Doing it unconditionally would break legacy irq chip
implementations.

For hierarchial irq domains this works correctly as none of the drivers can
have a dependency on affinity setting in inactive state by design.

Remove the X86 workaround as it is not longer required.

Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive interrupts")
Reported-by: Ali Saidi <alisaidi@amazon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20200529015501.15771-1-alisaidi@amazon.com
---
V2: Fix the fallout for CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n (0day)
---
 arch/x86/kernel/apic/vector.c |   22 +++++-----------------
 kernel/irq/manage.c           |   37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 19 deletions(-)

--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
 	trace_vector_activate(irqd->irq, apicd->is_managed,
 			      apicd->can_reserve, reserve);
 
-	/* Nothing to do for fixed assigned vectors */
-	if (!apicd->can_reserve && !apicd->is_managed)
-		return 0;
-
 	raw_spin_lock_irqsave(&vector_lock, flags);
-	if (reserve || irqd_is_managed_and_shutdown(irqd))
+	if (!apicd->can_reserve && !apicd->is_managed)
+		assign_irq_vector_any_locked(irqd);
+	else if (reserve || irqd_is_managed_and_shutdown(irqd))
 		vector_assign_managed_shutdown(irqd);
 	else if (apicd->is_managed)
 		ret = activate_managed(irqd);
@@ -775,20 +773,10 @@ void lapic_offline(void)
 static int apic_set_affinity(struct irq_data *irqd,
 			     const struct cpumask *dest, bool force)
 {
-	struct apic_chip_data *apicd = apic_chip_data(irqd);
 	int err;
 
-	/*
-	 * Core code can call here for inactive interrupts. For inactive
-	 * interrupts which use managed or reservation mode there is no
-	 * point in going through the vector assignment right now as the
-	 * activation will assign a vector which fits the destination
-	 * cpumask. Let the core code store the destination mask and be
-	 * done with it.
-	 */
-	if (!irqd_is_activated(irqd) &&
-	    (apicd->is_managed || apicd->can_reserve))
-		return IRQ_SET_MASK_OK;
+	if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
+		return -EIO;
 
 	raw_spin_lock(&vector_lock);
 	cpumask_and(vector_searchmask, dest, cpu_online_mask);
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -195,9 +195,9 @@ void irq_set_thread_affinity(struct irq_
 			set_bit(IRQTF_AFFINITY, &action->thread_flags);
 }
 
+#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 static void irq_validate_effective_affinity(struct irq_data *data)
 {
-#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	const struct cpumask *m = irq_data_get_effective_affinity_mask(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 
@@ -205,9 +205,19 @@ static void irq_validate_effective_affin
 		return;
 	pr_warn_once("irq_chip %s did not update eff. affinity mask of irq %u\n",
 		     chip->name, data->irq);
-#endif
 }
 
+static inline void irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask)
+{
+	cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
+}
+#else
+static inline void irq_validate_effective_affinity(struct irq_data *data) { }
+static inline void irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask) { }
+#endif
+
 int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 			bool force)
 {
@@ -304,6 +314,26 @@ static int irq_try_set_affinity(struct i
 	return ret;
 }
 
+static bool irq_set_affinity_deactivated(struct irq_data *data,
+					 const struct cpumask *mask, bool force)
+{
+	struct irq_desc *desc = irq_data_to_desc(data);
+
+	/*
+	 * If the interrupt is not yet activated, just store the affinity
+	 * mask and do not call the chip driver at all. On activation the
+	 * driver has to make sure anyway that the interrupt is in a
+	 * useable state so startup works.
+	 */
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+		return false;
+
+	cpumask_copy(desc->irq_common_data.affinity, mask);
+	irq_init_effective_affinity(data, mask);
+	irqd_set(data, IRQD_AFFINITY_SET);
+	return true;
+}
+
 int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 			    bool force)
 {
@@ -314,6 +344,9 @@ int irq_set_affinity_locked(struct irq_d
 	if (!chip || !chip->irq_set_affinity)
 		return -EINVAL;
 
+	if (irq_set_affinity_deactivated(data, mask, force))
+		return 0;
+
 	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
 		ret = irq_try_set_affinity(data, mask, force);
 	} else {

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-17 16:00 ` Thomas Gleixner
  2020-07-24 17:24   ` John Keeping
  0 siblings, 1 reply; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-17 16:00 UTC (permalink / raw)
  To: LKML; +Cc: Ben Herrenschmidt, x86, linux-arm-kernel, Ali Saidi, Marc Zyngier

Setting interrupt affinity on inactive interrupts is inconsistent when
hierarchical irq domains are enabled. The core code should just store the
affinity and not call into the irq chip driver for inactive interrupts
because the chip drivers may not be in a state to handle such requests.

X86 has a hacky workaround for that but all other irq chips have not which
causes problems e.g. on GIC V3 ITS.

Instead of adding more ugly hacks all over the place, solve the problem in
the core code. If the affinity is set on an inactive interrupt then:

    - Store it in the irq descriptors affinity mask
    - Update the effective affinity to reflect that so user space has
      a consistent view
    - Don't call into the irq chip driver

This is the core equivalent of the X86 workaround and works correctly
because the affinity setting is established in the irq chip when the
interrupt is activated later on.

Note, that this is only effective when hierarchical irq domains are enabled
by the architecture. Doing it unconditionally would break legacy irq chip
implementations.

For hierarchial irq domains this works correctly as none of the drivers can
have a dependency on affinity setting in inactive state by design.

Remove the X86 workaround as it is not longer required.

Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive interrupts")
Reported-by: Ali Saidi <alisaidi@amazon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20200529015501.15771-1-alisaidi@amazon.com
---
V2: Fix the fallout for CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n (0day)
---
 arch/x86/kernel/apic/vector.c |   22 +++++-----------------
 kernel/irq/manage.c           |   37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 19 deletions(-)

--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
 	trace_vector_activate(irqd->irq, apicd->is_managed,
 			      apicd->can_reserve, reserve);
 
-	/* Nothing to do for fixed assigned vectors */
-	if (!apicd->can_reserve && !apicd->is_managed)
-		return 0;
-
 	raw_spin_lock_irqsave(&vector_lock, flags);
-	if (reserve || irqd_is_managed_and_shutdown(irqd))
+	if (!apicd->can_reserve && !apicd->is_managed)
+		assign_irq_vector_any_locked(irqd);
+	else if (reserve || irqd_is_managed_and_shutdown(irqd))
 		vector_assign_managed_shutdown(irqd);
 	else if (apicd->is_managed)
 		ret = activate_managed(irqd);
@@ -775,20 +773,10 @@ void lapic_offline(void)
 static int apic_set_affinity(struct irq_data *irqd,
 			     const struct cpumask *dest, bool force)
 {
-	struct apic_chip_data *apicd = apic_chip_data(irqd);
 	int err;
 
-	/*
-	 * Core code can call here for inactive interrupts. For inactive
-	 * interrupts which use managed or reservation mode there is no
-	 * point in going through the vector assignment right now as the
-	 * activation will assign a vector which fits the destination
-	 * cpumask. Let the core code store the destination mask and be
-	 * done with it.
-	 */
-	if (!irqd_is_activated(irqd) &&
-	    (apicd->is_managed || apicd->can_reserve))
-		return IRQ_SET_MASK_OK;
+	if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
+		return -EIO;
 
 	raw_spin_lock(&vector_lock);
 	cpumask_and(vector_searchmask, dest, cpu_online_mask);
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -195,9 +195,9 @@ void irq_set_thread_affinity(struct irq_
 			set_bit(IRQTF_AFFINITY, &action->thread_flags);
 }
 
+#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 static void irq_validate_effective_affinity(struct irq_data *data)
 {
-#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	const struct cpumask *m = irq_data_get_effective_affinity_mask(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 
@@ -205,9 +205,19 @@ static void irq_validate_effective_affin
 		return;
 	pr_warn_once("irq_chip %s did not update eff. affinity mask of irq %u\n",
 		     chip->name, data->irq);
-#endif
 }
 
+static inline void irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask)
+{
+	cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
+}
+#else
+static inline void irq_validate_effective_affinity(struct irq_data *data) { }
+static inline void irq_init_effective_affinity(struct irq_data *data,
+					       const struct cpumask *mask) { }
+#endif
+
 int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 			bool force)
 {
@@ -304,6 +314,26 @@ static int irq_try_set_affinity(struct i
 	return ret;
 }
 
+static bool irq_set_affinity_deactivated(struct irq_data *data,
+					 const struct cpumask *mask, bool force)
+{
+	struct irq_desc *desc = irq_data_to_desc(data);
+
+	/*
+	 * If the interrupt is not yet activated, just store the affinity
+	 * mask and do not call the chip driver at all. On activation the
+	 * driver has to make sure anyway that the interrupt is in a
+	 * useable state so startup works.
+	 */
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+		return false;
+
+	cpumask_copy(desc->irq_common_data.affinity, mask);
+	irq_init_effective_affinity(data, mask);
+	irqd_set(data, IRQD_AFFINITY_SET);
+	return true;
+}
+
 int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 			    bool force)
 {
@@ -314,6 +344,9 @@ int irq_set_affinity_locked(struct irq_d
 	if (!chip || !chip->irq_set_affinity)
 		return -EINVAL;
 
+	if (irq_set_affinity_deactivated(data, mask, force))
+		return 0;
+
 	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
 		ret = irq_try_set_affinity(data, mask, force);
 	} else {

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-17 13:14 [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly Thomas Gleixner
  2020-07-17 15:25 ` kernel test robot
  2020-07-17 16:00 ` [PATCH V2] " Thomas Gleixner
@ 2020-07-17 18:04 ` kernel test robot
  2 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-07-17 18:04 UTC (permalink / raw)
  To: Thomas Gleixner, LKML
  Cc: kbuild-all, clang-built-linux, x86, Ben Herrenschmidt, Ali Saidi,
	Marc Zyngier, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 9045 bytes --]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on tip/x86/core tip/irq/core linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Thomas-Gleixner/genirq-affinity-Handle-affinity-setting-on-inactive-interrupts-correctly/20200717-211602
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68
config: s390-randconfig-r015-20200717 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew(cpu_to_le16(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel(cpu_to_le32(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> kernel/irq/manage.c:217:15: error: unknown type name 'boot'; did you mean 'bool'?
   static inline boot irq_init_effective_affinity(struct irq_data *data,
                 ^~~~
                 bool
   include/linux/types.h:30:17: note: 'bool' declared here
   typedef _Bool                   bool;
                                   ^
   20 warnings and 1 error generated.

vim +217 kernel/irq/manage.c

   209	
   210	static inline void irq_init_effective_affinity(struct irq_data *data,
   211						       const struct cpumask *mask)
   212	{
   213		cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
   214	}
   215	#else
   216	static inline void irq_validate_effective_affinity(struct irq_data *data) { }
 > 217	static inline boot irq_init_effective_affinity(struct irq_data *data,
   218						       const struct cpumask *mask) { }
   219	#endif
   220	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24820 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-17 18:04 ` kernel test robot
  0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-07-17 18:04 UTC (permalink / raw)
  To: Thomas Gleixner, LKML
  Cc: Ben Herrenschmidt, Ali Saidi, kbuild-all, Marc Zyngier, x86,
	clang-built-linux, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 9045 bytes --]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on tip/x86/core tip/irq/core linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Thomas-Gleixner/genirq-affinity-Handle-affinity-setting-on-inactive-interrupts-correctly/20200717-211602
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68
config: s390-randconfig-r015-20200717 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from kernel/irq/manage.c:11:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew(cpu_to_le16(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel(cpu_to_le32(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> kernel/irq/manage.c:217:15: error: unknown type name 'boot'; did you mean 'bool'?
   static inline boot irq_init_effective_affinity(struct irq_data *data,
                 ^~~~
                 bool
   include/linux/types.h:30:17: note: 'bool' declared here
   typedef _Bool                   bool;
                                   ^
   20 warnings and 1 error generated.

vim +217 kernel/irq/manage.c

   209	
   210	static inline void irq_init_effective_affinity(struct irq_data *data,
   211						       const struct cpumask *mask)
   212	{
   213		cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
   214	}
   215	#else
   216	static inline void irq_validate_effective_affinity(struct irq_data *data) { }
 > 217	static inline boot irq_init_effective_affinity(struct irq_data *data,
   218						       const struct cpumask *mask) { }
   219	#endif
   220	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24820 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-17 16:00 ` [PATCH V2] " Thomas Gleixner
@ 2020-07-24 17:24   ` John Keeping
  2020-07-24 20:03     ` Thomas Gleixner
  0 siblings, 1 reply; 23+ messages in thread
From: John Keeping @ 2020-07-24 17:24 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, x86, Ben Herrenschmidt, Ali Saidi, Marc Zyngier, linux-arm-kernel

Hi,

On Fri, 17 Jul 2020 18:00:02 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Setting interrupt affinity on inactive interrupts is inconsistent when
> hierarchical irq domains are enabled. The core code should just store the
> affinity and not call into the irq chip driver for inactive interrupts
> because the chip drivers may not be in a state to handle such requests.
> 
> X86 has a hacky workaround for that but all other irq chips have not which
> causes problems e.g. on GIC V3 ITS.
> 
> Instead of adding more ugly hacks all over the place, solve the problem in
> the core code. If the affinity is set on an inactive interrupt then:
> 
>     - Store it in the irq descriptors affinity mask
>     - Update the effective affinity to reflect that so user space has
>       a consistent view
>     - Don't call into the irq chip driver
> 
> This is the core equivalent of the X86 workaround and works correctly
> because the affinity setting is established in the irq chip when the
> interrupt is activated later on.
> 
> Note, that this is only effective when hierarchical irq domains are enabled
> by the architecture. Doing it unconditionally would break legacy irq chip
> implementations.
> 
> For hierarchial irq domains this works correctly as none of the drivers can
> have a dependency on affinity setting in inactive state by design.
> 
> Remove the X86 workaround as it is not longer required.
> 
> Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive interrupts")
> Reported-by: Ali Saidi <alisaidi@amazon.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/r/20200529015501.15771-1-alisaidi@amazon.com
> ---
> V2: Fix the fallout for CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n (0day)

It seems that this patch breaks perf events on RK3288 because the PMU
interrupts that should be per-cpu are now all on CPU0 so no events are
collected from CPUs 1-3 and those interrupts are killed as spurious
after a few seconds.

I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
relevant code hasn't changed through to next-20200723.  Reverting the
backport of this change fixes the problem.

It looks like what happens is that because the interrupts are not
per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
while the interrupt is deactivated and then request_irq() with
IRQF_PERCPU | IRQF_NOBALANCING.

Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
irq_setup_affinity() which returns early because IRQF_PERCPU and
IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.

At this point /proc/interrupts clearly shows the interrupts occurring on
CPU0 despite /proc/irq/N/effective_affinity and /proc/irq/N/smp_affinity
showing them spread across the cores as expected.

I don't think I understand what's meant to happen well enough to propose
a patch, but hopefully the above explanation explains the problem.


Regards,
John

> ---
>  arch/x86/kernel/apic/vector.c |   22 +++++-----------------
>  kernel/irq/manage.c           |   37 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 19 deletions(-)
> 
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
>  	trace_vector_activate(irqd->irq, apicd->is_managed,
>  			      apicd->can_reserve, reserve);
>  
> -	/* Nothing to do for fixed assigned vectors */
> -	if (!apicd->can_reserve && !apicd->is_managed)
> -		return 0;
> -
>  	raw_spin_lock_irqsave(&vector_lock, flags);
> -	if (reserve || irqd_is_managed_and_shutdown(irqd))
> +	if (!apicd->can_reserve && !apicd->is_managed)
> +		assign_irq_vector_any_locked(irqd);
> +	else if (reserve || irqd_is_managed_and_shutdown(irqd))
>  		vector_assign_managed_shutdown(irqd);
>  	else if (apicd->is_managed)
>  		ret = activate_managed(irqd);
> @@ -775,20 +773,10 @@ void lapic_offline(void)
>  static int apic_set_affinity(struct irq_data *irqd,
>  			     const struct cpumask *dest, bool force)
>  {
> -	struct apic_chip_data *apicd = apic_chip_data(irqd);
>  	int err;
>  
> -	/*
> -	 * Core code can call here for inactive interrupts. For inactive
> -	 * interrupts which use managed or reservation mode there is no
> -	 * point in going through the vector assignment right now as the
> -	 * activation will assign a vector which fits the destination
> -	 * cpumask. Let the core code store the destination mask and be
> -	 * done with it.
> -	 */
> -	if (!irqd_is_activated(irqd) &&
> -	    (apicd->is_managed || apicd->can_reserve))
> -		return IRQ_SET_MASK_OK;
> +	if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
> +		return -EIO;
>  
>  	raw_spin_lock(&vector_lock);
>  	cpumask_and(vector_searchmask, dest, cpu_online_mask);
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -195,9 +195,9 @@ void irq_set_thread_affinity(struct irq_
>  			set_bit(IRQTF_AFFINITY, &action->thread_flags);
>  }
>  
> +#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>  static void irq_validate_effective_affinity(struct irq_data *data)
>  {
> -#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>  	const struct cpumask *m = irq_data_get_effective_affinity_mask(data);
>  	struct irq_chip *chip = irq_data_get_irq_chip(data);
>  
> @@ -205,9 +205,19 @@ static void irq_validate_effective_affin
>  		return;
>  	pr_warn_once("irq_chip %s did not update eff. affinity mask of irq %u\n",
>  		     chip->name, data->irq);
> -#endif
>  }
>  
> +static inline void irq_init_effective_affinity(struct irq_data *data,
> +					       const struct cpumask *mask)
> +{
> +	cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
> +}
> +#else
> +static inline void irq_validate_effective_affinity(struct irq_data *data) { }
> +static inline void irq_init_effective_affinity(struct irq_data *data,
> +					       const struct cpumask *mask) { }
> +#endif
> +
>  int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
>  			bool force)
>  {
> @@ -304,6 +314,26 @@ static int irq_try_set_affinity(struct i
>  	return ret;
>  }
>  
> +static bool irq_set_affinity_deactivated(struct irq_data *data,
> +					 const struct cpumask *mask, bool force)
> +{
> +	struct irq_desc *desc = irq_data_to_desc(data);
> +
> +	/*
> +	 * If the interrupt is not yet activated, just store the affinity
> +	 * mask and do not call the chip driver at all. On activation the
> +	 * driver has to make sure anyway that the interrupt is in a
> +	 * useable state so startup works.
> +	 */
> +	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
> +		return false;
> +
> +	cpumask_copy(desc->irq_common_data.affinity, mask);
> +	irq_init_effective_affinity(data, mask);
> +	irqd_set(data, IRQD_AFFINITY_SET);
> +	return true;
> +}
> +
>  int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
>  			    bool force)
>  {
> @@ -314,6 +344,9 @@ int irq_set_affinity_locked(struct irq_d
>  	if (!chip || !chip->irq_set_affinity)
>  		return -EINVAL;
>  
> +	if (irq_set_affinity_deactivated(data, mask, force))
> +		return 0;
> +
>  	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
>  		ret = irq_try_set_affinity(data, mask, force);
>  	} else {


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-24 17:24   ` John Keeping
  2020-07-24 20:03     ` Thomas Gleixner
  0 siblings, 1 reply; 23+ messages in thread
From: John Keeping @ 2020-07-24 17:24 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ben Herrenschmidt, Marc Zyngier, x86, LKML, Ali Saidi, linux-arm-kernel

Hi,

On Fri, 17 Jul 2020 18:00:02 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Setting interrupt affinity on inactive interrupts is inconsistent when
> hierarchical irq domains are enabled. The core code should just store the
> affinity and not call into the irq chip driver for inactive interrupts
> because the chip drivers may not be in a state to handle such requests.
> 
> X86 has a hacky workaround for that but all other irq chips have not which
> causes problems e.g. on GIC V3 ITS.
> 
> Instead of adding more ugly hacks all over the place, solve the problem in
> the core code. If the affinity is set on an inactive interrupt then:
> 
>     - Store it in the irq descriptors affinity mask
>     - Update the effective affinity to reflect that so user space has
>       a consistent view
>     - Don't call into the irq chip driver
> 
> This is the core equivalent of the X86 workaround and works correctly
> because the affinity setting is established in the irq chip when the
> interrupt is activated later on.
> 
> Note, that this is only effective when hierarchical irq domains are enabled
> by the architecture. Doing it unconditionally would break legacy irq chip
> implementations.
> 
> For hierarchial irq domains this works correctly as none of the drivers can
> have a dependency on affinity setting in inactive state by design.
> 
> Remove the X86 workaround as it is not longer required.
> 
> Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive interrupts")
> Reported-by: Ali Saidi <alisaidi@amazon.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/r/20200529015501.15771-1-alisaidi@amazon.com
> ---
> V2: Fix the fallout for CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n (0day)

It seems that this patch breaks perf events on RK3288 because the PMU
interrupts that should be per-cpu are now all on CPU0 so no events are
collected from CPUs 1-3 and those interrupts are killed as spurious
after a few seconds.

I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
relevant code hasn't changed through to next-20200723.  Reverting the
backport of this change fixes the problem.

It looks like what happens is that because the interrupts are not
per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
while the interrupt is deactivated and then request_irq() with
IRQF_PERCPU | IRQF_NOBALANCING.

Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
irq_setup_affinity() which returns early because IRQF_PERCPU and
IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.

At this point /proc/interrupts clearly shows the interrupts occurring on
CPU0 despite /proc/irq/N/effective_affinity and /proc/irq/N/smp_affinity
showing them spread across the cores as expected.

I don't think I understand what's meant to happen well enough to propose
a patch, but hopefully the above explanation explains the problem.


Regards,
John

> ---
>  arch/x86/kernel/apic/vector.c |   22 +++++-----------------
>  kernel/irq/manage.c           |   37 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 19 deletions(-)
> 
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
>  	trace_vector_activate(irqd->irq, apicd->is_managed,
>  			      apicd->can_reserve, reserve);
>  
> -	/* Nothing to do for fixed assigned vectors */
> -	if (!apicd->can_reserve && !apicd->is_managed)
> -		return 0;
> -
>  	raw_spin_lock_irqsave(&vector_lock, flags);
> -	if (reserve || irqd_is_managed_and_shutdown(irqd))
> +	if (!apicd->can_reserve && !apicd->is_managed)
> +		assign_irq_vector_any_locked(irqd);
> +	else if (reserve || irqd_is_managed_and_shutdown(irqd))
>  		vector_assign_managed_shutdown(irqd);
>  	else if (apicd->is_managed)
>  		ret = activate_managed(irqd);
> @@ -775,20 +773,10 @@ void lapic_offline(void)
>  static int apic_set_affinity(struct irq_data *irqd,
>  			     const struct cpumask *dest, bool force)
>  {
> -	struct apic_chip_data *apicd = apic_chip_data(irqd);
>  	int err;
>  
> -	/*
> -	 * Core code can call here for inactive interrupts. For inactive
> -	 * interrupts which use managed or reservation mode there is no
> -	 * point in going through the vector assignment right now as the
> -	 * activation will assign a vector which fits the destination
> -	 * cpumask. Let the core code store the destination mask and be
> -	 * done with it.
> -	 */
> -	if (!irqd_is_activated(irqd) &&
> -	    (apicd->is_managed || apicd->can_reserve))
> -		return IRQ_SET_MASK_OK;
> +	if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
> +		return -EIO;
>  
>  	raw_spin_lock(&vector_lock);
>  	cpumask_and(vector_searchmask, dest, cpu_online_mask);
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -195,9 +195,9 @@ void irq_set_thread_affinity(struct irq_
>  			set_bit(IRQTF_AFFINITY, &action->thread_flags);
>  }
>  
> +#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>  static void irq_validate_effective_affinity(struct irq_data *data)
>  {
> -#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>  	const struct cpumask *m = irq_data_get_effective_affinity_mask(data);
>  	struct irq_chip *chip = irq_data_get_irq_chip(data);
>  
> @@ -205,9 +205,19 @@ static void irq_validate_effective_affin
>  		return;
>  	pr_warn_once("irq_chip %s did not update eff. affinity mask of irq %u\n",
>  		     chip->name, data->irq);
> -#endif
>  }
>  
> +static inline void irq_init_effective_affinity(struct irq_data *data,
> +					       const struct cpumask *mask)
> +{
> +	cpumask_copy(irq_data_get_effective_affinity_mask(data), mask);
> +}
> +#else
> +static inline void irq_validate_effective_affinity(struct irq_data *data) { }
> +static inline void irq_init_effective_affinity(struct irq_data *data,
> +					       const struct cpumask *mask) { }
> +#endif
> +
>  int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
>  			bool force)
>  {
> @@ -304,6 +314,26 @@ static int irq_try_set_affinity(struct i
>  	return ret;
>  }
>  
> +static bool irq_set_affinity_deactivated(struct irq_data *data,
> +					 const struct cpumask *mask, bool force)
> +{
> +	struct irq_desc *desc = irq_data_to_desc(data);
> +
> +	/*
> +	 * If the interrupt is not yet activated, just store the affinity
> +	 * mask and do not call the chip driver at all. On activation the
> +	 * driver has to make sure anyway that the interrupt is in a
> +	 * useable state so startup works.
> +	 */
> +	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
> +		return false;
> +
> +	cpumask_copy(desc->irq_common_data.affinity, mask);
> +	irq_init_effective_affinity(data, mask);
> +	irqd_set(data, IRQD_AFFINITY_SET);
> +	return true;
> +}
> +
>  int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
>  			    bool force)
>  {
> @@ -314,6 +344,9 @@ int irq_set_affinity_locked(struct irq_d
>  	if (!chip || !chip->irq_set_affinity)
>  		return -EINVAL;
>  
> +	if (irq_set_affinity_deactivated(data, mask, force))
> +		return 0;
> +
>  	if (irq_can_move_pcntxt(data) && !irqd_is_setaffinity_pending(data)) {
>  		ret = irq_try_set_affinity(data, mask, force);
>  	} else {


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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-24 17:24   ` John Keeping
@ 2020-07-24 20:03     ` Thomas Gleixner
  2020-07-24 20:44       ` Thomas Gleixner
  2020-07-25 12:08       ` [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly Marc Zyngier
  0 siblings, 2 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-24 20:03 UTC (permalink / raw)
  To: John Keeping
  Cc: LKML, x86, Ben Herrenschmidt, Ali Saidi, Marc Zyngier, linux-arm-kernel

John,

John Keeping <john@metanate.com> writes:
> On Fri, 17 Jul 2020 18:00:02 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> It seems that this patch breaks perf events on RK3288 because the PMU
> interrupts that should be per-cpu are now all on CPU0 so no events are
> collected from CPUs 1-3 and those interrupts are killed as spurious
> after a few seconds.
>
> I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
> relevant code hasn't changed through to next-20200723.  Reverting the
> backport of this change fixes the problem.

Bah.

> It looks like what happens is that because the interrupts are not
> per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
> while the interrupt is deactivated and then request_irq() with
> IRQF_PERCPU | IRQF_NOBALANCING.
>
> Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
> irq_setup_affinity() which returns early because IRQF_PERCPU and
> IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.

Right. My brain tricked me to believe that we made activation mandatory,
but that's not.

I have some ideas for a trivial generic way to solve this without
undoing the commit in question and without going through all the irq
chip drivers. So far everything I came up with is butt ugly. Maybe Marc
has some brilliant idea.

Sorry for the wreckage and thanks for the excellent problem
description. I'll come back to you in the next days.

Thanks,

        tglx



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-24 20:03     ` Thomas Gleixner
  2020-07-24 20:44       ` Thomas Gleixner
  2020-07-25 12:08       ` [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly Marc Zyngier
  0 siblings, 2 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-24 20:03 UTC (permalink / raw)
  To: John Keeping
  Cc: Ben Herrenschmidt, Marc Zyngier, x86, LKML, Ali Saidi, linux-arm-kernel

John,

John Keeping <john@metanate.com> writes:
> On Fri, 17 Jul 2020 18:00:02 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> It seems that this patch breaks perf events on RK3288 because the PMU
> interrupts that should be per-cpu are now all on CPU0 so no events are
> collected from CPUs 1-3 and those interrupts are killed as spurious
> after a few seconds.
>
> I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
> relevant code hasn't changed through to next-20200723.  Reverting the
> backport of this change fixes the problem.

Bah.

> It looks like what happens is that because the interrupts are not
> per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
> while the interrupt is deactivated and then request_irq() with
> IRQF_PERCPU | IRQF_NOBALANCING.
>
> Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
> irq_setup_affinity() which returns early because IRQF_PERCPU and
> IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.

Right. My brain tricked me to believe that we made activation mandatory,
but that's not.

I have some ideas for a trivial generic way to solve this without
undoing the commit in question and without going through all the irq
chip drivers. So far everything I came up with is butt ugly. Maybe Marc
has some brilliant idea.

Sorry for the wreckage and thanks for the excellent problem
description. I'll come back to you in the next days.

Thanks,

        tglx



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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-24 20:03     ` Thomas Gleixner
@ 2020-07-24 20:44       ` Thomas Gleixner
  2020-07-25 12:30         ` Marc Zyngier
  2020-07-27 14:25         ` [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in tip-bot2 for Thomas Gleixner
  2020-07-25 12:08       ` [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly Marc Zyngier
  1 sibling, 2 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-24 20:44 UTC (permalink / raw)
  To: John Keeping
  Cc: LKML, x86, Ben Herrenschmidt, Ali Saidi, Marc Zyngier, linux-arm-kernel

John,

Thomas Gleixner <tglx@linutronix.de> writes:
> I have some ideas for a trivial generic way to solve this without
> undoing the commit in question and without going through all the irq
> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> has some brilliant idea.
>
> Sorry for the wreckage and thanks for the excellent problem
> description. I'll come back to you in the next days.

couldn't give up :)

So after staring in too many drivers, I resorted to make this mode
opt-in and mark the interrupts accordingly for the two drivers which are
known to want this. Not that I love it, but it's the least dangerous
option. Completely untested patch below.

Thanks,

        tglx
---
 arch/x86/kernel/apic/vector.c    |    4 ++++
 drivers/irqchip/irq-gic-v3-its.c |    5 ++++-
 include/linux/irq.h              |   13 +++++++++++++
 kernel/irq/manage.c              |    6 +++++-
 4 files changed, 26 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct
 		 * as that can corrupt the affinity move state.
 		 */
 		irqd_set_handle_enforce_irqctx(irqd);
+
+		/* Don't invoke affinity setter on deactivated interrupts */
+		irqd_set_affinity_on_activate(irqd);
+
 		/*
 		 * Legacy vectors are already assigned when the IOAPIC
 		 * takes them over. They stay on the same vector. This is
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct i
 	msi_alloc_info_t *info = args;
 	struct its_device *its_dev = info->scratchpad[0].ptr;
 	struct its_node *its = its_dev->its;
+	struct irq_data *irqd;
 	irq_hw_number_t hwirq;
 	int err;
 	int i;
@@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct i
 
 		irq_domain_set_hwirq_and_chip(domain, virq + i,
 					      hwirq + i, &its_irq_chip, its_dev);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq + i)));
+		irqd = irq_get_irq_data(virq + i);
+		irqd_set_single_target(irqd);
+		irqd_set_affinity_on_activate(irqd);
 		pr_debug("ID:%d pID:%d vID:%d\n",
 			 (int)(hwirq + i - its_dev->event_map.lpi_base),
 			 (int)(hwirq + i), virq + i);
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -213,6 +213,8 @@ struct irq_data {
  *				  required
  * IRQD_HANDLE_ENFORCE_IRQCTX	- Enforce that handle_irq_*() is only invoked
  *				  from actual interrupt context.
+ * IRQD_AFFINITY_ON_ACTIVATE	- Affinity is set on activation. Don't call
+ *				  irq_chip::irq_set_affinity() when deactivated.
  */
 enum {
 	IRQD_TRIGGER_MASK		= 0xf,
@@ -237,6 +239,7 @@ enum {
 	IRQD_CAN_RESERVE		= (1 << 26),
 	IRQD_MSI_NOMASK_QUIRK		= (1 << 27),
 	IRQD_HANDLE_ENFORCE_IRQCTX	= (1 << 28),
+	IRQD_AFFINITY_ON_ACTIVATE	= (1 << 29),
 };
 
 #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk
 	return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
 }
 
+static inline void irqd_set_affinity_on_activate(struct irq_data *d)
+{
+	__irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
+}
+
+static inline bool irqd_affinity_on_activate(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated
 	struct irq_desc *desc = irq_data_to_desc(data);
 
 	/*
+	 * Handle irq chips which can handle affinity only in activated
+	 * state correctly
+	 *
 	 * If the interrupt is not yet activated, just store the affinity
 	 * mask and do not call the chip driver at all. On activation the
 	 * driver has to make sure anyway that the interrupt is in a
 	 * useable state so startup works.
 	 */
-	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
+	    irqd_is_activated(data) || !irqd_affinity_on_activate(data))
 		return false;
 
 	cpumask_copy(desc->irq_common_data.affinity, mask);



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-24 20:44       ` Thomas Gleixner
  2020-07-25 12:30         ` Marc Zyngier
  2020-07-27 14:25         ` [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in tip-bot2 for Thomas Gleixner
  0 siblings, 2 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-24 20:44 UTC (permalink / raw)
  To: John Keeping
  Cc: Ben Herrenschmidt, Marc Zyngier, x86, LKML, Ali Saidi, linux-arm-kernel

John,

Thomas Gleixner <tglx@linutronix.de> writes:
> I have some ideas for a trivial generic way to solve this without
> undoing the commit in question and without going through all the irq
> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> has some brilliant idea.
>
> Sorry for the wreckage and thanks for the excellent problem
> description. I'll come back to you in the next days.

couldn't give up :)

So after staring in too many drivers, I resorted to make this mode
opt-in and mark the interrupts accordingly for the two drivers which are
known to want this. Not that I love it, but it's the least dangerous
option. Completely untested patch below.

Thanks,

        tglx
---
 arch/x86/kernel/apic/vector.c    |    4 ++++
 drivers/irqchip/irq-gic-v3-its.c |    5 ++++-
 include/linux/irq.h              |   13 +++++++++++++
 kernel/irq/manage.c              |    6 +++++-
 4 files changed, 26 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct
 		 * as that can corrupt the affinity move state.
 		 */
 		irqd_set_handle_enforce_irqctx(irqd);
+
+		/* Don't invoke affinity setter on deactivated interrupts */
+		irqd_set_affinity_on_activate(irqd);
+
 		/*
 		 * Legacy vectors are already assigned when the IOAPIC
 		 * takes them over. They stay on the same vector. This is
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct i
 	msi_alloc_info_t *info = args;
 	struct its_device *its_dev = info->scratchpad[0].ptr;
 	struct its_node *its = its_dev->its;
+	struct irq_data *irqd;
 	irq_hw_number_t hwirq;
 	int err;
 	int i;
@@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct i
 
 		irq_domain_set_hwirq_and_chip(domain, virq + i,
 					      hwirq + i, &its_irq_chip, its_dev);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq + i)));
+		irqd = irq_get_irq_data(virq + i);
+		irqd_set_single_target(irqd);
+		irqd_set_affinity_on_activate(irqd);
 		pr_debug("ID:%d pID:%d vID:%d\n",
 			 (int)(hwirq + i - its_dev->event_map.lpi_base),
 			 (int)(hwirq + i), virq + i);
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -213,6 +213,8 @@ struct irq_data {
  *				  required
  * IRQD_HANDLE_ENFORCE_IRQCTX	- Enforce that handle_irq_*() is only invoked
  *				  from actual interrupt context.
+ * IRQD_AFFINITY_ON_ACTIVATE	- Affinity is set on activation. Don't call
+ *				  irq_chip::irq_set_affinity() when deactivated.
  */
 enum {
 	IRQD_TRIGGER_MASK		= 0xf,
@@ -237,6 +239,7 @@ enum {
 	IRQD_CAN_RESERVE		= (1 << 26),
 	IRQD_MSI_NOMASK_QUIRK		= (1 << 27),
 	IRQD_HANDLE_ENFORCE_IRQCTX	= (1 << 28),
+	IRQD_AFFINITY_ON_ACTIVATE	= (1 << 29),
 };
 
 #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk
 	return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
 }
 
+static inline void irqd_set_affinity_on_activate(struct irq_data *d)
+{
+	__irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
+}
+
+static inline bool irqd_affinity_on_activate(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated
 	struct irq_desc *desc = irq_data_to_desc(data);
 
 	/*
+	 * Handle irq chips which can handle affinity only in activated
+	 * state correctly
+	 *
 	 * If the interrupt is not yet activated, just store the affinity
 	 * mask and do not call the chip driver at all. On activation the
 	 * driver has to make sure anyway that the interrupt is in a
 	 * useable state so startup works.
 	 */
-	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
+	    irqd_is_activated(data) || !irqd_affinity_on_activate(data))
 		return false;
 
 	cpumask_copy(desc->irq_common_data.affinity, mask);



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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-24 20:03     ` Thomas Gleixner
  2020-07-24 20:44       ` Thomas Gleixner
@ 2020-07-25 12:08       ` Marc Zyngier
  2020-07-27 13:35         ` Thomas Gleixner
  1 sibling, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2020-07-25 12:08 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: John Keeping, LKML, x86, Ben Herrenschmidt, Ali Saidi, linux-arm-kernel

Hi both,

On Fri, 24 Jul 2020 21:03:50 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> John,
> 
> John Keeping <john@metanate.com> writes:
> > On Fri, 17 Jul 2020 18:00:02 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > It seems that this patch breaks perf events on RK3288 because the PMU
> > interrupts that should be per-cpu are now all on CPU0 so no events are
> > collected from CPUs 1-3 and those interrupts are killed as spurious
> > after a few seconds.

SPI-backed PMUs. Urgh...

> >
> > I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
> > relevant code hasn't changed through to next-20200723.  Reverting the
> > backport of this change fixes the problem.
> 
> Bah.
> 
> > It looks like what happens is that because the interrupts are not
> > per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
> > while the interrupt is deactivated and then request_irq() with
> > IRQF_PERCPU | IRQF_NOBALANCING.
> >
> > Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
> > irq_setup_affinity() which returns early because IRQF_PERCPU and
> > IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.
> 
> Right. My brain tricked me to believe that we made activation mandatory,
> but that's not.
> 
> I have some ideas for a trivial generic way to solve this without
> undoing the commit in question and without going through all the irq
> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> has some brilliant idea.

Not really. We have contradicting behaviours here, where some
interrupts want to see the set_affinity early (the above case), and
some cannot handle that (x86 vectors and the GICv3 ITS). We could key
it on the presence of an activate callback, but it feels fragile.

I'll follow up on your patch in the next email, which seems like a
sensible approach.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-25 12:08       ` Marc Zyngier
  2020-07-27 13:35         ` Thomas Gleixner
  0 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2020-07-25 12:08 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ben Herrenschmidt, x86, LKML, John Keeping, Ali Saidi, linux-arm-kernel

Hi both,

On Fri, 24 Jul 2020 21:03:50 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> John,
> 
> John Keeping <john@metanate.com> writes:
> > On Fri, 17 Jul 2020 18:00:02 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > It seems that this patch breaks perf events on RK3288 because the PMU
> > interrupts that should be per-cpu are now all on CPU0 so no events are
> > collected from CPUs 1-3 and those interrupts are killed as spurious
> > after a few seconds.

SPI-backed PMUs. Urgh...

> >
> > I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
> > relevant code hasn't changed through to next-20200723.  Reverting the
> > backport of this change fixes the problem.
> 
> Bah.
> 
> > It looks like what happens is that because the interrupts are not
> > per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
> > while the interrupt is deactivated and then request_irq() with
> > IRQF_PERCPU | IRQF_NOBALANCING.
> >
> > Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
> > irq_setup_affinity() which returns early because IRQF_PERCPU and
> > IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.
> 
> Right. My brain tricked me to believe that we made activation mandatory,
> but that's not.
> 
> I have some ideas for a trivial generic way to solve this without
> undoing the commit in question and without going through all the irq
> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> has some brilliant idea.

Not really. We have contradicting behaviours here, where some
interrupts want to see the set_affinity early (the above case), and
some cannot handle that (x86 vectors and the GICv3 ITS). We could key
it on the presence of an activate callback, but it feels fragile.

I'll follow up on your patch in the next email, which seems like a
sensible approach.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-24 20:44       ` Thomas Gleixner
@ 2020-07-25 12:30         ` Marc Zyngier
  2020-07-27 14:25           ` [tip: irq/urgent] genirq/debugfs: Add missing irqchip flags tip-bot2 for Marc Zyngier
  2020-07-27 14:25         ` [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in tip-bot2 for Thomas Gleixner
  1 sibling, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2020-07-25 12:30 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: John Keeping, LKML, x86, Ben Herrenschmidt, Ali Saidi, linux-arm-kernel

Hi Thomas,

On Fri, 24 Jul 2020 21:44:41 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> John,
> 
> Thomas Gleixner <tglx@linutronix.de> writes:
> > I have some ideas for a trivial generic way to solve this without
> > undoing the commit in question and without going through all the irq
> > chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> > has some brilliant idea.
> >
> > Sorry for the wreckage and thanks for the excellent problem
> > description. I'll come back to you in the next days.
> 
> couldn't give up :)
> 
> So after staring in too many drivers, I resorted to make this mode
> opt-in and mark the interrupts accordingly for the two drivers which are
> known to want this. Not that I love it, but it's the least dangerous
> option. Completely untested patch below.
> 
> Thanks,
> 
>         tglx
> ---
>  arch/x86/kernel/apic/vector.c    |    4 ++++
>  drivers/irqchip/irq-gic-v3-its.c |    5 ++++-
>  include/linux/irq.h              |   13 +++++++++++++
>  kernel/irq/manage.c              |    6 +++++-
>  4 files changed, 26 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct
>  		 * as that can corrupt the affinity move state.
>  		 */
>  		irqd_set_handle_enforce_irqctx(irqd);
> +
> +		/* Don't invoke affinity setter on deactivated interrupts */
> +		irqd_set_affinity_on_activate(irqd);
> +
>  		/*
>  		 * Legacy vectors are already assigned when the IOAPIC
>  		 * takes them over. They stay on the same vector. This is
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct i
>  	msi_alloc_info_t *info = args;
>  	struct its_device *its_dev = info->scratchpad[0].ptr;
>  	struct its_node *its = its_dev->its;
> +	struct irq_data *irqd;
>  	irq_hw_number_t hwirq;
>  	int err;
>  	int i;
> @@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct i
>  
>  		irq_domain_set_hwirq_and_chip(domain, virq + i,
>  					      hwirq + i, &its_irq_chip, its_dev);
> -		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq + i)));
> +		irqd = irq_get_irq_data(virq + i);
> +		irqd_set_single_target(irqd);
> +		irqd_set_affinity_on_activate(irqd);
>  		pr_debug("ID:%d pID:%d vID:%d\n",
>  			 (int)(hwirq + i - its_dev->event_map.lpi_base),
>  			 (int)(hwirq + i), virq + i);
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -213,6 +213,8 @@ struct irq_data {
>   *				  required
>   * IRQD_HANDLE_ENFORCE_IRQCTX	- Enforce that handle_irq_*() is only invoked
>   *				  from actual interrupt context.
> + * IRQD_AFFINITY_ON_ACTIVATE	- Affinity is set on activation. Don't call
> + *				  irq_chip::irq_set_affinity() when deactivated.
>   */
>  enum {
>  	IRQD_TRIGGER_MASK		= 0xf,
> @@ -237,6 +239,7 @@ enum {
>  	IRQD_CAN_RESERVE		= (1 << 26),
>  	IRQD_MSI_NOMASK_QUIRK		= (1 << 27),
>  	IRQD_HANDLE_ENFORCE_IRQCTX	= (1 << 28),
> +	IRQD_AFFINITY_ON_ACTIVATE	= (1 << 29),
>  };
>  
>  #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
> @@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk
>  	return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
>  }
>  
> +static inline void irqd_set_affinity_on_activate(struct irq_data *d)
> +{
> +	__irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
> +}
> +
> +static inline bool irqd_affinity_on_activate(struct irq_data *d)
> +{
> +	return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
> +}
> +
>  #undef __irqd_to_state
>  
>  static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated
>  	struct irq_desc *desc = irq_data_to_desc(data);
>  
>  	/*
> +	 * Handle irq chips which can handle affinity only in activated
> +	 * state correctly
> +	 *
>  	 * If the interrupt is not yet activated, just store the affinity
>  	 * mask and do not call the chip driver at all. On activation the
>  	 * driver has to make sure anyway that the interrupt is in a
>  	 * useable state so startup works.
>  	 */
> -	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
> +	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
> +	    irqd_is_activated(data) || !irqd_affinity_on_activate(data))
>  		return false;
>  
>  	cpumask_copy(desc->irq_common_data.affinity, mask);
> 

I have given this a go on two systems: one with a GICv2 that has its
PMUs wired in a similar way to John's system (each CPU PMU is on a
separate SPI), and another one that has an ITS. Both came up normally
and their interrupts are routed as expected:

* GICv2 PMU:
30:  121    0    0    0    0    0    0    0 GICv2  39 Level arm-pmu
31:    0  167    0    0    0    0    0    0 GICv2  40 Level arm-pmu
32:    0    0  145    0    0    0    0    0 GICv2  41 Level arm-pmu
33:    0    0    0  400    0    0    0    0 GICv2  42 Level arm-pmu
34:    0    0    0    0   97    0    0    0 GICv2  43 Level arm-pmu
35:    0    0    0    0    0  463    0    0 GICv2  44 Level arm-pmu
36:    0    0    0    0    0    0   74    0 GICv2  45 Level arm-pmu
37:    0    0    0    0    0    0    0    8 GICv2  46 Level arm-pmu

* GICv3+ITS:
241:  219    0    0    0    0    0   ITS-MSI 524289 Edge nvme0q1
242:    0  251    0    0    0    0   ITS-MSI 524290 Edge nvme0q2
243:    0    0  197    0    0    0   ITS-MSI 524291 Edge nvme0q3
244:    0    0    0  380    0    0   ITS-MSI 524292 Edge nvme0q4
245:    0    0    0    0  675    0   ITS-MSI 524293 Edge nvme0q5
246:    0    0    0    0    0  436   ITS-MSI 524294 Edge nvme0q6

For a good measure, I've added this on top, adding the missing bits to
the debugfs entries:

diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 4f9f844074db..d44fc8a5dab2 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -120,6 +120,9 @@ static const struct irq_bit_descr irqdata_states[] = {
 
 	BIT_MASK_DESCR(IRQD_WAKEUP_STATE),
 	BIT_MASK_DESCR(IRQD_WAKEUP_ARMED),
+	BIT_MASK_DESCR(IRQD_DEFAULT_TRIGGER_SET),
+	BIT_MASK_DESCR(IRQD_HANDLE_ENFORCE_IRQCTX),
+	BIT_MASK_DESCR(IRQD_AFFINITY_ON_ACTIVATE),
 };
 
 static const struct irq_bit_descr irqdesc_states[] = {

FWIW:

Acked-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>


	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-25 12:30         ` Marc Zyngier
  2020-07-27 14:25           ` [tip: irq/urgent] genirq/debugfs: Add missing irqchip flags tip-bot2 for Marc Zyngier
  0 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2020-07-25 12:30 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ben Herrenschmidt, x86, LKML, John Keeping, Ali Saidi, linux-arm-kernel

Hi Thomas,

On Fri, 24 Jul 2020 21:44:41 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> John,
> 
> Thomas Gleixner <tglx@linutronix.de> writes:
> > I have some ideas for a trivial generic way to solve this without
> > undoing the commit in question and without going through all the irq
> > chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> > has some brilliant idea.
> >
> > Sorry for the wreckage and thanks for the excellent problem
> > description. I'll come back to you in the next days.
> 
> couldn't give up :)
> 
> So after staring in too many drivers, I resorted to make this mode
> opt-in and mark the interrupts accordingly for the two drivers which are
> known to want this. Not that I love it, but it's the least dangerous
> option. Completely untested patch below.
> 
> Thanks,
> 
>         tglx
> ---
>  arch/x86/kernel/apic/vector.c    |    4 ++++
>  drivers/irqchip/irq-gic-v3-its.c |    5 ++++-
>  include/linux/irq.h              |   13 +++++++++++++
>  kernel/irq/manage.c              |    6 +++++-
>  4 files changed, 26 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct
>  		 * as that can corrupt the affinity move state.
>  		 */
>  		irqd_set_handle_enforce_irqctx(irqd);
> +
> +		/* Don't invoke affinity setter on deactivated interrupts */
> +		irqd_set_affinity_on_activate(irqd);
> +
>  		/*
>  		 * Legacy vectors are already assigned when the IOAPIC
>  		 * takes them over. They stay on the same vector. This is
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct i
>  	msi_alloc_info_t *info = args;
>  	struct its_device *its_dev = info->scratchpad[0].ptr;
>  	struct its_node *its = its_dev->its;
> +	struct irq_data *irqd;
>  	irq_hw_number_t hwirq;
>  	int err;
>  	int i;
> @@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct i
>  
>  		irq_domain_set_hwirq_and_chip(domain, virq + i,
>  					      hwirq + i, &its_irq_chip, its_dev);
> -		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq + i)));
> +		irqd = irq_get_irq_data(virq + i);
> +		irqd_set_single_target(irqd);
> +		irqd_set_affinity_on_activate(irqd);
>  		pr_debug("ID:%d pID:%d vID:%d\n",
>  			 (int)(hwirq + i - its_dev->event_map.lpi_base),
>  			 (int)(hwirq + i), virq + i);
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -213,6 +213,8 @@ struct irq_data {
>   *				  required
>   * IRQD_HANDLE_ENFORCE_IRQCTX	- Enforce that handle_irq_*() is only invoked
>   *				  from actual interrupt context.
> + * IRQD_AFFINITY_ON_ACTIVATE	- Affinity is set on activation. Don't call
> + *				  irq_chip::irq_set_affinity() when deactivated.
>   */
>  enum {
>  	IRQD_TRIGGER_MASK		= 0xf,
> @@ -237,6 +239,7 @@ enum {
>  	IRQD_CAN_RESERVE		= (1 << 26),
>  	IRQD_MSI_NOMASK_QUIRK		= (1 << 27),
>  	IRQD_HANDLE_ENFORCE_IRQCTX	= (1 << 28),
> +	IRQD_AFFINITY_ON_ACTIVATE	= (1 << 29),
>  };
>  
>  #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
> @@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk
>  	return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
>  }
>  
> +static inline void irqd_set_affinity_on_activate(struct irq_data *d)
> +{
> +	__irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
> +}
> +
> +static inline bool irqd_affinity_on_activate(struct irq_data *d)
> +{
> +	return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
> +}
> +
>  #undef __irqd_to_state
>  
>  static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated
>  	struct irq_desc *desc = irq_data_to_desc(data);
>  
>  	/*
> +	 * Handle irq chips which can handle affinity only in activated
> +	 * state correctly
> +	 *
>  	 * If the interrupt is not yet activated, just store the affinity
>  	 * mask and do not call the chip driver at all. On activation the
>  	 * driver has to make sure anyway that the interrupt is in a
>  	 * useable state so startup works.
>  	 */
> -	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
> +	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
> +	    irqd_is_activated(data) || !irqd_affinity_on_activate(data))
>  		return false;
>  
>  	cpumask_copy(desc->irq_common_data.affinity, mask);
> 

I have given this a go on two systems: one with a GICv2 that has its
PMUs wired in a similar way to John's system (each CPU PMU is on a
separate SPI), and another one that has an ITS. Both came up normally
and their interrupts are routed as expected:

* GICv2 PMU:
30:  121    0    0    0    0    0    0    0 GICv2  39 Level arm-pmu
31:    0  167    0    0    0    0    0    0 GICv2  40 Level arm-pmu
32:    0    0  145    0    0    0    0    0 GICv2  41 Level arm-pmu
33:    0    0    0  400    0    0    0    0 GICv2  42 Level arm-pmu
34:    0    0    0    0   97    0    0    0 GICv2  43 Level arm-pmu
35:    0    0    0    0    0  463    0    0 GICv2  44 Level arm-pmu
36:    0    0    0    0    0    0   74    0 GICv2  45 Level arm-pmu
37:    0    0    0    0    0    0    0    8 GICv2  46 Level arm-pmu

* GICv3+ITS:
241:  219    0    0    0    0    0   ITS-MSI 524289 Edge nvme0q1
242:    0  251    0    0    0    0   ITS-MSI 524290 Edge nvme0q2
243:    0    0  197    0    0    0   ITS-MSI 524291 Edge nvme0q3
244:    0    0    0  380    0    0   ITS-MSI 524292 Edge nvme0q4
245:    0    0    0    0  675    0   ITS-MSI 524293 Edge nvme0q5
246:    0    0    0    0    0  436   ITS-MSI 524294 Edge nvme0q6

For a good measure, I've added this on top, adding the missing bits to
the debugfs entries:

diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 4f9f844074db..d44fc8a5dab2 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -120,6 +120,9 @@ static const struct irq_bit_descr irqdata_states[] = {
 
 	BIT_MASK_DESCR(IRQD_WAKEUP_STATE),
 	BIT_MASK_DESCR(IRQD_WAKEUP_ARMED),
+	BIT_MASK_DESCR(IRQD_DEFAULT_TRIGGER_SET),
+	BIT_MASK_DESCR(IRQD_HANDLE_ENFORCE_IRQCTX),
+	BIT_MASK_DESCR(IRQD_AFFINITY_ON_ACTIVATE),
 };
 
 static const struct irq_bit_descr irqdesc_states[] = {

FWIW:

Acked-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>


	M.

-- 
Without deviation from the norm, progress is not possible.

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
  2020-07-25 12:08       ` [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly Marc Zyngier
@ 2020-07-27 13:35         ` Thomas Gleixner
  0 siblings, 0 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-27 13:35 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: John Keeping, LKML, x86, Ben Herrenschmidt, Ali Saidi, linux-arm-kernel

Marc,

Marc Zyngier <maz@kernel.org> writes:
> On Fri, 24 Jul 2020 21:03:50 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> Right. My brain tricked me to believe that we made activation mandatory,
>> but that's not.
>> 
>> I have some ideas for a trivial generic way to solve this without
>> undoing the commit in question and without going through all the irq
>> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
>> has some brilliant idea.
>
> Not really. We have contradicting behaviours here, where some
> interrupts want to see the set_affinity early (the above case), and
> some cannot handle that (x86 vectors and the GICv3 ITS). We could key
> it on the presence of an activate callback, but it feels fragile.

Yes, I thought about that briefly, but yeah, it's fragile and bound to
break in weird ways.

That said, we should make activate mandatory and actually set up the
affinity during activation correctly.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
@ 2020-07-27 13:35         ` Thomas Gleixner
  0 siblings, 0 replies; 23+ messages in thread
From: Thomas Gleixner @ 2020-07-27 13:35 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Ben Herrenschmidt, x86, LKML, John Keeping, Ali Saidi, linux-arm-kernel

Marc,

Marc Zyngier <maz@kernel.org> writes:
> On Fri, 24 Jul 2020 21:03:50 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> Right. My brain tricked me to believe that we made activation mandatory,
>> but that's not.
>> 
>> I have some ideas for a trivial generic way to solve this without
>> undoing the commit in question and without going through all the irq
>> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
>> has some brilliant idea.
>
> Not really. We have contradicting behaviours here, where some
> interrupts want to see the set_affinity early (the above case), and
> some cannot handle that (x86 vectors and the GICv3 ITS). We could key
> it on the presence of an activate callback, but it feels fragile.

Yes, I thought about that briefly, but yeah, it's fragile and bound to
break in weird ways.

That said, we should make activate mandatory and actually set up the
affinity during activation correctly.

Thanks,

        tglx

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [tip: irq/urgent] genirq/debugfs: Add missing irqchip flags
  2020-07-25 12:30         ` Marc Zyngier
@ 2020-07-27 14:25           ` tip-bot2 for Marc Zyngier
  0 siblings, 0 replies; 23+ messages in thread
From: tip-bot2 for Marc Zyngier @ 2020-07-27 14:25 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: Marc Zyngier, Thomas Gleixner, x86, LKML

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     aa251fc5b936d3ddb4b4c4b36427eb9aa3347c82
Gitweb:        https://git.kernel.org/tip/aa251fc5b936d3ddb4b4c4b36427eb9aa3347c82
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Sat, 25 Jul 2020 13:30:55 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 27 Jul 2020 16:20:40 +02:00

genirq/debugfs: Add missing irqchip flags

Recently introduced irqchip flags lack the corresponding printouts in
debugfs. Add them.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/874kpvydxc.wl-maz@kernel.org


---
 kernel/irq/debugfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 4f9f844..b95ff5d 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -112,6 +112,7 @@ static const struct irq_bit_descr irqdata_states[] = {
 	BIT_MASK_DESCR(IRQD_AFFINITY_SET),
 	BIT_MASK_DESCR(IRQD_SETAFFINITY_PENDING),
 	BIT_MASK_DESCR(IRQD_AFFINITY_MANAGED),
+	BIT_MASK_DESCR(IRQD_AFFINITY_ON_ACTIVATE),
 	BIT_MASK_DESCR(IRQD_MANAGED_SHUTDOWN),
 	BIT_MASK_DESCR(IRQD_CAN_RESERVE),
 	BIT_MASK_DESCR(IRQD_MSI_NOMASK_QUIRK),
@@ -120,6 +121,10 @@ static const struct irq_bit_descr irqdata_states[] = {
 
 	BIT_MASK_DESCR(IRQD_WAKEUP_STATE),
 	BIT_MASK_DESCR(IRQD_WAKEUP_ARMED),
+
+	BIT_MASK_DESCR(IRQD_DEFAULT_TRIGGER_SET),
+
+	BIT_MASK_DESCR(IRQD_HANDLE_ENFORCE_IRQCTX),
 };
 
 static const struct irq_bit_descr irqdesc_states[] = {

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in
  2020-07-24 20:44       ` Thomas Gleixner
  2020-07-25 12:30         ` Marc Zyngier
@ 2020-07-27 14:25         ` tip-bot2 for Thomas Gleixner
  2020-07-30 13:48           ` Sasha Levin
  1 sibling, 1 reply; 23+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2020-07-27 14:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: John Keeping, Thomas Gleixner, Marc Zyngier, stable, x86, LKML

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     f0c7baca180046824e07fc5f1326e83a8fd150c7
Gitweb:        https://git.kernel.org/tip/f0c7baca180046824e07fc5f1326e83a8fd150c7
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Fri, 24 Jul 2020 22:44:41 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 27 Jul 2020 16:20:40 +02:00

genirq/affinity: Make affinity setting if activated opt-in

John reported that on a RK3288 system the perf per CPU interrupts are all
affine to CPU0 and provided the analysis:

 "It looks like what happens is that because the interrupts are not per-CPU
  in the hardware, armpmu_request_irq() calls irq_force_affinity() while
  the interrupt is deactivated and then request_irq() with IRQF_PERCPU |
  IRQF_NOBALANCING.  

  Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
  irq_setup_affinity() which returns early because IRQF_PERCPU and
  IRQF_NOBALANCING are set, leaving the interrupt on its original CPU."

This was broken by the recent commit which blocked interrupt affinity
setting in hardware before activation of the interrupt. While this works in
general, it does not work for this particular case. As contrary to the
initial analysis not all interrupt chip drivers implement an activate
callback, the safe cure is to make the deferred interrupt affinity setting
at activation time opt-in.

Implement the necessary core logic and make the two irqchip implementations
for which this is required opt-in. In hindsight this would have been the
right thing to do, but ...

Fixes: baedb87d1b53 ("genirq/affinity: Handle affinity setting on inactive interrupts correctly")
Reported-by: John Keeping <john@metanate.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Marc Zyngier <maz@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/87blk4tzgm.fsf@nanos.tec.linutronix.de
---
 arch/x86/kernel/apic/vector.c    |  4 ++++
 drivers/irqchip/irq-gic-v3-its.c |  5 ++++-
 include/linux/irq.h              | 13 +++++++++++++
 kernel/irq/manage.c              |  6 +++++-
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 7649da2..dae32d9 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct irq_domain *domain, unsigned int virq,
 		 * as that can corrupt the affinity move state.
 		 */
 		irqd_set_handle_enforce_irqctx(irqd);
+
+		/* Don't invoke affinity setter on deactivated interrupts */
+		irqd_set_affinity_on_activate(irqd);
+
 		/*
 		 * Legacy vectors are already assigned when the IOAPIC
 		 * takes them over. They stay on the same vector. This is
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index beac4ca..103d850 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	msi_alloc_info_t *info = args;
 	struct its_device *its_dev = info->scratchpad[0].ptr;
 	struct its_node *its = its_dev->its;
+	struct irq_data *irqd;
 	irq_hw_number_t hwirq;
 	int err;
 	int i;
@@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 
 		irq_domain_set_hwirq_and_chip(domain, virq + i,
 					      hwirq + i, &its_irq_chip, its_dev);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq + i)));
+		irqd = irq_get_irq_data(virq + i);
+		irqd_set_single_target(irqd);
+		irqd_set_affinity_on_activate(irqd);
 		pr_debug("ID:%d pID:%d vID:%d\n",
 			 (int)(hwirq + i - its_dev->event_map.lpi_base),
 			 (int)(hwirq + i), virq + i);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 8d5bc2c..1b7f4df 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -213,6 +213,8 @@ struct irq_data {
  *				  required
  * IRQD_HANDLE_ENFORCE_IRQCTX	- Enforce that handle_irq_*() is only invoked
  *				  from actual interrupt context.
+ * IRQD_AFFINITY_ON_ACTIVATE	- Affinity is set on activation. Don't call
+ *				  irq_chip::irq_set_affinity() when deactivated.
  */
 enum {
 	IRQD_TRIGGER_MASK		= 0xf,
@@ -237,6 +239,7 @@ enum {
 	IRQD_CAN_RESERVE		= (1 << 26),
 	IRQD_MSI_NOMASK_QUIRK		= (1 << 27),
 	IRQD_HANDLE_ENFORCE_IRQCTX	= (1 << 28),
+	IRQD_AFFINITY_ON_ACTIVATE	= (1 << 29),
 };
 
 #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk(struct irq_data *d)
 	return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
 }
 
+static inline void irqd_set_affinity_on_activate(struct irq_data *d)
+{
+	__irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
+}
+
+static inline bool irqd_affinity_on_activate(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2a9fec5..48c38e0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated(struct irq_data *data,
 	struct irq_desc *desc = irq_data_to_desc(data);
 
 	/*
+	 * Handle irq chips which can handle affinity only in activated
+	 * state correctly
+	 *
 	 * If the interrupt is not yet activated, just store the affinity
 	 * mask and do not call the chip driver at all. On activation the
 	 * driver has to make sure anyway that the interrupt is in a
 	 * useable state so startup works.
 	 */
-	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+	if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
+	    irqd_is_activated(data) || !irqd_affinity_on_activate(data))
 		return false;
 
 	cpumask_copy(desc->irq_common_data.affinity, mask);

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in
  2020-07-27 14:25         ` [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in tip-bot2 for Thomas Gleixner
@ 2020-07-30 13:48           ` Sasha Levin
  0 siblings, 0 replies; 23+ messages in thread
From: Sasha Levin @ 2020-07-30 13:48 UTC (permalink / raw)
  To: Sasha Levin, tip-bot2 for Thomas Gleixner, linux-tip-commits
  Cc: John Keeping, stable, stable

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: baedb87d1b53 ("genirq/affinity: Handle affinity setting on inactive interrupts correctly").

The bot has tested the following trees: v5.7.11, v5.4.54, v4.19.135.

v5.7.11: Build OK!
v5.4.54: Failed to apply! Possible dependencies:
    008f1d60fe25 ("x86/apic/vector: Force interupt handler invocation to irq context")
    c16816acd086 ("genirq: Add protection against unsafe usage of generic_handle_irq()")

v4.19.135: Failed to apply! Possible dependencies:
    008f1d60fe25 ("x86/apic/vector: Force interupt handler invocation to irq context")
    35ae7df21be0 ("irqchip/gic-v3-its: Don't map the MSI page in its_irq_compose_msi_msg()")
    c16816acd086 ("genirq: Add protection against unsafe usage of generic_handle_irq()")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-07-30 13:48 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 13:14 [PATCH] genirq/affinity: Handle affinity setting on inactive interrupts correctly Thomas Gleixner
2020-07-17 15:25 ` kernel test robot
2020-07-17 16:00 ` [PATCH V2] " Thomas Gleixner
2020-07-24 17:24   ` John Keeping
2020-07-24 20:03     ` Thomas Gleixner
2020-07-24 20:44       ` Thomas Gleixner
2020-07-25 12:30         ` Marc Zyngier
2020-07-27 14:25           ` [tip: irq/urgent] genirq/debugfs: Add missing irqchip flags tip-bot2 for Marc Zyngier
2020-07-27 14:25         ` [tip: irq/urgent] genirq/affinity: Make affinity setting if activated opt-in tip-bot2 for Thomas Gleixner
2020-07-30 13:48           ` Sasha Levin
2020-07-25 12:08       ` [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly Marc Zyngier
2020-07-27 13:35         ` Thomas Gleixner
2020-07-17 18:04 ` [PATCH] " kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.