| Message ID | 20240626221520.2846-3-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] leds: leds-lp5569: Convert to sysfs_emit API | expand |
> Convert any entry of mutex lock/unlock to guard API and simplify code. Thanks that you would like to support another bit of collateral evolution. * Would you get into the mood to benefit any more from applications of scope-based resource management? * Will development interests accordingly grow to adjust further source code places according to known pairs of function calls? > With the use of guard API, handling for selttest functions can be selftest? > greatly simplified. I find cover letters helpful for patch series. … > +++ b/drivers/leds/leds-lp5521.c > @@ -9,6 +9,7 @@ > * Milo(Woogyom) Kim <milo.kim@ti.com> > */ > > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/firmware.h> … I guess that this proposed addition is not directly needed here (and related places) because the header file is included for the macro call “DEFINE_GUARD(mutex, …)” already. https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L22 … > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > struct lp55xx_chip *chip = led->chip; > int ret; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp5521_run_selftest(chip, buf); > - mutex_unlock(&chip->lock); > > return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK"); > } … How do you think about to omit any blank lines (also at similar places)? Regards, Markus
On Thu, 27 Jun 2024, Markus Elfring wrote: > > Convert any entry of mutex lock/unlock to guard API and simplify code. > > Thanks that you would like to support another bit of collateral evolution. > > * Would you get into the mood to benefit any more from applications > of scope-based resource management? > > * Will development interests accordingly grow to adjust further source code places > according to known pairs of function calls? > > > > With the use of guard API, handling for selttest functions can be > > selftest? > > > > greatly simplified. > > I find cover letters helpful for patch series. > > > … > > +++ b/drivers/leds/leds-lp5521.c > > @@ -9,6 +9,7 @@ > > * Milo(Woogyom) Kim <milo.kim@ti.com> > > */ > > > > +#include <linux/cleanup.h> > > #include <linux/delay.h> > > #include <linux/firmware.h> > … > > I guess that this proposed addition is not directly needed here (and related places) > because the header file is included for the macro call “DEFINE_GUARD(mutex, …)” already. > https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L22 > > > … > > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > > struct lp55xx_chip *chip = led->chip; > > int ret; > > > > - mutex_lock(&chip->lock); > > + guard(mutex, &chip->lock); > > + > > ret = lp5521_run_selftest(chip, buf); > > - mutex_unlock(&chip->lock); > > > > return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK"); > > } > … > > How do you think about to omit any blank lines (also at similar places)? Please do not omit the blank lines.
On Thu, 27 Jun 2024, Markus Elfring wrote: > > Convert any entry of mutex lock/unlock to guard API and simplify code. > > Thanks that you would like to support another bit of collateral evolution. > > * Would you get into the mood to benefit any more from applications > of scope-based resource management? Why don't you submit them yourself instead of asking others to do work?
>>> Convert any entry of mutex lock/unlock to guard API and simplify code. >> >> Thanks that you would like to support another bit of collateral evolution. >> >> * Would you get into the mood to benefit any more from applications >> of scope-based resource management? > > Why don't you submit them yourself instead of asking others to do work? 1. The change resistance (or acceptance) is varying for possible software transformations in wide ranges, isn't it? 2. I would appreciate better support and collaboration with additional development resources. 3. I hope that further improvements can be achieved also by the means of the semantic patch language (Coccinelle software) in safer and more convenient ways. Are you looking for any extensions according to the coccicheck tool? Regards, Markus
On Thu, 27 Jun 2024, Markus Elfring wrote: > >>> Convert any entry of mutex lock/unlock to guard API and simplify code. > >> > >> Thanks that you would like to support another bit of collateral evolution. > >> > >> * Would you get into the mood to benefit any more from applications > >> of scope-based resource management? > > > > Why don't you submit them yourself instead of asking others to do work? > > 1. The change resistance (or acceptance) is varying for possible software transformations > in wide ranges, isn't it? How would that be any different for anyone else? Resistance/acceptance should be based on patch quality alone. > 2. I would appreciate better support and collaboration with additional development resources. In what regard? Make the changes and submit them. What additional resources could you possibly need? > 3. I hope that further improvements can be achieved also by the means of > the semantic patch language (Coccinelle software) in safer and more convenient ways. > Are you looking for any extensions according to the coccicheck tool? Sounds good. Submit a patch.
>>>>> Convert any entry of mutex lock/unlock to guard API and simplify code. >>>> >>>> Thanks that you would like to support another bit of collateral evolution. >>>> >>>> * Would you get into the mood to benefit any more from applications >>>> of scope-based resource management? >>> >>> Why don't you submit them yourself instead of asking others to do work? >> >> 1. The change resistance (or acceptance) is varying for possible software transformations >> in wide ranges, isn't it? > > How would that be any different for anyone else? Maybe not. > Resistance/acceptance should be based on patch quality alone. There are further factors involved also according to usual communication challenges. >> 2. I would appreciate better support and collaboration with additional development resources. > > In what regard? I got the impression that progress would be hindered (or even blocked?) for a while in selected subsystem areas. > What additional resources could you possibly need? Possibly known examples: * More powerful computation equipment? * Software improvements? * Financial incentives? >> 3. I hope that further improvements can be achieved also by the means of >> the semantic patch language (Coccinelle software) in safer and more convenient ways. >> Are you looking for any extensions according to the coccicheck tool? > > Sounds good. Thanks for such positive feedback. > Submit a patch. How long will the integration take (if you would like to take another look at growing product backlogs)? Regards, Markus
On Thu, 27 Jun 2024, Christian Marangi wrote: > Convert any entry of mutex lock/unlock to guard API and simplify code. > With the use of guard API, handling for selttest functions can be > greatly simplified. > > Suggested-by: Markus Elfring <Markus.Elfring@web.de> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > drivers/leds/leds-lp5521.c | 5 +- > drivers/leds/leds-lp5523.c | 25 +++----- > drivers/leds/leds-lp5562.c | 13 +++-- > drivers/leds/leds-lp5569.c | 18 ++---- > drivers/leds/leds-lp55xx-common.c | 94 +++++++++++++------------------ > 5 files changed, 64 insertions(+), 91 deletions(-) How was this one tested? Can you try build-testing it again please? > diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c > index de0f8ea48eba..56d16ea18617 100644 > --- a/drivers/leds/leds-lp5521.c > +++ b/drivers/leds/leds-lp5521.c > @@ -9,6 +9,7 @@ > * Milo(Woogyom) Kim <milo.kim@ti.com> > */ > > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/firmware.h> > #include <linux/i2c.h> > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > struct lp55xx_chip *chip = led->chip; > int ret; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp5521_run_selftest(chip, buf); > - mutex_unlock(&chip->lock); > > return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK"); > } > diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c > index 095060533d1a..baa1a3ac1a56 100644 > --- a/drivers/leds/leds-lp5523.c > +++ b/drivers/leds/leds-lp5523.c > @@ -9,6 +9,7 @@ > * Milo(Woogyom) Kim <milo.kim@ti.com> > */ > > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/firmware.h> > #include <linux/i2c.h> > @@ -188,16 +189,16 @@ static ssize_t lp5523_selftest(struct device *dev, > int ret, pos = 0; > u8 status, adc, vdd, i; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > ret = lp55xx_read(chip, LP5523_REG_STATUS, &status); > if (ret < 0) > - goto fail; > + return sysfs_emit(buf, "FAIL\n"); > > /* Check that ext clock is really in use if requested */ > if (pdata->clock_mode == LP55XX_CLOCK_EXT) { > if ((status & LP5523_EXT_CLK_USED) == 0) > - goto fail; > + return sysfs_emit(buf, "FAIL\n"); > } > > /* Measure VDD (i.e. VBAT) first (channel 16 corresponds to VDD) */ > @@ -205,14 +206,14 @@ static ssize_t lp5523_selftest(struct device *dev, > usleep_range(3000, 6000); /* ADC conversion time is typically 2.7 ms */ > ret = lp55xx_read(chip, LP5523_REG_STATUS, &status); > if (ret < 0) > - goto fail; > + return sysfs_emit(buf, "FAIL\n"); > > if (!(status & LP5523_LEDTEST_DONE)) > usleep_range(3000, 6000); /* Was not ready. Wait little bit */ > > ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &vdd); > if (ret < 0) > - goto fail; > + return sysfs_emit(buf, "FAIL\n"); > > vdd--; /* There may be some fluctuation in measurement */ > > @@ -235,14 +236,14 @@ static ssize_t lp5523_selftest(struct device *dev, > usleep_range(3000, 6000); > ret = lp55xx_read(chip, LP5523_REG_STATUS, &status); > if (ret < 0) > - goto fail; > + return sysfs_emit(buf, "FAIL\n"); > > if (!(status & LP5523_LEDTEST_DONE)) > usleep_range(3000, 6000); /* Was not ready. Wait. */ > > ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &adc); > if (ret < 0) > - goto fail; > + return sysfs_emit(buf, "FAIL\n"); > > if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM) > pos += sysfs_emit_at(buf, pos, "LED %d FAIL\n", > @@ -256,16 +257,8 @@ static ssize_t lp5523_selftest(struct device *dev, > led->led_current); > led++; > } > - if (pos == 0) > - pos = sysfs_emit(buf, "OK\n"); > - goto release_lock; > -fail: > - pos = sysfs_emit(buf, "FAIL\n"); > > -release_lock: > - mutex_unlock(&chip->lock); > - > - return pos; > + return pos == 0 ? sysfs_emit(buf, "OK\n") : pos; > } > > LP55XX_DEV_ATTR_ENGINE_MODE(1); > diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c > index 6ba5dbb9cace..69a4e3d5a126 100644 > --- a/drivers/leds/leds-lp5562.c > +++ b/drivers/leds/leds-lp5562.c > @@ -7,6 +7,7 @@ > * Author: Milo(Woogyom) Kim <milo.kim@ti.com> > */ > > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/firmware.h> > #include <linux/i2c.h> > @@ -171,9 +172,9 @@ static int lp5562_led_brightness(struct lp55xx_led *led) > }; > int ret; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp55xx_write(chip, addr[led->chan_nr], led->brightness); > - mutex_unlock(&chip->lock); > > return ret; > } > @@ -268,9 +269,9 @@ static ssize_t lp5562_store_pattern(struct device *dev, > if (mode > num_patterns || !ptn) > return -EINVAL; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp5562_run_predef_led_pattern(chip, mode); > - mutex_unlock(&chip->lock); > > if (ret) > return ret; > @@ -320,9 +321,9 @@ static ssize_t lp5562_store_engine_mux(struct device *dev, > return -EINVAL; > } > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > lp55xx_update_bits(chip, LP5562_REG_ENG_SEL, mask, val); > - mutex_unlock(&chip->lock); > > return len; > } > diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c > index e5e7e61c8916..dc8efb25b78e 100644 > --- a/drivers/leds/leds-lp5569.c > +++ b/drivers/leds/leds-lp5569.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/bitfield.h> > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/firmware.h> > #include <linux/i2c.h> > @@ -396,17 +397,17 @@ static ssize_t lp5569_selftest(struct device *dev, > struct lp55xx_chip *chip = led->chip; > int i, pos = 0; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > /* Test LED Open */ > pos = lp5569_led_open_test(led, buf); > if (pos < 0) > - goto fail; > + return sprintf(buf, "FAIL\n"); > > /* Test LED Shorted */ > pos += lp5569_led_short_test(led, buf); > if (pos < 0) > - goto fail; > + return sprintf(buf, "FAIL\n"); > > for (i = 0; i < chip->pdata->num_channels; i++) { > /* Restore current */ > @@ -419,16 +420,7 @@ static ssize_t lp5569_selftest(struct device *dev, > led++; > } > > - if (pos == 0) > - pos = sysfs_emit(buf, "OK\n"); > - goto release_lock; > -fail: > - pos = sysfs_emit(buf, "FAIL\n"); > - > -release_lock: > - mutex_unlock(&chip->lock); > - > - return pos; > + return pos == 0 ? sysfs_emit(buf, "OK\n") : pos; > } > > LP55XX_DEV_ATTR_ENGINE_MODE(1); > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c > index 1b71f512206d..f8cf5c0e983a 100644 > --- a/drivers/leds/leds-lp55xx-common.c > +++ b/drivers/leds/leds-lp55xx-common.c > @@ -10,6 +10,7 @@ > */ > > #include <linux/bitfield.h> > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/firmware.h> > @@ -272,10 +273,10 @@ int lp55xx_led_brightness(struct lp55xx_led *led) > const struct lp55xx_device_config *cfg = chip->cfg; > int ret; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + led->chan_nr, > led->brightness); > - mutex_unlock(&chip->lock); > return ret; > } > EXPORT_SYMBOL_GPL(lp55xx_led_brightness); > @@ -287,7 +288,8 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led) > int ret; > int i; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > for (i = 0; i < led->mc_cdev.num_colors; i++) { > ret = lp55xx_write(chip, > cfg->reg_led_pwm_base.addr + > @@ -296,7 +298,7 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led) > if (ret) > break; > } > - mutex_unlock(&chip->lock); > + > return ret; > } > EXPORT_SYMBOL_GPL(lp55xx_multicolor_brightness); > @@ -404,9 +406,9 @@ static ssize_t led_current_store(struct device *dev, > if (!chip->cfg->set_led_current) > return len; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > chip->cfg->set_led_current(led, (u8)curr); > - mutex_unlock(&chip->lock); > > return len; > } > @@ -541,14 +543,12 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) > } > > /* handling firmware data is chip dependent */ > - mutex_lock(&chip->lock); > - > - chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD; > - chip->fw = fw; > - if (chip->cfg->firmware_cb) > - chip->cfg->firmware_cb(chip); > - > - mutex_unlock(&chip->lock); > + scoped_guard(mutex, &chip->lock) { > + chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD; > + chip->fw = fw; > + if (chip->cfg->firmware_cb) > + chip->cfg->firmware_cb(chip); > + } > > /* firmware should be released for other channel use */ > release_firmware(chip->fw); > @@ -592,10 +592,10 @@ static ssize_t select_engine_store(struct device *dev, > case LP55XX_ENGINE_1: > case LP55XX_ENGINE_2: > case LP55XX_ENGINE_3: > - mutex_lock(&chip->lock); > - chip->engine_idx = val; > - ret = lp55xx_request_firmware(chip); > - mutex_unlock(&chip->lock); > + scoped_guard(mutex, &chip->lock) { > + chip->engine_idx = val; > + ret = lp55xx_request_firmware(chip); > + } > break; > default: > dev_err(dev, "%lu: invalid engine index. (1, 2, 3)\n", val); > @@ -634,9 +634,9 @@ static ssize_t run_engine_store(struct device *dev, > return len; > } > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > lp55xx_run_engine(chip, true); > - mutex_unlock(&chip->lock); > > return len; > } > @@ -673,7 +673,7 @@ ssize_t lp55xx_store_engine_mode(struct device *dev, > const struct lp55xx_device_config *cfg = chip->cfg; > struct lp55xx_engine *engine = &chip->engines[nr - 1]; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > chip->engine_idx = nr; > > @@ -689,8 +689,6 @@ ssize_t lp55xx_store_engine_mode(struct device *dev, > engine->mode = LP55XX_ENGINE_DISABLED; > } > > - mutex_unlock(&chip->lock); > - > return len; > } > EXPORT_SYMBOL_GPL(lp55xx_store_engine_mode); > @@ -703,14 +701,12 @@ ssize_t lp55xx_store_engine_load(struct device *dev, > struct lp55xx_chip *chip = led->chip; > int ret; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > chip->engine_idx = nr; > lp55xx_load_engine(chip); > ret = lp55xx_update_program_memory(chip, buf, len); > > - mutex_unlock(&chip->lock); > - > return ret; > } > EXPORT_SYMBOL_GPL(lp55xx_store_engine_load); > @@ -804,21 +800,17 @@ ssize_t lp55xx_store_engine_leds(struct device *dev, > if (lp55xx_mux_parse(chip, buf, &mux, len)) > return -EINVAL; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > chip->engine_idx = nr; > - ret = -EINVAL; > > if (engine->mode != LP55XX_ENGINE_LOAD) > - goto leave; > + return -EINVAL; > > if (lp55xx_load_mux(chip, mux, nr)) > - goto leave; > + return -EINVAL; > > - ret = len; > -leave: > - mutex_unlock(&chip->lock); > - return ret; > + return len; > } > EXPORT_SYMBOL_GPL(lp55xx_store_engine_leds); > > @@ -832,9 +824,9 @@ ssize_t lp55xx_show_master_fader(struct device *dev, > int ret; > u8 val; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp55xx_read(chip, cfg->reg_master_fader_base.addr + nr - 1, &val); > - mutex_unlock(&chip->lock); > > return ret ? ret : sysfs_emit(buf, "%u\n", val); > } > @@ -856,10 +848,10 @@ ssize_t lp55xx_store_master_fader(struct device *dev, > if (val > 0xff) > return -EINVAL; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > + > ret = lp55xx_write(chip, cfg->reg_master_fader_base.addr + nr - 1, > (u8)val); > - mutex_unlock(&chip->lock); > > return ret ? ret : len; > } > @@ -875,25 +867,22 @@ ssize_t lp55xx_show_master_fader_leds(struct device *dev, > int i, ret, pos = 0; > u8 val; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > for (i = 0; i < cfg->max_channel; i++) { > ret = lp55xx_read(chip, cfg->reg_led_ctrl_base.addr + i, &val); > if (ret) > - goto leave; > + return ret; > > val = FIELD_GET(LP55xx_FADER_MAPPING_MASK, val); > if (val > FIELD_MAX(LP55xx_FADER_MAPPING_MASK)) { > - ret = -EINVAL; > - goto leave; > + return -EINVAL; > } > buf[pos++] = val + '0'; > } > buf[pos++] = '\n'; > - ret = pos; > -leave: > - mutex_unlock(&chip->lock); > - return ret; > + > + return pos; > } > EXPORT_SYMBOL_GPL(lp55xx_show_master_fader_leds); > > @@ -909,7 +898,7 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev, > > n = min_t(int, len, cfg->max_channel); > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); > > for (i = 0; i < n; i++) { > if (buf[i] >= '0' && buf[i] <= '3') { > @@ -919,16 +908,13 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev, > LP55xx_FADER_MAPPING_MASK, > val); > if (ret) > - goto leave; > + return ret; > } else { > - ret = -EINVAL; > - goto leave; > + return -EINVAL; > } > } > - ret = len; > -leave: > - mutex_unlock(&chip->lock); > - return ret; > + > + return len; > } > EXPORT_SYMBOL_GPL(lp55xx_store_master_fader_leds); > > -- > 2.45.1 >
On Wed, Jul 10, 2024 at 05:55:28PM +0100, Lee Jones wrote: > On Wed, 10 Jul 2024, Markus Elfring wrote: > > > … > > > +++ b/drivers/leds/leds-lp5521.c > > … > > > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > > > struct lp55xx_chip *chip = led->chip; > > > int ret; > > > > > > - mutex_lock(&chip->lock); > > > + guard(mutex, &chip->lock); > > > > How did you come to the conclusion to try such a syntax variant out? > > > > Would the following statement (with additional parentheses) be more appropriate? > > > > guard(mutex)(&chip->lock); > > Yes, that's the fix. > > I'm more concerned with how untested patches came to being submitted. > Hi Lee, profoundly sorry for the happening... Obviusly something went wrong in me changing branch and the driver wasn't actually compiled in the test... Also with the comments from Markus I tought this needed more changes and I leaved out for a bit, so again I'm really sorry that this manage to reach next. What is the next step? Any way I can pose a fix on this and apologize for the situation?
… > +++ b/drivers/leds/leds-lp5521.c … > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > struct lp55xx_chip *chip = led->chip; > int ret; > > - mutex_lock(&chip->lock); > + guard(mutex, &chip->lock); How did you come to the conclusion to try such a syntax variant out? Would the following statement (with additional parentheses) be more appropriate? guard(mutex)(&chip->lock); Regards, Markus
On Wed, 10 Jul 2024, Markus Elfring wrote: > … > > +++ b/drivers/leds/leds-lp5521.c > … > > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > > struct lp55xx_chip *chip = led->chip; > > int ret; > > > > - mutex_lock(&chip->lock); > > + guard(mutex, &chip->lock); > > How did you come to the conclusion to try such a syntax variant out? > > Would the following statement (with additional parentheses) be more appropriate? > > guard(mutex)(&chip->lock); Yes, that's the fix. I'm more concerned with how untested patches came to being submitted.
> What is the next step?
I hope that you would dare to offer a subsequent improved patch version.
Regards,
Markus
On Wed, 10 Jul 2024, Christian Marangi wrote: > On Wed, Jul 10, 2024 at 05:55:28PM +0100, Lee Jones wrote: > > On Wed, 10 Jul 2024, Markus Elfring wrote: > > > > > … > > > > +++ b/drivers/leds/leds-lp5521.c > > > … > > > > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, > > > > struct lp55xx_chip *chip = led->chip; > > > > int ret; > > > > > > > > - mutex_lock(&chip->lock); > > > > + guard(mutex, &chip->lock); > > > > > > How did you come to the conclusion to try such a syntax variant out? > > > > > > Would the following statement (with additional parentheses) be more appropriate? > > > > > > guard(mutex)(&chip->lock); > > > > Yes, that's the fix. > > > > I'm more concerned with how untested patches came to being submitted. > > > > Hi Lee, > profoundly sorry for the happening... Obviusly something went wrong in > me changing branch and the driver wasn't actually compiled in the > test... > > Also with the comments from Markus I tought this needed more changes and > I leaved out for a bit, so again I'm really sorry that this manage to > reach next. No worries. > What is the next step? Any way I can pose a fix on this and apologize for > the situation? I'll fix it up and test it.
On Wed, 10 Jul 2024, Markus Elfring wrote: > > What is the next step? > > I hope that you would dare to offer a subsequent improved patch version. That arrangement should only be made between the submitter and the maintainer.
diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c index de0f8ea48eba..56d16ea18617 100644 --- a/drivers/leds/leds-lp5521.c +++ b/drivers/leds/leds-lp5521.c @@ -9,6 +9,7 @@ * Milo(Woogyom) Kim <milo.kim@ti.com> */ +#include <linux/cleanup.h> #include <linux/delay.h> #include <linux/firmware.h> #include <linux/i2c.h> @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev, struct lp55xx_chip *chip = led->chip; int ret; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + ret = lp5521_run_selftest(chip, buf); - mutex_unlock(&chip->lock); return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK"); } diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c index 095060533d1a..baa1a3ac1a56 100644 --- a/drivers/leds/leds-lp5523.c +++ b/drivers/leds/leds-lp5523.c @@ -9,6 +9,7 @@ * Milo(Woogyom) Kim <milo.kim@ti.com> */ +#include <linux/cleanup.h> #include <linux/delay.h> #include <linux/firmware.h> #include <linux/i2c.h> @@ -188,16 +189,16 @@ static ssize_t lp5523_selftest(struct device *dev, int ret, pos = 0; u8 status, adc, vdd, i; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); ret = lp55xx_read(chip, LP5523_REG_STATUS, &status); if (ret < 0) - goto fail; + return sysfs_emit(buf, "FAIL\n"); /* Check that ext clock is really in use if requested */ if (pdata->clock_mode == LP55XX_CLOCK_EXT) { if ((status & LP5523_EXT_CLK_USED) == 0) - goto fail; + return sysfs_emit(buf, "FAIL\n"); } /* Measure VDD (i.e. VBAT) first (channel 16 corresponds to VDD) */ @@ -205,14 +206,14 @@ static ssize_t lp5523_selftest(struct device *dev, usleep_range(3000, 6000); /* ADC conversion time is typically 2.7 ms */ ret = lp55xx_read(chip, LP5523_REG_STATUS, &status); if (ret < 0) - goto fail; + return sysfs_emit(buf, "FAIL\n"); if (!(status & LP5523_LEDTEST_DONE)) usleep_range(3000, 6000); /* Was not ready. Wait little bit */ ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &vdd); if (ret < 0) - goto fail; + return sysfs_emit(buf, "FAIL\n"); vdd--; /* There may be some fluctuation in measurement */ @@ -235,14 +236,14 @@ static ssize_t lp5523_selftest(struct device *dev, usleep_range(3000, 6000); ret = lp55xx_read(chip, LP5523_REG_STATUS, &status); if (ret < 0) - goto fail; + return sysfs_emit(buf, "FAIL\n"); if (!(status & LP5523_LEDTEST_DONE)) usleep_range(3000, 6000); /* Was not ready. Wait. */ ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &adc); if (ret < 0) - goto fail; + return sysfs_emit(buf, "FAIL\n"); if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM) pos += sysfs_emit_at(buf, pos, "LED %d FAIL\n", @@ -256,16 +257,8 @@ static ssize_t lp5523_selftest(struct device *dev, led->led_current); led++; } - if (pos == 0) - pos = sysfs_emit(buf, "OK\n"); - goto release_lock; -fail: - pos = sysfs_emit(buf, "FAIL\n"); -release_lock: - mutex_unlock(&chip->lock); - - return pos; + return pos == 0 ? sysfs_emit(buf, "OK\n") : pos; } LP55XX_DEV_ATTR_ENGINE_MODE(1); diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c index 6ba5dbb9cace..69a4e3d5a126 100644 --- a/drivers/leds/leds-lp5562.c +++ b/drivers/leds/leds-lp5562.c @@ -7,6 +7,7 @@ * Author: Milo(Woogyom) Kim <milo.kim@ti.com> */ +#include <linux/cleanup.h> #include <linux/delay.h> #include <linux/firmware.h> #include <linux/i2c.h> @@ -171,9 +172,9 @@ static int lp5562_led_brightness(struct lp55xx_led *led) }; int ret; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + ret = lp55xx_write(chip, addr[led->chan_nr], led->brightness); - mutex_unlock(&chip->lock); return ret; } @@ -268,9 +269,9 @@ static ssize_t lp5562_store_pattern(struct device *dev, if (mode > num_patterns || !ptn) return -EINVAL; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + ret = lp5562_run_predef_led_pattern(chip, mode); - mutex_unlock(&chip->lock); if (ret) return ret; @@ -320,9 +321,9 @@ static ssize_t lp5562_store_engine_mux(struct device *dev, return -EINVAL; } - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + lp55xx_update_bits(chip, LP5562_REG_ENG_SEL, mask, val); - mutex_unlock(&chip->lock); return len; } diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c index e5e7e61c8916..dc8efb25b78e 100644 --- a/drivers/leds/leds-lp5569.c +++ b/drivers/leds/leds-lp5569.c @@ -4,6 +4,7 @@ */ #include <linux/bitfield.h> +#include <linux/cleanup.h> #include <linux/delay.h> #include <linux/firmware.h> #include <linux/i2c.h> @@ -396,17 +397,17 @@ static ssize_t lp5569_selftest(struct device *dev, struct lp55xx_chip *chip = led->chip; int i, pos = 0; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); /* Test LED Open */ pos = lp5569_led_open_test(led, buf); if (pos < 0) - goto fail; + return sprintf(buf, "FAIL\n"); /* Test LED Shorted */ pos += lp5569_led_short_test(led, buf); if (pos < 0) - goto fail; + return sprintf(buf, "FAIL\n"); for (i = 0; i < chip->pdata->num_channels; i++) { /* Restore current */ @@ -419,16 +420,7 @@ static ssize_t lp5569_selftest(struct device *dev, led++; } - if (pos == 0) - pos = sysfs_emit(buf, "OK\n"); - goto release_lock; -fail: - pos = sysfs_emit(buf, "FAIL\n"); - -release_lock: - mutex_unlock(&chip->lock); - - return pos; + return pos == 0 ? sysfs_emit(buf, "OK\n") : pos; } LP55XX_DEV_ATTR_ENGINE_MODE(1); diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c index 1b71f512206d..f8cf5c0e983a 100644 --- a/drivers/leds/leds-lp55xx-common.c +++ b/drivers/leds/leds-lp55xx-common.c @@ -10,6 +10,7 @@ */ #include <linux/bitfield.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/firmware.h> @@ -272,10 +273,10 @@ int lp55xx_led_brightness(struct lp55xx_led *led) const struct lp55xx_device_config *cfg = chip->cfg; int ret; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + led->chan_nr, led->brightness); - mutex_unlock(&chip->lock); return ret; } EXPORT_SYMBOL_GPL(lp55xx_led_brightness); @@ -287,7 +288,8 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led) int ret; int i; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + for (i = 0; i < led->mc_cdev.num_colors; i++) { ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + @@ -296,7 +298,7 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led) if (ret) break; } - mutex_unlock(&chip->lock); + return ret; } EXPORT_SYMBOL_GPL(lp55xx_multicolor_brightness); @@ -404,9 +406,9 @@ static ssize_t led_current_store(struct device *dev, if (!chip->cfg->set_led_current) return len; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + chip->cfg->set_led_current(led, (u8)curr); - mutex_unlock(&chip->lock); return len; } @@ -541,14 +543,12 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) } /* handling firmware data is chip dependent */ - mutex_lock(&chip->lock); - - chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD; - chip->fw = fw; - if (chip->cfg->firmware_cb) - chip->cfg->firmware_cb(chip); - - mutex_unlock(&chip->lock); + scoped_guard(mutex, &chip->lock) { + chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD; + chip->fw = fw; + if (chip->cfg->firmware_cb) + chip->cfg->firmware_cb(chip); + } /* firmware should be released for other channel use */ release_firmware(chip->fw); @@ -592,10 +592,10 @@ static ssize_t select_engine_store(struct device *dev, case LP55XX_ENGINE_1: case LP55XX_ENGINE_2: case LP55XX_ENGINE_3: - mutex_lock(&chip->lock); - chip->engine_idx = val; - ret = lp55xx_request_firmware(chip); - mutex_unlock(&chip->lock); + scoped_guard(mutex, &chip->lock) { + chip->engine_idx = val; + ret = lp55xx_request_firmware(chip); + } break; default: dev_err(dev, "%lu: invalid engine index. (1, 2, 3)\n", val); @@ -634,9 +634,9 @@ static ssize_t run_engine_store(struct device *dev, return len; } - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + lp55xx_run_engine(chip, true); - mutex_unlock(&chip->lock); return len; } @@ -673,7 +673,7 @@ ssize_t lp55xx_store_engine_mode(struct device *dev, const struct lp55xx_device_config *cfg = chip->cfg; struct lp55xx_engine *engine = &chip->engines[nr - 1]; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); chip->engine_idx = nr; @@ -689,8 +689,6 @@ ssize_t lp55xx_store_engine_mode(struct device *dev, engine->mode = LP55XX_ENGINE_DISABLED; } - mutex_unlock(&chip->lock); - return len; } EXPORT_SYMBOL_GPL(lp55xx_store_engine_mode); @@ -703,14 +701,12 @@ ssize_t lp55xx_store_engine_load(struct device *dev, struct lp55xx_chip *chip = led->chip; int ret; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); chip->engine_idx = nr; lp55xx_load_engine(chip); ret = lp55xx_update_program_memory(chip, buf, len); - mutex_unlock(&chip->lock); - return ret; } EXPORT_SYMBOL_GPL(lp55xx_store_engine_load); @@ -804,21 +800,17 @@ ssize_t lp55xx_store_engine_leds(struct device *dev, if (lp55xx_mux_parse(chip, buf, &mux, len)) return -EINVAL; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); chip->engine_idx = nr; - ret = -EINVAL; if (engine->mode != LP55XX_ENGINE_LOAD) - goto leave; + return -EINVAL; if (lp55xx_load_mux(chip, mux, nr)) - goto leave; + return -EINVAL; - ret = len; -leave: - mutex_unlock(&chip->lock); - return ret; + return len; } EXPORT_SYMBOL_GPL(lp55xx_store_engine_leds); @@ -832,9 +824,9 @@ ssize_t lp55xx_show_master_fader(struct device *dev, int ret; u8 val; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + ret = lp55xx_read(chip, cfg->reg_master_fader_base.addr + nr - 1, &val); - mutex_unlock(&chip->lock); return ret ? ret : sysfs_emit(buf, "%u\n", val); } @@ -856,10 +848,10 @@ ssize_t lp55xx_store_master_fader(struct device *dev, if (val > 0xff) return -EINVAL; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); + ret = lp55xx_write(chip, cfg->reg_master_fader_base.addr + nr - 1, (u8)val); - mutex_unlock(&chip->lock); return ret ? ret : len; } @@ -875,25 +867,22 @@ ssize_t lp55xx_show_master_fader_leds(struct device *dev, int i, ret, pos = 0; u8 val; - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); for (i = 0; i < cfg->max_channel; i++) { ret = lp55xx_read(chip, cfg->reg_led_ctrl_base.addr + i, &val); if (ret) - goto leave; + return ret; val = FIELD_GET(LP55xx_FADER_MAPPING_MASK, val); if (val > FIELD_MAX(LP55xx_FADER_MAPPING_MASK)) { - ret = -EINVAL; - goto leave; + return -EINVAL; } buf[pos++] = val + '0'; } buf[pos++] = '\n'; - ret = pos; -leave: - mutex_unlock(&chip->lock); - return ret; + + return pos; } EXPORT_SYMBOL_GPL(lp55xx_show_master_fader_leds); @@ -909,7 +898,7 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev, n = min_t(int, len, cfg->max_channel); - mutex_lock(&chip->lock); + guard(mutex, &chip->lock); for (i = 0; i < n; i++) { if (buf[i] >= '0' && buf[i] <= '3') { @@ -919,16 +908,13 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev, LP55xx_FADER_MAPPING_MASK, val); if (ret) - goto leave; + return ret; } else { - ret = -EINVAL; - goto leave; + return -EINVAL; } } - ret = len; -leave: - mutex_unlock(&chip->lock); - return ret; + + return len; } EXPORT_SYMBOL_GPL(lp55xx_store_master_fader_leds);
Convert any entry of mutex lock/unlock to guard API and simplify code. With the use of guard API, handling for selttest functions can be greatly simplified. Suggested-by: Markus Elfring <Markus.Elfring@web.de> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/leds/leds-lp5521.c | 5 +- drivers/leds/leds-lp5523.c | 25 +++----- drivers/leds/leds-lp5562.c | 13 +++-- drivers/leds/leds-lp5569.c | 18 ++---- drivers/leds/leds-lp55xx-common.c | 94 +++++++++++++------------------ 5 files changed, 64 insertions(+), 91 deletions(-)