* Re: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()
2023-04-19 14:23 [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write() Dan Carpenter
@ 2023-04-19 14:36 ` William Breathitt Gray
2023-04-19 15:37 ` Dan Carpenter
2023-04-20 6:04 ` Biju Das
2023-04-20 14:17 ` William Breathitt Gray
2 siblings, 1 reply; 7+ messages in thread
From: William Breathitt Gray @ 2023-04-19 14:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Biju Das, Lee Jones, linux-iio, linux-renesas-soc,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]
On Wed, Apr 19, 2023 at 05:23:55PM +0300, Dan Carpenter wrote:
> The return -ERANGE error paths need to call mutex_unlock(&priv->lock);
> before returning.
>
> Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
> index a371bab68499..aeadce5e2853 100644
> --- a/drivers/counter/rz-mtu3-cnt.c
> +++ b/drivers/counter/rz-mtu3-cnt.c
> @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
> switch (count->id) {
> case RZ_MTU3_16_BIT_MTU1_CH:
> case RZ_MTU3_16_BIT_MTU2_CH:
> - if (ceiling > U16_MAX)
> - return -ERANGE;
> + if (ceiling > U16_MAX) {
> + ret = -ERANGE;
> + goto unlock;
> + }
> priv->mtu_16bit_max[ch_id] = ceiling;
> break;
> case RZ_MTU3_32_BIT_CH:
> - if (ceiling > U32_MAX)
> - return -ERANGE;
> + if (ceiling > U32_MAX) {
> + ret = -ERANGE;
> + goto unlock;
> + }
> priv->mtu_32bit_max = ceiling;
> break;
> default:
> /* should never reach this path */
> - mutex_unlock(&priv->lock);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto unlock;
> }
>
> pm_runtime_get_sync(ch->dev);
> @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
>
> rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> pm_runtime_put(ch->dev);
> +unlock:
> mutex_unlock(&priv->lock);
> -
> - return 0;
> + return ret;
> }
>
> static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter)
> --
> 2.39.2
>
The lock is acquired by rz_mtu3_lock_if_counter_is_valid(), so that
function needs a sparse __acquires(&priv->lock) annotation too.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()
2023-04-19 14:36 ` William Breathitt Gray
@ 2023-04-19 15:37 ` Dan Carpenter
2023-04-19 15:30 ` William Breathitt Gray
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2023-04-19 15:37 UTC (permalink / raw)
To: William Breathitt Gray
Cc: Biju Das, Lee Jones, linux-iio, linux-renesas-soc,
kernel-janitors
On Wed, Apr 19, 2023 at 10:36:49AM -0400, William Breathitt Gray wrote:
> The lock is acquired by rz_mtu3_lock_if_counter_is_valid(), so that
> function needs a sparse __acquires(&priv->lock) annotation too.
I found this bug using Smatch. It's a competing static checker which
uses Sparse as a parser. I am the author of Smatch so I am naturally
biased.
I don't think it's as simple as that. I don't think Sparse has
annotations for mutexes, only for spinlocks? Also it's really
complicated to annotate something as taking the lock on the success path
but not on the failure path. You have to set up a wrapper and use
__cond_lock().
Every other feature in Sparse is awesome, but for locking, it's better
to just use Smatch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()
2023-04-19 15:37 ` Dan Carpenter
@ 2023-04-19 15:30 ` William Breathitt Gray
0 siblings, 0 replies; 7+ messages in thread
From: William Breathitt Gray @ 2023-04-19 15:30 UTC (permalink / raw)
To: Dan Carpenter
Cc: Biju Das, Lee Jones, linux-iio, linux-renesas-soc,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]
On Wed, Apr 19, 2023 at 06:37:05PM +0300, Dan Carpenter wrote:
> On Wed, Apr 19, 2023 at 10:36:49AM -0400, William Breathitt Gray wrote:
> > The lock is acquired by rz_mtu3_lock_if_counter_is_valid(), so that
> > function needs a sparse __acquires(&priv->lock) annotation too.
>
> I found this bug using Smatch. It's a competing static checker which
> uses Sparse as a parser. I am the author of Smatch so I am naturally
> biased.
>
> I don't think it's as simple as that. I don't think Sparse has
> annotations for mutexes, only for spinlocks? Also it's really
> complicated to annotate something as taking the lock on the success path
> but not on the failure path. You have to set up a wrapper and use
> __cond_lock().
>
> Every other feature in Sparse is awesome, but for locking, it's better
> to just use Smatch.
>
> regards,
> dan carpenter
Ah that's a fair point, I can see how involved that would be to set up a
wrapper and handle the various paths correctly. The marginal benefit
just doesn't seem worth the effort afterall.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()
2023-04-19 14:23 [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write() Dan Carpenter
2023-04-19 14:36 ` William Breathitt Gray
@ 2023-04-20 6:04 ` Biju Das
2023-04-20 14:17 ` William Breathitt Gray
2 siblings, 0 replies; 7+ messages in thread
From: Biju Das @ 2023-04-20 6:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: William Breathitt Gray, Lee Jones, linux-iio@vger.kernel.org,
linux-renesas-soc@vger.kernel.org,
kernel-janitors@vger.kernel.org
Hi Dan Carpenter,
Thanks for the patch.
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Wednesday, April 19, 2023 3:24 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: William Breathitt Gray <william.gray@linaro.org>; Lee Jones
> <lee@kernel.org>; linux-iio@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: [PATCH] counter: rz-mtu3-cnt: Unlock on error in
> rz_mtu3_count_write()
>
> The return -ERANGE error paths need to call mutex_unlock(&priv->lock);
> before returning.
>
> Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Cheers,
Biju
> ---
> drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
> index a371bab68499..aeadce5e2853 100644
> --- a/drivers/counter/rz-mtu3-cnt.c
> +++ b/drivers/counter/rz-mtu3-cnt.c
> @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct
> counter_device *counter,
> switch (count->id) {
> case RZ_MTU3_16_BIT_MTU1_CH:
> case RZ_MTU3_16_BIT_MTU2_CH:
> - if (ceiling > U16_MAX)
> - return -ERANGE;
> + if (ceiling > U16_MAX) {
> + ret = -ERANGE;
> + goto unlock;
> + }
> priv->mtu_16bit_max[ch_id] = ceiling;
> break;
> case RZ_MTU3_32_BIT_CH:
> - if (ceiling > U32_MAX)
> - return -ERANGE;
> + if (ceiling > U32_MAX) {
> + ret = -ERANGE;
> + goto unlock;
> + }
> priv->mtu_32bit_max = ceiling;
> break;
> default:
> /* should never reach this path */
> - mutex_unlock(&priv->lock);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto unlock;
> }
>
> pm_runtime_get_sync(ch->dev);
> @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct
> counter_device *counter,
>
> rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> pm_runtime_put(ch->dev);
> +unlock:
> mutex_unlock(&priv->lock);
> -
> - return 0;
> + return ret;
> }
>
> static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter)
> --
> 2.39.2
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()
2023-04-19 14:23 [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write() Dan Carpenter
2023-04-19 14:36 ` William Breathitt Gray
2023-04-20 6:04 ` Biju Das
@ 2023-04-20 14:17 ` William Breathitt Gray
2023-04-20 14:58 ` Dan Carpenter
2 siblings, 1 reply; 7+ messages in thread
From: William Breathitt Gray @ 2023-04-20 14:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: Biju Das, Lee Jones, linux-iio, linux-renesas-soc,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]
On Wed, Apr 19, 2023 at 05:23:55PM +0300, Dan Carpenter wrote:
> The return -ERANGE error paths need to call mutex_unlock(&priv->lock);
> before returning.
>
> Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
The changes in this patch are for rz_mtu3_count_ceiling_write(), so the
title of this patch should be fixed.
> ---
> drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
> index a371bab68499..aeadce5e2853 100644
> --- a/drivers/counter/rz-mtu3-cnt.c
> +++ b/drivers/counter/rz-mtu3-cnt.c
> @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
> switch (count->id) {
> case RZ_MTU3_16_BIT_MTU1_CH:
> case RZ_MTU3_16_BIT_MTU2_CH:
> - if (ceiling > U16_MAX)
> - return -ERANGE;
> + if (ceiling > U16_MAX) {
> + ret = -ERANGE;
> + goto unlock;
> + }
> priv->mtu_16bit_max[ch_id] = ceiling;
> break;
> case RZ_MTU3_32_BIT_CH:
> - if (ceiling > U32_MAX)
> - return -ERANGE;
> + if (ceiling > U32_MAX) {
> + ret = -ERANGE;
> + goto unlock;
> + }
> priv->mtu_32bit_max = ceiling;
> break;
> default:
> /* should never reach this path */
> - mutex_unlock(&priv->lock);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto unlock;
> }
Normally, I like "goto unlock" patterns, but I think in this context it
makes the flow of code confusing with the mix-match of the switch cases;
default case jumps with a goto, but RZ_MTU3_* cases passes will break,
yet RZ_MUT3_* failures have a goto path. Rather than a "goto unlock"
pattern, I'd prefer to see simply mutex_lock() called for these ceiling
checks. That also has the benefit of reducing the number of changes we
have to make for this fix.
William Breathitt Gray
>
> pm_runtime_get_sync(ch->dev);
> @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
>
> rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> pm_runtime_put(ch->dev);
> +unlock:
> mutex_unlock(&priv->lock);
> -
> - return 0;
> + return ret;
> }
>
> static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter)
> --
> 2.39.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()
2023-04-20 14:17 ` William Breathitt Gray
@ 2023-04-20 14:58 ` Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2023-04-20 14:58 UTC (permalink / raw)
To: William Breathitt Gray
Cc: Biju Das, Lee Jones, linux-iio, linux-renesas-soc,
kernel-janitors
On Thu, Apr 20, 2023 at 10:17:21AM -0400, William Breathitt Gray wrote:
> On Wed, Apr 19, 2023 at 05:23:55PM +0300, Dan Carpenter wrote:
> > The return -ERANGE error paths need to call mutex_unlock(&priv->lock);
> > before returning.
> >
> > Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>
> The changes in this patch are for rz_mtu3_count_ceiling_write(), so the
> title of this patch should be fixed.
>
Huh. That's weird... I don't know why I got that wrong.
> > ---
> > drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
> > index a371bab68499..aeadce5e2853 100644
> > --- a/drivers/counter/rz-mtu3-cnt.c
> > +++ b/drivers/counter/rz-mtu3-cnt.c
> > @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
> > switch (count->id) {
> > case RZ_MTU3_16_BIT_MTU1_CH:
> > case RZ_MTU3_16_BIT_MTU2_CH:
> > - if (ceiling > U16_MAX)
> > - return -ERANGE;
> > + if (ceiling > U16_MAX) {
> > + ret = -ERANGE;
> > + goto unlock;
> > + }
> > priv->mtu_16bit_max[ch_id] = ceiling;
> > break;
> > case RZ_MTU3_32_BIT_CH:
> > - if (ceiling > U32_MAX)
> > - return -ERANGE;
> > + if (ceiling > U32_MAX) {
> > + ret = -ERANGE;
> > + goto unlock;
> > + }
> > priv->mtu_32bit_max = ceiling;
> > break;
> > default:
> > /* should never reach this path */
> > - mutex_unlock(&priv->lock);
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto unlock;
> > }
>
> Normally, I like "goto unlock" patterns, but I think in this context it
> makes the flow of code confusing with the mix-match of the switch cases;
> default case jumps with a goto, but RZ_MTU3_* cases passes will break,
> yet RZ_MUT3_* failures have a goto path. Rather than a "goto unlock"
> pattern, I'd prefer to see simply mutex_lock() called for these ceiling
> checks. That also has the benefit of reducing the number of changes we
> have to make for this fix.
Sure. I'll resend.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread