[PATCH -next] iio: core: fix missing mutex_unlock() in iio_device_claim_buffer_mode()

[PATCH -next] iio: core: fix missing mutex_unlock() in iio_device_claim_buffer_mode()

[Peng Wu]

In the success path of iio_device_claim_buffer_mode(), mutex_unlock()
should be called before return.

Fixes: e6dad5edfb8c ("iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs")
Signed-off-by: Peng Wu <wupeng58@huawei.com>
---
 drivers/iio/industrialio-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 52e690f031cb..88987cdc9e3f 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2102,8 +2102,10 @@ int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
 
 	mutex_lock(&iio_dev_opaque->mlock);
 
-	if (iio_buffer_enabled(indio_dev))
+	if (iio_buffer_enabled(indio_dev)) {
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return 0;
+	}
 
 	mutex_unlock(&iio_dev_opaque->mlock);
 	return -EBUSY;
-- 
2.17.1

Re: [PATCH -next] iio: core: fix missing mutex_unlock() in iio_device_claim_buffer_mode()

[Andy Shevchenko]

On Fri, Nov 4, 2022 at 11:05 AM Peng Wu <wupeng58@huawei.com> wrote:
>
> In the success path of iio_device_claim_buffer_mode(), mutex_unlock()
> should be called before return.

Please, understand how the code works.
NAK.

Nuno, perhaps we need to put a comment inside the function to explain this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH -next] iio: core: fix missing mutex_unlock() in iio_device_claim_buffer_mode()

[Nuno Sá]

On Fri, 2022-11-04 at 11:54 +0200, Andy Shevchenko wrote:
> On Fri, Nov 4, 2022 at 11:05 AM Peng Wu <wupeng58@huawei.com> wrote:
> > 
> > In the success path of iio_device_claim_buffer_mode(),
> > mutex_unlock()
> > should be called before return.
> 
> Please, understand how the code works.
> NAK.
> 
> Nuno, perhaps we need to put a comment inside the function to explain
> this?
> 

Hmm... I guess we can do that but, honestly, the docs in the function
should be pretty straight. Maybe saying that it's supposed to work as
iio_device_claim_direct_mode() would help (as that one is familiar)...

Alternatively, having sparse annotations could also make it more easy
to understand the intent.

- Nuno Sá

Re: [PATCH -next] iio: core: fix missing mutex_unlock() in iio_device_claim_buffer_mode()

[Andy Shevchenko]

On Fri, Nov 4, 2022 at 1:11 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Fri, 2022-11-04 at 11:54 +0200, Andy Shevchenko wrote:
> > On Fri, Nov 4, 2022 at 11:05 AM Peng Wu <wupeng58@huawei.com> wrote:
> > >
> > > In the success path of iio_device_claim_buffer_mode(),
> > > mutex_unlock()
> > > should be called before return.
> >
> > Please, understand how the code works.
> > NAK.
> >
> > Nuno, perhaps we need to put a comment inside the function to explain
> > this?
> >
>
> Hmm... I guess we can do that but, honestly, the docs in the function
> should be pretty straight. Maybe saying that it's supposed to work as
> iio_device_claim_direct_mode() would help (as that one is familiar)...
>
> Alternatively, having sparse annotations could also make it more easy
> to understand the intent.

Maybe simply

/* Yes, we leave it locked here */

before return 0; would suffice?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH -next] iio: core: fix missing mutex_unlock() in iio_device_claim_buffer_mode()

[Nuno Sá]

On Fri, 2022-11-04 at 14:52 +0200, Andy Shevchenko wrote:
> On Fri, Nov 4, 2022 at 1:11 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Fri, 2022-11-04 at 11:54 +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 4, 2022 at 11:05 AM Peng Wu <wupeng58@huawei.com>
> > > wrote:
> > > > 
> > > > In the success path of iio_device_claim_buffer_mode(),
> > > > mutex_unlock()
> > > > should be called before return.
> > > 
> > > Please, understand how the code works.
> > > NAK.
> > > 
> > > Nuno, perhaps we need to put a comment inside the function to
> > > explain
> > > this?
> > > 
> > 
> > Hmm... I guess we can do that but, honestly, the docs in the
> > function
> > should be pretty straight. Maybe saying that it's supposed to work
> > as
> > iio_device_claim_direct_mode() would help (as that one is
> > familiar)...
> > 
> > Alternatively, having sparse annotations could also make it more
> > easy
> > to understand the intent.
> 
> Maybe simply
> 
> /* Yes, we leave it locked here */
> 
> before return 0; would suffice?

Yes, I think so...

- Nuno Sá