[PATCH 05/24] drm/vkms: Use devm_drm_dev_alloc

[Melissa Wen]

Hi Daniel,

Thanks for this work.

This change works smoothly to me. However, there is a check in the vkms_exit
that doesn't look very good. Please, see inline comment.

On 09/04, Daniel Vetter wrote:
> This means we also need to slightly restructure the exit code, so that
> final cleanup of the drm_device is triggered by unregistering the
> platform device. Note that devres is both clean up when the driver is
> unbound (not the case for vkms, we don't bind), and also when unregistering
> the device (very much the case for vkms). Therefore we can rely on devres
> even though vkms isn't a proper platform device driver.
> 
> This also somewhat untangles the load code, since the drm and platform device
> setup are no longer interleaved, but two distinct steps.
> 
> v2: use devres_open/release_group so we can use devm without real
> hacks in the driver core or having to create an entire fake bus for
> testing drivers. Might want to extract this into helpers eventually,
> maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa at gmail.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 54 ++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 83dd5567de8b..0d2c6dcf73c3 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -61,9 +61,6 @@ static void vkms_release(struct drm_device *dev)
>  {
>  	struct vkms_device *vkms = container_of(dev, struct vkms_device, drm);
>  
> -	platform_device_unregister(vkms->platform);
> -	drm_atomic_helper_shutdown(&vkms->drm);
> -	drm_mode_config_cleanup(&vkms->drm);
>  	destroy_workqueue(vkms->output.composer_workq);
>  }
>  
> @@ -144,30 +141,31 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  static int __init vkms_init(void)
>  {
>  	int ret;
> +	struct platform_device *pdev;
>  
> -	vkms_device = kzalloc(sizeof(*vkms_device), GFP_KERNEL);
> -	if (!vkms_device)
> -		return -ENOMEM;
> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
>  
> -	vkms_device->platform =
> -		platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> -	if (IS_ERR(vkms_device->platform)) {
> -		ret = PTR_ERR(vkms_device->platform);
> -		goto out_free;
> +	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto out_unregister;
>  	}
>  
> -	ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
> -			   &vkms_device->platform->dev);
> -	if (ret)
> -		goto out_unregister;
> -	drmm_add_final_kfree(&vkms_device->drm, vkms_device);
> +	vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver,
> +					 struct vkms_device, drm);
> +	if (IS_ERR(vkms_device)) {
> +		ret = PTR_ERR(vkms_device);
> +		goto out_devres;
> +	}
> +	vkms_device->platform = pdev;
>  
>  	ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
>  					   DMA_BIT_MASK(64));
>  
>  	if (ret) {
>  		DRM_ERROR("Could not initialize DMA support\n");
> -		goto out_put;
> +		goto out_devres;
>  	}
>  
>  	vkms_device->drm.irq_enabled = true;
> @@ -175,39 +173,39 @@ static int __init vkms_init(void)
>  	ret = drm_vblank_init(&vkms_device->drm, 1);
>  	if (ret) {
>  		DRM_ERROR("Failed to vblank\n");
> -		goto out_put;
> +		goto out_devres;
>  	}
>  
>  	ret = vkms_modeset_init(vkms_device);
>  	if (ret)
> -		goto out_put;
> +		goto out_devres;
>  
>  	ret = drm_dev_register(&vkms_device->drm, 0);
>  	if (ret)
> -		goto out_put;
> +		goto out_devres;
>  
>  	return 0;
>  
> -out_put:
> -	drm_dev_put(&vkms_device->drm);
> -	platform_device_unregister(vkms_device->platform);
> -	return ret;
> +out_devres:
> +	devres_release_group(&pdev->dev, NULL);
>  out_unregister:
> -	platform_device_unregister(vkms_device->platform);
> -out_free:
> -	kfree(vkms_device);
> +	platform_device_unregister(pdev);
>  	return ret;
>  }
>  
>  static void __exit vkms_exit(void)
>  {
> +	struct platform_device *pdev = vkms_device->platform;
> +
>  	if (!vkms_device) {
>  		DRM_INFO("vkms_device is NULL.\n");
>  		return;
>  	}

I think it would be better to check vkms_device before assigning
vkms_device->platform to the pdev above. I don't know if the compiler
handles this, but doing the validation first seems more clear.

>  
>  	drm_dev_unregister(&vkms_device->drm);
> -	drm_dev_put(&vkms_device->drm);
> +	drm_atomic_helper_shutdown(&vkms_device->drm);
> +	devres_release_group(&pdev->dev, NULL);
> +	platform_device_unregister(pdev);
>  }
>  
>  module_init(vkms_init);
> -- 
> 2.28.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel