linux-kernel - Re: [PATCH 2/5] staging: iio: ad9832: convert to guard(mutex)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251221192253.5e2b29b4@jic23-huawei>
Date: Sun, 21 Dec 2025 19:22:53 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Tomas Borquez <tomasborquez13@...il.com>
Cc: Marcelo Schmitt <marcelo.schmitt1@...il.com>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH 2/5] staging: iio: ad9832: convert to guard(mutex)

On Thu, 18 Dec 2025 12:16:32 -0300
Tomas Borquez <tomasborquez13@...il.com> wrote:

> On Thu, Dec 18, 2025 at 11:34:05AM -0300, Marcelo Schmitt wrote:
> > On 12/15, Tomas Borquez wrote:  
> ...
> > > -	mutex_lock(&st->lock);
> > > +	guard(mutex)(&st->lock);
> > >  	switch ((u32)this_attr->address) {
> > >  	case AD9832_FREQ0HM:
> > >  	case AD9832_FREQ1HM:
> > > @@ -203,22 +205,18 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> > >  		ret = spi_sync(st->spi, &st->msg);
> > >  		break;
> > >  	case AD9832_FREQ_SYM:
> > > -		if (val == 1 || val == 0) {
> > > -			st->ctrl_fp &= ~AD9832_FREQ;
> > > -			st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
> > > -		} else {
> > > -			ret = -EINVAL;
> > > -			break;
> > > -		}
> > > +		if (val != 1 && val != 0)
> > > +			return -EINVAL;
> > > +
> > > +		st->ctrl_fp &= ~AD9832_FREQ;
> > > +		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
> > >  		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
> > >  						  st->ctrl_fp);
> > >  		ret = spi_sync(st->spi, &st->msg);
> > >  		break;  
> > Since we now have the mutex unlock handled by guard, why not returning directly
> > from each case?
> > E.g.
> > 	case AD9832_FREQ1HM:
> > -		ret = ad9832_write_frequency(st, this_attr->address, val);
> > -		break;
> > +		return ad9832_write_frequency(st, this_attr->address, val);
> > I think the last return (outside the default clause) won't be needed anymore.  
> Wouldn't work because we need to return len too, so it would be more like:
> 
>  case AD9832_FREQ1HM:
> 		 ret = ad9832_write_frequency(st, this_attr->address, val);
> +    return ret ?: len;

I'd prefer the error dealt with locally as it's more consistent with the other
paths in the code where we return directly.  The fact this is the last one
in each block doesn't to me mean we should bother to handle it in a special
way.

You could stick to
		if (ret)
			return ret;

		break;
	...
	}

	return len;

If you prefer to keep that shared return of len in the good path. Or just
return it as you suggested in each case: block using the ternary.

Jonathan



> 
> Which is a bit more repetitive than just returning at the end ret ?: len,
> but lemme know what you think.
> 
> >
> > And, since you are touching the lock, you may also update to use
> > devm_mutex_init() (that would probably be best appreciated as a separate patch).  
> 
> Yup, sounds good :)
> 
> Tomas