| Message ID | 20200119123434.17567-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] media: vimc: streamer: if kthread_stop fails, ignore the error | expand |
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
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 >
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);
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(-)