media: entity: Catch unbalanced media_pipeline_stop calls

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(-)
  

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))) {
  

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))) {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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.
  

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
  

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/>
  

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