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

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;

}