Re: [PATCH 2/3] drm/exynos: avoid race condition when adding a drm component

[Inki Dae <inki.dae@samsung.com>]

On 2014년 11월 21일 08:54, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> exynos_drm_component_add() correctly checks if a component is present on
> drm_component_list however it release the lock right after the check
> and before we add the new component to the list. That just creates room
> to add the same component more than once to the list.

A little bit strange. drm_component_list is protected from race
condition with mutex_lock. How the same component can be added to the
drm_component_list again? And a new cdev object cannot be same cdev
object already added to the drm_component_list because the new cdev
object is allocated by kzalloc(). And the only case the same kms driver
can request to add a new cdev to drm_component_list again is when the
probe of the driver failed. However, in this case, the driver will call
exynos_drm_component_del function to remove previous cdev. So the same
cdev cannot be added to the drm_component_list even in such case.

Thanks,
Inki Dae

> 
> The lock should be held for the whole journey while adding a new
> component.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index cb3ed9b..230573d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -402,10 +402,8 @@ int exynos_drm_component_add(struct device *dev,
>  			 * added already just return.
>  			 */
>  			if (cdev->dev_type_flag == (EXYNOS_DEVICE_TYPE_CRTC |
> -						EXYNOS_DEVICE_TYPE_CONNECTOR)) {
> -				mutex_unlock(&drm_component_lock);
> -				return 0;
> -			}
> +						EXYNOS_DEVICE_TYPE_CONNECTOR))
> +				goto unlock;
>  
>  			if (dev_type == EXYNOS_DEVICE_TYPE_CRTC) {
>  				cdev->crtc_dev = dev;
> @@ -417,14 +415,11 @@ int exynos_drm_component_add(struct device *dev,
>  				cdev->dev_type_flag |= dev_type;
>  			}
>  
> -			mutex_unlock(&drm_component_lock);
> -			return 0;
> +			goto unlock;
>  		}
>  	}
>  
> -	mutex_unlock(&drm_component_lock);
> -
> -	cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
> +	cdev = kzalloc(sizeof(*cdev), GFP_ATOMIC);
>  	if (!cdev)
>  		return -ENOMEM;
>  
> @@ -436,10 +431,9 @@ int exynos_drm_component_add(struct device *dev,
>  	cdev->out_type = out_type;
>  	cdev->dev_type_flag = dev_type;
>  
> -	mutex_lock(&drm_component_lock);
>  	list_add_tail(&cdev->list, &drm_component_list);
> +unlock:
>  	mutex_unlock(&drm_component_lock);
> -
>  	return 0;
>  }
>  
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel