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;

}

On Saturday, November 22, 2014 2:40 AM, Ajay kumar wrote:

On Fri, Nov 21, 2014 at 5:24 AM, Gustavo Padovan gustavo@padovan.org wrote:

...

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

DP was leaked everytime function returns EPROBE_DEFER, free it before returning.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/exynos/exynos_dp_core.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c index 85762cf..6fd4a46 100644 --- a/drivers/gpu/drm/exynos/exynos_dp_core.c +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c @@ -1336,8 +1336,10 @@ static int exynos_dp_probe(struct platform_device *pdev) if (panel_node) { dp->panel = of_drm_find_panel(panel_node); of_node_put(panel_node);

•           if (!dp->panel)
  

•                   return -EPROBE_DEFER;
  

•           if (!dp->panel) {
  

•                   ret = -EPROBE_DEFER;
  

•                   goto free_dp;
  

•           }
    }
  
    endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
  

@@ -1346,10 +1348,14 @@ static int exynos_dp_probe(struct platform_device *pdev) if (bridge_node) { dp->bridge = of_drm_find_bridge(bridge_node); of_node_put(bridge_node);

•                   if (!dp->bridge)
  

•                           return -EPROBE_DEFER;
  

•           } else
  

•                   return -EPROBE_DEFER;
  

•                   if (!dp->bridge) {
  

•                           ret = -EPROBE_DEFER;
  

•                           goto free_dp;
  

•                   }
  

•           } else {
  

•                   ret = -EPROBE_DEFER;
  

•                   goto free_dp;
  

•           }
    }
  
    exynos_dp_display.ctx = dp;
  

@@ -1359,6 +1365,9 @@ static int exynos_dp_probe(struct platform_device *pdev) exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR);

+free_dp:

•   devm_kfree(dev, dp);
  

I guess the driver core takes care of freeing the devm memory when the probe fails? Will it not happen during PROBE_DEFER?

Inki/Jingoo - Is this change really necessary?

As far as I know, this devm_kfree() in unnecessary, even though PROBE_DEFER is returned. If I am wrong, please let me know. Thanks.

Best regards, Jingoo Han

Ajay

...

    return ret;

}

-- 1.9.3

-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-11-22 Inki Dae inki.dae@samsung.com:

2014-11-21 8:54 GMT+09:00 Gustavo Padovan gustavo@padovan.org:

...

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

DP was leaked everytime function returns EPROBE_DEFER, free it before returning.

It seems that you misunderstood devm_* api.

Yeah, I though it would only free memory at unload of the module but it also free it when the probe fails.

Thanks, Inki Dae

...

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/exynos/exynos_dp_core.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c index 85762cf..6fd4a46 100644 --- a/drivers/gpu/drm/exynos/exynos_dp_core.c +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c @@ -1336,8 +1336,10 @@ static int exynos_dp_probe(struct platform_device *pdev) if (panel_node) { dp->panel = of_drm_find_panel(panel_node); of_node_put(panel_node);

•           if (!dp->panel)
  

•                   return -EPROBE_DEFER;
  

•           if (!dp->panel) {
  

•                   ret = -EPROBE_DEFER;
  

•                   goto free_dp;
  

•           }
    }
  
    endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
  

@@ -1346,10 +1348,14 @@ static int exynos_dp_probe(struct platform_device *pdev) if (bridge_node) { dp->bridge = of_drm_find_bridge(bridge_node); of_node_put(bridge_node);

•                   if (!dp->bridge)
  

•                           return -EPROBE_DEFER;
  

•           } else
  

•                   return -EPROBE_DEFER;
  

•                   if (!dp->bridge) {
  

•                           ret = -EPROBE_DEFER;
  

•                           goto free_dp;
  

•                   }
  

•           } else {
  

•                   ret = -EPROBE_DEFER;
  

•                   goto free_dp;
  

•           }
    }
  
    exynos_dp_display.ctx = dp;
  

@@ -1359,6 +1365,9 @@ static int exynos_dp_probe(struct platform_device *pdev) exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR);

+free_dp:

•   devm_kfree(dev, dp);
  

•   return ret;
  

}

-- 1.9.3

---

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

Gustavo