* [PATCH 0/2] omap3 devm usage removal
@ 2016-12-15 19:40 Shuah Khan
2016-12-15 19:40 ` [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister Shuah Khan
2016-12-15 19:40 ` [PATCH 2/2] media: omap3isp change to devm for resources Shuah Khan
0 siblings, 2 replies; 12+ messages in thread
From: Shuah Khan @ 2016-12-15 19:40 UTC (permalink / raw)
To: sakari.ailus, laurent.pinchart, mchehab
Cc: Shuah Khan, linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister 2016-12-15 19:40 [PATCH 0/2] omap3 devm usage removal Shuah Khan @ 2016-12-15 19:40 ` Shuah Khan 2016-12-15 22:32 ` Laurent Pinchart 2016-12-15 19:40 ` [PATCH 2/2] media: omap3isp change to devm for resources Shuah Khan 1 sibling, 1 reply; 12+ messages in thread From: Shuah Khan @ 2016-12-15 19:40 UTC (permalink / raw) To: sakari.ailus, laurent.pinchart, mchehab Cc: Shuah Khan, linux-media, linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister 2016-12-15 19:40 ` [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister Shuah Khan @ 2016-12-15 22:32 ` Laurent Pinchart 2016-12-15 22:53 ` Shuah Khan 0 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2016-12-15 22:32 UTC (permalink / raw) To: Shuah Khan; +Cc: sakari.ailus, mchehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister 2016-12-15 22:32 ` Laurent Pinchart @ 2016-12-15 22:53 ` Shuah Khan 0 siblings, 0 replies; 12+ messages in thread From: Shuah Khan @ 2016-12-15 22:53 UTC (permalink / raw) To: Laurent Pinchart Cc: sakari.ailus, mchehab, linux-media, linux-kernel, 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); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-15 19:40 [PATCH 0/2] omap3 devm usage removal Shuah Khan 2016-12-15 19:40 ` [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister Shuah Khan @ 2016-12-15 19:40 ` Shuah Khan 2016-12-15 22:33 ` Laurent Pinchart ` (2 more replies) 1 sibling, 3 replies; 12+ messages in thread From: Shuah Khan @ 2016-12-15 19:40 UTC (permalink / raw) To: sakari.ailus, laurent.pinchart, mchehab Cc: Shuah Khan, linux-media, linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-15 19:40 ` [PATCH 2/2] media: omap3isp change to devm for resources Shuah Khan @ 2016-12-15 22:33 ` Laurent Pinchart 2016-12-15 22:51 ` Shuah Khan 2016-12-16 8:17 ` kbuild test robot 2016-12-16 11:39 ` Hans Verkuil 2 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2016-12-15 22:33 UTC (permalink / raw) To: Shuah Khan; +Cc: sakari.ailus, mchehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-15 22:33 ` Laurent Pinchart @ 2016-12-15 22:51 ` Shuah Khan 2016-12-15 23:06 ` Laurent Pinchart 0 siblings, 1 reply; 12+ messages in thread From: Shuah Khan @ 2016-12-15 22:51 UTC (permalink / raw) To: Laurent Pinchart, sakari.ailus Cc: mchehab, linux-media, linux-kernel, 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(-) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-15 22:51 ` Shuah Khan @ 2016-12-15 23:06 ` Laurent Pinchart 0 siblings, 0 replies; 12+ messages in thread From: Laurent Pinchart @ 2016-12-15 23:06 UTC (permalink / raw) To: Shuah Khan; +Cc: sakari.ailus, mchehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-15 19:40 ` [PATCH 2/2] media: omap3isp change to devm for resources Shuah Khan 2016-12-15 22:33 ` Laurent Pinchart @ 2016-12-16 8:17 ` kbuild test robot 2016-12-16 11:39 ` Hans Verkuil 2 siblings, 0 replies; 12+ messages in thread From: kbuild test robot @ 2016-12-16 8:17 UTC (permalink / raw) To: Shuah Khan Cc: kbuild-all, sakari.ailus, laurent.pinchart, mchehab, Shuah Khan, linux-media, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-15 19:40 ` [PATCH 2/2] media: omap3isp change to devm for resources Shuah Khan 2016-12-15 22:33 ` Laurent Pinchart 2016-12-16 8:17 ` kbuild test robot @ 2016-12-16 11:39 ` Hans Verkuil 2016-12-16 12:19 ` Laurent Pinchart 2 siblings, 1 reply; 12+ messages in thread From: Hans Verkuil @ 2016-12-16 11:39 UTC (permalink / raw) To: Shuah Khan, sakari.ailus, laurent.pinchart, mchehab Cc: linux-media, linux-kernel 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; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-16 11:39 ` Hans Verkuil @ 2016-12-16 12:19 ` Laurent Pinchart 2016-12-16 12:34 ` Hans Verkuil 0 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2016-12-16 12:19 UTC (permalink / raw) To: Hans Verkuil; +Cc: Shuah Khan, sakari.ailus, mchehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: omap3isp change to devm for resources 2016-12-16 12:19 ` Laurent Pinchart @ 2016-12-16 12:34 ` Hans Verkuil 0 siblings, 0 replies; 12+ messages in thread From: Hans Verkuil @ 2016-12-16 12:34 UTC (permalink / raw) To: Laurent Pinchart Cc: Shuah Khan, sakari.ailus, mchehab, linux-media, linux-kernel 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! > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-12-16 12:34 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-15 19:40 [PATCH 0/2] omap3 devm usage removal Shuah Khan 2016-12-15 19:40 ` [PATCH 1/2] media: omap3isp fix media_entity_cleanup() after media device unregister Shuah Khan 2016-12-15 22:32 ` Laurent Pinchart 2016-12-15 22:53 ` Shuah Khan 2016-12-15 19:40 ` [PATCH 2/2] media: omap3isp change to devm for resources Shuah Khan 2016-12-15 22:33 ` Laurent Pinchart 2016-12-15 22:51 ` Shuah Khan 2016-12-15 23:06 ` Laurent Pinchart 2016-12-16 8:17 ` kbuild test robot 2016-12-16 11:39 ` Hans Verkuil 2016-12-16 12:19 ` Laurent Pinchart 2016-12-16 12:34 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).