So far, I have not found scoped_cond_guard() to be nice to work with. We have been using it quite a bit in the IIO subsystem via the iio_device_claim_direct_scoped() macro. The main thing I don't like is that scoped_cond_guard() uses a for loop internally. In the IIO subsystem, we usually try to return as early as possible, so often we are returning from all paths from withing this hidden for loop. However, since it is a for loop, the compiler thinks that it possible to exit the for loop and so we end up having to use unreachable() after the end of the scope to avoid a compiler warning. This is illustrated in the ad7380 patch in this series and there are 36 more instance of unreachable() already introduced in the IIO subsystem because of this. Also, scoped_cond_guard() is they only macro for conditional guards in cleanup.h currently. This means that so far, patches adopting this are generally converting something that wasn't scoped to be scoped. This results in changing the indentation of a lot of lines of code, which is just noise in the patches. To avoid these issues, the natural thing to do would be to have a non-scoped version of the scoped_cond_guard() macro. There was was a rejected attempt to do this in [1], where one of the complaints was: > > - rc = down_read_interruptible(&cxl_region_rwsem); > > - if (rc) > > - return rc; > > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); > > Yeah, this is an example of how NOT to do things. > > If you can't make the syntax be something clean and sane like > > if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem)) > return -EINTR; > > then this code should simply not be converted to guards AT ALL. [1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/ I couldn't find a way to make a cond_guard() macro that would work like exactly as suggested (the problem is that you can't declare a variable in the condition expression of an if statement in C). So I am proposing a macro that reads basically the same as the above so it still reads almost like normal C code even though it hides the if statement a bit. if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem) return -EINTR; The "not" is baked into the macro because in most cases, failing to obtain the lock is the abnormal condition and generally we want to have the abnormal path be the indented one. As example users, I've include a modified version of [2] from the rejected series and an ADC patch that shows how this avoids the unreachable() and too much indentation issues in the IIO subsystem. [2]: https://lore.kernel.org/all/170905254443.2268463.935306988251313983.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/ --- David Lechner (3): cleanup: add conditional guard helper iio: adc: ad7380: use if_not_cond_guard for claim direct cxl/region: Use cond_guard() in show_targetN() drivers/cxl/core/region.c | 18 ++++-------- drivers/iio/adc/ad7380.c | 70 +++++++++++++++++++++++------------------------ include/linux/cleanup.h | 11 ++++++++ 3 files changed, 51 insertions(+), 48 deletions(-) --- base-commit: 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e change-id: 20241001-cleanup-if_not_cond_guard-0981d867ddf8 Best regards, -- David Lechner <dlechner@xxxxxxxxxxxx>
