[PATCH v3 1/2] drm/etnaviv: Check for platform_device_register_simple() failure

[PATCH v3 1/2] drm/etnaviv: Check for platform_device_register_simple() failure

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

platform_device_register_simple() may fail, so we should better
check its return value and propagate it in the case of error.

Cc: <stable@vger.kernel.org>
Fixes: 246774d17fc0 ("drm/etnaviv: remove the need for a gpu-subsystem DT node")
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Changes since v2:
- Newly introduced in this version

 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index e5013a9..ac23c0f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -631,6 +631,8 @@ static struct platform_driver etnaviv_platform_driver = {
 	},
 };
 
+static struct platform_device *etnaviv_drm;
+
 static int __init etnaviv_init(void)
 {
 	int ret;
@@ -644,7 +646,7 @@ static int __init etnaviv_init(void)
 
 	ret = platform_driver_register(&etnaviv_platform_driver);
 	if (ret != 0)
-		platform_driver_unregister(&etnaviv_gpu_driver);
+		goto unregister_gpu_driver;
 
 	/*
 	 * If the DT contains at least one available GPU device, instantiate
@@ -653,12 +655,22 @@ static int __init etnaviv_init(void)
 	for_each_compatible_node(np, NULL, "vivante,gc") {
 		if (!of_device_is_available(np))
 			continue;
-
-		platform_device_register_simple("etnaviv", -1, NULL, 0);
+		etnaviv_drm = platform_device_register_simple("etnaviv", -1,
+							      NULL, 0);
+		if (IS_ERR(etnaviv_drm)) {
+			ret = PTR_ERR(etnaviv_drm);
+			goto unregister_platform_driver;
+		}
 		of_node_put(np);
 		break;
 	}
 
+	return 0;
+
+unregister_platform_driver:
+	platform_driver_unregister(&etnaviv_platform_driver);
+unregister_gpu_driver:
+	platform_driver_unregister(&etnaviv_gpu_driver);
 	return ret;
 }
 module_init(etnaviv_init);
-- 
2.7.4

[PATCH v3 2/2] drm/etnaviv: Fix driver unregistering

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

Russell King reported:

"When removing and reloading the etnaviv module, the following splat
occurs:

sysfs: cannot create duplicate filename '/devices/platform/etnaviv'
CPU: 0 PID: 1471 Comm: modprobe Not tainted 4.17.0+ #1608
Hardware name: Marvell Dove (Cubox)
Backtrace:
[<c00157d4>] (dump_backtrace) from [<c0015b8c>] (show_stack+0x18/0x1c)
 r6:ef033e38 r5:ee07b340 r4:edb9d000 r3:00000000
[<c0015b74>] (show_stack) from [<c0620784>] (dump_stack+0x20/0x28)
[<c0620764>] (dump_stack) from [<c01bcd24>] (sysfs_warn_dup+0x5c/0x70)
[<c01bccc8>] (sysfs_warn_dup) from [<c01bce14>] (sysfs_create_dir_ns+0x90/0x98)
..."

Commit 246774d17fc0 ("drm/etnaviv: remove the need for a gpu-subsystem
DT node") introduced DRM registration via
platform_device_register_simple(), but missed to call
platform_device_unregister() inside etnaviv_exit().

Fix the problem by calling platform_device_unregister() inside
etnaviv_exit(). While at it, also rearrange the function calls
in the exit path to make them happen in the opposite order of
registration.

Tested on a imx6-sabresd board.

Cc: <stable@vger.kernel.org>
Fixes: 246774d17fc0 ("drm/etnaviv: remove the need for a gpu-subsystem DT node")
Reported-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Changes since v2:
- Add a 'available_gpu' bool variable to indicate that an available
"vivante,gc" has been found

 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index ac23c0f..9044bf9 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -632,6 +632,7 @@ static struct platform_driver etnaviv_platform_driver = {
 };
 
 static struct platform_device *etnaviv_drm;
+static bool available_gpu;
 
 static int __init etnaviv_init(void)
 {
@@ -655,6 +656,7 @@ static int __init etnaviv_init(void)
 	for_each_compatible_node(np, NULL, "vivante,gc") {
 		if (!of_device_is_available(np))
 			continue;
+		available_gpu = true;
 		etnaviv_drm = platform_device_register_simple("etnaviv", -1,
 							      NULL, 0);
 		if (IS_ERR(etnaviv_drm)) {
@@ -677,8 +679,10 @@ module_init(etnaviv_init);
 
 static void __exit etnaviv_exit(void)
 {
-	platform_driver_unregister(&etnaviv_gpu_driver);
+	if (available_gpu)
+		platform_device_unregister(etnaviv_drm);
 	platform_driver_unregister(&etnaviv_platform_driver);
+	platform_driver_unregister(&etnaviv_gpu_driver);
 }
 module_exit(etnaviv_exit);
 
-- 
2.7.4

Re: [PATCH v3 1/2] drm/etnaviv: Check for platform_device_register_simple() failure

[Philipp Zabel]

On Mon, 2018-06-25 at 16:16 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> platform_device_register_simple() may fail, so we should better
> check its return value and propagate it in the case of error.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 246774d17fc0 ("drm/etnaviv: remove the need for a gpu-subsystem DT node")
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
> Changes since v2:
> - Newly introduced in this version
> 
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index e5013a9..ac23c0f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -631,6 +631,8 @@ static struct platform_driver etnaviv_platform_driver = {
>  	},
>  };
>  
> +static struct platform_device *etnaviv_drm;
> +
>  static int __init etnaviv_init(void)
>  {
>  	int ret;
> @@ -644,7 +646,7 @@ static int __init etnaviv_init(void)
>  
>  	ret = platform_driver_register(&etnaviv_platform_driver);
>  	if (ret != 0)
> -		platform_driver_unregister(&etnaviv_gpu_driver);
> +		goto unregister_gpu_driver;
>  
>  	/*
>  	 * If the DT contains at least one available GPU device, instantiate
> @@ -653,12 +655,22 @@ static int __init etnaviv_init(void)
>  	for_each_compatible_node(np, NULL, "vivante,gc") {
>  		if (!of_device_is_available(np))
>  			continue;
> -
> -		platform_device_register_simple("etnaviv", -1, NULL, 0);
> +		etnaviv_drm = platform_device_register_simple("etnaviv", -1,
> +							      NULL, 0);

Rather than store the error value in the global variable, I'd use a
local variable here and only set etnaviv_drm if this doesn't fail.

> +		if (IS_ERR(etnaviv_drm)) {
> +			ret = PTR_ERR(etnaviv_drm);
> +			goto unregister_platform_driver;
> +		}
>  		of_node_put(np);
>  		break;

Ah, focusing on etnaviv_exit last time I've completely overlooked this
break.

>  	}
>  
> +	return 0;
> +
> +unregister_platform_driver:
> +	platform_driver_unregister(&etnaviv_platform_driver);
> +unregister_gpu_driver:
> +	platform_driver_unregister(&etnaviv_gpu_driver);
>  	return ret;
>  }
>  module_init(etnaviv_init);

Re: [PATCH v3 2/2] drm/etnaviv: Fix driver unregistering

[Philipp Zabel]

On Mon, 2018-06-25 at 16:16 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Russell King reported:
> 
> "When removing and reloading the etnaviv module, the following splat
> occurs:
> 
> sysfs: cannot create duplicate filename '/devices/platform/etnaviv'
> CPU: 0 PID: 1471 Comm: modprobe Not tainted 4.17.0+ #1608
> Hardware name: Marvell Dove (Cubox)
> Backtrace:
> [<c00157d4>] (dump_backtrace) from [<c0015b8c>] (show_stack+0x18/0x1c)
>  r6:ef033e38 r5:ee07b340 r4:edb9d000 r3:00000000
> [<c0015b74>] (show_stack) from [<c0620784>] (dump_stack+0x20/0x28)
> [<c0620764>] (dump_stack) from [<c01bcd24>] (sysfs_warn_dup+0x5c/0x70)
> [<c01bccc8>] (sysfs_warn_dup) from [<c01bce14>] (sysfs_create_dir_ns+0x90/0x98)
> ..."
> 
> Commit 246774d17fc0 ("drm/etnaviv: remove the need for a gpu-subsystem
> DT node") introduced DRM registration via
> platform_device_register_simple(), but missed to call
> platform_device_unregister() inside etnaviv_exit().
> 
> Fix the problem by calling platform_device_unregister() inside
> etnaviv_exit(). While at it, also rearrange the function calls
> in the exit path to make them happen in the opposite order of
> registration.
> 
> Tested on a imx6-sabresd board.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 246774d17fc0 ("drm/etnaviv: remove the need for a gpu-subsystem DT node")
> Reported-by: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
> Changes since v2:
> - Add a 'available_gpu' bool variable to indicate that an available
> "vivante,gc" has been found
> 
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index ac23c0f..9044bf9 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -632,6 +632,7 @@ static struct platform_driver etnaviv_platform_driver = {
>  };
>  
>  static struct platform_device *etnaviv_drm;
> +static bool available_gpu;

This would not be necessary if etnaviv_drm weren't ever set to an error
value.
 
>  static int __init etnaviv_init(void)
>  {
> @@ -655,6 +656,7 @@ static int __init etnaviv_init(void)
>  	for_each_compatible_node(np, NULL, "vivante,gc") {
>  		if (!of_device_is_available(np))
>  			continue;
> +		available_gpu = true;
>  		etnaviv_drm = platform_device_register_simple("etnaviv", -1,
>  							      NULL, 0);
>  		if (IS_ERR(etnaviv_drm)) {
> @@ -677,8 +679,10 @@ module_init(etnaviv_init);
>  
>  static void __exit etnaviv_exit(void)
>  {
> -	platform_driver_unregister(&etnaviv_gpu_driver);
> +	if (available_gpu)
> +		platform_device_unregister(etnaviv_drm);

Both platform_device_del and platform_device_put (called by
platform_device_unregister) will gracefully degrade to a no-op if
etnaviv_drm == NULL, so this could just be:

	platform_device_unregister(etnaviv_drm);

>  	platform_driver_unregister(&etnaviv_platform_driver);
> +	platform_driver_unregister(&etnaviv_gpu_driver);
>  }
>  module_exit(etnaviv_exit);

I'd change the first patch and drop the available_gpu static variable,
but either way:

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp