Use __cleanup.

The series converts the rest of the ROHM sensors (maintained by me) to
use guard(mutex). This simplifies the error paths.

As a note, kx022a accelerometer driver is handled in another series,
which also adds support for two new accelerometers. I did also patch the
driver for the BU27008 and BU27010 - but when I was testing the changes
I found that the BU27008 status is set to "obsolete". I'll try to dig
some information about the BU27010 and decide if having the driver
in-tree is still worth the effort, or if I should just send out patches
to drop it all. Hence patch to rohm-bu27008.c is not included in the
series. If someone is actually using the BU27008 or BU27010 and wants
to patch it - feel free to pick
131315de97ff ("iio: bu27008: simplify using guard(mutex)")
from
https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup

---

Matti Vaittinen (2):
  iio: bu27034: simplify using guard(mutex)
  iio: bm1390: simplify using guard(mutex)

 drivers/iio/light/rohm-bu27034.c   | 73 ++++++++++------------------
 drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
 2 files changed, 55 insertions(+), 96 deletions(-)


base-commit: adc218676eef25575469234709c2d87185ca223a
-- 
2.47.0

On Thu, 21 Nov 2024 15:04:53 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Use __cleanup.
> 
> The series converts the rest of the ROHM sensors (maintained by me) to
> use guard(mutex). This simplifies the error paths.
> 
> As a note, kx022a accelerometer driver is handled in another series,
> which also adds support for two new accelerometers. I did also patch the
> driver for the BU27008 and BU27010 - but when I was testing the changes
> I found that the BU27008 status is set to "obsolete". I'll try to dig
> some information about the BU27010 and decide if having the driver
> in-tree is still worth the effort, or if I should just send out patches
> to drop it all. Hence patch to rohm-bu27008.c is not included in the
> series. If someone is actually using the BU27008 or BU27010 and wants
> to patch it - feel free to pick
> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
> from
> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
> 
Applied. Thanks,

J
> ---
> 
> Matti Vaittinen (2):
>   iio: bu27034: simplify using guard(mutex)
>   iio: bm1390: simplify using guard(mutex)
> 
>  drivers/iio/light/rohm-bu27034.c   | 73 ++++++++++------------------
>  drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
>  2 files changed, 55 insertions(+), 96 deletions(-)
> 
> 
> base-commit: adc218676eef25575469234709c2d87185ca223a

On 21/11/2024 14:04, Matti Vaittinen wrote:
> Use __cleanup.
> 
> The series converts the rest of the ROHM sensors (maintained by me) to
> use guard(mutex). This simplifies the error paths.
> 
> As a note, kx022a accelerometer driver is handled in another series,
> which also adds support for two new accelerometers. I did also patch the
> driver for the BU27008 and BU27010 - but when I was testing the changes
> I found that the BU27008 status is set to "obsolete". I'll try to dig
> some information about the BU27010 and decide if having the driver
> in-tree is still worth the effort, or if I should just send out patches
> to drop it all. Hence patch to rohm-bu27008.c is not included in the
> series. If someone is actually using the BU27008 or BU27010 and wants
> to patch it - feel free to pick
> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
> from
> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
> 
> ---
> 
> Matti Vaittinen (2):
>   iio: bu27034: simplify using guard(mutex)
>   iio: bm1390: simplify using guard(mutex)
> 
>  drivers/iio/light/rohm-bu27034.c   | 73 ++++++++++------------------
>  drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
>  2 files changed, 55 insertions(+), 96 deletions(-)
> 
> 
> base-commit: adc218676eef25575469234709c2d87185ca223a

Hi Matti,

Both patches look good to me, but I noticed that you kept a few
mutex_lock() + mutex_unlock() in both drivers, in particular in the
cases where a scoped_guard() could simplify the code. Did you leave
those cases untouched on purpose?

Best regards,
Javier Carrasco

Hi Javier,

