diff mbox

media: entity: Catch unbalanced media_pipeline_stop calls

Message ID 1483449131-18075-1-git-send-email-kieran.bingham+renesas@ideasonboard.com (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Kieran Bingham Jan. 3, 2017, 1:12 p.m. UTC
Drivers must not perform unbalanced calls to stop the entity pipeline,
however if they do they will fault in the core media code, as the
entity->pipe will be set as NULL. We handle this gracefully in the core
with a WARN for the developer.

Replace the erroneous check on zero streaming counts, with a check on
NULL pipe elements instead, as this is the symptom of unbalanced
media_pipeline_stop calls.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/media-entity.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Jan. 3, 2017, 1:36 p.m. UTC | #1
Hi Kieran,

Thank you for the patch.

On Tuesday 03 Jan 2017 13:12:11 Kieran Bingham wrote:
> Drivers must not perform unbalanced calls to stop the entity pipeline,
> however if they do they will fault in the core media code, as the
> entity->pipe will be set as NULL. We handle this gracefully in the core
> with a WARN for the developer.
> 
> Replace the erroneous check on zero streaming counts, with a check on
> NULL pipe elements instead, as this is the symptom of unbalanced
> media_pipeline_stop calls.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

This looks good to me,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll let Sakari review and merge the patch.

> ---
>  drivers/media/media-entity.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index caa13e6f09f5..cb1fb2c17f85 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -534,8 +534,13 @@ void __media_pipeline_stop(struct media_entity *entity)
> struct media_graph *graph = &entity->pipe->graph;
>  	struct media_pipeline *pipe = entity->pipe;
> 
> +	/*
> +	 * If the following check fails, the driver has performed an
> +	 * unbalanced call to media_pipeline_stop()
> +	 */
> +	if (WARN_ON(!pipe))
> +		return;
> 
> -	WARN_ON(!pipe->streaming_count);
>  	media_graph_walk_start(graph, entity);
> 
>  	while ((entity = media_graph_walk_next(graph))) {
Kieran Bingham Jan. 3, 2017, 5:05 p.m. UTC | #2
On 03/01/17 13:36, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tuesday 03 Jan 2017 13:12:11 Kieran Bingham wrote:
>> Drivers must not perform unbalanced calls to stop the entity pipeline,
>> however if they do they will fault in the core media code, as the
>> entity->pipe will be set as NULL. We handle this gracefully in the core
>> with a WARN for the developer.
>>
>> Replace the erroneous check on zero streaming counts, with a check on
>> NULL pipe elements instead, as this is the symptom of unbalanced
>> media_pipeline_stop calls.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> This looks good to me,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I'll let Sakari review and merge the patch.

Ahh, yes - I forgot to mention, although perhaps it will be obvious for
Sakari - but this patch is based on top of Sakari's pending media
pipeline and graph walk cleanup series :D

--
Regards

Kieran

> 
>> ---
>>  drivers/media/media-entity.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index caa13e6f09f5..cb1fb2c17f85 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -534,8 +534,13 @@ void __media_pipeline_stop(struct media_entity *entity)
>> struct media_graph *graph = &entity->pipe->graph;
>>  	struct media_pipeline *pipe = entity->pipe;
>>
>> +	/*
>> +	 * If the following check fails, the driver has performed an
>> +	 * unbalanced call to media_pipeline_stop()
>> +	 */
>> +	if (WARN_ON(!pipe))
>> +		return;
>>
>> -	WARN_ON(!pipe->streaming_count);
>>  	media_graph_walk_start(graph, entity);
>>
>>  	while ((entity = media_graph_walk_next(graph))) {
>
Sakari Ailus Jan. 4, 2017, 8:57 a.m. UTC | #3
Hi Kieran,

Thanks for the patch!

On Tue, Jan 03, 2017 at 05:05:58PM +0000, Kieran Bingham wrote:
> On 03/01/17 13:36, Laurent Pinchart wrote:
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > On Tuesday 03 Jan 2017 13:12:11 Kieran Bingham wrote:
> >> Drivers must not perform unbalanced calls to stop the entity pipeline,
> >> however if they do they will fault in the core media code, as the
> >> entity->pipe will be set as NULL. We handle this gracefully in the core
> >> with a WARN for the developer.
> >>
> >> Replace the erroneous check on zero streaming counts, with a check on
> >> NULL pipe elements instead, as this is the symptom of unbalanced
> >> media_pipeline_stop calls.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > This looks good to me,
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I'll let Sakari review and merge the patch.
> 
> Ahh, yes - I forgot to mention, although perhaps it will be obvious for
> Sakari - but this patch is based on top of Sakari's pending media
> pipeline and graph walk cleanup series :D

I've applied this on top of the other patches.

It's always good to mention dependencies to other patches, that's very
relevant for reviewers.
Kieran Bingham May 5, 2017, 5:33 p.m. UTC | #4
Hi Sakari,

On 04/01/17 08:57, Sakari Ailus wrote:
> Hi Kieran,
> 
> Thanks for the patch!
> 
> On Tue, Jan 03, 2017 at 05:05:58PM +0000, Kieran Bingham wrote:
>> On 03/01/17 13:36, Laurent Pinchart wrote:
>>> Hi Kieran,
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday 03 Jan 2017 13:12:11 Kieran Bingham wrote:
>>>> Drivers must not perform unbalanced calls to stop the entity pipeline,
>>>> however if they do they will fault in the core media code, as the
>>>> entity->pipe will be set as NULL. We handle this gracefully in the core
>>>> with a WARN for the developer.
>>>>
>>>> Replace the erroneous check on zero streaming counts, with a check on
>>>> NULL pipe elements instead, as this is the symptom of unbalanced
>>>> media_pipeline_stop calls.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>
>>> This looks good to me,
>>>
>>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> I'll let Sakari review and merge the patch.
>>
>> Ahh, yes - I forgot to mention, although perhaps it will be obvious for
>> Sakari - but this patch is based on top of Sakari's pending media
>> pipeline and graph walk cleanup series :D
> 
> I've applied this on top of the other patches.
> 
> It's always good to mention dependencies to other patches, that's very
> relevant for reviewers.

I've just been going through my old branches doing some clean up - and I can't
see that this patch [0] made it to integration anywhere.

Did it get lost?
 It looks like the cleanup series it was based on made it through...

Mauro, perhaps you could pick this one up now ?

Regards

Kieran


[0] https://www.spinics.net/lists/linux-media/msg109715.html
Sakari Ailus May 5, 2017, 9:11 p.m. UTC | #5
Hi Kieran / Mauro,

On Fri, May 05, 2017 at 06:33:22PM +0100, Kieran Bingham wrote:
> Hi Sakari,
> 
> On 04/01/17 08:57, Sakari Ailus wrote:
> > Hi Kieran,
> > 
> > Thanks for the patch!
> > 
> > On Tue, Jan 03, 2017 at 05:05:58PM +0000, Kieran Bingham wrote:
> >> On 03/01/17 13:36, Laurent Pinchart wrote:
> >>> Hi Kieran,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Tuesday 03 Jan 2017 13:12:11 Kieran Bingham wrote:
> >>>> Drivers must not perform unbalanced calls to stop the entity pipeline,
> >>>> however if they do they will fault in the core media code, as the
> >>>> entity->pipe will be set as NULL. We handle this gracefully in the core
> >>>> with a WARN for the developer.
> >>>>
> >>>> Replace the erroneous check on zero streaming counts, with a check on
> >>>> NULL pipe elements instead, as this is the symptom of unbalanced
> >>>> media_pipeline_stop calls.
> >>>>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>
> >>> This looks good to me,
> >>>
> >>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>
> >>> I'll let Sakari review and merge the patch.
> >>
> >> Ahh, yes - I forgot to mention, although perhaps it will be obvious for
> >> Sakari - but this patch is based on top of Sakari's pending media
> >> pipeline and graph walk cleanup series :D
> > 
> > I've applied this on top of the other patches.
> > 
> > It's always good to mention dependencies to other patches, that's very
> > relevant for reviewers.
> 
> I've just been going through my old branches doing some clean up - and I can't
> see that this patch [0] made it to integration anywhere.
> 
> Did it get lost?
>  It looks like the cleanup series it was based on made it through...

What I think happened was that I had applied it to the correct branch BUT I
already had sent a pull request on it. My apologies.

> 
> Mauro, perhaps you could pick this one up now ?

The patchwork link is here:

<URL:https://patchwork.linuxtv.org/patch/38883/>
Kieran Bingham May 6, 2017, 5:23 p.m. UTC | #6
Hi Sakari,

>> Did it get lost?
>>  It looks like the cleanup series it was based on made it through...
> 
> What I think happened was that I had applied it to the correct branch BUT I
> already had sent a pull request on it. My apologies.

No worries - thanks for checking!

> 
>>
>> Mauro, perhaps you could pick this one up now ?
> 
> The patchwork link is here:
> 
> <URL:https://patchwork.linuxtv.org/patch/38883/>

Regards

Kieran
diff mbox

Patch

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index caa13e6f09f5..cb1fb2c17f85 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -534,8 +534,13 @@  void __media_pipeline_stop(struct media_entity *entity)
 	struct media_graph *graph = &entity->pipe->graph;
 	struct media_pipeline *pipe = entity->pipe;
 
+	/*
+	 * If the following check fails, the driver has performed an
+	 * unbalanced call to media_pipeline_stop()
+	 */
+	if (WARN_ON(!pipe))
+		return;
 
-	WARN_ON(!pipe->streaming_count);
 	media_graph_walk_start(graph, entity);
 
 	while ((entity = media_graph_walk_next(graph))) {