[patch 0/8] genirq, perf: Cleanup the abuse of irq_set_affinity_hint()

Thomas Gleixner

author

2021-05-18 09:33 UTC #0 lore raw

cover genirq, perf: Cleanup the abuse of irq_set_affinity_hint()

The modular PMU drivers use irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Sigh, if people would at least talk if something is missing...

Clean up the mess by exposing irq_set_affinity() and converting the drivers over to that.

Thanks,

tglx

---
 drivers/perf/arm-ccn.c                        |    6 +---
 drivers/perf/arm-cmn.c                        |    9 +-----
 drivers/perf/arm_dmc620_pmu.c                 |    5 +--
 drivers/perf/arm_dsu_pmu.c                    |    8 +----
 drivers/perf/arm_smmuv3_pmu.c                 |   10 ++-----
 drivers/perf/fsl_imx8_ddr_perf.c              |    5 +--
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c |    3 --
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  |    3 --
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  |    3 --
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   |    3 --
 drivers/perf/hisilicon/hisi_uncore_pmu.c      |    4 +-
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c |    3 --
 include/linux/interrupt.h                     |   35 +-------------------------
 kernel/irq/manage.c                           |   33 +++++++++++++++++++++++-
 14 files changed, 49 insertions(+), 81 deletions(-)

Thomas Gleixner

author

2021-05-18 09:33 UTC #1 lore raw

PATCH 1/8 genirq: Export affinity setter for modules

Perf modules abuse irq_set_affinity_hint() to set the affinity of system PMU interrupts just because irq_set_affinity() was not exported.

The fact that irq_set_affinity_hint() actually sets the affinity is a non-documented side effect and the name is clearly saying it's a hint.

To clean this up, export the real affinity setter.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Thomas Gleixner <[email protected]>

---
 include/linux/interrupt.h |   35 ++---------------------------------
 kernel/irq/manage.c       |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 34 deletions(-)

include/linux/interrupt.h +2 ~~-33~~

@@ -322,39 +322,8 @@ struct irq_affinity_desc {
 
 extern cpumask_var_t irq_default_affinity;
 
-/* Internal implementation. Use the helpers below */
-extern int __irq_set_affinity(unsigned int irq, const struct cpumask *cpumask,
-			      bool force);
-
-/**
- * irq_set_affinity - Set the irq affinity of a given irq
- * @irq:	Interrupt to set affinity
- * @cpumask:	cpumask
- *
- * Fails if cpumask does not contain an online CPU
- */
-static inline int
-irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
-{
-	return __irq_set_affinity(irq, cpumask, false);
-}
-
-/**
- * irq_force_affinity - Force the irq affinity of a given irq
- * @irq:	Interrupt to set affinity
- * @cpumask:	cpumask
- *
- * Same as irq_set_affinity, but without checking the mask against
- * online cpus.
- *
- * Solely for low level cpu hotplug code, where we need to make per
- * cpu interrupts affine before the cpu becomes online.
- */
-static inline int
-irq_force_affinity(unsigned int irq, const struct cpumask *cpumask)
-{
-	return __irq_set_affinity(irq, cpumask, true);
-}
+extern int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask);
+extern int irq_force_affinity(unsigned int irq, const struct cpumask *cpumask);
 
 extern int irq_can_set_affinity(unsigned int irq);
 extern int irq_select_affinity(unsigned int irq);

kernel/irq/manage.c +32 -1

@@ -441,7 +441,8 @@ int irq_update_affinity_desc(unsigned in
 	return ret;
 }
 
-int __irq_set_affinity(unsigned int irq, const struct cpumask *mask, bool force)
+static int __irq_set_affinity(unsigned int irq, const struct cpumask *mask,
+			      bool force)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	unsigned long flags;

@@ -456,6 +457,36 @@ int __irq_set_affinity(unsigned int irq,
 	return ret;
 }
 
+/**
+ * irq_set_affinity - Set the irq affinity of a given irq
+ * @irq:	Interrupt to set affinity
+ * @cpumask:	cpumask
+ *
+ * Fails if cpumask does not contain an online CPU
+ */
+int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
+{
+	return __irq_set_affinity(irq, cpumask, false);
+}
+EXPORT_SYMBOL_GPL(irq_set_affinity);
+
+/**
+ * irq_force_affinity - Force the irq affinity of a given irq
+ * @irq:	Interrupt to set affinity
+ * @cpumask:	cpumask
+ *
+ * Same as irq_set_affinity, but without checking the mask against
+ * online cpus.
+ *
+ * Solely for low level cpu hotplug code, where we need to make per
+ * cpu interrupts affine before the cpu becomes online.
+ */
+int irq_force_affinity(unsigned int irq, const struct cpumask *cpumask)
+{
+	return __irq_set_affinity(irq, cpumask, true);
+}
+EXPORT_SYMBOL_GPL(irq_force_affinity);
+
 int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m)
 {
 	unsigned long flags;

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-05-19 09:14 UTC #2 lore raw

[tip: irq/core] genirq: Export affinity setter for modules

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

Commit-ID:     4d80d6ca5d77fde9880da8466e5b64f250e5bf82
Gitweb:        https://git.kernel.org/tip/4d80d6ca5d77fde9880da8466e5b64f250e5bf82
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Tue, 18 May 2021 11:17:26 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Wed, 19 May 2021 11:01:51 +02:00

genirq: Export affinity setter for modules

Perf modules abuse irq_set_affinity_hint() to set the affinity of system PMU interrupts just because irq_set_affinity() was not exported.

The fact that irq_set_affinity_hint() actually sets the affinity is a non-documented side effect and the name is clearly saying it's a hint.

To clean this up, export the real affinity setter.

Signed-off-by Thomas Gleixner <[email protected]>

Acked-by Mark Rutland <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 include/linux/interrupt.h | 35 ++---------------------------------
 kernel/irq/manage.c       | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 34 deletions(-)

include/linux/interrupt.h +2 ~~-33~~

index 4777850..35a3742 100644
@@ -319,39 +319,8 @@ struct irq_affinity_desc {
 
 extern cpumask_var_t irq_default_affinity;
 
-/* Internal implementation. Use the helpers below */
-extern int __irq_set_affinity(unsigned int irq, const struct cpumask *cpumask,
-			      bool force);
-
-/**
- * irq_set_affinity - Set the irq affinity of a given irq
- * @irq:	Interrupt to set affinity
- * @cpumask:	cpumask
- *
- * Fails if cpumask does not contain an online CPU
- */
-static inline int
-irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
-{
-	return __irq_set_affinity(irq, cpumask, false);
-}
-
-/**
- * irq_force_affinity - Force the irq affinity of a given irq
- * @irq:	Interrupt to set affinity
- * @cpumask:	cpumask
- *
- * Same as irq_set_affinity, but without checking the mask against
- * online cpus.
- *
- * Solely for low level cpu hotplug code, where we need to make per
- * cpu interrupts affine before the cpu becomes online.
- */
-static inline int
-irq_force_affinity(unsigned int irq, const struct cpumask *cpumask)
-{
-	return __irq_set_affinity(irq, cpumask, true);
-}
+extern int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask);
+extern int irq_force_affinity(unsigned int irq, const struct cpumask *cpumask);
 
 extern int irq_can_set_affinity(unsigned int irq);
 extern int irq_select_affinity(unsigned int irq);

kernel/irq/manage.c +32 -1

index 4c14356..a847dd2 100644
@@ -441,7 +441,8 @@ out_unlock:
 	return ret;
 }
 
-int __irq_set_affinity(unsigned int irq, const struct cpumask *mask, bool force)
+static int __irq_set_affinity(unsigned int irq, const struct cpumask *mask,
+			      bool force)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	unsigned long flags;

@@ -456,6 +457,36 @@ int __irq_set_affinity(unsigned int irq, const struct cpumask *mask, bool force)
 	return ret;
 }
 
