Re: [PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Kieran Bingham]

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

Regards

Kieran

Re: [PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Sakari Ailus]

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 +, 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 
> >>>
> >>> This looks good to me,
> >>>
> >>> Acked-by: Laurent Pinchart 
> >>>
> >>> 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:



-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Kieran Bingham]

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 +, 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 
>>>
>>> This looks good to me,
>>>
>>> Acked-by: Laurent Pinchart 
>>>
>>> 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

Re: [PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Sakari Ailus]

Hi Kieran,

Thanks for the patch!

On Tue, Jan 03, 2017 at 05:05:58PM +, 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 
> > 
> > This looks good to me,
> > 
> > Acked-by: Laurent Pinchart 
> > 
> > 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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Kieran Bingham]

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 
> 
> This looks good to me,
> 
> Acked-by: Laurent Pinchart 
> 
> 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 = >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))) {
> 

Re: [PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Laurent Pinchart]

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 

This looks good to me,

Acked-by: Laurent Pinchart 

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

-- 
Regards,

Laurent Pinchart

[PATCH] media: entity: Catch unbalanced media_pipeline_stop calls

[Kieran Bingham]

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