| Message ID | 20240920142715.2246323-1-laurentiu.palcu@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: nxp: imx8-isi: add m2m usage_count check in streamoff | expand |
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;
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(-)