[PATCH 0/2] omap3 devm usage removal

[PATCH 0/2] omap3 devm usage removal

[Shuah Khan]

This patch series contains two patches. The first one removes
calls to media_entity_cleanup() after  media device has been
unregistered. The second one removes devm usage.

Shuah Khan (2):
  media: omap3isp fix media_entity_cleanup() after media device
    unregister
  media: omap3isp change to devm for resources

 drivers/media/platform/omap3isp/isp.c         | 71 +++++++++++++++++++--------
 drivers/media/platform/omap3isp/ispccdc.c     |  1 -
 drivers/media/platform/omap3isp/ispccp2.c     | 11 +++--
 drivers/media/platform/omap3isp/ispcsi2.c     |  1 -
 drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++---
 drivers/media/platform/omap3isp/isph3a_af.c   | 21 +++++---
 drivers/media/platform/omap3isp/isphist.c     |  5 +-
 drivers/media/platform/omap3isp/isppreview.c  |  1 -
 drivers/media/platform/omap3isp/ispresizer.c  |  1 -
 drivers/media/platform/omap3isp/ispstat.c     |  1 -
 drivers/media/platform/omap3isp/ispvideo.c    |  1 -
 11 files changed, 92 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister

[Shuah Khan]

During unbind isp_remove() media_entity_cleanup() after it unregisters the
media_device. Cleanup routine calls media_entity_cleanup() accessing subdev
entities that have been removed. This will cause problems during unbind.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/platform/omap3isp/ispccdc.c    | 1 -
 drivers/media/platform/omap3isp/ispccp2.c    | 1 -
 drivers/media/platform/omap3isp/ispcsi2.c    | 1 -
 drivers/media/platform/omap3isp/isppreview.c | 1 -
 drivers/media/platform/omap3isp/ispresizer.c | 1 -
 drivers/media/platform/omap3isp/ispstat.c    | 1 -
 drivers/media/platform/omap3isp/ispvideo.c   | 1 -
 7 files changed, 7 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 882310e..6d27e48 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -2726,7 +2726,6 @@ void omap3isp_ccdc_cleanup(struct isp_device *isp)
 	struct isp_ccdc_device *ccdc = &isp->isp_ccdc;
 
 	omap3isp_video_cleanup(&ccdc->video_out);
-	media_entity_cleanup(&ccdc->subdev.entity);
 
 	/* Free LSC requests. As the CCDC is stopped there's no active request,
 	 * so only the pending request and the free queue need to be handled.
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..4c1e7f0 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1162,5 +1162,4 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
 	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
 
 	omap3isp_video_cleanup(&ccp2->video_in);
-	media_entity_cleanup(&ccp2->subdev.entity);
 }
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index f75a1be..840756e 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -1318,5 +1318,4 @@ void omap3isp_csi2_cleanup(struct isp_device *isp)
 	struct isp_csi2_device *csi2a = &isp->isp_csi2a;
 
 	omap3isp_video_cleanup(&csi2a->video_out);
-	media_entity_cleanup(&csi2a->subdev.entity);
 }
diff --git a/drivers/media/platform/omap3isp/isppreview.c b/drivers/media/platform/omap3isp/isppreview.c
index ac30a0f..a179dac 100644
--- a/drivers/media/platform/omap3isp/isppreview.c
+++ b/drivers/media/platform/omap3isp/isppreview.c
@@ -2348,5 +2348,4 @@ void omap3isp_preview_cleanup(struct isp_device *isp)
 	v4l2_ctrl_handler_free(&prev->ctrls);
 	omap3isp_video_cleanup(&prev->video_in);
 	omap3isp_video_cleanup(&prev->video_out);
-	media_entity_cleanup(&prev->subdev.entity);
 }
diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c
index 0b6a875..d22a54a 100644
--- a/drivers/media/platform/omap3isp/ispresizer.c
+++ b/drivers/media/platform/omap3isp/ispresizer.c
@@ -1791,5 +1791,4 @@ void omap3isp_resizer_cleanup(struct isp_device *isp)
 
 	omap3isp_video_cleanup(&res->video_in);
 	omap3isp_video_cleanup(&res->video_out);
-	media_entity_cleanup(&res->subdev.entity);
 }
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 1b9217d..47b8e43 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1055,7 +1055,6 @@ int omap3isp_stat_init(struct ispstat *stat, const char *name,
 
 void omap3isp_stat_cleanup(struct ispstat *stat)
 {
-	media_entity_cleanup(&stat->subdev.entity);
 	mutex_destroy(&stat->ioctl_lock);
 	isp_stat_bufs_free(stat);
 	kfree(stat->buf);
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 7354469..6914035 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1470,7 +1470,6 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
 
 void omap3isp_video_cleanup(struct isp_video *video)
 {
-	media_entity_cleanup(&video->video.entity);
 	mutex_destroy(&video->queue_lock);
 	mutex_destroy(&video->stream_lock);
 	mutex_destroy(&video->mutex);
-- 
2.7.4

[PATCH 2/2] media: omap3isp change to devm for resources

[Shuah Khan]

Using devm resources that have external dependencies such as a dev
for a file handler could result in devm resources getting released
durin unbind while an application has the file open holding pointer
to the devm resource. This results in use-after-free errors when the
application exits.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/platform/omap3isp/isp.c         | 71 +++++++++++++++++++--------
 drivers/media/platform/omap3isp/ispccp2.c     | 10 +++-
 drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++---
 drivers/media/platform/omap3isp/isph3a_af.c   | 21 +++++---
 drivers/media/platform/omap3isp/isphist.c     |  5 +-
 5 files changed, 92 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 0321d84..a11c509 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1374,7 +1374,7 @@ static int isp_get_clocks(struct isp_device *isp)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) {
-		clk = devm_clk_get(isp->dev, isp_clocks[i]);
+		clk = clk_get(isp->dev, isp_clocks[i]);
 		if (IS_ERR(clk)) {
 			dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]);
 			return PTR_ERR(clk);
@@ -1386,6 +1386,14 @@ static int isp_get_clocks(struct isp_device *isp)
 	return 0;
 }
 
+static void isp_put_clocks(struct isp_device *isp)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i)
+		clk_put(isp->clock[i]);
+}
+
 /*
  * omap3isp_get - Acquire the ISP resource.
  *
@@ -2015,6 +2023,11 @@ static int isp_remove(struct platform_device *pdev)
 
 	media_entity_enum_cleanup(&isp->crashed);
 
+	regulator_put(isp->isp_csiphy2.vdd);
+	regulator_put(isp->isp_csiphy1.vdd);
+
+	isp_put_clocks(isp);
+	kfree(isp);
 	return 0;
 }
 
@@ -2107,8 +2120,8 @@ static int isp_of_parse_nodes(struct device *dev,
 {
 	struct device_node *node = NULL;
 
-	notifier->subdevs = devm_kcalloc(
-		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
+	notifier->subdevs = kcalloc(
+		ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
 	if (!notifier->subdevs)
 		return -ENOMEM;
 
@@ -2116,11 +2129,9 @@ static int isp_of_parse_nodes(struct device *dev,
 	       (node = of_graph_get_next_endpoint(dev->of_node, node))) {
 		struct isp_async_subdev *isd;
 
-		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
-		if (!isd) {
-			of_node_put(node);
+		isd = kzalloc(sizeof(*isd), GFP_KERNEL);
+		if (!isd)
 			return -ENOMEM;
-		}
 
 		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
 
@@ -2204,7 +2215,7 @@ static int isp_probe(struct platform_device *pdev)
 	int ret;
 	int i, m;
 
-	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
+	isp = kzalloc(sizeof(*isp), GFP_KERNEL);
 	if (!isp) {
 		dev_err(&pdev->dev, "could not allocate memory\n");
 		return -ENOMEM;
@@ -2213,21 +2224,23 @@ static int isp_probe(struct platform_device *pdev)
 	ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
 				   &isp->phy_type);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
 						      "syscon");
-	if (IS_ERR(isp->syscon))
-		return PTR_ERR(isp->syscon);
+	if (IS_ERR(isp->syscon)) {
+		ret = PTR_ERR(isp->syscon);
+		goto error_release_isp;
+	}
 
 	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
 					 &isp->syscon_offset);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
 	if (ret < 0)
-		return ret;
+		goto error_release_isp;
 
 	isp->autoidle = autoidle;
 
@@ -2244,8 +2257,18 @@ static int isp_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, isp);
 
 	/* Regulators */
