| Message ID | 20241111222310.12339-1-jiashengjiangcool@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4] iio: trigger: stm32-timer-trigger: Add check for clk_enable() | expand |
On 11/11/24 4:23 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> > --- Reviewed-by: David Lechner <dlechner@baylibre.com>
On Mon, 11 Nov 2024 22:23:10 +0000 Jiasheng Jiang <jiashengjiangcool@gmail.com> 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> Hi Jiasheng, Should definitely mention the changes to use guard() to simplify the resulting code. One minor comment on the code inline. Otherwise looks good to me. Thanks, Jonathan > --- > Changelog: > > v3 -> v4: > > 1. Place braces around the case body. > > v2 -> v3: > > 1. Simplify code with cleanup helpers. > > v1 -> v2: > > 1. Remove unsuitable dev_err_probe(). > @@ -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: > @@ -491,12 +494,14 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > /* fixed scale */ > return -EINVAL; > > - case IIO_CHAN_INFO_ENABLE: > - mutex_lock(&priv->lock); > + case IIO_CHAN_INFO_ENABLE: { > + guard(mutex)(&priv->lock); > if (val) { > if (!priv->enabled) { > priv->enabled = true; > - clk_enable(priv->clk); > + ret = clk_enable(priv->clk); > + if (ret) > + return ret; > } > regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > } else { > @@ -506,9 +511,10 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > clk_disable(priv->clk); > } > } > - mutex_unlock(&priv->lock); > + > return 0; > } Add a default for reasons David mentioned and it also makes it visually clear that we expect to get in here for other cases but they are all errors. default: return -EINVAL; > + } > And drop this return as unreachable. > return -EINVAL; > }
Hi Jonathan, On Sat, Nov 23, 2024 at 10:08 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 11 Nov 2024 22:23:10 +0000 > Jiasheng Jiang <jiashengjiangcool@gmail.com> 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> > Hi Jiasheng, > > > Should definitely mention the changes to use guard() to simplify > the resulting code. Thanks, I have revised the "v2 -> v3" in the Changelog to clarify the changes. > One minor comment on the code inline. Otherwise looks good to me. > > Thanks, > > Jonathan > > > --- > > Changelog: > > > > v3 -> v4: > > > > 1. Place braces around the case body. > > > > v2 -> v3: > > > > 1. Simplify code with cleanup helpers. > > > > v1 -> v2: > > > > 1. Remove unsuitable dev_err_probe(). > > > @@ -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: > > @@ -491,12 +494,14 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > > /* fixed scale */ > > return -EINVAL; > > > > - case IIO_CHAN_INFO_ENABLE: > > - mutex_lock(&priv->lock); > > + case IIO_CHAN_INFO_ENABLE: { > > + guard(mutex)(&priv->lock); > > if (val) { > > if (!priv->enabled) { > > priv->enabled = true; > > - clk_enable(priv->clk); > > + ret = clk_enable(priv->clk); > > + if (ret) > > + return ret; > > } > > regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); > > } else { > > @@ -506,9 +511,10 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > > clk_disable(priv->clk); > > } > > } > > - mutex_unlock(&priv->lock); > > + > > return 0; > > } > Add a default for reasons David mentioned and it also makes it visually clear > that we expect to get in here for other cases but they are all errors. > default: > return -EINVAL; > > + } > > > And drop this return as unreachable. > > > return -EINVAL; > > } Thanks, I have submitted v5 to include a default and remove the return. -Jiasheng
diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c index 0684329956d9..d599d50fbb3b 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: @@ -491,12 +494,14 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, /* fixed scale */ return -EINVAL; - case IIO_CHAN_INFO_ENABLE: - mutex_lock(&priv->lock); + case IIO_CHAN_INFO_ENABLE: { + guard(mutex)(&priv->lock); if (val) { if (!priv->enabled) { priv->enabled = true; - clk_enable(priv->clk); + ret = clk_enable(priv->clk); + if (ret) + return ret; } regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); } else { @@ -506,9 +511,10 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, clk_disable(priv->clk); } } - mutex_unlock(&priv->lock); + return 0; } + } return -EINVAL; } @@ -601,7 +607,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 +615,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: v3 -> v4: 1. Place braces around the case body. v2 -> v3: 1. Simplify code with cleanup helpers. v1 -> v2: 1. Remove unsuitable dev_err_probe(). --- drivers/iio/trigger/stm32-timer-trigger.c | 45 ++++++++++++++--------- 1 file changed, 27 insertions(+), 18 deletions(-)