linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE
@ 2016-12-21 10:19 Alexander Stein
  2016-12-21 10:19 ` [PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators Alexander Stein
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Alexander Stein @ 2016-12-21 10:19 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Will Deacon, Mark Rutland, Russell King
  Cc: Alexander Stein, linux-kernel, linux-arm-kernel

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

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

* [PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators
  2016-12-21 10:19 [PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE Alexander Stein
@ 2016-12-21 10:19 ` Alexander Stein
  2016-12-21 10:19 ` [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove Alexander Stein
  2016-12-21 10:19 ` [PATCH 3/3] arm: perf: Add platform driver removal function Alexander Stein
  2 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2016-12-21 10:19 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Will Deacon, Mark Rutland, Russell King
  Cc: Alexander Stein, linux-kernel, linux-arm-kernel

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

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

* [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove
  2016-12-21 10:19 [PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE Alexander Stein
  2016-12-21 10:19 ` [PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators Alexander Stein
@ 2016-12-21 10:19 ` Alexander Stein
  2016-12-21 13:38   ` Peter Zijlstra
  2016-12-21 10:19 ` [PATCH 3/3] arm: perf: Add platform driver removal function Alexander Stein
  2 siblings, 1 reply; 6+ messages in thread
From: Alexander Stein @ 2016-12-21 10:19 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Will Deacon, Mark Rutland, Russell King
  Cc: Alexander Stein, linux-kernel, linux-arm-kernel

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

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

* [PATCH 3/3] arm: perf: Add platform driver removal function
  2016-12-21 10:19 [PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE Alexander Stein
  2016-12-21 10:19 ` [PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators Alexander Stein
  2016-12-21 10:19 ` [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove Alexander Stein
@ 2016-12-21 10:19 ` Alexander Stein
  2 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2016-12-21 10:19 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Will Deacon, Mark Rutland, Russell King
  Cc: Alexander Stein, linux-kernel, linux-arm-kernel

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

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

* Re: [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove
  2016-12-21 10:19 ` [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove Alexander Stein
@ 2016-12-21 13:38   ` Peter Zijlstra
  2016-12-21 14:45     ` Alexander Stein
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2016-12-21 13:38 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
	Will Deacon, Mark Rutland, Russell King, linux-kernel,
	linux-arm-kernel

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?

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

* Re: [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove
  2016-12-21 13:38   ` Peter Zijlstra
@ 2016-12-21 14:45     ` Alexander Stein
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2016-12-21 14:45 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
	Will Deacon, Mark Rutland, Russell King, linux-kernel,
	linux-arm-kernel

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

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

end of thread, other threads:[~2016-12-21 14:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-21 10:19 [PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE Alexander Stein
2016-12-21 10:19 ` [PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators Alexander Stein
2016-12-21 10:19 ` [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove Alexander Stein
2016-12-21 13:38   ` Peter Zijlstra
2016-12-21 14:45     ` Alexander Stein
2016-12-21 10:19 ` [PATCH 3/3] arm: perf: Add platform driver removal function Alexander Stein

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).