-	isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1");
-	isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2");
+	isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1");
+	if (IS_ERR(isp->isp_csiphy1.vdd)) {
+		ret = PTR_ERR(isp->isp_csiphy1.vdd);
+		isp->isp_csiphy1.vdd = NULL;
+		goto error;
+	}
+	isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2");
+	if (IS_ERR(isp->isp_csiphy2.vdd)) {
+		ret = PTR_ERR(isp->isp_csiphy2.vdd);
+		isp->isp_csiphy2.vdd = NULL;
+		goto error_put_vdd_csiphy1;
+	}
 
 	/* Clocks
 	 *
@@ -2264,16 +2287,17 @@ static int isp_probe(struct platform_device *pdev)
 		isp->mmio_base[map_idx] =
 			devm_ioremap_resource(isp->dev, mem);
 		if (IS_ERR(isp->mmio_base[map_idx]))
-			return PTR_ERR(isp->mmio_base[map_idx]);
+			ret = PTR_ERR(isp->mmio_base[map_idx]);
+			goto error_put_vdd_csiphy2;
 	}
 
 	ret = isp_get_clocks(isp);
 	if (ret < 0)
-		goto error;
+		goto error_put_vdd_csiphy2;
 
 	ret = clk_enable(isp->clock[ISP_CLK_CAM_ICK]);
 	if (ret < 0)
-		goto error;
+		goto error_put_vdd_csiphy2;
 
 	isp->revision = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION);
 	dev_info(isp->dev, "Revision %d.%d found\n",
@@ -2283,7 +2307,7 @@ static int isp_probe(struct platform_device *pdev)
 
 	if (__omap3isp_get(isp, false) == NULL) {
 		ret = -ENODEV;
-		goto error;
+		goto error_put_vdd_csiphy2;
 	}
 
 	ret = isp_reset(isp);
@@ -2334,7 +2358,7 @@ static int isp_probe(struct platform_device *pdev)
 	}
 	isp->irq_num = ret;
 
-	if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED,
+	if (request_irq(isp->irq_num, isp_isr, IRQF_SHARED,
 			     "OMAP3 ISP", isp)) {
 		dev_err(isp->dev, "Unable to request IRQ\n");
 		ret = -EINVAL;
@@ -2375,8 +2399,15 @@ static int isp_probe(struct platform_device *pdev)
 error_isp:
 	isp_xclk_cleanup(isp);
 	__omap3isp_put(isp, false);
+error_put_vdd_csiphy2:
+	regulator_put(isp->isp_csiphy2.vdd);
+error_put_vdd_csiphy1:
+	regulator_put(isp->isp_csiphy1.vdd);
 error:
 	mutex_destroy(&isp->isp_mutex);
+	isp_put_clocks(isp);
+error_release_isp:
+	kfree(isp);
 
 	return ret;
 }
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 4c1e7f0..adf4191 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
 	 */
 	if (isp->revision == ISP_REVISION_2_0) {
-		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
+		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
@@ -1147,10 +1147,15 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 
 	ret = ccp2_init_entities(ccp2);
 	if (ret < 0)
-		return ret;
+		goto error_put_vdds_csib;
 
 	ccp2_reset(ccp2);
 	return 0;
+
+error_put_vdds_csib:
+	regulator_put(ccp2->vdds_csib);
+
+	return ret;
 }
 
 /*
@@ -1162,4 +1167,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
 	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
 
 	omap3isp_video_cleanup(&ccp2->video_in);
+	regulator_put(ccp2->vdds_csib);
 }
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index ccaf92f..042de3e 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 {
 	struct ispstat *aewb = &isp->isp_aewb;
 	struct omap3isp_h3a_aewb_config *aewb_cfg;
-	struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
+	struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
+	int ret;
 
-	aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
+	aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
 	if (!aewb_cfg)
 		return -ENOMEM;
 
@@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	aewb->isp = isp;
 
 	/* Set recover state configuration */
-	aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
-					GFP_KERNEL);
+	aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
 	if (!aewb_recover_cfg) {
 		dev_err(aewb->isp->dev, "AEWB: cannot allocate memory for "
 					"recover configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_release_aewb_cfg;
 	}
 
 	aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,21 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
 		dev_err(aewb->isp->dev, "AEWB: recover configuration is "
 					"invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_release_aewb_recover_cfg;
 	}
 
 	aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
 	aewb->recover_priv = aewb_recover_cfg;
 
 	return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+
+err_release_aewb_recover_cfg:
+	kfree(aewb_recover_cfg);
+err_release_aewb_cfg:
+	kfree(aewb_cfg);
+
+	return ret;
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 92937f7..1919cb2 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 {
 	struct ispstat *af = &isp->isp_af;
 	struct omap3isp_h3a_af_config *af_cfg;
-	struct omap3isp_h3a_af_config *af_recover_cfg;
+	struct omap3isp_h3a_af_config *af_recover_cfg = NULL;
+	int ret;
 
-	af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL);
+	af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL);
 	if (af_cfg == NULL)
 		return -ENOMEM;
 
@@ -364,12 +365,12 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	af->isp = isp;
 
 	/* Set recover state configuration */
-	af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg),
-				      GFP_KERNEL);
+	af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL);
 	if (!af_recover_cfg) {
 		dev_err(af->isp->dev, "AF: cannot allocate memory for recover "
 				      "configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_release_af_cfg;
 	}
 
 	af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
@@ -381,13 +382,21 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	if (h3a_af_validate_params(af, af_recover_cfg)) {
 		dev_err(af->isp->dev, "AF: recover configuration is "
 				      "invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_release_af_recover_cfg;
 	}
 
 	af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg);
 	af->recover_priv = af_recover_cfg;
 
 	return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+
+err_release_af_recover_cfg:
+	kfree(af_recover_cfg);
+err_release_af_cfg:
+	kfree(af_cfg);
+
+	return ret;
 }
 
 void omap3isp_h3a_af_cleanup(struct isp_device *isp)
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index 7138b04..5d8f04b 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp)
 {
 	struct ispstat *hist = &isp->isp_hist;
 	struct omap3isp_hist_config *hist_cfg;
-	int ret = -1;
+	int ret = 0;
 
-	hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
+	hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
 	if (hist_cfg == NULL)
 		return -ENOMEM;
 
@@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp)
 	if (ret) {
 		if (hist->dma_ch)
 			dma_release_channel(hist->dma_ch);
+		kfree(hist_cfg);
 	}
 
 	return ret;
-- 
2.7.4

Re: [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister

[Laurent Pinchart]

Hello Shuah,

Thank you for the patch.

On Thursday 15 Dec 2016 12:40:07 Shuah Khan wrote:
> During unbind isp_remove() media_entity_cleanup() after it unregisters the

I assume you meant "During unbind isp_remove() calls media_entity_cleanup()"

> media_device. Cleanup routine calls media_entity_cleanup() accessing subdev
> entities that have been removed. This will cause problems during unbind.

What problems ? media_entity_cleanup() is a no-op. And regardless of that, you 
shouldn't remove the function call (unless the function is considered 
deprecated, in which case it should be removed completely, and that should be 
discussed beforehand) but move it to the appropriate place.

> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/platform/omap3isp/ispccdc.c    | 1 -
>  drivers/media/platform/omap3isp/ispccp2.c    | 1 -
>  drivers/media/platform/omap3isp/ispcsi2.c    | 1 -
>  drivers/media/platform/omap3isp/isppreview.c | 1 -
>  drivers/media/platform/omap3isp/ispresizer.c | 1 -
>  drivers/media/platform/omap3isp/ispstat.c    | 1 -
>  drivers/media/platform/omap3isp/ispvideo.c   | 1 -
>  7 files changed, 7 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> b/drivers/media/platform/omap3isp/ispccdc.c index 882310e..6d27e48 100644
> --- a/drivers/media/platform/omap3isp/ispccdc.c
> +++ b/drivers/media/platform/omap3isp/ispccdc.c
> @@ -2726,7 +2726,6 @@ void omap3isp_ccdc_cleanup(struct isp_device *isp)
>  	struct isp_ccdc_device *ccdc = &isp->isp_ccdc;
> 
>  	omap3isp_video_cleanup(&ccdc->video_out);
> -	media_entity_cleanup(&ccdc->subdev.entity);
> 
>  	/* Free LSC requests. As the CCDC is stopped there's no active 
request,
>  	 * so only the pending request and the free queue need to be handled.
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4c1e7f0 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1162,5 +1162,4 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
>  	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
> 
>  	omap3isp_video_cleanup(&ccp2->video_in);
> -	media_entity_cleanup(&ccp2->subdev.entity);
>  }
> diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> b/drivers/media/platform/omap3isp/ispcsi2.c index f75a1be..840756e 100644
> --- a/drivers/media/platform/omap3isp/ispcsi2.c
> +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> @@ -1318,5 +1318,4 @@ void omap3isp_csi2_cleanup(struct isp_device *isp)
>  	struct isp_csi2_device *csi2a = &isp->isp_csi2a;
> 
>  	omap3isp_video_cleanup(&csi2a->video_out);
> -	media_entity_cleanup(&csi2a->subdev.entity);
>  }
> diff --git a/drivers/media/platform/omap3isp/isppreview.c
> b/drivers/media/platform/omap3isp/isppreview.c index ac30a0f..a179dac
> 100644
> --- a/drivers/media/platform/omap3isp/isppreview.c
> +++ b/drivers/media/platform/omap3isp/isppreview.c
> @@ -2348,5 +2348,4 @@ void omap3isp_preview_cleanup(struct isp_device *isp)
>  	v4l2_ctrl_handler_free(&prev->ctrls);
>  	omap3isp_video_cleanup(&prev->video_in);
>  	omap3isp_video_cleanup(&prev->video_out);
> -	media_entity_cleanup(&prev->subdev.entity);
>  }
> diff --git a/drivers/media/platform/omap3isp/ispresizer.c
> b/drivers/media/platform/omap3isp/ispresizer.c index 0b6a875..d22a54a
> 100644
> --- a/drivers/media/platform/omap3isp/ispresizer.c
> +++ b/drivers/media/platform/omap3isp/ispresizer.c
> @@ -1791,5 +1791,4 @@ void omap3isp_resizer_cleanup(struct isp_device *isp)
> 
>  	omap3isp_video_cleanup(&res->video_in);
>  	omap3isp_video_cleanup(&res->video_out);
> -	media_entity_cleanup(&res->subdev.entity);
>  }
> diff --git a/drivers/media/platform/omap3isp/ispstat.c
> b/drivers/media/platform/omap3isp/ispstat.c index 1b9217d..47b8e43 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -1055,7 +1055,6 @@ int omap3isp_stat_init(struct ispstat *stat, const
> char *name,
> 
>  void omap3isp_stat_cleanup(struct ispstat *stat)
>  {
> -	media_entity_cleanup(&stat->subdev.entity);
>  	mutex_destroy(&stat->ioctl_lock);
>  	isp_stat_bufs_free(stat);
>  	kfree(stat->buf);
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 7354469..6914035 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1470,7 +1470,6 @@ int omap3isp_video_init(struct isp_video *video, const
> char *name)
> 
>  void omap3isp_video_cleanup(struct isp_video *video)
>  {
> -	media_entity_cleanup(&video->video.entity);
>  	mutex_destroy(&video->queue_lock);
>  	mutex_destroy(&video->stream_lock);
>  	mutex_destroy(&video->mutex);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Laurent Pinchart]

Hi Shuah,

Thank you for the patch.

Sakari has submitted a similar patch as part of his kref series. Please use it 
as a base point and rework it if you want to get it merged separately. I've 
reviewed the patch and left quite a few comments that need to be addressed.

On Thursday 15 Dec 2016 12:40:08 Shuah Khan wrote:
> Using devm resources that have external dependencies such as a dev
> for a file handler could result in devm resources getting released
> durin unbind while an application has the file open holding pointer
> to the devm resource. This results in use-after-free errors when the
> application exits.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/platform/omap3isp/isp.c         | 71 ++++++++++++++++--------
>  drivers/media/platform/omap3isp/ispccp2.c     | 10 +++-
>  drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++---
>  drivers/media/platform/omap3isp/isph3a_af.c   | 21 +++++---
>  drivers/media/platform/omap3isp/isphist.c     |  5 +-
>  5 files changed, 92 insertions(+), 36 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Shuah Khan]

On 12/15/2016 03:33 PM, Laurent Pinchart wrote:
> Hi Shuah,
> 
> Thank you for the patch.
> 
> Sakari has submitted a similar patch as part of his kref series. Please use it 
> as a base point and rework it if you want to get it merged separately. I've 
> reviewed the patch and left quite a few comments that need to be addressed.
> 

I really don't mind if Sakari uses this patch as is and makes the changes
you requested and submits devm removal as an independent patch.

My intent behind sending this one is to help him out since I already did
this patch that is on top of 4.9-rc8 without any dependencies on Sakari's
RFC patch.

thanks,
-- Shuah

> On Thursday 15 Dec 2016 12:40:08 Shuah Khan wrote:
>> Using devm resources that have external dependencies such as a dev
>> for a file handler could result in devm resources getting released
>> durin unbind while an application has the file open holding pointer
>> to the devm resource. This results in use-after-free errors when the
>> application exits.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/platform/omap3isp/isp.c         | 71 ++++++++++++++++--------
>>  drivers/media/platform/omap3isp/ispccp2.c     | 10 +++-
>>  drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++---
>>  drivers/media/platform/omap3isp/isph3a_af.c   | 21 +++++---
>>  drivers/media/platform/omap3isp/isphist.c     |  5 +-
>>  5 files changed, 92 insertions(+), 36 deletions(-)
> 

Re: [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister

[Shuah Khan]

On 12/15/2016 03:32 PM, Laurent Pinchart wrote:
> Hello Shuah,
> 
> Thank you for the patch.
> 
> On Thursday 15 Dec 2016 12:40:07 Shuah Khan wrote:
>> During unbind isp_remove() media_entity_cleanup() after it unregisters the
> 
> I assume you meant "During unbind isp_remove() calls media_entity_cleanup()"
> 
>> media_device. Cleanup routine calls media_entity_cleanup() accessing subdev
>> entities that have been removed. This will cause problems during unbind.
> 
> What problems ? media_entity_cleanup() is a no-op. And regardless of that, you 
> shouldn't remove the function call (unless the function is considered 
> deprecated, in which case it should be removed completely, and that should be 
> discussed beforehand) but move it to the appropriate place.

It is in the wrong place for one thing, It accesses entity after it
is removed. It could be moved to the right place or like you said,
remove media_entity_cleanup() interface as it does nothing and we
don't seem to have any concrete future plan for what it do.

> 
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/platform/omap3isp/ispccdc.c    | 1 -
>>  drivers/media/platform/omap3isp/ispccp2.c    | 1 -
>>  drivers/media/platform/omap3isp/ispcsi2.c    | 1 -
>>  drivers/media/platform/omap3isp/isppreview.c | 1 -
>>  drivers/media/platform/omap3isp/ispresizer.c | 1 -
>>  drivers/media/platform/omap3isp/ispstat.c    | 1 -
>>  drivers/media/platform/omap3isp/ispvideo.c   | 1 -
>>  7 files changed, 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/ispccdc.c
>> b/drivers/media/platform/omap3isp/ispccdc.c index 882310e..6d27e48 100644
>> --- a/drivers/media/platform/omap3isp/ispccdc.c
>> +++ b/drivers/media/platform/omap3isp/ispccdc.c
>> @@ -2726,7 +2726,6 @@ void omap3isp_ccdc_cleanup(struct isp_device *isp)
>>  	struct isp_ccdc_device *ccdc = &isp->isp_ccdc;
>>
>>  	omap3isp_video_cleanup(&ccdc->video_out);
>> -	media_entity_cleanup(&ccdc->subdev.entity);
>>
>>  	/* Free LSC requests. As the CCDC is stopped there's no active 
> request,
>>  	 * so only the pending request and the free queue need to be handled.
>> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
>> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4c1e7f0 100644
>> --- a/drivers/media/platform/omap3isp/ispccp2.c
>> +++ b/drivers/media/platform/omap3isp/ispccp2.c
>> @@ -1162,5 +1162,4 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
>>  	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
>>
>>  	omap3isp_video_cleanup(&ccp2->video_in);
>> -	media_entity_cleanup(&ccp2->subdev.entity);
>>  }
>> diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
>> b/drivers/media/platform/omap3isp/ispcsi2.c index f75a1be..840756e 100644
>> --- a/drivers/media/platform/omap3isp/ispcsi2.c
>> +++ b/drivers/media/platform/omap3isp/ispcsi2.c
>> @@ -1318,5 +1318,4 @@ void omap3isp_csi2_cleanup(struct isp_device *isp)
>>  	struct isp_csi2_device *csi2a = &isp->isp_csi2a;
>>
>>  	omap3isp_video_cleanup(&csi2a->video_out);
>> -	media_entity_cleanup(&csi2a->subdev.entity);
>>  }
>> diff --git a/drivers/media/platform/omap3isp/isppreview.c
>> b/drivers/media/platform/omap3isp/isppreview.c index ac30a0f..a179dac
>> 100644
>> --- a/drivers/media/platform/omap3isp/isppreview.c
>> +++ b/drivers/media/platform/omap3isp/isppreview.c
>> @@ -2348,5 +2348,4 @@ void omap3isp_preview_cleanup(struct isp_device *isp)
>>  	v4l2_ctrl_handler_free(&prev->ctrls);
>>  	omap3isp_video_cleanup(&prev->video_in);
>>  	omap3isp_video_cleanup(&prev->video_out);
>> -	media_entity_cleanup(&prev->subdev.entity);
>>  }
>> diff --git a/drivers/media/platform/omap3isp/ispresizer.c
>> b/drivers/media/platform/omap3isp/ispresizer.c index 0b6a875..d22a54a
>> 100644
>> --- a/drivers/media/platform/omap3isp/ispresizer.c
>> +++ b/drivers/media/platform/omap3isp/ispresizer.c
>> @@ -1791,5 +1791,4 @@ void omap3isp_resizer_cleanup(struct isp_device *isp)
>>
>>  	omap3isp_video_cleanup(&res->video_in);
>>  	omap3isp_video_cleanup(&res->video_out);
>> -	media_entity_cleanup(&res->subdev.entity);
>>  }
>> diff --git a/drivers/media/platform/omap3isp/ispstat.c
>> b/drivers/media/platform/omap3isp/ispstat.c index 1b9217d..47b8e43 100644
>> --- a/drivers/media/platform/omap3isp/ispstat.c
>> +++ b/drivers/media/platform/omap3isp/ispstat.c
>> @@ -1055,7 +1055,6 @@ int omap3isp_stat_init(struct ispstat *stat, const
>> char *name,
>>
>>  void omap3isp_stat_cleanup(struct ispstat *stat)
>>  {
>> -	media_entity_cleanup(&stat->subdev.entity);
>>  	mutex_destroy(&stat->ioctl_lock);
>>  	isp_stat_bufs_free(stat);
>>  	kfree(stat->buf);
>> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
>> b/drivers/media/platform/omap3isp/ispvideo.c index 7354469..6914035 100644
>> --- a/drivers/media/platform/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/omap3isp/ispvideo.c
>> @@ -1470,7 +1470,6 @@ int omap3isp_video_init(struct isp_video *video, const
>> char *name)
>>
>>  void omap3isp_video_cleanup(struct isp_video *video)
>>  {
>> -	media_entity_cleanup(&video->video.entity);
>>  	mutex_destroy(&video->queue_lock);
>>  	mutex_destroy(&video->stream_lock);
>>  	mutex_destroy(&video->mutex);
> 

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Laurent Pinchart]

Hi Shuah,

On Thursday 15 Dec 2016 15:51:41 Shuah Khan wrote:
> On 12/15/2016 03:33 PM, Laurent Pinchart wrote:
> > Hi Shuah,
> > 
> > Thank you for the patch.
> > 
> > Sakari has submitted a similar patch as part of his kref series. Please
> > use it as a base point and rework it if you want to get it merged
> > separately. I've reviewed the patch and left quite a few comments that
> > need to be addressed.
>
> I really don't mind if Sakari uses this patch as is and makes the changes
> you requested and submits devm removal as an independent patch.
> 
> My intent behind sending this one is to help him out since I already did
> this patch that is on top of 4.9-rc8 without any dependencies on Sakari's
> RFC patch.

I've only seen your reply to Sakari's patch after replying to this one. Thank 
you for providing your version, I'll let Sakari merge both and resubmit.

> > On Thursday 15 Dec 2016 12:40:08 Shuah Khan wrote:
> >> Using devm resources that have external dependencies such as a dev
> >> for a file handler could result in devm resources getting released
> >> durin unbind while an application has the file open holding pointer
> >> to the devm resource. This results in use-after-free errors when the
> >> application exits.
> >> 
> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >> ---
> >> 
> >>  drivers/media/platform/omap3isp/isp.c         | 71 +++++++++++++--------
> >>  drivers/media/platform/omap3isp/ispccp2.c     | 10 +++-
> >>  drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++---
> >>  drivers/media/platform/omap3isp/isph3a_af.c   | 21 +++++---
> >>  drivers/media/platform/omap3isp/isphist.c     |  5 +-
> >>  5 files changed, 92 insertions(+), 36 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4443 bytes --]

Hi Shuah,

[auto build test WARNING on v4.9-rc8]
[cannot apply to linuxtv-media/master next-20161215]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Shuah-Khan/omap3-devm-usage-removal/20161216-111439
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/media/platform/omap3isp/isp.c: In function 'isp_probe':
>> drivers/media/platform/omap3isp/isp.c:2289:3: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
      if (IS_ERR(isp->mmio_base[map_idx]))
      ^~
   drivers/media/platform/omap3isp/isp.c:2291:4: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
       goto error_put_vdd_csiphy2;
       ^~~~

vim +/if +2289 drivers/media/platform/omap3isp/isp.c

d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  2273  	/* Clocks
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  2274  	 *
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  2275  	 * The ISP clock tree is revision-dependent. We thus need to enable ICLK
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  2276  	 * manually to read the revision before calling __omap3isp_get().
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2277  	 *
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2278  	 * Start by mapping the ISP MMIO area, which is in two pieces.
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2279  	 * The ISP IOMMU is in between. Map both now, and fill in the
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2280  	 * ISP revision specific portions a little later in the
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2281  	 * function.
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  2282  	 */
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2283  	for (i = 0; i < 2; i++) {
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2284  		unsigned int map_idx = i ? OMAP3_ISP_IOMEM_CSI2A_REGS1 : 0;
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2285  
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2286  		mem = platform_get_resource(pdev, IORESOURCE_MEM, i);
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2287  		isp->mmio_base[map_idx] =
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2288  			devm_ioremap_resource(isp->dev, mem);
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25 @2289  		if (IS_ERR(isp->mmio_base[map_idx]))
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan       2016-12-15  2290  			ret = PTR_ERR(isp->mmio_base[map_idx]);
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan       2016-12-15  2291  			goto error_put_vdd_csiphy2;
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus     2015-03-25  2292  	}
448de7e7 drivers/media/video/omap3isp/isp.c    Sakari Ailus     2011-02-12  2293  
448de7e7 drivers/media/video/omap3isp/isp.c    Sakari Ailus     2011-02-12  2294  	ret = isp_get_clocks(isp);
448de7e7 drivers/media/video/omap3isp/isp.c    Sakari Ailus     2011-02-12  2295  	if (ret < 0)
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan       2016-12-15  2296  		goto error_put_vdd_csiphy2;
448de7e7 drivers/media/video/omap3isp/isp.c    Sakari Ailus     2011-02-12  2297  

:::::: The code at line 2289 was first introduced by commit
:::::: 8644cdf972dd6dfebf98161025900f6a9d1ad58a [media] omap3isp: Replace many MMIO regions by two

:::::: TO: Sakari Ailus <sakari.ailus@iki.fi>
:::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28487 bytes --]

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Hans Verkuil]

On 15/12/16 20:40, Shuah Khan wrote:
> Using devm resources that have external dependencies such as a dev
> for a file handler could result in devm resources getting released
> durin unbind while an application has the file open holding pointer
> to the devm resource. This results in use-after-free errors when the
> application exits.

That's solving the wrong problem.

The real problem is that when registering a video_device it should do
this:

devnode->cdev.kobj.parent = &devnode->dev.kobj;

(taken from cec-core.c)

This will prevent isp->dev from being released as long as there is a
filehandle still open.

After that change I believe that this will work correctly, but this
has to be tested first!

Regards,

	Hans

>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/platform/omap3isp/isp.c         | 71 +++++++++++++++++++--------
>  drivers/media/platform/omap3isp/ispccp2.c     | 10 +++-
>  drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++---
>  drivers/media/platform/omap3isp/isph3a_af.c   | 21 +++++---
>  drivers/media/platform/omap3isp/isphist.c     |  5 +-
>  5 files changed, 92 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 0321d84..a11c509 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1374,7 +1374,7 @@ static int isp_get_clocks(struct isp_device *isp)
>  	unsigned int i;
>
>  	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) {
> -		clk = devm_clk_get(isp->dev, isp_clocks[i]);
> +		clk = clk_get(isp->dev, isp_clocks[i]);
>  		if (IS_ERR(clk)) {
>  			dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]);
>  			return PTR_ERR(clk);
> @@ -1386,6 +1386,14 @@ static int isp_get_clocks(struct isp_device *isp)
>  	return 0;
>  }
>
> +static void isp_put_clocks(struct isp_device *isp)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i)
> +		clk_put(isp->clock[i]);
> +}
> +
>  /*
>   * omap3isp_get - Acquire the ISP resource.
>   *
> @@ -2015,6 +2023,11 @@ static int isp_remove(struct platform_device *pdev)
>
>  	media_entity_enum_cleanup(&isp->crashed);
>
> +	regulator_put(isp->isp_csiphy2.vdd);
> +	regulator_put(isp->isp_csiphy1.vdd);
> +
> +	isp_put_clocks(isp);
> +	kfree(isp);
>  	return 0;
>  }
>
> @@ -2107,8 +2120,8 @@ static int isp_of_parse_nodes(struct device *dev,
>  {
>  	struct device_node *node = NULL;
>
> -	notifier->subdevs = devm_kcalloc(
> -		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
> +	notifier->subdevs = kcalloc(
> +		ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
>  	if (!notifier->subdevs)
>  		return -ENOMEM;
>
> @@ -2116,11 +2129,9 @@ static int isp_of_parse_nodes(struct device *dev,
>  	       (node = of_graph_get_next_endpoint(dev->of_node, node))) {
>  		struct isp_async_subdev *isd;
>
> -		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> -		if (!isd) {
> -			of_node_put(node);
> +		isd = kzalloc(sizeof(*isd), GFP_KERNEL);
> +		if (!isd)
>  			return -ENOMEM;
> -		}
>
>  		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>
> @@ -2204,7 +2215,7 @@ static int isp_probe(struct platform_device *pdev)
>  	int ret;
>  	int i, m;
>
> -	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
> +	isp = kzalloc(sizeof(*isp), GFP_KERNEL);
>  	if (!isp) {
>  		dev_err(&pdev->dev, "could not allocate memory\n");
>  		return -ENOMEM;
> @@ -2213,21 +2224,23 @@ static int isp_probe(struct platform_device *pdev)
>  	ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
>  				   &isp->phy_type);
>  	if (ret)
> -		return ret;
> +		goto error_release_isp;
>
>  	isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>  						      "syscon");
> -	if (IS_ERR(isp->syscon))
> -		return PTR_ERR(isp->syscon);
> +	if (IS_ERR(isp->syscon)) {
> +		ret = PTR_ERR(isp->syscon);
> +		goto error_release_isp;
> +	}
>
>  	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
>  					 &isp->syscon_offset);
>  	if (ret)
> -		return ret;
> +		goto error_release_isp;
>
>  	ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
>  	if (ret < 0)
> -		return ret;
> +		goto error_release_isp;
>
>  	isp->autoidle = autoidle;
>
> @@ -2244,8 +2257,18 @@ static int isp_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, isp);
>
>  	/* Regulators */
> -	isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1");
> -	isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2");
> +	isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1");
> +	if (IS_ERR(isp->isp_csiphy1.vdd)) {
> +		ret = PTR_ERR(isp->isp_csiphy1.vdd);
> +		isp->isp_csiphy1.vdd = NULL;
> +		goto error;
> +	}
> +	isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2");
> +	if (IS_ERR(isp->isp_csiphy2.vdd)) {
> +		ret = PTR_ERR(isp->isp_csiphy2.vdd);
> +		isp->isp_csiphy2.vdd = NULL;
> +		goto error_put_vdd_csiphy1;
> +	}
>
>  	/* Clocks
>  	 *
> @@ -2264,16 +2287,17 @@ static int isp_probe(struct platform_device *pdev)
>  		isp->mmio_base[map_idx] =
>  			devm_ioremap_resource(isp->dev, mem);
>  		if (IS_ERR(isp->mmio_base[map_idx]))
> -			return PTR_ERR(isp->mmio_base[map_idx]);
> +			ret = PTR_ERR(isp->mmio_base[map_idx]);
> +			goto error_put_vdd_csiphy2;
>  	}
>
>  	ret = isp_get_clocks(isp);
>  	if (ret < 0)
> -		goto error;
> +		goto error_put_vdd_csiphy2;
>
>  	ret = clk_enable(isp->clock[ISP_CLK_CAM_ICK]);
>  	if (ret < 0)
> -		goto error;
> +		goto error_put_vdd_csiphy2;
>
>  	isp->revision = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION);
>  	dev_info(isp->dev, "Revision %d.%d found\n",
> @@ -2283,7 +2307,7 @@ static int isp_probe(struct platform_device *pdev)
>
>  	if (__omap3isp_get(isp, false) == NULL) {
>  		ret = -ENODEV;
> -		goto error;
> +		goto error_put_vdd_csiphy2;
>  	}
>
>  	ret = isp_reset(isp);
> @@ -2334,7 +2358,7 @@ static int isp_probe(struct platform_device *pdev)
>  	}
>  	isp->irq_num = ret;
>
> -	if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED,
> +	if (request_irq(isp->irq_num, isp_isr, IRQF_SHARED,
>  			     "OMAP3 ISP", isp)) {
>  		dev_err(isp->dev, "Unable to request IRQ\n");
>  		ret = -EINVAL;
> @@ -2375,8 +2399,15 @@ static int isp_probe(struct platform_device *pdev)
>  error_isp:
>  	isp_xclk_cleanup(isp);
>  	__omap3isp_put(isp, false);
> +error_put_vdd_csiphy2:
> +	regulator_put(isp->isp_csiphy2.vdd);
> +error_put_vdd_csiphy1:
> +	regulator_put(isp->isp_csiphy1.vdd);
>  error:
>  	mutex_destroy(&isp->isp_mutex);
> +	isp_put_clocks(isp);
> +error_release_isp:
> +	kfree(isp);
>
>  	return ret;
>  }
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
> index 4c1e7f0..adf4191 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
>  	 */
>  	if (isp->revision == ISP_REVISION_2_0) {
> -		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> +		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
>  		if (IS_ERR(ccp2->vdds_csib)) {
>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
> @@ -1147,10 +1147,15 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>
>  	ret = ccp2_init_entities(ccp2);
>  	if (ret < 0)
> -		return ret;
> +		goto error_put_vdds_csib;
>
>  	ccp2_reset(ccp2);
>  	return 0;
> +
> +error_put_vdds_csib:
> +	regulator_put(ccp2->vdds_csib);
> +
> +	return ret;
>  }
>
>  /*
> @@ -1162,4 +1167,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
>  	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
>
>  	omap3isp_video_cleanup(&ccp2->video_in);
> +	regulator_put(ccp2->vdds_csib);
>  }
> diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
> index ccaf92f..042de3e 100644
> --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> @@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
>  {
>  	struct ispstat *aewb = &isp->isp_aewb;
>  	struct omap3isp_h3a_aewb_config *aewb_cfg;
> -	struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
> +	struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
> +	int ret;
>
> -	aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
> +	aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
>  	if (!aewb_cfg)
>  		return -ENOMEM;
>
> @@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
>  	aewb->isp = isp;
>
>  	/* Set recover state configuration */
> -	aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
> -					GFP_KERNEL);
> +	aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
>  	if (!aewb_recover_cfg) {
>  		dev_err(aewb->isp->dev, "AEWB: cannot allocate memory for "
>  					"recover configuration.\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_release_aewb_cfg;
>  	}
>
>  	aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
> @@ -323,13 +324,21 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
>  	if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
>  		dev_err(aewb->isp->dev, "AEWB: recover configuration is "
>  					"invalid.\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_release_aewb_recover_cfg;
>  	}
>
>  	aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
>  	aewb->recover_priv = aewb_recover_cfg;
>
>  	return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
> +
> +err_release_aewb_recover_cfg:
> +	kfree(aewb_recover_cfg);
> +err_release_aewb_cfg:
> +	kfree(aewb_cfg);
> +
> +	return ret;
>  }
>
>  /*
> diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
> index 92937f7..1919cb2 100644
> --- a/drivers/media/platform/omap3isp/isph3a_af.c
> +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> @@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
>  {
>  	struct ispstat *af = &isp->isp_af;
>  	struct omap3isp_h3a_af_config *af_cfg;
> -	struct omap3isp_h3a_af_config *af_recover_cfg;
> +	struct omap3isp_h3a_af_config *af_recover_cfg = NULL;
> +	int ret;
>
> -	af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL);
> +	af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL);
>  	if (af_cfg == NULL)
>  		return -ENOMEM;
>
> @@ -364,12 +365,12 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
>  	af->isp = isp;
>
>  	/* Set recover state configuration */
> -	af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg),
> -				      GFP_KERNEL);
> +	af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL);
>  	if (!af_recover_cfg) {
>  		dev_err(af->isp->dev, "AF: cannot allocate memory for recover "
>  				      "configuration.\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_release_af_cfg;
>  	}
>
>  	af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
> @@ -381,13 +382,21 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
>  	if (h3a_af_validate_params(af, af_recover_cfg)) {
>  		dev_err(af->isp->dev, "AF: recover configuration is "
>  				      "invalid.\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_release_af_recover_cfg;
>  	}
>
>  	af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg);
>  	af->recover_priv = af_recover_cfg;
>
>  	return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
> +
> +err_release_af_recover_cfg:
> +	kfree(af_recover_cfg);
> +err_release_af_cfg:
> +	kfree(af_cfg);
> +
> +	return ret;
>  }
>
>  void omap3isp_h3a_af_cleanup(struct isp_device *isp)
> diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
> index 7138b04..5d8f04b 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp)
>  {
>  	struct ispstat *hist = &isp->isp_hist;
>  	struct omap3isp_hist_config *hist_cfg;
> -	int ret = -1;
> +	int ret = 0;
>
> -	hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
> +	hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
>  	if (hist_cfg == NULL)
>  		return -ENOMEM;
>
> @@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp)
>  	if (ret) {
>  		if (hist->dma_ch)
>  			dma_release_channel(hist->dma_ch);
> +		kfree(hist_cfg);
>  	}
>
>  	return ret;
>

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Laurent Pinchart]

Hi Hans,

On Friday 16 Dec 2016 12:39:49 Hans Verkuil wrote:
> On 15/12/16 20:40, Shuah Khan wrote:
> > Using devm resources that have external dependencies such as a dev
> > for a file handler could result in devm resources getting released
> > durin unbind while an application has the file open holding pointer
> > to the devm resource. This results in use-after-free errors when the
> > application exits.
> 
> That's solving the wrong problem.
> 
> The real problem is that when registering a video_device it should do
> this:
> 
> devnode->cdev.kobj.parent = &devnode->dev.kobj;
> 
> (taken from cec-core.c)
> 
> This will prevent isp->dev from being released as long as there is a
> filehandle still open.

But it won't be enough, devm_* resources are released at unbind time, not at 
device release time. Right after the unbind (.remove() for platform devices) 
handler returns, devm_kzalloc allocated memory goes away.

> After that change I believe that this will work correctly, but this
> has to be tested first!

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Hans Verkuil]

On 16/12/16 13:19, Laurent Pinchart wrote:
> Hi Hans,
>
> On Friday 16 Dec 2016 12:39:49 Hans Verkuil wrote:
>> On 15/12/16 20:40, Shuah Khan wrote:
>>> Using devm resources that have external dependencies such as a dev
>>> for a file handler could result in devm resources getting released
>>> durin unbind while an application has the file open holding pointer
>>> to the devm resource. This results in use-after-free errors when the
>>> application exits.
>>
>> That's solving the wrong problem.
>>
>> The real problem is that when registering a video_device it should do
>> this:
>>
>> devnode->cdev.kobj.parent = &devnode->dev.kobj;
>>
>> (taken from cec-core.c)
>>
>> This will prevent isp->dev from being released as long as there is a
>> filehandle still open.
>
> But it won't be enough, devm_* resources are released at unbind time, not at
> device release time. Right after the unbind (.remove() for platform devices)
> handler returns, devm_kzalloc allocated memory goes away.

You're completely right, I keep forgetting about that.

Sorry for the noise.

	Hans

>
>> After that change I believe that this will work correctly, but this
>> has to be tested first!
>