Re: [PATCH] drm: fixup error path after drm_fill_in_dev

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 12 Jun 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Within drm_fill_in_dev we call drm_lastclose to clean up the dirt in
> case of failures. But the callers of drm_fill_in_dev simply don't do
> anything. Now from a cursory look drm_lastclose doesn't look like the
> best cleanup function in itself, but whom am I to judge the drm error
> paths. Imo before we could tackle this we need to move more of the
> legacy drm services setup and teardown into the respective drivers,
> that way drm_lastclose would become more manageble.
>
> Anyway, make things at least consistent by adding drm_lastclose at the
> right places in the error paths for all callers of drm_fill_in_dev.
>
> v2: Fixup the error-path confusion in drm_usb.c, noticed by Jani
> Nikula.

You could just claim you planted it there for me. ;)

On the series,
Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com>

>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      |    8 +++++---
>  drivers/gpu/drm/drm_platform.c |    8 +++++---
>  drivers/gpu/drm/drm_usb.c      |    8 +++++---
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 73218ac..40737a8 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -334,14 +334,14 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
>  
>  	if ((ret = drm_fill_in_dev(dev, ent, driver))) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g2;
> +		goto err_pci;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		pci_set_drvdata(pdev, dev);
>  		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  		if (ret)
> -			goto err_g2;
> +			goto err_drm_fill_in;
>  	}
>  
>  	if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY)))
> @@ -375,7 +375,9 @@ err_g4:
>  err_g3:
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_put_minor(&dev->control);
> -err_g2:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_pci:
>  	pci_disable_device(pdev);
>  err_g1:
>  	kfree(dev);
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
> index 82431dc..177893a 100644
> --- a/drivers/gpu/drm/drm_platform.c
> +++ b/drivers/gpu/drm/drm_platform.c
> @@ -60,14 +60,14 @@ int drm_get_platform_dev(struct platform_device *platdev,
>  
>  	if (ret) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g1;
> +		goto err_kfree;
>  	}
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		dev_set_drvdata(&platdev->dev, dev);
>  		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  		if (ret)
> -			goto err_g1;
> +			goto err_drm_fill_in;
>  	}
>  
>  	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
> @@ -103,7 +103,9 @@ err_g3:
>  err_g2:
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_put_minor(&dev->control);
> -err_g1:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_kfree:
>  	kfree(dev);
>  	mutex_unlock(&drm_global_mutex);
>  	return ret;
> diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
> index 37c9a52..f9d645f 100644
> --- a/drivers/gpu/drm/drm_usb.c
> +++ b/drivers/gpu/drm/drm_usb.c
> @@ -25,13 +25,13 @@ int drm_get_usb_dev(struct usb_interface *interface,
>  	ret = drm_fill_in_dev(dev, NULL, driver);
>  	if (ret) {
>  		printk(KERN_ERR "DRM: Fill_in_dev failed.\n");
> -		goto err_g1;
> +		goto err_kfree;
>  	}
>  
>  	usb_set_intfdata(interface, dev);
>  	ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
>  	if (ret)
> -		goto err_g1;
> +		goto err_drm_fill_in;
>  
>  	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
>  	if (ret)
> @@ -63,7 +63,9 @@ err_g3:
>  	drm_put_minor(&dev->primary);
>  err_g2:
>  	drm_put_minor(&dev->control);
> -err_g1:
> +err_drm_fill_in:
> +	drm_lastclose(dev);
> +err_kfree:
>  	kfree(dev);
>  	mutex_unlock(&drm_global_mutex);
>  	return ret;
> -- 
> 1.7.7.6