On 21/11/2024 15:54, Javier Carrasco wrote:
> On 21/11/2024 14:04, Matti Vaittinen wrote:
>> Use __cleanup.
>>
>> The series converts the rest of the ROHM sensors (maintained by me) to
>> use guard(mutex). This simplifies the error paths.
>>
>> As a note, kx022a accelerometer driver is handled in another series,
>> which also adds support for two new accelerometers. I did also patch the
>> driver for the BU27008 and BU27010 - but when I was testing the changes
>> I found that the BU27008 status is set to "obsolete". I'll try to dig
>> some information about the BU27010 and decide if having the driver
>> in-tree is still worth the effort, or if I should just send out patches
>> to drop it all. Hence patch to rohm-bu27008.c is not included in the
>> series. If someone is actually using the BU27008 or BU27010 and wants
>> to patch it - feel free to pick
>> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
>> from
>> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
>>
>> ---
>>
>> Matti Vaittinen (2):
>>    iio: bu27034: simplify using guard(mutex)
>>    iio: bm1390: simplify using guard(mutex)
>>
>>   drivers/iio/light/rohm-bu27034.c   | 73 ++++++++++------------------
>>   drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
>>   2 files changed, 55 insertions(+), 96 deletions(-)
>>
>>
>> base-commit: adc218676eef25575469234709c2d87185ca223a
> 
> Hi Matti,
> 
> Both patches look good to me, but I noticed that you kept a few
> mutex_lock() + mutex_unlock() in both drivers, in particular in the
> cases where a scoped_guard() could simplify the code. Did you leave
> those cases untouched on purpose?

Thanks for taking a look at the patches. Much appreciated :)

I remember leaving couple of direct calls to mutex_lock() and 
mutex_unlock() - but I think I left them only to places where I saw no 
real improvement by the use of guard() or scoped_guard(). It is likely I 
considered the locking in these cases being trivial. (Probably only for 
a duration of one or couple of function calls, with no error handling 
when a lock is held). The direct mutex_lock()/mutex_unlock() has no real 
room for usual errors (like leaving the function while lock was taken) 
in such case.

For me,

mutex_lock();
ret = foo();
mutex_unlock();

is as clear as it gets. I don't think scoped_guard() has benefits there. 
On the contrary, for me the scoped_guard() would be more complex and 
less obvious :)

Yours,
	-- Matti

On 22/11/2024 07:10, Matti Vaittinen wrote:
> Hi Javier,
> 
> On 21/11/2024 15:54, Javier Carrasco wrote:
>> On 21/11/2024 14:04, Matti Vaittinen wrote:
>>> Use __cleanup.
>>>
>>> The series converts the rest of the ROHM sensors (maintained by me) to
>>> use guard(mutex). This simplifies the error paths.
>>>
>>> As a note, kx022a accelerometer driver is handled in another series,
>>> which also adds support for two new accelerometers. I did also patch the
>>> driver for the BU27008 and BU27010 - but when I was testing the changes
>>> I found that the BU27008 status is set to "obsolete". I'll try to dig
>>> some information about the BU27010 and decide if having the driver
>>> in-tree is still worth the effort, or if I should just send out patches
>>> to drop it all. Hence patch to rohm-bu27008.c is not included in the
>>> series. If someone is actually using the BU27008 or BU27010 and wants
>>> to patch it - feel free to pick
>>> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
>>> from
>>> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
>>>
>>> ---
>>>
>>> Matti Vaittinen (2):
>>>    iio: bu27034: simplify using guard(mutex)
>>>    iio: bm1390: simplify using guard(mutex)
>>>
>>>   drivers/iio/light/rohm-bu27034.c   | 73 ++++++++++------------------
>>>   drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
>>>   2 files changed, 55 insertions(+), 96 deletions(-)
>>>
>>>
>>> base-commit: adc218676eef25575469234709c2d87185ca223a
>>
>> Hi Matti,
>>
>> Both patches look good to me, but I noticed that you kept a few
>> mutex_lock() + mutex_unlock() in both drivers, in particular in the
>> cases where a scoped_guard() could simplify the code. Did you leave
>> those cases untouched on purpose?
> 
> Thanks for taking a look at the patches. Much appreciated :)
> 
> I remember leaving couple of direct calls to mutex_lock() and
> mutex_unlock() - but I think I left them only to places where I saw no
> real improvement by the use of guard() or scoped_guard(). It is likely I
> considered the locking in these cases being trivial. (Probably only for
> a duration of one or couple of function calls, with no error handling
> when a lock is held). The direct mutex_lock()/mutex_unlock() has no real
> room for usual errors (like leaving the function while lock was taken)
> in such case.
> 
> For me,
> 
> mutex_lock();
> ret = foo();
> mutex_unlock();
> 
> is as clear as it gets. I don't think scoped_guard() has benefits there.
> On the contrary, for me the scoped_guard() would be more complex and
> less obvious :)
> 
> Yours,
>     -- Matti
> 

Yes, the cases I saw had very restricted scope. I just wanted to make
sure that you left them untouched on purpose. Often such refactoring of
mutex handling opts for removing all calls of mutex_lock/unlock to avoid
mixing both approaches in the same driver.

Personally, I like the scoped_guard() for short scopes too because it is
more robust if new code is added in that scope. But that is just a
preference :)

Best regards,
Javier Carrasco