On 2014ë
11ì 21ì¼ 08:54, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan at 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 at 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;
> }
>
>
