| Message ID | cover.1732193263.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: Use __cleanup for a few ROHM sensors | expand |
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
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
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