[PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE

[PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE

[Alexander Stein]

Using DEBUG_TEST_DRIVER_REMOVE every driver gets probed, removed and probed
again. This breaks drivers without removal function, e.g. arm perf
resulting in the following dump:
------------[ cut here ]------------
WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6c/0x7c
sysfs: cannot create duplicate filename '/devices/armv7_cortex_a7'
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.9.0+ #212
Hardware name: Freescale LS1021A
Backtrace:
[<8020c044>] (dump_backtrace) from [<8020c2f0>] (show_stack+0x18/0x1c)
 r7:00000009 r6:60000013 r5:80c1776c r4:00000000
[<8020c2d8>] (show_stack) from [<8046ead8>] (dump_stack+0x94/0xa8)
[<8046ea44>] (dump_stack) from [<8021ecd0>] (__warn+0xec/0x104)
 r7:00000009 r6:8092efc8 r5:00000000 r4:bf04bd80
[<8021ebe4>] (__warn) from [<8021ed28>] (warn_slowpath_fmt+0x40/0x48)
 r9:80a32848 r8:00000000 r7:00000000 r6:bf0ab780 r5:8091d590 r4:bf0df000
[<8021ecec>] (warn_slowpath_fmt) from [<8036d53c>] (sysfs_warn_dup+0x6c/0x7c)
 r3:bf0df000 r2:8092ef94
[<8036d4d0>] (sysfs_warn_dup) from [<8036d628>] (sysfs_create_dir_ns+0x8c/0x9c)
 r6:bf0ab780 r5:bf20ee08 r4:ffffffef
[<8036d59c>] (sysfs_create_dir_ns) from [<80471860>] (kobject_add_internal+0xa8/0x34c)
 r6:bf0aa198 r5:00000000 r4:bf20ee08
[<804717b8>] (kobject_add_internal) from [<80471b54>] (kobject_add+0x50/0x94)
 r7:00000000 r6:00000000 r5:00000000 r4:bf20ee08
[<80471b08>] (kobject_add) from [<80524590>] (device_add+0xe4/0x590)
 r3:00000000 r2:00000000
 r6:bf20ee00 r5:00000000 r4:bf20ee08
[<805244ac>] (device_add) from [<802a38a8>] (pmu_dev_alloc+0x88/0xd8)
 r10:00000091 r9:80a32848 r8:00000000 r7:80a32840 r6:80c0ed3c r5:00000000
 r4:bf1fe800
[<802a3820>] (pmu_dev_alloc) from [<80a0cd20>] (perf_event_sysfs_init+0x5c/0xb4)
 r7:80a32840 r6:00000000 r5:bf1fe800 r4:80c0ede0
[<80a0ccc4>] (perf_event_sysfs_init) from [<80201778>] (do_one_initcall+0x48/0x178)
 r6:80c45000 r5:80a0ccc4 r4:00000006
[<80201730>] (do_one_initcall) from [<80a00ecc>] (kernel_init_freeable+0x168/0x20c)
 r8:80a00610 r7:80a32840 r6:80c45000 r5:80c45000 r4:00000006
[<80a00d64>] (kernel_init_freeable) from [<806febcc>] (kernel_init+0x10/0x11c)
 r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:806febbc
 r4:00000000
[<806febbc>] (kernel_init) from [<80208388>] (ret_from_fork+0x14/0x2c)
 r5:806febbc r4:00000000
---[ end trace 9d251d389382804f ]---

This patchset add a removal support for arm_pmu and perf_events_v7 and while
at it, use devm_ allocators in arm_pmu.

Alexander Stein (3):
  drivers/perf: arm_pmu: Use devm_ allocators
  drivers/perf: arm_pmu: add arm_pmu_device_remove
  arm: perf: Add platform driver removal function

 arch/arm/kernel/perf_event_v7.c |  6 ++++++
 drivers/perf/arm_pmu.c          | 28 ++++++++++++++++++----------
 include/linux/perf/arm_pmu.h    |  2 ++
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.10.2

[PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators

[Alexander Stein]

This eliminates several calls to kfree.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/perf/arm_pmu.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index b37b572..a9bbdbf 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -917,7 +917,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 	bool using_spi = false;
 	struct platform_device *pdev = pmu->plat_device;
 
-	irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+	irqs = devm_kcalloc(&pdev->dev, pdev->num_resources, sizeof(*irqs),
+			    GFP_KERNEL);
 	if (!irqs)
 		return -ENOMEM;
 
@@ -939,7 +940,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 				pr_err("PPI/SPI IRQ type mismatch for %s!\n",
 					dn->name);
 				of_node_put(dn);
-				kfree(irqs);
 				return -EINVAL;
 			}
 
@@ -988,10 +988,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 			int ret;
 
 			ret = irq_get_percpu_devid_partition(irq, &pmu->supported_cpus);
-			if (ret) {
-				kfree(irqs);
+			if (ret)
 				return ret;
-			}
 		} else {
 			/* Otherwise default to all CPUs */
 			cpumask_setall(&pmu->supported_cpus);
@@ -1001,8 +999,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 	/* If we matched up the IRQ affinities, use them to route the SPIs */
 	if (using_spi && i == pdev->num_resources)
 		pmu->irq_affinity = irqs;
-	else
-		kfree(irqs);
 
 	return 0;
 }
@@ -1017,7 +1013,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	struct arm_pmu *pmu;
 	int ret = -ENODEV;
 
-	pmu = kzalloc(sizeof(struct arm_pmu), GFP_KERNEL);
+	pmu = devm_kzalloc(&pdev->dev, sizeof(struct arm_pmu), GFP_KERNEL);
 	if (!pmu) {
 		pr_info("failed to allocate PMU device!\n");
 		return -ENOMEM;
@@ -1074,8 +1070,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 out_free:
 	pr_info("%s: failed to register PMU devices!\n",
 		of_node_full_name(node));
-	kfree(pmu->irq_affinity);
-	kfree(pmu);
 	return ret;
 }
 
-- 
2.10.2

[PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Alexander Stein]

Add ARM PMU removal function. This will be required by perf event drivers
when option DEBUG_TEST_DRIVER_REMOVE is enabled.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/perf/arm_pmu.c       | 14 ++++++++++++++
 include/linux/perf/arm_pmu.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index a9bbdbf..b7ddc4c 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	armpmu_init(pmu);
 
 	pmu->plat_device = pdev;
+	platform_set_drvdata(pdev, pmu);
 
 	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
 		init_fn = of_id->data;
@@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	return ret;
 }
 
+int arm_pmu_device_remove(struct platform_device *pdev)
+{
+	struct arm_pmu *pmu = platform_get_drvdata(pdev);
+
+	__oprofile_cpu_pmu = NULL;
+
+	perf_pmu_unregister(&pmu->pmu);
+
+	cpu_pmu_destroy(pmu);
+
+	return 0;
+}
+
 static int arm_pmu_hp_init(void)
 {
 	int ret;
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 8462da2..8106f27 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -159,6 +159,8 @@ struct pmu_probe_info {
 int arm_pmu_device_probe(struct platform_device *pdev,
 			 const struct of_device_id *of_table,
 			 const struct pmu_probe_info *probe_table);
+int arm_pmu_device_remove(struct platform_device *pdev);
+
 
 #define ARMV8_PMU_PDEV_NAME "armv8-pmu"
 
-- 
2.10.2

[PATCH 3/3] arm: perf: Add platform driver removal function

[Alexander Stein]

Despite the driver cannot be built as a module using the option
DEBUG_TEST_DRIVER_REMOVE requires the drivers to actually remove the
device.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 arch/arm/kernel/perf_event_v7.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index b942349..3ad02a2 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -2026,12 +2026,18 @@ static int armv7_pmu_device_probe(struct platform_device *pdev)
 				    armv7_pmu_probe_table);
 }
 
