| Message ID | 20240920142715.2246323-1-laurentiu.palcu@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: nxp: imx8-isi: add m2m usage_count check in streamoff | expand |
Hi Laurentiu On Fri, Sep 20, 2024 at 05:27:15PM GMT, Laurentiu Palcu wrote: > Currently, if streamon/streamoff calls are imbalanced we can end up > with a negative ISI m2m usage_count. When that happens, the next > streamon call will not enable the ISI m2m channel. > > So, instead of throwing a warning in streamoff when usage_count drops > below 0, just make sure we don't get there. Isn't the whole purpose of the WARN() to highlight something's wrong with userspace ? I think it's expected to have the same number of streamon and streamoff calls, do you have any idea why it might not be happening ? Thanks j > > Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver") > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com> > --- > drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > index 9745d6219a166..b71195a3ba256 100644 > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > @@ -575,6 +575,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, > > mutex_lock(&m2m->lock); > > + if (m2m->usage_count == 0) > + goto unlock; > + > /* > * If the last context is this one, reset it to make sure the device > * will be reconfigured when streaming is restarted. > @@ -594,8 +597,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, > mxc_isi_channel_release(m2m->pipe); > } > > - WARN_ON(m2m->usage_count < 0); > - > +unlock: > mutex_unlock(&m2m->lock); > > return 0; > -- > 2.34.1 > >
On Mon, Sep 23, 2024 at 07:50:18PM +0200, Jacopo Mondi wrote: > On Fri, Sep 20, 2024 at 05:27:15PM GMT, Laurentiu Palcu wrote: > > Currently, if streamon/streamoff calls are imbalanced we can end up > > with a negative ISI m2m usage_count. When that happens, the next > > streamon call will not enable the ISI m2m channel. > > > > So, instead of throwing a warning in streamoff when usage_count drops > > below 0, just make sure we don't get there. > > Isn't the whole purpose of the WARN() to highlight something's wrong > with userspace ? I think it's expected to have the same number of > streamon and streamoff calls, do you have any idea why it might not be > happening ? WARN() is much too strong to report userspace problems. > > Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver") > > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com> > > --- > > drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > > index 9745d6219a166..b71195a3ba256 100644 > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > > @@ -575,6 +575,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, > > > > mutex_lock(&m2m->lock); > > > > + if (m2m->usage_count == 0) > > + goto unlock; This isn't right. Userspace can still call VIDIOC_STREAMOFF while not streaming, which will decrement the usage count when it shouldn't. The right fix is to return from mxc_isi_m2m_streamoff() without decrementing the usage count if the particular context the function is called for is not streaming. You will possibly need to also modify mxc_isi_m2m_streamon() to make sure the two functions won't race each other. Could you work on an improved patch ? Please let me know if you need help, it's important to fix this issue. > > + > > /* > > * If the last context is this one, reset it to make sure the device > > * will be reconfigured when streaming is restarted. > > @@ -594,8 +597,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, > > mxc_isi_channel_release(m2m->pipe); > > } > > > > - WARN_ON(m2m->usage_count < 0); > > - > > +unlock: > > mutex_unlock(&m2m->lock); > > > > return 0;
Hi Laurent, On Tue, Sep 24, 2024 at 12:52:46AM +0300, Laurent Pinchart wrote: > On Mon, Sep 23, 2024 at 07:50:18PM +0200, Jacopo Mondi wrote: > > On Fri, Sep 20, 2024 at 05:27:15PM GMT, Laurentiu Palcu wrote: > > > Currently, if streamon/streamoff calls are imbalanced we can end up > > > with a negative ISI m2m usage_count. When that happens, the next > > > streamon call will not enable the ISI m2m channel. > > > > > > So, instead of throwing a warning in streamoff when usage_count drops > > > below 0, just make sure we don't get there. > > > > Isn't the whole purpose of the WARN() to highlight something's wrong > > with userspace ? I think it's expected to have the same number of > > streamon and streamoff calls, do you have any idea why it might not be > > happening ? > > WARN() is much too strong to report userspace problems. > > > > Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver") > > > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com> > > > --- > > > drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > > > index 9745d6219a166..b71195a3ba256 100644 > > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c > > > @@ -575,6 +575,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, > > > > > > mutex_lock(&m2m->lock); > > > > > > + if (m2m->usage_count == 0) > > > + goto unlock; > > This isn't right. Userspace can still call VIDIOC_STREAMOFF while not > streaming, which will decrement the usage count when it shouldn't. Hmm, I didn't consider that... :/ > The right fix is to return from mxc_isi_m2m_streamoff() without > decrementing the usage count if the particular context the function is > called for is not streaming. You will possibly need to also modify > mxc_isi_m2m_streamon() to make sure the two functions won't race each > other. > > Could you work on an improved patch ? I'll give it a shot. > Please let me know if you need help, it's important to fix this issue. Thanks, I'll ping you on IRC if I get stuck. Laurentiu > > > > + > > > /* > > > * If the last context is this one, reset it to make sure the device > > > * will be reconfigured when streaming is restarted. > > > @@ -594,8 +597,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, > > > mxc_isi_channel_release(m2m->pipe); > > > } > > > > > > - WARN_ON(m2m->usage_count < 0); > > > - > > > +unlock: > > > mutex_unlock(&m2m->lock); > > > > > > return 0; > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c index 9745d6219a166..b71195a3ba256 100644 --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c @@ -575,6 +575,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, mutex_lock(&m2m->lock); + if (m2m->usage_count == 0) + goto unlock; + /* * If the last context is this one, reset it to make sure the device * will be reconfigured when streaming is restarted. @@ -594,8 +597,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh, mxc_isi_channel_release(m2m->pipe); } - WARN_ON(m2m->usage_count < 0); - +unlock: mutex_unlock(&m2m->lock); return 0;
Currently, if streamon/streamoff calls are imbalanced we can end up with a negative ISI m2m usage_count. When that happens, the next streamon call will not enable the ISI m2m channel. So, instead of throwing a warning in streamoff when usage_count drops below 0, just make sure we don't get there. Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver") Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com> --- drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)