| Message ID | 1416527664-10553-2-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > >
2014-11-21 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. Hmm, right. I haven't payed attention that each one of them is called just once. I did this patch in my first days working with this code so I ignored what exynos_drm_component_add() was really doing. Gustavo
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; }