Re: [PATCH v2] drm/etnaviv: Fix driver unregistering

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Fabio,

On Sun, 2018-06-24 at 19:40 -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 v1:
> - Make the exit path symmetrical to the init path by calling
> platform_device_unregister() inside for_each_compatible_node()
> 
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index e5013a9..626ad8b 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;
> @@ -654,7 +656,8 @@ static int __init etnaviv_init(void)
>  		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 there are multiple vivante,gc device nodes, this overwrites
etnaviv_drm with each additional device after the first one.

Also, it would be better to store NULL instead of the error value.

>  		of_node_put(np);
>  		break;
>  	}
> @@ -665,8 +668,19 @@ module_init(etnaviv_init);
>  
>  static void __exit etnaviv_exit(void)
>  {
> -	platform_driver_unregister(&etnaviv_gpu_driver);
> +	struct device_node *np;
> +
> +	for_each_compatible_node(np, NULL, "vivante,gc") {
> +		if (!of_device_is_available(np))
> +			continue;
> +
> +		platform_device_unregister(etnaviv_drm);

If there are multiple vivante,gc nodes, this will try to unregister the
last device multiple times. Also, this will break if the pointer
contains an error value.

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

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel