Re: [PATCH 3/3] drm/exynos: move Exynos platform drivers registration to init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2014년 11월 21일 08:42, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> 
> Registering the Exynos DRM subdevices platform drivers in the probe
> function is causing an infinite loop. Fix this by moving it to the
> exynos_drm_init() function to register the drivers on module init.
> 
> Registering drivers in the probe functions causes a deadlock in the parent
> device lock. See Grant Likely explanation on the topic:
> 
> "I think the problem is that exynos_drm_init() is registering a normal
> (non-OF) platform device, so the parent will be /sys/devices/platform.
> It immediately gets bound against exynos_drm_platform_driver which
> calls the exynos drm_platform_probe() hook. The driver core obtains
> device_lock() on the device *and on the device parent*.
> 
> Inside the probe hook, additional platform_drivers get registered.
> Each time one does, it tries to bind against every platform device in
> the system, which includes the ones created by OF. When it attempts to
> bind, it obtains device_lock() on the device *and on the device
> parent*.
> 
> Before the change to move of-generated platform devices into
> /sys/devices/platform, the devices had different parents. Now both
> devices have /sys/devices/platform as the parent, so yes they are
> going to deadlock.
> 
> The real problem is registering drivers from within a probe hook. That
> is completely wrong for the above deadlock reason. __driver_attach()
> will deadlock. Those registrations must be pulled out of .probe().
> 
> Registering devices in .probe() is okay because __device_attach()
> doesn't try to obtain device_lock() on the parent."
> 
>  INFO: task swapper/0:1 blocked for more than 120 seconds.
>        Not tainted 3.18.0-rc3-next-20141105 #794
>  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>  swapper/0       D c052534c     0     1      0 0x00000000
>  [<c052534c>] (__schedule) from [<c0525b34>] (schedule_preempt_disabled+0x14/0x20)
>  [<c0525b34>] (schedule_preempt_disabled) from [<c0526d44>] (mutex_lock_nested+0x1c4/0x464
> 
>  [<c0526d44>] (mutex_lock_nested) from [<c02be908>] (__driver_attach+0x48/0x98)
>  [<c02be908>] (__driver_attach) from [<c02bcc00>] (bus_for_each_dev+0x54/0x88)
>  [<c02bcc00>] (bus_for_each_dev) from [<c02bdce0>] (bus_add_driver+0xe4/0x200)
>  [<c02bdce0>] (bus_add_driver) from [<c02bef94>] (driver_register+0x78/0xf4)
>  [<c02bef94>] (driver_register) from [<c029e99c>] (exynos_drm_platform_probe+0x34/0x234)
>  [<c029e99c>] (exynos_drm_platform_probe) from [<c02bfcf0>] (platform_drv_probe+0x48/0xa4)
>  [<c02bfcf0>] (platform_drv_probe) from [<c02be680>] (driver_probe_device+0x13c/0x37c)
>  [<c02be680>] (driver_probe_device) from [<c02be954>] (__driver_attach+0x94/0x98)
>  [<c02be954>] (__driver_attach) from [<c02bcc00>] (bus_for_each_dev+0x54/0x88)
>  [<c02bcc00>] (bus_for_each_dev) from [<c02bdce0>] (bus_add_driver+0xe4/0x200)
>  [<c02bdce0>] (bus_add_driver) from [<c02bef94>] (driver_register+0x78/0xf4)
>  [<c02bef94>] (driver_register) from [<c029e938>] (exynos_drm_init+0x70/0xa0)
>  [<c029e938>] (exynos_drm_init) from [<c00089b0>] (do_one_initcall+0xac/0x1f0)
>  [<c00089b0>] (do_one_initcall) from [<c074bd90>] (kernel_init_freeable+0x10c/0x1d8)
>  [<c074bd90>] (kernel_init_freeable) from [<c051eabc>] (kernel_init+0x8/0xec)
>  [<c051eabc>] (kernel_init) from [<c000f268>] (ret_from_fork+0x14/0x2c)
>  3 locks held by swapper/0/1:
>   #0:  (&dev->mutex){......}, at: [<c02be908>] __driver_attach+0x48/0x98
>   #1:  (&dev->mutex){......}, at: [<c02be918>] __driver_attach+0x58/0x98
>   #2:  (&dev->mutex){......}, at: [<c02be908>] __driver_attach+0x48/0x98
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 124 +++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 91891cf..cb3ed9b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -548,6 +548,38 @@ static const struct component_master_ops exynos_drm_ops = {
>  	.unbind		= exynos_drm_unbind,
>  };
>  
> +static int exynos_drm_platform_probe(struct platform_device *pdev)
> +{
> +	struct component_match *match;
> +
> +	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +	exynos_drm_driver.num_ioctls = ARRAY_SIZE(exynos_ioctls);
> +
> +	match = exynos_drm_match_add(&pdev->dev);
> +	if (IS_ERR(match)) {
> +		return PTR_ERR(match);
> +	}
> +
> +	return component_master_add_with_match(&pdev->dev, &exynos_drm_ops,
> +					       match);
> +}
> +
> +static int exynos_drm_platform_remove(struct platform_device *pdev)
> +{
> +	component_master_del(&pdev->dev, &exynos_drm_ops);
> +	return 0;
> +}
> +
> +static struct platform_driver exynos_drm_platform_driver = {
> +	.probe	= exynos_drm_platform_probe,
> +	.remove	= exynos_drm_platform_remove,
> +	.driver	= {
> +		.owner	= THIS_MODULE,
> +		.name	= "exynos-drm",
> +		.pm	= &exynos_drm_pm_ops,
> +	},
> +};
> +
>  static struct platform_driver *const exynos_drm_kms_drivers[] = {
>  #ifdef CONFIG_DRM_EXYNOS_FIMD
>  	&fimd_driver,
> @@ -582,13 +614,24 @@ static struct platform_driver *const exynos_drm_non_kms_drivers[] = {
>  #endif
>  };
>  
> -static int exynos_drm_platform_probe(struct platform_device *pdev)
> +static int exynos_drm_init(void)
>  {
> -	struct component_match *match;
>  	int ret, i, j;
>  
> -	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> -	exynos_drm_driver.num_ioctls = ARRAY_SIZE(exynos_ioctls);
> +	exynos_drm_pdev = platform_device_register_simple("exynos-drm", -1,
> +								NULL, 0);
> +	if (IS_ERR(exynos_drm_pdev))
> +		return PTR_ERR(exynos_drm_pdev);
> +
> +#ifdef CONFIG_DRM_EXYNOS_VIDI
> +	ret = exynos_drm_probe_vidi();
> +	if (ret < 0)
> +		goto err_unregister_pd;
> +#endif

If vidi driver is enabled then Exynos drm driver doesn't work.

> +
> +	ret = platform_driver_register(&exynos_drm_platform_driver);
> +	if (ret)
> +		goto err_remove_vidi;

Above platform_driver_register should be called after all kms and
non-kms drivers are registered. And your patch should be re-based on top
of exynos-drm-next.

I just re-based it on top of exynos-drm-next and changed the
platform_driver_register to be called at the end.

Thanks,
Inki Dae

>  
>  	for (i = 0; i < ARRAY_SIZE(exynos_drm_kms_drivers); ++i) {
>  		ret = platform_driver_register(exynos_drm_kms_drivers[i]);
> @@ -596,26 +639,17 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
>  			goto err_unregister_kms_drivers;
>  	}
>  
> -	match = exynos_drm_match_add(&pdev->dev);
> -	if (IS_ERR(match)) {
> -		ret = PTR_ERR(match);
> -		goto err_unregister_kms_drivers;
> -	}
> -
> -	ret = component_master_add_with_match(&pdev->dev, &exynos_drm_ops,
> -						match);
> -	if (ret < 0)
> -		goto err_unregister_kms_drivers;
> -
>  	for (j = 0; j < ARRAY_SIZE(exynos_drm_non_kms_drivers); ++j) {
>  		ret = platform_driver_register(exynos_drm_non_kms_drivers[j]);
>  		if (ret < 0)
> -			goto err_del_component_master;
> +			goto err_unregister_non_kms_drivers;
>  	}
>  
> +#ifdef CONFIG_DRM_EXYNOS_IPP
>  	ret = exynos_platform_device_ipp_register();
>  	if (ret < 0)
>  		goto err_unregister_non_kms_drivers;
> +#endif
>  
>  	return ret;
>  
> @@ -626,17 +660,22 @@ err_unregister_non_kms_drivers:
>  	while (--j >= 0)
>  		platform_driver_unregister(exynos_drm_non_kms_drivers[j]);
>  
> -err_del_component_master:
> -	component_master_del(&pdev->dev, &exynos_drm_ops);
> -
>  err_unregister_kms_drivers:
>  	while (--i >= 0)
>  		platform_driver_unregister(exynos_drm_kms_drivers[i]);
>  
> +err_remove_vidi:
> +#ifdef CONFIG_DRM_EXYNOS_VIDI
> +	exynos_drm_remove_vidi();
> +
> +err_unregister_pd:
> +#endif
> +	platform_device_unregister(exynos_drm_pdev);
> +
>  	return ret;
>  }
>  
> -static int exynos_drm_platform_remove(struct platform_device *pdev)
> +static void exynos_drm_exit(void)
>  {
>  	int i;
>  
> @@ -647,54 +686,9 @@ static int exynos_drm_platform_remove(struct platform_device *pdev)
>  	for (i = ARRAY_SIZE(exynos_drm_non_kms_drivers) - 1; i >= 0; --i)
>  		platform_driver_unregister(exynos_drm_non_kms_drivers[i]);
>  
> -	component_master_del(&pdev->dev, &exynos_drm_ops);
> -
>  	for (i = ARRAY_SIZE(exynos_drm_kms_drivers) - 1; i >= 0; --i)
>  		platform_driver_unregister(exynos_drm_kms_drivers[i]);
>  
> -	return 0;
> -}
> -
> -static struct platform_driver exynos_drm_platform_driver = {
> -	.probe	= exynos_drm_platform_probe,
> -	.remove	= exynos_drm_platform_remove,
> -	.driver	= {
> -		.owner	= THIS_MODULE,
> -		.name	= "exynos-drm",
> -		.pm	= &exynos_drm_pm_ops,
> -	},
> -};
> -
> -static int exynos_drm_init(void)
> -{
> -	int ret;
> -
> -	exynos_drm_pdev = platform_device_register_simple("exynos-drm", -1,
> -								NULL, 0);
> -	if (IS_ERR(exynos_drm_pdev))
> -		return PTR_ERR(exynos_drm_pdev);
> -
> -	ret = exynos_drm_probe_vidi();
> -	if (ret < 0)
> -		goto err_unregister_pd;
> -
> -	ret = platform_driver_register(&exynos_drm_platform_driver);
> -	if (ret)
> -		goto err_remove_vidi;
> -
> -	return 0;
> -
> -err_remove_vidi:
> -	exynos_drm_remove_vidi();
> -
> -err_unregister_pd:
> -	platform_device_unregister(exynos_drm_pdev);
> -
> -	return ret;
> -}
> -
> -static void exynos_drm_exit(void)
> -{
>  	platform_driver_unregister(&exynos_drm_platform_driver);
>  
>  	exynos_drm_remove_vidi();
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux