| Message ID | 20241111191934.17231-1-jiashengjiangcool@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable() | expand |
On 11/11/24 1:19 PM, Jiasheng Jiang wrote: > Add check for the return value of clk_enable() in order to catch the > potential exception. > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > --- > Changelog: > > v2 -> v3: > > 1. Simplify code with cleanup helpers. > > v1 -> v2: > > 1. Remove unsuitable dev_err_probe(). > --- ... > @@ -492,21 +495,25 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > return -EINVAL; > > case IIO_CHAN_INFO_ENABLE: > - mutex_lock(&priv->lock); > - if (val) { > - if (!priv->enabled) { > - priv->enabled = true; > - clk_enable(priv->clk); > - } > - regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > - } else { > - regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > - if (priv->enabled) { > - priv->enabled = false; > - clk_disable(priv->clk); > + > + scoped_guard(mutex, &priv->lock) { > + if (val) { > + if (!priv->enabled) { > + priv->enabled = true; > + ret = clk_enable(priv->clk); > + if (ret) > + return ret; > + } > + regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > + } else { > + regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > + if (priv->enabled) { > + priv->enabled = false; > + clk_disable(priv->clk); > + } > } > } > - mutex_unlock(&priv->lock); > + > return 0; > } Another way to do this that avoids changing the indent so much is placing braces around the case body like this. This also avoids the compile error from using guard after case directly. case IIO_CHAN_INFO_ENABLE: { guard(mutex)(&priv->lock); if (val) { if (!priv->enabled) { priv->enabled = true; ret = clk_enable(priv->clk); if (ret) return ret; } regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); } else { regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); if (priv->enabled) { priv->enabled = false; clk_disable(priv->clk); } } return 0; }
On Mon, Nov 11, 2024 at 2:45 PM David Lechner <dlechner@baylibre.com> wrote: > > On 11/11/24 1:19 PM, Jiasheng Jiang wrote: > > Add check for the return value of clk_enable() in order to catch the > > potential exception. > > > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > > --- > > Changelog: > > > > v2 -> v3: > > > > 1. Simplify code with cleanup helpers. > > > > v1 -> v2: > > > > 1. Remove unsuitable dev_err_probe(). > > --- > > ... > > > @@ -492,21 +495,25 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > > return -EINVAL; > > > > case IIO_CHAN_INFO_ENABLE: > > - mutex_lock(&priv->lock); > > - if (val) { > > - if (!priv->enabled) { > > - priv->enabled = true; > > - clk_enable(priv->clk); > > - } > > - regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > > - } else { > > - regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > > - if (priv->enabled) { > > - priv->enabled = false; > > - clk_disable(priv->clk); > > + > > + scoped_guard(mutex, &priv->lock) { > > + if (val) { > > + if (!priv->enabled) { > > + priv->enabled = true; > > + ret = clk_enable(priv->clk); > > + if (ret) > > + return ret; > > + } > > + regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > > + } else { > > + regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > > + if (priv->enabled) { > > + priv->enabled = false; > > + clk_disable(priv->clk); > > + } > > } > > } > > - mutex_unlock(&priv->lock); > > + > > return 0; > > } > > > Another way to do this that avoids changing the indent > so much is placing braces around the case body like this. > This also avoids the compile error from using guard after > case directly. > > > case IIO_CHAN_INFO_ENABLE: { > guard(mutex)(&priv->lock); > > if (val) { > if (!priv->enabled) { > priv->enabled = true; > ret = clk_enable(priv->clk); > if (ret) > return ret; > } > regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > } else { > regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > if (priv->enabled) { > priv->enabled = false; > clk_disable(priv->clk); > } > } > > return 0; > } > Looks great. But there is no indentation between "switch" and "case". As a result, the closing braces of "switch" and "case" will be placed in the same column. Like this: switch(mask) { case IIO_CHAN_INFO_ENABLE: { } } -Jiasheng
On 11/11/24 2:36 PM, Jiasheng Jiang wrote: > On Mon, Nov 11, 2024 at 2:45 PM David Lechner <dlechner@baylibre.com> wrote: >> >> On 11/11/24 1:19 PM, Jiasheng Jiang wrote: >>> Add check for the return value of clk_enable() in order to catch the >>> potential exception. >>> >>> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> >>> --- >>> Changelog: >>> >>> v2 -> v3: >>> >>> 1. Simplify code with cleanup helpers. >>> >>> v1 -> v2: >>> >>> 1. Remove unsuitable dev_err_probe(). >>> --- >> >> ... >> >>> @@ -492,21 +495,25 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, >>> return -EINVAL; >>> >>> case IIO_CHAN_INFO_ENABLE: >>> - mutex_lock(&priv->lock); >>> - if (val) { >>> - if (!priv->enabled) { >>> - priv->enabled = true; >>> - clk_enable(priv->clk); >>> - } >>> - regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); >>> - } else { >>> - regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); >>> - if (priv->enabled) { >>> - priv->enabled = false; >>> - clk_disable(priv->clk); >>> + >>> + scoped_guard(mutex, &priv->lock) { >>> + if (val) { >>> + if (!priv->enabled) { >>> + priv->enabled = true; >>> + ret = clk_enable(priv->clk); >>> + if (ret) >>> + return ret; >>> + } >>> + regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); >>> + } else { >>> + regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); >>> + if (priv->enabled) { >>> + priv->enabled = false; >>> + clk_disable(priv->clk); >>> + } >>> } >>> } >>> - mutex_unlock(&priv->lock); >>> + >>> return 0; >>> } >> >> >> Another way to do this that avoids changing the indent >> so much is placing braces around the case body like this. >> This also avoids the compile error from using guard after >> case directly. >> >> >> case IIO_CHAN_INFO_ENABLE: { >> guard(mutex)(&priv->lock); >> >> if (val) { >> if (!priv->enabled) { >> priv->enabled = true; >> ret = clk_enable(priv->clk); >> if (ret) >> return ret; >> } >> regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); >> } else { >> regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); >> if (priv->enabled) { >> priv->enabled = false; >> clk_disable(priv->clk); >> } >> } >> >> return 0; >> } >> > > Looks great. > But there is no indentation between "switch" and "case". > As a result, the closing braces of "switch" and "case" will > be placed in the same column. > > Like this: > > switch(mask) { > case IIO_CHAN_INFO_ENABLE: { > > } > } > > -Jiasheng Usually, there is a default: case as well, so we could move the final return and make it look like this: switch (mask) { case IIO_CHAN_INFO_RAW: return regmap_write(priv->regmap, TIM_CNT, val); case IIO_CHAN_INFO_SCALE: /* fixed scale */ return -EINVAL; case IIO_CHAN_INFO_ENABLE: { guard(mutex)(&priv->lock); if (val) { if (!priv->enabled) { priv->enabled = true; clk_enable(priv->clk); } regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); } else { regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); if (priv->enabled) { priv->enabled = false; clk_disable(priv->clk); } } return 0; } default: return -EINVAL; } And it is unusual, but I found kvm_arm_pmu_v3_get_attr() that also has this double inline brace at the end of a switch statement. } } So even if it doesn't look so nice, it does seem to be the "correct" style.
On Mon, Nov 11, 2024 at 4:15 PM David Lechner <dlechner@baylibre.com> wrote: > > On 11/11/24 2:36 PM, Jiasheng Jiang wrote: > > On Mon, Nov 11, 2024 at 2:45 PM David Lechner <dlechner@baylibre.com> wrote: > >> > >> On 11/11/24 1:19 PM, Jiasheng Jiang wrote: > >>> Add check for the return value of clk_enable() in order to catch the > >>> potential exception. > >>> > >>> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > >>> --- > >>> Changelog: > >>> > >>> v2 -> v3: > >>> > >>> 1. Simplify code with cleanup helpers. > >>> > >>> v1 -> v2: > >>> > >>> 1. Remove unsuitable dev_err_probe(). > >>> --- > >> > >> ... > >> > >>> @@ -492,21 +495,25 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > >>> return -EINVAL; > >>> > >>> case IIO_CHAN_INFO_ENABLE: > >>> - mutex_lock(&priv->lock); > >>> - if (val) { > >>> - if (!priv->enabled) { > >>> - priv->enabled = true; > >>> - clk_enable(priv->clk); > >>> - } > >>> - regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > >>> - } else { > >>> - regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > >>> - if (priv->enabled) { > >>> - priv->enabled = false; > >>> - clk_disable(priv->clk); > >>> + > >>> + scoped_guard(mutex, &priv->lock) { > >>> + if (val) { > >>> + if (!priv->enabled) { > >>> + priv->enabled = true; > >>> + ret = clk_enable(priv->clk); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> + regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > >>> + } else { > >>> + regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > >>> + if (priv->enabled) { > >>> + priv->enabled = false; > >>> + clk_disable(priv->clk); > >>> + } > >>> } > >>> } > >>> - mutex_unlock(&priv->lock); > >>> + > >>> return 0; > >>> } > >> > >> > >> Another way to do this that avoids changing the indent > >> so much is placing braces around the case body like this. > >> This also avoids the compile error from using guard after > >> case directly. > >> > >> > >> case IIO_CHAN_INFO_ENABLE: { > >> guard(mutex)(&priv->lock); > >> > >> if (val) { > >> if (!priv->enabled) { > >> priv->enabled = true; > >> ret = clk_enable(priv->clk); > >> if (ret) > >> return ret; > >> } > >> regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > >> } else { > >> regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > >> if (priv->enabled) { > >> priv->enabled = false; > >> clk_disable(priv->clk); > >> } > >> } > >> > >> return 0; > >> } > >> > > > > Looks great. > > But there is no indentation between "switch" and "case". > > As a result, the closing braces of "switch" and "case" will > > be placed in the same column. > > > > Like this: > > > > switch(mask) { > > case IIO_CHAN_INFO_ENABLE: { > > > > } > > } > > > > -Jiasheng > > > Usually, there is a default: case as well, so we could move the > final return and make it look like this: > > switch (mask) { > case IIO_CHAN_INFO_RAW: > return regmap_write(priv->regmap, TIM_CNT, val); > > case IIO_CHAN_INFO_SCALE: > /* fixed scale */ > return -EINVAL; > > case IIO_CHAN_INFO_ENABLE: { > guard(mutex)(&priv->lock); > if (val) { > if (!priv->enabled) { > priv->enabled = true; > clk_enable(priv->clk); > } > regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > } else { > regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > if (priv->enabled) { > priv->enabled = false; > clk_disable(priv->clk); > } > } > return 0; > } > default: > return -EINVAL; > } > > > And it is unusual, but I found kvm_arm_pmu_v3_get_attr() that > also has this double inline brace at the end of a switch statement. > > } > } > > So even if it doesn't look so nice, it does seem to be the > "correct" style. Thanks, I will submit a v4 patch. -Jiasheng
diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c index 0684329956d9..9fb4f7eefa86 100644 --- a/drivers/iio/trigger/stm32-timer-trigger.c +++ b/drivers/iio/trigger/stm32-timer-trigger.c @@ -119,7 +119,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, unsigned int frequency) { unsigned long long prd, div; - int prescaler = 0; + int prescaler = 0, ret; u32 ccer; /* Period and prescaler values depends of clock rate */ @@ -150,10 +150,12 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, if (ccer & TIM_CCER_CCXE) return -EBUSY; - mutex_lock(&priv->lock); + guard(mutex)(&priv->lock); if (!priv->enabled) { priv->enabled = true; - clk_enable(priv->clk); + ret = clk_enable(priv->clk); + if (ret) + return ret; } regmap_write(priv->regmap, TIM_PSC, prescaler); @@ -173,7 +175,6 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, /* Enable controller */ regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); - mutex_unlock(&priv->lock); return 0; } @@ -307,7 +308,7 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev, struct stm32_timer_trigger *priv = dev_get_drvdata(dev); struct iio_trigger *trig = to_iio_trigger(dev); u32 mask, shift, master_mode_max; - int i; + int i, ret; if (stm32_timer_is_trgo2_name(trig->name)) { mask = TIM_CR2_MMS2; @@ -322,15 +323,16 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev, for (i = 0; i <= master_mode_max; i++) { if (!strncmp(master_mode_table[i], buf, strlen(master_mode_table[i]))) { - mutex_lock(&priv->lock); + guard(mutex)(&priv->lock); if (!priv->enabled) { /* Clock should be enabled first */ priv->enabled = true; - clk_enable(priv->clk); + ret = clk_enable(priv->clk); + if (ret) + return ret; } regmap_update_bits(priv->regmap, TIM_CR2, mask, i << shift); - mutex_unlock(&priv->lock); return len; } } @@ -482,6 +484,7 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, int val, int val2, long mask) { struct stm32_timer_trigger *priv = iio_priv(indio_dev); + int ret; switch (mask) { case IIO_CHAN_INFO_RAW: @@ -492,21 +495,25 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, return -EINVAL; case IIO_CHAN_INFO_ENABLE: - mutex_lock(&priv->lock); - if (val) { - if (!priv->enabled) { - priv->enabled = true; - clk_enable(priv->clk); - } - regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); - } else { - regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); - if (priv->enabled) { - priv->enabled = false; - clk_disable(priv->clk); + + scoped_guard(mutex, &priv->lock) { + if (val) { + if (!priv->enabled) { + priv->enabled = true; + ret = clk_enable(priv->clk); + if (ret) + return ret; + } + regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); + } else { + regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); + if (priv->enabled) { + priv->enabled = false; + clk_disable(priv->clk); + } } } - mutex_unlock(&priv->lock); + return 0; } @@ -601,7 +608,7 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev, unsigned int mode) { struct stm32_timer_trigger *priv = iio_priv(indio_dev); - int sms = stm32_enable_mode2sms(mode); + int sms = stm32_enable_mode2sms(mode), ret; if (sms < 0) return sms; @@ -609,12 +616,15 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev, * Triggered mode sets CEN bit automatically by hardware. So, first * enable counter clock, so it can use it. Keeps it in sync with CEN. */ - mutex_lock(&priv->lock); - if (sms == 6 && !priv->enabled) { - clk_enable(priv->clk); - priv->enabled = true; + scoped_guard(mutex, &priv->lock) { + if (sms == 6 && !priv->enabled) { + ret = clk_enable(priv->clk); + if (ret) + return ret; + + priv->enabled = true; + } } - mutex_unlock(&priv->lock); regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
Add check for the return value of clk_enable() in order to catch the potential exception. Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> --- Changelog: v2 -> v3: 1. Simplify code with cleanup helpers. v1 -> v2: 1. Remove unsuitable dev_err_probe(). --- drivers/iio/trigger/stm32-timer-trigger.c | 64 +++++++++++++---------- 1 file changed, 37 insertions(+), 27 deletions(-)