+static int armv7_pmu_device_remove(struct platform_device *pdev)
+{
+	return arm_pmu_device_remove(pdev);
+}
+
 static struct platform_driver armv7_pmu_driver = {
 	.driver		= {
 		.name	= "armv7-pmu",
 		.of_match_table = armv7_pmu_of_device_ids,
 	},
 	.probe		= armv7_pmu_device_probe,
+	.remove		= armv7_pmu_device_remove,
 };
 
 static int __init register_armv7_pmu_driver(void)
-- 
2.10.2

Re: [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Peter Zijlstra]

On Wed, Dec 21, 2016 at 11:19:34AM +0100, Alexander Stein wrote:
> Add ARM PMU removal function. This will be required by perf event drivers
> when option DEBUG_TEST_DRIVER_REMOVE is enabled.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/perf/arm_pmu.c       | 14 ++++++++++++++
>  include/linux/perf/arm_pmu.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index a9bbdbf..b7ddc4c 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  	armpmu_init(pmu);
>  
>  	pmu->plat_device = pdev;
> +	platform_set_drvdata(pdev, pmu);
>  
>  	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
>  		init_fn = of_id->data;
> @@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +int arm_pmu_device_remove(struct platform_device *pdev)
> +{
> +	struct arm_pmu *pmu = platform_get_drvdata(pdev);
> +
> +	__oprofile_cpu_pmu = NULL;
> +
> +	perf_pmu_unregister(&pmu->pmu);
> +
> +	cpu_pmu_destroy(pmu);
> +
> +	return 0;
> +}

So normally, if there are events that use this pmu, we hold a reference
on its module, which avoids removal from happening.

How is that guarantee made by DEBUG_TEST_DRIVER_REMOVE ? Or will it
simply kill everything even though there's active events for the PMU?

Re: [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Alexander Stein]

On Wednesday 21 December 2016 14:38:54, Peter Zijlstra wrote:
> On Wed, Dec 21, 2016 at 11:19:34AM +0100, Alexander Stein wrote:
> > Add ARM PMU removal function. This will be required by perf event drivers
> > when option DEBUG_TEST_DRIVER_REMOVE is enabled.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > ---
> > 
> >  drivers/perf/arm_pmu.c       | 14 ++++++++++++++
> >  include/linux/perf/arm_pmu.h |  2 ++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index a9bbdbf..b7ddc4c 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device
> > *pdev,> 
> >  	armpmu_init(pmu);
> >  	
> >  	pmu->plat_device = pdev;
> > 
> > +	platform_set_drvdata(pdev, pmu);
> > 
> >  	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
> >  	
> >  		init_fn = of_id->data;
> > 
> > @@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device
> > *pdev,> 
> >  	return ret;
> >  
> >  }
> > 
> > +int arm_pmu_device_remove(struct platform_device *pdev)
> > +{
> > +	struct arm_pmu *pmu = platform_get_drvdata(pdev);
> > +
> > +	__oprofile_cpu_pmu = NULL;
> > +
> > +	perf_pmu_unregister(&pmu->pmu);
> > +
> > +	cpu_pmu_destroy(pmu);
> > +
> > +	return 0;
> > +}
> 
> So normally, if there are events that use this pmu, we hold a reference
> on its module, which avoids removal from happening.
> 
> How is that guarantee made by DEBUG_TEST_DRIVER_REMOVE ? Or will it
> simply kill everything even though there's active events for the PMU?

AFAICS you won't be able to hold any reference until this test remove is done. 
This feature is implemented in really_probe(). If the driver is successfully 
probed it will be removed and probed again.
But reading that part of the code I stumbled over suppress_bind_attrs which 
would prevent this procedure. After some grepping I found commit 80c6397c3 
("clk: oxnas: make it explicitly non-modular").
Essentially setting
> .suppress_bind_attrs = true
in the platform_driver.
IMHO this seems far better than to add some remove functions only for testing 
a non-removable driver. I'll come up with a 2nd series, patch 1/3 is still 
valid.

Best regards,
Alexander