Re: [PATCH] counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> 
> 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




[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux