[PATCH v4] media: vimc: streamer: if kthread_stop fails, ignore the error

[PATCH v4] media: vimc: streamer: if kthread_stop fails, ignore the error

[Dafna Hirschfeld]

Ignore errors returned from kthread_stop since the
vimc subdevices should still be notified that
streaming stopped so they can release the memory for
the streaming, and also kthread should be set to NULL.
kthread_stop can return -EINTR in case the thread
did not yet run. This can happen if userspace calls
streamon and streamoff right after.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
Changes from v3: change the comment to explain when kthread fails

 drivers/media/platform/vimc/vimc-streamer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
index cd6b55433c9e..26ec81b265c4 100644
--- a/drivers/media/platform/vimc/vimc-streamer.c
+++ b/drivers/media/platform/vimc/vimc-streamer.c
@@ -215,9 +215,15 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
 			return 0;
 
 		ret = kthread_stop(stream->kthread);
-		if (ret)
-			return ret;
 
+		/*
+		 * kthread_stop returns -EINTR in cases when streamon was
+		 * immediately followed by streamoff, and the thread didn't had
+		 * a chance to run. Ignore errors to stop the stream in the
+		 * pipeline.
+		 */
+		if (ret)
+			dev_warn(ved->dev, "kthread_stop returned '%d'\n", ret);
 		stream->kthread = NULL;
 
 		vimc_streamer_pipeline_terminate(stream);
-- 
2.17.1

Re: [PATCH v4] media: vimc: streamer: if kthread_stop fails, ignore the error

[Ezequiel Garcia]

Hi Dafna,

Thanks for the patch.

On Sun, 19 Jan 2020 at 09:34, Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Ignore errors returned from kthread_stop since the
> vimc subdevices should still be notified that
> streaming stopped so they can release the memory for
> the streaming, and also kthread should be set to NULL.
> kthread_stop can return -EINTR in case the thread
> did not yet run. This can happen if userspace calls
> streamon and streamoff right after.
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> Changes from v3: change the comment to explain when kthread fails
>

Please keep all the history here, so we'd see the changes
done to v1, v2, v3.

>  drivers/media/platform/vimc/vimc-streamer.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> index cd6b55433c9e..26ec81b265c4 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.c
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -215,9 +215,15 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>                         return 0;
>
>                 ret = kthread_stop(stream->kthread);
> -               if (ret)
> -                       return ret;
>
> +               /*
> +                * kthread_stop returns -EINTR in cases when streamon was
> +                * immediately followed by streamoff, and the thread didn't had
> +                * a chance to run. Ignore errors to stop the stream in the
> +                * pipeline.
> +                */
> +               if (ret)
> +                       dev_warn(ved->dev, "kthread_stop returned '%d'\n", ret);

Is this situation serious enough to deserve warning the user?
If not, perhaps dev_dbg would be better.

Regards,
Ezequiel

Re: [PATCH v4] media: vimc: streamer: if kthread_stop fails, ignore the error

[Helen Koike]

On 1/19/20 10:57 PM, Ezequiel Garcia wrote:
> Hi Dafna,
> 
> Thanks for the patch.
> 
> On Sun, 19 Jan 2020 at 09:34, Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> Ignore errors returned from kthread_stop since the
>> vimc subdevices should still be notified that
>> streaming stopped so they can release the memory for
>> the streaming, and also kthread should be set to NULL.
>> kthread_stop can return -EINTR in case the thread
>> did not yet run. This can happen if userspace calls
>> streamon and streamoff right after.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>> Changes from v3: change the comment to explain when kthread fails
>>
> 
> Please keep all the history here, so we'd see the changes
> done to v1, v2, v3.
> 
>>  drivers/media/platform/vimc/vimc-streamer.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
>> index cd6b55433c9e..26ec81b265c4 100644
>> --- a/drivers/media/platform/vimc/vimc-streamer.c
>> +++ b/drivers/media/platform/vimc/vimc-streamer.c
>> @@ -215,9 +215,15 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
>>                         return 0;
>>
>>                 ret = kthread_stop(stream->kthread);
>> -               if (ret)
>> -                       return ret;
>>

no new line is required here.
The idea is to keep the error check as close as possible of the function
who generated the error.

>> +               /*
>> +                * kthread_stop returns -EINTR in cases when streamon was
>> +                * immediately followed by streamoff, and the thread didn't had
>> +                * a chance to run. Ignore errors to stop the stream in the
>> +                * pipeline.
>> +                */
>> +               if (ret)
>> +                       dev_warn(ved->dev, "kthread_stop returned '%d'\n", ret);

Please, re-add the new line here (I thought you had already done this in
a previous version).

> 
> Is this situation serious enough to deserve warning the user?
> If not, perhaps dev_dbg would be better.

I agree, this is a normal usecase, please change to dev_dgb().

With these changes
Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> 
> Regards,
> Ezequiel
>