+/**
+ * irq_set_affinity - Set the irq affinity of a given irq
+ * @irq:	Interrupt to set affinity
+ * @cpumask:	cpumask
+ *
+ * Fails if cpumask does not contain an online CPU
+ */
+int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
+{
+	return __irq_set_affinity(irq, cpumask, false);
+}
+EXPORT_SYMBOL_GPL(irq_set_affinity);
+
+/**
+ * irq_force_affinity - Force the irq affinity of a given irq
+ * @irq:	Interrupt to set affinity
+ * @cpumask:	cpumask
+ *
+ * Same as irq_set_affinity, but without checking the mask against
+ * online cpus.
+ *
+ * Solely for low level cpu hotplug code, where we need to make per
+ * cpu interrupts affine before the cpu becomes online.
+ */
+int irq_force_affinity(unsigned int irq, const struct cpumask *cpumask)
+{
+	return __irq_set_affinity(irq, cpumask, true);
+}
+EXPORT_SYMBOL_GPL(irq_force_affinity);
+
 int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m)
 {
 	unsigned long flags;

Thomas Gleixner

author

2021-05-18 09:33 UTC #3 lore raw

PATCH 2/8 perf/arm-ccn: Use irq_set_affinity()

In reply to Thomas Gleixner

The driver uses irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc [email protected]

---
 drivers/perf/arm-ccn.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/perf/arm-ccn.c +2 -4

@@ -1211,7 +1211,7 @@ static int arm_ccn_pmu_offline_cpu(unsig
 	perf_pmu_migrate_context(&dt->pmu, cpu, target);
 	dt->cpu = target;
 	if (ccn->irq)
-		WARN_ON(irq_set_affinity_hint(ccn->irq, cpumask_of(dt->cpu)));
+		WARN_ON(irq_set_affinity(ccn->irq, cpumask_of(dt->cpu)));
 	return 0;
 }

@@ -1291,7 +1291,7 @@ static int arm_ccn_pmu_init(struct arm_c
 
 	/* Also make sure that the overflow interrupt is handled by this CPU */
 	if (ccn->irq) {
-		err = irq_set_affinity_hint(ccn->irq, cpumask_of(ccn->dt.cpu));
+		err = irq_set_affinity(ccn->irq, cpumask_of(ccn->dt.cpu));
 		if (err) {
 			dev_err(ccn->dev, "Failed to set interrupt affinity!\n");
 			goto error_set_affinity;

@@ -1325,8 +1325,6 @@ static void arm_ccn_pmu_cleanup(struct a
 
 	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE,
 					    &ccn->dt.node);
-	if (ccn->irq)
-		irq_set_affinity_hint(ccn->irq, NULL);
 	for (i = 0; i < ccn->num_xps; i++)
 		writel(0, ccn->xp[i].base + CCN_XP_DT_CONTROL);
 	writel(0, ccn->dt.base + CCN_DT_PMCR);

Thomas Gleixner

author

2021-05-18 09:33 UTC #4 lore raw

PATCH 3/8 perf/arm-cmn: Use irq_set_affinity()

In reply to Thomas Gleixner

The driver uses irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc [email protected]

---
 drivers/perf/arm-cmn.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

drivers/perf/arm-cmn.c +2 -7

@@ -1162,7 +1162,7 @@ static int arm_cmn_pmu_offline_cpu(unsig
 
 	perf_pmu_migrate_context(&cmn->pmu, cpu, target);
 	for (i = 0; i < cmn->num_dtcs; i++)
-		irq_set_affinity_hint(cmn->dtc[i].irq, cpumask_of(target));
+		irq_set_affinity(cmn->dtc[i].irq, cpumask_of(target));
 	cmn->cpu = target;
 	return 0;
 }

@@ -1222,7 +1222,7 @@ static int arm_cmn_init_irqs(struct arm_
 		if (err)
 			return err;
 
-		err = irq_set_affinity_hint(irq, cpumask_of(cmn->cpu));
+		err = irq_set_affinity(irq, cpumask_of(cmn->cpu));
 		if (err)
 			return err;
 	next:

@@ -1568,16 +1568,11 @@ static int arm_cmn_probe(struct platform
 static int arm_cmn_remove(struct platform_device *pdev)
 {
 	struct arm_cmn *cmn = platform_get_drvdata(pdev);
-	int i;
 
 	writel_relaxed(0, cmn->dtc[0].base + CMN_DT_DTC_CTL);
 
 	perf_pmu_unregister(&cmn->pmu);
 	cpuhp_state_remove_instance(arm_cmn_hp_state, &cmn->cpuhp_node);
-
-	for (i = 0; i < cmn->num_dtcs; i++)
-		irq_set_affinity_hint(cmn->dtc[i].irq, NULL);
-
 	return 0;
 }

Thomas Gleixner

author

2021-05-18 09:33 UTC #5 lore raw

PATCH 4/8 perf/arm-dmc620: Use irq_set_affinity()

In reply to Thomas Gleixner

The driver uses irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc [email protected]

---
 drivers/perf/arm_dmc620_pmu.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/perf/arm_dmc620_pmu.c +2 -3

@@ -421,7 +421,7 @@ static struct dmc620_pmu_irq *__dmc620_p
 	if (ret)
 		goto out_free_aff;
 
-	ret = irq_set_affinity_hint(irq_num, cpumask_of(irq->cpu));
+	ret = irq_set_affinity(irq_num, cpumask_of(irq->cpu));
 	if (ret)
 		goto out_free_irq;

@@ -475,7 +475,6 @@ static void dmc620_pmu_put_irq(struct dm
 	list_del(&irq->irqs_node);
 	mutex_unlock(&dmc620_pmu_irqs_lock);
 
-	WARN_ON(irq_set_affinity_hint(irq->irq_num, NULL));
 	free_irq(irq->irq_num, irq);
 	cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &irq->node);
 	kfree(irq);

@@ -622,7 +621,7 @@ static int dmc620_pmu_cpu_teardown(unsig
 		perf_pmu_migrate_context(&dmc620_pmu->pmu, irq->cpu, target);
 	mutex_unlock(&dmc620_pmu_irqs_lock);
 
-	WARN_ON(irq_set_affinity_hint(irq->irq_num, cpumask_of(target)));
+	WARN_ON(irq_set_affinity(irq->irq_num, cpumask_of(target)));
 	irq->cpu = target;
 
 	return 0;

Thomas Gleixner

author

2021-05-18 09:33 UTC #6 lore raw

PATCH 5/8 perf/arm-dsu: Use irq_set_affinity()

In reply to Thomas Gleixner

The driver uses irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc [email protected]

---
 drivers/perf/arm_dsu_pmu.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/perf/arm_dsu_pmu.c +2 -6

@@ -687,7 +687,7 @@ static void dsu_pmu_probe_pmu(struct dsu
 static void dsu_pmu_set_active_cpu(int cpu, struct dsu_pmu *dsu_pmu)
 {
 	cpumask_set_cpu(cpu, &dsu_pmu->active_cpu);
-	if (irq_set_affinity_hint(dsu_pmu->irq, &dsu_pmu->active_cpu))
+	if (irq_set_affinity(dsu_pmu->irq, &dsu_pmu->active_cpu))
 		pr_warn("Failed to set irq affinity to %d\n", cpu);
 }

@@ -769,7 +769,6 @@ static int dsu_pmu_device_probe(struct p
 	if (rc) {
 		cpuhp_state_remove_instance(dsu_pmu_cpuhp_state,
 						 &dsu_pmu->cpuhp_node);
-		irq_set_affinity_hint(dsu_pmu->irq, NULL);
 	}
 
 	return rc;

@@ -781,7 +780,6 @@ static int dsu_pmu_device_remove(struct
 
 	perf_pmu_unregister(&dsu_pmu->pmu);
 	cpuhp_state_remove_instance(dsu_pmu_cpuhp_state, &dsu_pmu->cpuhp_node);
-	irq_set_affinity_hint(dsu_pmu->irq, NULL);
 
 	return 0;
 }

@@ -840,10 +838,8 @@ static int dsu_pmu_cpu_teardown(unsigned
 
 	dst = dsu_pmu_get_online_cpu_any_but(dsu_pmu, cpu);
 	/* If there are no active CPUs in the DSU, leave IRQ disabled */
-	if (dst >= nr_cpu_ids) {
-		irq_set_affinity_hint(dsu_pmu->irq, NULL);
+	if (dst >= nr_cpu_ids)
 		return 0;
-	}
 
 	perf_pmu_migrate_context(&dsu_pmu->pmu, cpu, dst);
 	dsu_pmu_set_active_cpu(dst, dsu_pmu);

John Garry

2021-05-18 11:32 UTC #7 lore raw

Re: [patch 5/8] perf/arm-dsu: Use irq_set_affinity()

drivers/perf/arm_dsu_pmu.c static int dsu_pmu_device_probe(struct p · line 769

Quoted 3 lines · Thomas Gleixner

drivers/perf/arm_dsu_pmu.c +0 -0

@@ -769,7 +769,6 @@ static int dsu_pmu_device_probe(struct p
  	if (rc) {

nit: I think that someone will send a patch to remove these {} later...

drivers/perf/arm_dsu_pmu.c static int dsu_pmu_device_probe(struct p · line 772

Quoted 4 lines

drivers/perf/arm_dsu_pmu.c +0 -1

  		cpuhp_state_remove_instance(dsu_pmu_cpuhp_state,
  						 &dsu_pmu->cpuhp_node);
-		irq_set_affinity_hint(dsu_pmu->irq, NULL);
  	}

Thanks,
John

Thomas Gleixner

author

2021-05-18 15:51 UTC #8 lore raw

Re: [patch 5/8] perf/arm-dsu: Use irq_set_affinity()

Quoted 11 lines · John Garry

drivers/perf/arm_dsu_pmu.c static int dsu_pmu_device_probe(struct p · line 769
Quoted 3 lines · Thomas Gleixner
drivers/perf/arm_dsu_pmu.c +0 -0
@@ -769,7 +769,6 @@ static int dsu_pmu_device_probe(struct p
  	if (rc) {
nit: I think that someone will send a patch to remove these {} later...
drivers/perf/arm_dsu_pmu.c static int dsu_pmu_device_probe(struct p · line 772
Quoted 4 lines
drivers/perf/arm_dsu_pmu.c +0 -1
  		cpuhp_state_remove_instance(dsu_pmu_cpuhp_state,
  						 &dsu_pmu->cpuhp_node);
-		irq_set_affinity_hint(dsu_pmu->irq, NULL);
  	}

which should be rejected because

cpuhp_state_remove_instance(dsu_pmu_cpuhp_state,
&dsu_pmu->cpuhp_node);

is _NOT_ a one line statement.

	if (foo)
		cpuhp_state_remove_instance(state, &node);

is fine, but

if (foo)
cpuhp_state_remove_instance(dsu_pmu_cpuhp_state,
&dsu_pmu->cpuhp_node);

breaks the expectation of a single line following the condition which confuses my brain based OCR. :)

So I left the brackets there on purpose.

Thanks,

tglx

Thomas Gleixner

author

2021-05-18 09:33 UTC #9 lore raw

PATCH 6/8 perf/arm-smmuv3: Use irq_set_affinity()

In reply to Thomas Gleixner

The driver uses irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc [email protected]

---
 drivers/perf/arm_smmuv3_pmu.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

drivers/perf/arm_smmuv3_pmu.c +3 -7

@@ -628,7 +628,7 @@ static int smmu_pmu_offline_cpu(unsigned
 
 	perf_pmu_migrate_context(&smmu_pmu->pmu, cpu, target);
 	smmu_pmu->on_cpu = target;
-	WARN_ON(irq_set_affinity_hint(smmu_pmu->irq, cpumask_of(target)));
+	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(target)));
 
 	return 0;
 }

@@ -839,15 +839,14 @@ static int smmu_pmu_probe(struct platfor
 
 	/* Pick one CPU to be the preferred one to use */
 	smmu_pmu->on_cpu = raw_smp_processor_id();
-	WARN_ON(irq_set_affinity_hint(smmu_pmu->irq,
-				      cpumask_of(smmu_pmu->on_cpu)));
+	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));
 
 	err = cpuhp_state_add_instance_nocalls(cpuhp_state_num,
 					       &smmu_pmu->node);
 	if (err) {
 		dev_err(dev, "Error %d registering hotplug, PMU @%pa\n",
 			err, &res_0->start);
-		goto out_clear_affinity;
+		return err;
 	}
 
 	err = perf_pmu_register(&smmu_pmu->pmu, name, -1);

@@ -866,8 +865,6 @@ static int smmu_pmu_probe(struct platfor
 
 out_unregister:
 	cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node);
-out_clear_affinity:
-	irq_set_affinity_hint(smmu_pmu->irq, NULL);
 	return err;
 }

@@ -877,7 +874,6 @@ static int smmu_pmu_remove(struct platfo
 
 	perf_pmu_unregister(&smmu_pmu->pmu);
 	cpuhp_state_remove_instance_nocalls(cpuhp_state_num, &smmu_pmu->node);
-	irq_set_affinity_hint(smmu_pmu->irq, NULL);
 
 	return 0;
 }

Thomas Gleixner

author

2021-05-18 09:33 UTC #10 lore raw

PATCH 7/8 perf/imx_ddr: Use irq_set_affinity()

In reply to Thomas Gleixner

The driver uses irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Frank Li <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Pengutronix Kernel Team <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc NXP Linux Team <[email protected]>

Cc [email protected]

---
 drivers/perf/fsl_imx8_ddr_perf.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/perf/fsl_imx8_ddr_perf.c +2 -3

@@ -674,7 +674,7 @@ static int ddr_perf_offline_cpu(unsigned
 	perf_pmu_migrate_context(&pmu->pmu, cpu, target);
 	pmu->cpu = target;
 
-	WARN_ON(irq_set_affinity_hint(pmu->irq, cpumask_of(pmu->cpu)));
+	WARN_ON(irq_set_affinity(pmu->irq, cpumask_of(pmu->cpu)));
 
 	return 0;
 }

@@ -749,7 +749,7 @@ static int ddr_perf_probe(struct platfor
 	}
 
 	pmu->irq = irq;
-	ret = irq_set_affinity_hint(pmu->irq, cpumask_of(pmu->cpu));
+	ret = irq_set_affinity(pmu->irq, cpumask_of(pmu->cpu));
 	if (ret) {
 		dev_err(pmu->dev, "Failed to set interrupt affinity!\n");
 		goto ddr_perf_err;

@@ -777,7 +777,6 @@ static int ddr_perf_remove(struct platfo
 
 	cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
 	cpuhp_remove_multi_state(pmu->cpuhp_state);
-	irq_set_affinity_hint(pmu->irq, NULL);
 
 	perf_pmu_unregister(&pmu->pmu);

Thomas Gleixner

author

2021-05-18 09:33 UTC #11 lore raw

PATCH 8/8 perf/hisi: Use irq_set_affinity()

In reply to Thomas Gleixner

These drivers use irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.

Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.

Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Replace the mindless abuse with irq_set_affinity().

Signed-off-by Thomas Gleixner <[email protected]>

Cc Shaokun Zhang <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc [email protected]

---
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c |    3 ---
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  |    3 ---
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  |    3 ---
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   |    3 ---
 drivers/perf/hisilicon/hisi_uncore_pmu.c      |    4 ++--
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c |    3 ---
 6 files changed, 2 insertions(+), 17 deletions(-)

drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c +0 -3

@@ -537,7 +537,6 @@ static int hisi_ddrc_pmu_probe(struct pl
 		dev_err(ddrc_pmu->dev, "DDRC PMU register failed!\n");
 		cpuhp_state_remove_instance_nocalls(
 			CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE, &ddrc_pmu->node);
-		irq_set_affinity_hint(ddrc_pmu->irq, NULL);
 	}
 
 	return ret;

@@ -550,8 +549,6 @@ static int hisi_ddrc_pmu_remove(struct p
 	perf_pmu_unregister(&ddrc_pmu->pmu);
 	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
 					    &ddrc_pmu->node);
-	irq_set_affinity_hint(ddrc_pmu->irq, NULL);
-
 	return 0;
 }

drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +0 -3

@@ -540,7 +540,6 @@ static int hisi_hha_pmu_probe(struct pla
 		dev_err(hha_pmu->dev, "HHA PMU register failed!\n");
 		cpuhp_state_remove_instance_nocalls(
 			CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE, &hha_pmu->node);
-		irq_set_affinity_hint(hha_pmu->irq, NULL);
 	}
 
 	return ret;

@@ -553,8 +552,6 @@ static int hisi_hha_pmu_remove(struct pl
 	perf_pmu_unregister(&hha_pmu->pmu);
 	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE,
 					    &hha_pmu->node);
-	irq_set_affinity_hint(hha_pmu->irq, NULL);
-
 	return 0;
 }

drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c +0 -3

@@ -578,7 +578,6 @@ static int hisi_l3c_pmu_probe(struct pla
 		dev_err(l3c_pmu->dev, "L3C PMU register failed!\n");
 		cpuhp_state_remove_instance_nocalls(
 			CPUHP_AP_PERF_ARM_HISI_L3_ONLINE, &l3c_pmu->node);
-		irq_set_affinity_hint(l3c_pmu->irq, NULL);
 	}
 
 	return ret;

@@ -591,8 +590,6 @@ static int hisi_l3c_pmu_remove(struct pl
 	perf_pmu_unregister(&l3c_pmu->pmu);
 	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
 					    &l3c_pmu->node);
-	irq_set_affinity_hint(l3c_pmu->irq, NULL);
-
 	return 0;
 }

drivers/perf/hisilicon/hisi_uncore_pa_pmu.c +0 -3

@@ -436,7 +436,6 @@ static int hisi_pa_pmu_probe(struct plat
 		dev_err(pa_pmu->dev, "PMU register failed, ret = %d\n", ret);
 		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
 					    &pa_pmu->node);
-		irq_set_affinity_hint(pa_pmu->irq, NULL);
 		return ret;
 	}

@@ -451,8 +450,6 @@ static int hisi_pa_pmu_remove(struct pla
 	perf_pmu_unregister(&pa_pmu->pmu);
 	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
 					    &pa_pmu->node);
-	irq_set_affinity_hint(pa_pmu->irq, NULL);
-
 	return 0;
 }

drivers/perf/hisilicon/hisi_uncore_pmu.c +2 -2

@@ -488,7 +488,7 @@ int hisi_uncore_pmu_online_cpu(unsigned
 	hisi_pmu->on_cpu = cpu;
 
 	/* Overflow interrupt also should use the same CPU */
-	WARN_ON(irq_set_affinity_hint(hisi_pmu->irq, cpumask_of(cpu)));
+	WARN_ON(irq_set_affinity(hisi_pmu->irq, cpumask_of(cpu)));
 
 	return 0;
 }

@@ -521,7 +521,7 @@ int hisi_uncore_pmu_offline_cpu(unsigned
 	perf_pmu_migrate_context(&hisi_pmu->pmu, cpu, target);
 	/* Use this CPU for event counting */
 	hisi_pmu->on_cpu = target;
-	WARN_ON(irq_set_affinity_hint(hisi_pmu->irq, cpumask_of(target)));
+	WARN_ON(irq_set_affinity(hisi_pmu->irq, cpumask_of(target)));
 
 	return 0;
 }

drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c +0 -3

@@ -465,7 +465,6 @@ static int hisi_sllc_pmu_probe(struct pl
 		dev_err(sllc_pmu->dev, "PMU register failed, ret = %d\n", ret);
 		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
 					    &sllc_pmu->node);
-		irq_set_affinity_hint(sllc_pmu->irq, NULL);
 		return ret;
 	}

@@ -481,8 +480,6 @@ static int hisi_sllc_pmu_remove(struct p
 	perf_pmu_unregister(&sllc_pmu->pmu);
 	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
 					    &sllc_pmu->node);
-	irq_set_affinity_hint(sllc_pmu->irq, NULL);
-
 	return 0;
 }

Mark Rutland

maintainer Acked-by on the v1 series

2021-05-18 10:11 UTC #12 lore raw

In reply to Thomas Gleixner

Hi Thomas,

Quoted 14 lines · Thomas Gleixner

The modular PMU drivers use irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.
Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.
Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Quoted 4 lines

Sigh, if people would at least talk if something is missing...
Clean up the mess by exposing irq_set_affinity() and converting the drivers over to that.

Sorry about this, and thanks for cleaning this up.

For the series:

Acked-by Mark Rutland <[email protected]>

Mark.

Quoted 22 lines

Thanks,

tglx

---
 drivers/perf/arm-ccn.c                        |    6 +---
 drivers/perf/arm-cmn.c                        |    9 +-----
 drivers/perf/arm_dmc620_pmu.c                 |    5 +--
 drivers/perf/arm_dsu_pmu.c                    |    8 +----
 drivers/perf/arm_smmuv3_pmu.c                 |   10 ++-----
 drivers/perf/fsl_imx8_ddr_perf.c              |    5 +--
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c |    3 --
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  |    3 --
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  |    3 --
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   |    3 --
 drivers/perf/hisilicon/hisi_uncore_pmu.c      |    4 +-
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c |    3 --
 include/linux/interrupt.h                     |   35 +-------------------------
 kernel/irq/manage.c                           |   33 +++++++++++++++++++++++-
 14 files changed, 49 insertions(+), 81 deletions(-)

Will Deacon

maintainer

2021-05-18 10:49 UTC #13 lore raw

In reply to Thomas Gleixner

Hi Thomas,

Quoted 14 lines · Thomas Gleixner

The modular PMU drivers use irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.
Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.
Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.

Quoted 4 lines

Sigh, if people would at least talk if something is missing...
Clean up the mess by exposing irq_set_affinity() and converting the drivers over to that.

This all looks good to me, thanks. Given the number of PMU drivers it touches, it may well conflict with driver work for 5.14. If you put the IRQ core stuff on a stable branch, then I could pull that into the Arm perf tree and stick all the driver changes on top. That also means any new drivers that come in can use irq_set_affinity() right away.

Does that work for you?

Cheers,

Will

Thomas Gleixner

author

2021-05-18 15:51 UTC #14 lore raw

Quoted 20 lines · Will Deacon

Quoted 14 lines · Thomas Gleixner
The modular PMU drivers use irq_set_affinity_hint() to set the affinity for the PMU interrupts, which relies on the undocumented side effect that this function actually sets the affinity under the hood.
Setting an hint is clearly not a guarantee and for these PMU interrupts an affinity hint, which is supposed to guide userspace for setting affinity, is beyond pointless, because the affinity of these interrupts cannot be modified from user space.
Aside of that the error checks are bogus because the only error which is returned from irq_set_affinity_hint() is when there is no irq descriptor for the interrupt number, but not when the affinity set fails. That's on purpose because the hint can point to an offline CPU.
Quoted 4 lines
Sigh, if people would at least talk if something is missing...
Clean up the mess by exposing irq_set_affinity() and converting the drivers over to that.

Quoted 7 lines

This all looks good to me, thanks. Given the number of PMU drivers it touches, it may well conflict with driver work for 5.14. If you put the IRQ core stuff on a stable branch, then I could pull that into the Arm perf tree and stick all the driver changes on top. That also means any new drivers that come in can use irq_set_affinity() right away.
Does that work for you?

Sure. I'll send you a tag to pull ...

Thanks,

tglx

Thomas Gleixner

author

2021-05-19 09:10 UTC #15 lore raw

Quoted 1 line · Thomas Gleixner

Sure. I'll send you a tag to pull ...

Here you go:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-export-set-affinity

Thanks,

tglx

Will Deacon

maintainer

2021-05-24 20:21 UTC #16 lore raw

Quoted 6 lines · Thomas Gleixner

Quoted 1 line · Thomas Gleixner
Sure. I'll send you a tag to pull ...
Here you go:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-export-set-affinity

Cheers, I've pulled that in and stuck the driver patches on top.

Will