* [PATCH] media: Media Controller fix to not let stream_count go negative
@ 2016-01-30 20:10 Shuah Khan
2016-02-02 22:53 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Shuah Khan @ 2016-01-30 20:10 UTC (permalink / raw)
To: mchehab; +Cc: Shuah Khan, linux-media, linux-kernel
Change media_entity_pipeline_stop() to not decrement
stream_count of an inactive media pipeline. Doing so,
results in preventing starting the pipeline.
Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
drivers/media/media-entity.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e89d85a..f2e4360 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -452,9 +452,12 @@ error:
media_entity_graph_walk_start(graph, entity_err);
while ((entity_err = media_entity_graph_walk_next(graph))) {
- entity_err->stream_count--;
- if (entity_err->stream_count == 0)
- entity_err->pipe = NULL;
+ /* don't let the stream_count go negative */
+ if (entity->stream_count > 0) {
+ entity_err->stream_count--;
+ if (entity_err->stream_count == 0)
+ entity_err->pipe = NULL;
+ }
/*
* We haven't increased stream_count further than this
@@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
media_entity_graph_walk_start(graph, entity);
while ((entity = media_entity_graph_walk_next(graph))) {
- entity->stream_count--;
- if (entity->stream_count == 0)
- entity->pipe = NULL;
+ /* don't let the stream_count go negative */
+ if (entity->stream_count > 0) {
+ entity->stream_count--;
+ if (entity->stream_count == 0)
+ entity->pipe = NULL;
+ }
}
if (!--pipe->streaming_count)
--
2.5.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] media: Media Controller fix to not let stream_count go negative
2016-01-30 20:10 [PATCH] media: Media Controller fix to not let stream_count go negative Shuah Khan
@ 2016-02-02 22:53 ` Sakari Ailus
2016-02-02 23:04 ` Shuah Khan
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2016-02-02 22:53 UTC (permalink / raw)
To: Shuah Khan; +Cc: mchehab, linux-media, linux-kernel
Hi Shuah,
On Sat, Jan 30, 2016 at 01:10:52PM -0700, Shuah Khan wrote:
> Change media_entity_pipeline_stop() to not decrement
> stream_count of an inactive media pipeline. Doing so,
> results in preventing starting the pipeline.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
> drivers/media/media-entity.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e89d85a..f2e4360 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -452,9 +452,12 @@ error:
> media_entity_graph_walk_start(graph, entity_err);
>
> while ((entity_err = media_entity_graph_walk_next(graph))) {
> - entity_err->stream_count--;
> - if (entity_err->stream_count == 0)
> - entity_err->pipe = NULL;
> + /* don't let the stream_count go negative */
> + if (entity->stream_count > 0) {
> + entity_err->stream_count--;
> + if (entity_err->stream_count == 0)
> + entity_err->pipe = NULL;
> + }
>
> /*
> * We haven't increased stream_count further than this
> @@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
> media_entity_graph_walk_start(graph, entity);
>
> while ((entity = media_entity_graph_walk_next(graph))) {
> - entity->stream_count--;
> - if (entity->stream_count == 0)
> - entity->pipe = NULL;
> + /* don't let the stream_count go negative */
> + if (entity->stream_count > 0) {
> + entity->stream_count--;
> + if (entity->stream_count == 0)
> + entity->pipe = NULL;
> + }
> }
>
> if (!--pipe->streaming_count)
Have you seen issues with a certain driver, for instance?
In the original design the streaming count is really a count --- streaming
starts when count becomes non-zero, and stops when it reaches zero again.
The calls to media_entity_pipeline_start() and media_entity_pipeline_stop()
should thus always be balanced. I'm fine with the patch, but the framework
should shout loud when the count would be decremented when it's zero.
That was some four or more years ago. I have to say I really haven't been
able to see good reasons for making this a count --- rather what's needed is
to mark the entity as busy so that its link configuration isn't touched. The
request API is a completely different matter then.
Such change would require more discussion IMHO.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: Media Controller fix to not let stream_count go negative
2016-02-02 22:53 ` Sakari Ailus
@ 2016-02-02 23:04 ` Shuah Khan
2016-02-02 23:09 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Shuah Khan @ 2016-02-02 23:04 UTC (permalink / raw)
To: Sakari Ailus; +Cc: mchehab, linux-media, linux-kernel, Shuah Khan
On 02/02/2016 03:53 PM, Sakari Ailus wrote:
> Hi Shuah,
>
> On Sat, Jan 30, 2016 at 01:10:52PM -0700, Shuah Khan wrote:
>> Change media_entity_pipeline_stop() to not decrement
>> stream_count of an inactive media pipeline. Doing so,
>> results in preventing starting the pipeline.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>> drivers/media/media-entity.c | 18 ++++++++++++------
>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index e89d85a..f2e4360 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -452,9 +452,12 @@ error:
>> media_entity_graph_walk_start(graph, entity_err);
>>
>> while ((entity_err = media_entity_graph_walk_next(graph))) {
>> - entity_err->stream_count--;
>> - if (entity_err->stream_count == 0)
>> - entity_err->pipe = NULL;
>> + /* don't let the stream_count go negative */
>> + if (entity->stream_count > 0) {
>> + entity_err->stream_count--;
>> + if (entity_err->stream_count == 0)
>> + entity_err->pipe = NULL;
>> + }
>>
>> /*
>> * We haven't increased stream_count further than this
>> @@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>> media_entity_graph_walk_start(graph, entity);
>>
>> while ((entity = media_entity_graph_walk_next(graph))) {
>> - entity->stream_count--;
>> - if (entity->stream_count == 0)
>> - entity->pipe = NULL;
>> + /* don't let the stream_count go negative */
>> + if (entity->stream_count > 0) {
>> + entity->stream_count--;
>> + if (entity->stream_count == 0)
>> + entity->pipe = NULL;
>> + }
>> }
>>
>> if (!--pipe->streaming_count)
>
> Have you seen issues with a certain driver, for instance?
>
> In the original design the streaming count is really a count --- streaming
> starts when count becomes non-zero, and stops when it reaches zero again.
>
> The calls to media_entity_pipeline_start() and media_entity_pipeline_stop()
> should thus always be balanced. I'm fine with the patch, but the framework
> should shout loud when the count would be decremented when it's zero.
>
> That was some four or more years ago. I have to say I really haven't been
> able to see good reasons for making this a count --- rather what's needed is
> to mark the entity as busy so that its link configuration isn't touched. The
> request API is a completely different matter then.
>
> Such change would require more discussion IMHO.
>
Yes. I found problems with au0828 and ALSA
media controller use-case. It got into a state
where pipeline was essentially locked in a state.
It took some work to debug and find that the
stream_count was negative.
Granted the start and stop should be balanced, however,
the media_entity_pipeline_stop() still needs to protect
the stream_count from going negative.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: Media Controller fix to not let stream_count go negative
2016-02-02 23:04 ` Shuah Khan
@ 2016-02-02 23:09 ` Sakari Ailus
2016-02-02 23:18 ` Shuah Khan
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2016-02-02 23:09 UTC (permalink / raw)
To: Shuah Khan; +Cc: mchehab, linux-media, linux-kernel
Hi Shuah,
On Tue, Feb 02, 2016 at 04:04:50PM -0700, Shuah Khan wrote:
> On 02/02/2016 03:53 PM, Sakari Ailus wrote:
> > Hi Shuah,
> >
> > On Sat, Jan 30, 2016 at 01:10:52PM -0700, Shuah Khan wrote:
> >> Change media_entity_pipeline_stop() to not decrement
> >> stream_count of an inactive media pipeline. Doing so,
> >> results in preventing starting the pipeline.
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >> ---
> >> drivers/media/media-entity.c | 18 ++++++++++++------
> >> 1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> >> index e89d85a..f2e4360 100644
> >> --- a/drivers/media/media-entity.c
> >> +++ b/drivers/media/media-entity.c
> >> @@ -452,9 +452,12 @@ error:
> >> media_entity_graph_walk_start(graph, entity_err);
> >>
> >> while ((entity_err = media_entity_graph_walk_next(graph))) {
> >> - entity_err->stream_count--;
> >> - if (entity_err->stream_count == 0)
> >> - entity_err->pipe = NULL;
> >> + /* don't let the stream_count go negative */
> >> + if (entity->stream_count > 0) {
> >> + entity_err->stream_count--;
> >> + if (entity_err->stream_count == 0)
> >> + entity_err->pipe = NULL;
> >> + }
> >>
> >> /*
> >> * We haven't increased stream_count further than this
> >> @@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
> >> media_entity_graph_walk_start(graph, entity);
> >>
> >> while ((entity = media_entity_graph_walk_next(graph))) {
> >> - entity->stream_count--;
> >> - if (entity->stream_count == 0)
> >> - entity->pipe = NULL;
> >> + /* don't let the stream_count go negative */
> >> + if (entity->stream_count > 0) {
> >> + entity->stream_count--;
> >> + if (entity->stream_count == 0)
> >> + entity->pipe = NULL;
> >> + }
> >> }
> >>
> >> if (!--pipe->streaming_count)
> >
> > Have you seen issues with a certain driver, for instance?
> >
> > In the original design the streaming count is really a count --- streaming
> > starts when count becomes non-zero, and stops when it reaches zero again.
> >
> > The calls to media_entity_pipeline_start() and media_entity_pipeline_stop()
> > should thus always be balanced. I'm fine with the patch, but the framework
> > should shout loud when the count would be decremented when it's zero.
> >
> > That was some four or more years ago. I have to say I really haven't been
> > able to see good reasons for making this a count --- rather what's needed is
> > to mark the entity as busy so that its link configuration isn't touched. The
> > request API is a completely different matter then.
> >
> > Such change would require more discussion IMHO.
> >
>
> Yes. I found problems with au0828 and ALSA
> media controller use-case. It got into a state
> where pipeline was essentially locked in a state.
> It took some work to debug and find that the
> stream_count was negative.
>
> Granted the start and stop should be balanced, however,
> the media_entity_pipeline_stop() still needs to protect
> the stream_count from going negative.
Agreed; I'm fine with the patch, except I think it should complain when that
happens rather than silently ignoring it.
WARN_ON(1) perhaps? Or how about dev_err() loudly complaining about a driver
bug?
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: Media Controller fix to not let stream_count go negative
2016-02-02 23:09 ` Sakari Ailus
@ 2016-02-02 23:18 ` Shuah Khan
0 siblings, 0 replies; 5+ messages in thread
From: Shuah Khan @ 2016-02-02 23:18 UTC (permalink / raw)
To: Sakari Ailus, mchehab; +Cc: linux-media, linux-kernel, Shuah Khan
On 02/02/2016 04:09 PM, Sakari Ailus wrote:
> Hi Shuah,
>
> On Tue, Feb 02, 2016 at 04:04:50PM -0700, Shuah Khan wrote:
>> On 02/02/2016 03:53 PM, Sakari Ailus wrote:
>>> Hi Shuah,
>>>
>>> On Sat, Jan 30, 2016 at 01:10:52PM -0700, Shuah Khan wrote:
>>>> Change media_entity_pipeline_stop() to not decrement
>>>> stream_count of an inactive media pipeline. Doing so,
>>>> results in preventing starting the pipeline.
>>>>
>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>> ---
>>>> drivers/media/media-entity.c | 18 ++++++++++++------
>>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>>> index e89d85a..f2e4360 100644
>>>> --- a/drivers/media/media-entity.c
>>>> +++ b/drivers/media/media-entity.c
>>>> @@ -452,9 +452,12 @@ error:
>>>> media_entity_graph_walk_start(graph, entity_err);
>>>>
>>>> while ((entity_err = media_entity_graph_walk_next(graph))) {
>>>> - entity_err->stream_count--;
>>>> - if (entity_err->stream_count == 0)
>>>> - entity_err->pipe = NULL;
>>>> + /* don't let the stream_count go negative */
>>>> + if (entity->stream_count > 0) {
>>>> + entity_err->stream_count--;
>>>> + if (entity_err->stream_count == 0)
>>>> + entity_err->pipe = NULL;
>>>> + }
>>>>
>>>> /*
>>>> * We haven't increased stream_count further than this
>>>> @@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>>>> media_entity_graph_walk_start(graph, entity);
>>>>
>>>> while ((entity = media_entity_graph_walk_next(graph))) {
>>>> - entity->stream_count--;
>>>> - if (entity->stream_count == 0)
>>>> - entity->pipe = NULL;
>>>> + /* don't let the stream_count go negative */
>>>> + if (entity->stream_count > 0) {
>>>> + entity->stream_count--;
>>>> + if (entity->stream_count == 0)
>>>> + entity->pipe = NULL;
>>>> + }
>>>> }
>>>>
>>>> if (!--pipe->streaming_count)
>>>
>>> Have you seen issues with a certain driver, for instance?
>>>
>>> In the original design the streaming count is really a count --- streaming
>>> starts when count becomes non-zero, and stops when it reaches zero again.
>>>
>>> The calls to media_entity_pipeline_start() and media_entity_pipeline_stop()
>>> should thus always be balanced. I'm fine with the patch, but the framework
>>> should shout loud when the count would be decremented when it's zero.
>>>
>>> That was some four or more years ago. I have to say I really haven't been
>>> able to see good reasons for making this a count --- rather what's needed is
>>> to mark the entity as busy so that its link configuration isn't touched. The
>>> request API is a completely different matter then.
>>>
>>> Such change would require more discussion IMHO.
>>>
>>
>> Yes. I found problems with au0828 and ALSA
>> media controller use-case. It got into a state
>> where pipeline was essentially locked in a state.
>> It took some work to debug and find that the
>> stream_count was negative.
>>
>> Granted the start and stop should be balanced, however,
>> the media_entity_pipeline_stop() still needs to protect
>> the stream_count from going negative.
>
> Agreed; I'm fine with the patch, except I think it should complain when that
> happens rather than silently ignoring it.
>
> WARN_ON(1) perhaps? Or how about dev_err() loudly complaining about a driver
> bug?
>
Yes. I can add a WARN_ON(1). Looks like we are using
it in other places in media-entity.c
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-02 23:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-30 20:10 [PATCH] media: Media Controller fix to not let stream_count go negative Shuah Khan
2016-02-02 22:53 ` Sakari Ailus
2016-02-02 23:04 ` Shuah Khan
2016-02-02 23:09 ` Sakari Ailus
2016-02-02 23:18 ` Shuah Khan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox