* [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
@ 2017-02-20 15:22 ` Sakari Ailus
2017-02-28 13:12 ` Laurent Pinchart
2017-02-20 15:22 ` [PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally Sakari Ailus
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-20 15:22 UTC (permalink / raw)
To: linux-media
devm functions are fine for managing resources that are directly related
to the device at hand and that have no other dependencies. However, a
process holding a file handle to a device created by a driver for a device
may result in the file handle left behind after the device is long gone.
This will result in accessing released (and potentially reallocated)
memory.
Instead, manage the memory resources in the driver. Releasing the
resources can be later on bound to e.g. by releasing a reference.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 18 ++++++++++++------
drivers/media/platform/omap3isp/isph3a_aewb.c | 24 +++++++++++++++++-------
drivers/media/platform/omap3isp/isph3a_af.c | 24 +++++++++++++++++-------
drivers/media/platform/omap3isp/isphist.c | 11 +++++++----
drivers/media/platform/omap3isp/ispstat.c | 2 ++
5 files changed, 55 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 084ecf4a..874f6fc 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2015,6 +2015,8 @@ static int isp_remove(struct platform_device *pdev)
media_entity_enum_cleanup(&isp->crashed);
+ kfree(isp);
+
return 0;
}
@@ -2203,7 +2205,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;
@@ -2212,21 +2214,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;
@@ -2376,6 +2380,8 @@ static int isp_probe(struct platform_device *pdev)
__omap3isp_put(isp, false);
error:
mutex_destroy(&isp->isp_mutex);
+error_release_isp:
+ kfree(isp);
return ret;
}
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f2..efddafd 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;
}
aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,22 @@ 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;
}
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);
+ ret = omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+
+err:
+ if (ret) {
+ kfree(aewb_cfg);
+ kfree(aewb_recover_cfg);
+ }
+
+ return ret;
}
/*
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 99bd6cc..fee3d19 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;
}
af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
@@ -381,13 +382,22 @@ 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;
}
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);
+ ret = omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+
+err:
+ if (ret) {
+ kfree(af_cfg);
+ kfree(af_recover_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 a4ed5d1..9e448c6 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -476,9 +476,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;
- 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;
@@ -500,7 +500,7 @@ int omap3isp_hist_init(struct isp_device *isp)
if (IS_ERR(hist->dma_ch)) {
ret = PTR_ERR(hist->dma_ch);
if (ret == -EPROBE_DEFER)
- return ret;
+ goto err;
hist->dma_ch = NULL;
dev_warn(isp->dev,
@@ -516,9 +516,12 @@ int omap3isp_hist_init(struct isp_device *isp)
hist->event_type = V4L2_EVENT_OMAP3ISP_HIST;
ret = omap3isp_stat_init(hist, "histogram", &hist_subdev_ops);
+
+err:
if (ret) {
- if (hist->dma_ch)
+ if (!IS_ERR(hist->dma_ch))
dma_release_channel(hist->dma_ch);
+ kfree(hist_cfg);
}
return ret;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 47cbc7e..aad0bc3 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1067,4 +1067,6 @@ void omap3isp_stat_cleanup(struct ispstat *stat)
mutex_destroy(&stat->ioctl_lock);
isp_stat_bufs_free(stat);
kfree(stat->buf);
+ kfree(stat->priv);
+ kfree(stat->recover_priv);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management
2017-02-20 15:22 ` [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management Sakari Ailus
@ 2017-02-28 13:12 ` Laurent Pinchart
2017-02-28 13:21 ` Sakari Ailus
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 13:12 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Monday 20 Feb 2017 17:22:17 Sakari Ailus wrote:
> devm functions are fine for managing resources that are directly related
> to the device at hand and that have no other dependencies. However, a
> process holding a file handle to a device created by a driver for a device
> may result in the file handle left behind after the device is long gone.
> This will result in accessing released (and potentially reallocated)
> memory.
>
> Instead, manage the memory resources in the driver. Releasing the
> resources can be later on bound to e.g. by releasing a reference.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/omap3isp/isp.c | 18 ++++++++++++------
> drivers/media/platform/omap3isp/isph3a_aewb.c | 24 +++++++++++++++++-------
> drivers/media/platform/omap3isp/isph3a_af.c | 24 +++++++++++++++++-------
> drivers/media/platform/omap3isp/isphist.c | 11 +++++++----
> drivers/media/platform/omap3isp/ispstat.c | 2 ++
> 5 files changed, 55 insertions(+), 24 deletions(-)
[snip]
> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index a4ed5d1..9e448c6 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -476,9 +476,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;
>
> - 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;
>
> @@ -500,7 +500,7 @@ int omap3isp_hist_init(struct isp_device *isp)
> if (IS_ERR(hist->dma_ch)) {
> ret = PTR_ERR(hist->dma_ch);
> if (ret == -EPROBE_DEFER)
> - return ret;
> + goto err;
>
> hist->dma_ch = NULL;
> dev_warn(isp->dev,
> @@ -516,9 +516,12 @@ int omap3isp_hist_init(struct isp_device *isp)
> hist->event_type = V4L2_EVENT_OMAP3ISP_HIST;
>
> ret = omap3isp_stat_init(hist, "histogram", &hist_subdev_ops);
> +
> +err:
> if (ret) {
> - if (hist->dma_ch)
> + if (!IS_ERR(hist->dma_ch))
I think this change is wrong. dma_ch is initialize to NULL by kzalloc(). You
will end up calling dma_release_channel() on a NULL channel if
omap3isp_stat_init() fails and HIST_CONFIG_DMA is false. The check should be
if (!IS_ERR_OR_NULL(hist->dma_ch))
Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> dma_release_channel(hist->dma_ch);
> + kfree(hist_cfg);
> }
>
> return ret;
[snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management
2017-02-28 13:12 ` Laurent Pinchart
@ 2017-02-28 13:21 ` Sakari Ailus
0 siblings, 0 replies; 18+ messages in thread
From: Sakari Ailus @ 2017-02-28 13:21 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Monday 20 Feb 2017 17:22:17 Sakari Ailus wrote:
...
>> @@ -516,9 +516,12 @@ int omap3isp_hist_init(struct isp_device *isp)
>> hist->event_type = V4L2_EVENT_OMAP3ISP_HIST;
>>
>> ret = omap3isp_stat_init(hist, "histogram", &hist_subdev_ops);
>> +
>> +err:
>> if (ret) {
>> - if (hist->dma_ch)
>> + if (!IS_ERR(hist->dma_ch))
>
> I think this change is wrong. dma_ch is initialize to NULL by kzalloc(). You
> will end up calling dma_release_channel() on a NULL channel if
> omap3isp_stat_init() fails and HIST_CONFIG_DMA is false. The check should be
>
> if (!IS_ERR_OR_NULL(hist->dma_ch))
Good catch! I'll fix that.
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thanks!
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
2017-02-20 15:22 ` [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management Sakari Ailus
@ 2017-02-20 15:22 ` Sakari Ailus
2017-02-28 13:42 ` Laurent Pinchart
2017-02-20 15:22 ` [PATCH 3/6] omap3isp: Remove misleading comment Sakari Ailus
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-20 15:22 UTC (permalink / raw)
To: linux-media
video_unregister_device() can be called on a never or an already
unregistered device. Drop the redundant check.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/ispvideo.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7..9e9b18c 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1495,6 +1495,5 @@ int omap3isp_video_register(struct isp_video *video, struct v4l2_device *vdev)
void omap3isp_video_unregister(struct isp_video *video)
{
- if (video_is_registered(&video->video))
- video_unregister_device(&video->video);
+ video_unregister_device(&video->video);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally
2017-02-20 15:22 ` [PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally Sakari Ailus
@ 2017-02-28 13:42 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 13:42 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Monday 20 Feb 2017 17:22:18 Sakari Ailus wrote:
> video_unregister_device() can be called on a never or an already
> unregistered device. Drop the redundant check.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 218e6d7..9e9b18c 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1495,6 +1495,5 @@ int omap3isp_video_register(struct isp_video *video,
> struct v4l2_device *vdev)
>
> void omap3isp_video_unregister(struct isp_video *video)
> {
> - if (video_is_registered(&video->video))
> - video_unregister_device(&video->video);
> + video_unregister_device(&video->video);
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/6] omap3isp: Remove misleading comment
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
2017-02-20 15:22 ` [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management Sakari Ailus
2017-02-20 15:22 ` [PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally Sakari Ailus
@ 2017-02-20 15:22 ` Sakari Ailus
2017-02-28 13:42 ` Laurent Pinchart
2017-02-20 15:22 ` [PATCH 4/6] omap3isp: Disable streaming at driver unbind time Sakari Ailus
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-20 15:22 UTC (permalink / raw)
To: linux-media
The intent of the check the comment is related to is to ensure we are
streaming --- still. Not that streaming wouldn't be enabled yet. Remove
it.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/ispvideo.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 9e9b18c..a3ca2a4 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1205,7 +1205,6 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
mutex_lock(&video->stream_lock);
- /* Make sure we're not streaming yet. */
mutex_lock(&video->queue_lock);
streaming = vb2_is_streaming(&vfh->queue);
mutex_unlock(&video->queue_lock);
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/6] omap3isp: Remove misleading comment
2017-02-20 15:22 ` [PATCH 3/6] omap3isp: Remove misleading comment Sakari Ailus
@ 2017-02-28 13:42 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 13:42 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Monday 20 Feb 2017 17:22:19 Sakari Ailus wrote:
> The intent of the check the comment is related to is to ensure we are
> streaming --- still. Not that streaming wouldn't be enabled yet. Remove
> it.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 9e9b18c..a3ca2a4 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1205,7 +1205,6 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type)
>
> mutex_lock(&video->stream_lock);
>
> - /* Make sure we're not streaming yet. */
> mutex_lock(&video->queue_lock);
> streaming = vb2_is_streaming(&vfh->queue);
> mutex_unlock(&video->queue_lock);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/6] omap3isp: Disable streaming at driver unbind time
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
` (2 preceding siblings ...)
2017-02-20 15:22 ` [PATCH 3/6] omap3isp: Remove misleading comment Sakari Ailus
@ 2017-02-20 15:22 ` Sakari Ailus
2017-02-28 13:51 ` Laurent Pinchart
2017-02-20 15:22 ` [PATCH 5/6] media: Remove useless curly braces and parentheses Sakari Ailus
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-20 15:22 UTC (permalink / raw)
To: linux-media
Once the driver is unbound accessing the hardware is not allowed anymore.
Due to this, disable streaming when the device driver is unbound. The
states of the associated objects related to Media controller and videobuf2
frameworks are updated as well, just like if the application disabled
streaming explicitly.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/ispvideo.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index a3ca2a4..c04d357 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1191,22 +1191,17 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
}
static int
-isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+__isp_video_streamoff(struct isp_video *video)
{
- struct isp_video_fh *vfh = to_isp_video_fh(fh);
- struct isp_video *video = video_drvdata(file);
struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
enum isp_pipeline_state state;
unsigned int streaming;
unsigned long flags;
- if (type != video->type)
- return -EINVAL;
-
mutex_lock(&video->stream_lock);
mutex_lock(&video->queue_lock);
- streaming = vb2_is_streaming(&vfh->queue);
+ streaming = video->queue && vb2_is_streaming(video->queue);
mutex_unlock(&video->queue_lock);
if (!streaming)
@@ -1229,7 +1224,7 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
omap3isp_video_cancel_stream(video);
mutex_lock(&video->queue_lock);
- vb2_streamoff(&vfh->queue, type);
+ vb2_streamoff(video->queue, video->queue->type);
mutex_unlock(&video->queue_lock);
video->queue = NULL;
video->error = false;
@@ -1245,6 +1240,17 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
}
static int
+isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+{
+ struct isp_video *video = video_drvdata(file);
+
+ if (type != video->type)
+ return -EINVAL;
+
+ return __isp_video_streamoff(video);
+}
+
+static int
isp_video_enum_input(struct file *file, void *fh, struct v4l2_input *input)
{
if (input->index > 0)
@@ -1494,5 +1500,6 @@ int omap3isp_video_register(struct isp_video *video, struct v4l2_device *vdev)
void omap3isp_video_unregister(struct isp_video *video)
{
+ __isp_video_streamoff(video);
video_unregister_device(&video->video);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time
2017-02-20 15:22 ` [PATCH 4/6] omap3isp: Disable streaming at driver unbind time Sakari Ailus
@ 2017-02-28 13:51 ` Laurent Pinchart
2017-02-28 14:00 ` Sakari Ailus
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 13:51 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
> Once the driver is unbound accessing the hardware is not allowed anymore.
> Due to this, disable streaming when the device driver is unbound. The
> states of the associated objects related to Media controller and videobuf2
> frameworks are updated as well, just like if the application disabled
> streaming explicitly.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
This looks mostly good to me, although I'm a bit concerned about race
conditions related to buffer handling. I don't think this patch introduces any
new one though, so
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
We'll have to go through buffer management at some point in the near future,
including from a V4L2 API point of view I think.
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index a3ca2a4..c04d357 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1191,22 +1191,17 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type)
> }
>
> static int
> -isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
> +__isp_video_streamoff(struct isp_video *video)
> {
> - struct isp_video_fh *vfh = to_isp_video_fh(fh);
> - struct isp_video *video = video_drvdata(file);
> struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
> enum isp_pipeline_state state;
> unsigned int streaming;
> unsigned long flags;
>
> - if (type != video->type)
> - return -EINVAL;
> -
> mutex_lock(&video->stream_lock);
>
> mutex_lock(&video->queue_lock);
> - streaming = vb2_is_streaming(&vfh->queue);
> + streaming = video->queue && vb2_is_streaming(video->queue);
> mutex_unlock(&video->queue_lock);
>
> if (!streaming)
> @@ -1229,7 +1224,7 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type) omap3isp_video_cancel_stream(video);
>
> mutex_lock(&video->queue_lock);
> - vb2_streamoff(&vfh->queue, type);
> + vb2_streamoff(video->queue, video->queue->type);
> mutex_unlock(&video->queue_lock);
> video->queue = NULL;
> video->error = false;
> @@ -1245,6 +1240,17 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type) }
>
> static int
> +isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
> +{
> + struct isp_video *video = video_drvdata(file);
> +
> + if (type != video->type)
> + return -EINVAL;
> +
> + return __isp_video_streamoff(video);
> +}
> +
> +static int
> isp_video_enum_input(struct file *file, void *fh, struct v4l2_input *input)
> {
> if (input->index > 0)
> @@ -1494,5 +1500,6 @@ int omap3isp_video_register(struct isp_video *video,
> struct v4l2_device *vdev)
>
> void omap3isp_video_unregister(struct isp_video *video)
> {
> + __isp_video_streamoff(video);
> video_unregister_device(&video->video);
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time
2017-02-28 13:51 ` Laurent Pinchart
@ 2017-02-28 14:00 ` Sakari Ailus
2017-02-28 14:53 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-28 14:00 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
>> Once the driver is unbound accessing the hardware is not allowed anymore.
>> Due to this, disable streaming when the device driver is unbound. The
>> states of the associated objects related to Media controller and videobuf2
>> frameworks are updated as well, just like if the application disabled
>> streaming explicitly.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> This looks mostly good to me, although I'm a bit concerned about race
> conditions related to buffer handling. I don't think this patch introduces any
> new one though, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> We'll have to go through buffer management at some point in the near future,
> including from a V4L2 API point of view I think.
Thanks for the review!
Are you happy with me sending a pull request on the set, or would you
prefer to pick the omap3isp patches? In the latter case I'll send a fix
for the issue in the first patch.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time
2017-02-28 14:00 ` Sakari Ailus
@ 2017-02-28 14:53 ` Laurent Pinchart
2017-02-24 13:13 ` Sakari Ailus
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 14:53 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
On Tuesday 28 Feb 2017 16:00:01 Sakari Ailus wrote:
> > On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
> >> Once the driver is unbound accessing the hardware is not allowed anymore.
> >> Due to this, disable streaming when the device driver is unbound. The
> >> states of the associated objects related to Media controller and
> >> videobuf2 frameworks are updated as well, just like if the application
> >> disabled streaming explicitly.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > This looks mostly good to me, although I'm a bit concerned about race
> > conditions related to buffer handling. I don't think this patch introduces
> > any new one though, so
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > We'll have to go through buffer management at some point in the near
> > future, including from a V4L2 API point of view I think.
>
> Thanks for the review!
>
> Are you happy with me sending a pull request on the set, or would you
> prefer to pick the omap3isp patches? In the latter case I'll send a fix
> for the issue in the first patch.
Feel free to send a pull request, I don't have anything conflicting queued for
v4.12.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time
2017-02-28 14:53 ` Laurent Pinchart
@ 2017-02-24 13:13 ` Sakari Ailus
0 siblings, 0 replies; 18+ messages in thread
From: Sakari Ailus @ 2017-02-24 13:13 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
On Tue, Feb 28, 2017 at 04:53:11PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tuesday 28 Feb 2017 16:00:01 Sakari Ailus wrote:
> > > On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
> > >> Once the driver is unbound accessing the hardware is not allowed anymore.
> > >> Due to this, disable streaming when the device driver is unbound. The
> > >> states of the associated objects related to Media controller and
> > >> videobuf2 frameworks are updated as well, just like if the application
> > >> disabled streaming explicitly.
> > >>
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >
> > > This looks mostly good to me, although I'm a bit concerned about race
> > > conditions related to buffer handling. I don't think this patch introduces
> > > any new one though, so
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > We'll have to go through buffer management at some point in the near
> > > future, including from a V4L2 API point of view I think.
> >
> > Thanks for the review!
> >
> > Are you happy with me sending a pull request on the set, or would you
> > prefer to pick the omap3isp patches? In the latter case I'll send a fix
> > for the issue in the first patch.
>
> Feel free to send a pull request, I don't have anything conflicting queued for
> v4.12.
Ack. I'll send a pull request then.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/6] media: Remove useless curly braces and parentheses
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
` (3 preceding siblings ...)
2017-02-20 15:22 ` [PATCH 4/6] omap3isp: Disable streaming at driver unbind time Sakari Ailus
@ 2017-02-20 15:22 ` Sakari Ailus
2017-02-28 13:42 ` Laurent Pinchart
2017-02-20 15:22 ` [PATCH 6/6] media: devnode: Rename mdev argument as devnode Sakari Ailus
2017-02-20 20:37 ` [PATCH 0/6] Media controller and omap3isp cleanups Hans Verkuil
6 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-20 15:22 UTC (permalink / raw)
To: linux-media
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 760e3e4..c51e2e5 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -591,9 +591,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
&entity->pads[i].graph_obj);
/* invoke entity_notify callbacks */
- list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
- (notify)->notify(entity, notify->notify_data);
- }
+ list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
+ notify->notify(entity, notify->notify_data);
if (mdev->entity_internal_idx_max
>= mdev->pm_count_walk.ent_enum.idx_max) {
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] media: Remove useless curly braces and parentheses
2017-02-20 15:22 ` [PATCH 5/6] media: Remove useless curly braces and parentheses Sakari Ailus
@ 2017-02-28 13:42 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 13:42 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Monday 20 Feb 2017 17:22:21 Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 760e3e4..c51e2e5 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -591,9 +591,8 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, &entity->pads[i].graph_obj);
>
> /* invoke entity_notify callbacks */
> - list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
> - (notify)->notify(entity, notify->notify_data);
> - }
> + list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
> + notify->notify(entity, notify->notify_data);
>
> if (mdev->entity_internal_idx_max
>
> >= mdev->pm_count_walk.ent_enum.idx_max) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/6] media: devnode: Rename mdev argument as devnode
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
` (4 preceding siblings ...)
2017-02-20 15:22 ` [PATCH 5/6] media: Remove useless curly braces and parentheses Sakari Ailus
@ 2017-02-20 15:22 ` Sakari Ailus
2017-02-28 13:42 ` Laurent Pinchart
2017-02-20 20:37 ` [PATCH 0/6] Media controller and omap3isp cleanups Hans Verkuil
6 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2017-02-20 15:22 UTC (permalink / raw)
To: linux-media
Historically, mdev argument name was being used on both struct
media_device and struct media_devnode. Recently most occurrences of mdev
referring to struct media_devnode were replaced by devnode, which makes
more sense. Fix the last remaining occurrence.
Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c51e2e5..fce91b5 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -537,9 +537,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
* Registration/unregistration
*/
-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)
{
- dev_dbg(mdev->parent, "Media device released\n");
+ dev_dbg(devnode->parent, "Media device released\n");
}
/**
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 6/6] media: devnode: Rename mdev argument as devnode
2017-02-20 15:22 ` [PATCH 6/6] media: devnode: Rename mdev argument as devnode Sakari Ailus
@ 2017-02-28 13:42 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2017-02-28 13:42 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Monday 20 Feb 2017 17:22:22 Sakari Ailus wrote:
> Historically, mdev argument name was being used on both struct
> media_device and struct media_devnode. Recently most occurrences of mdev
> referring to struct media_devnode were replaced by devnode, which makes
> more sense. Fix the last remaining occurrence.
>
> Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c51e2e5..fce91b5 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -537,9 +537,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
> * Registration/unregistration
> */
>
> -static void media_device_release(struct media_devnode *mdev)
> +static void media_device_release(struct media_devnode *devnode)
> {
> - dev_dbg(mdev->parent, "Media device released\n");
> + dev_dbg(devnode->parent, "Media device released\n");
> }
>
> /**
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] Media controller and omap3isp cleanups
2017-02-20 15:22 [PATCH 0/6] Media controller and omap3isp cleanups Sakari Ailus
` (5 preceding siblings ...)
2017-02-20 15:22 ` [PATCH 6/6] media: devnode: Rename mdev argument as devnode Sakari Ailus
@ 2017-02-20 20:37 ` Hans Verkuil
6 siblings, 0 replies; 18+ messages in thread
From: Hans Verkuil @ 2017-02-20 20:37 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Thanks!
Hans
On 02/20/2017 07:22 AM, Sakari Ailus wrote:
> Hi all,
>
> These patches prepare for media device refcounting but do not begin those
> changes yet. They're worthwhile on their own merits.
>
^ permalink raw reply [flat|nested] 18+ messages in thread