linux-kernel - Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

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] [day] [month] [year] [list]

Date: Sat, 29 Jun 2024 15:13:10 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
	lars@...afoo.de, Michael.Hennerich@...log.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	nuno.sa@...log.com, dlechner@...libre.com, corbet@....net,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-spi@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

On 06/29, Jonathan Cameron wrote:
> On Tue, 25 Jun 2024 18:55:27 -0300
> Marcelo Schmitt <marcelo.schmitt@...log.com> wrote:
> 
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation register (SAR) ADCs.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> Hi Marcelo,
> 
> You've clearly gotten some good review for this version so I only
> had a quick scan through.  One thing did jump out at me though.
> 
> > +
> > +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +				    struct iio_chan_spec const *chan, long mask)
> > +{
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		return IIO_VAL_INT_PLUS_NANO;
> > +	default:
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	}
> > +}
> > +
> > +static int ad4000_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int val, int val2,
> > +			    long mask)
> > +{
> > +	struct ad4000_state *st = iio_priv(indio_dev);
> > +	unsigned int reg_val;
> > +	bool span_comp_en;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		mutex_lock(&st->lock);
> > +		ret = ad4000_read_reg(st, &reg_val);
> > +		if (ret < 0)
> > +			goto err_unlock;
> > +
> > +		span_comp_en = val2 == st->scale_tbl[1][1];
> > +		reg_val &= ~AD4000_CFG_SPAN_COMP;
> > +		reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
> > +
> > +		ret = ad4000_write_reg(st, reg_val);
> > +		if (ret < 0)
> > +			goto err_unlock;
> > +
> > +		st->span_comp = span_comp_en;
> > +err_unlock:
> > +		iio_device_release_direct_mode(indio_dev);
> > +		mutex_unlock(&st->lock);
> 
> Lock ordering needs another look. I'm not sure we an trigger
> a deadlock but it definitely looks problematic. 

Oops. Oh, that's inddeed back lock release ordering.
I've changed to scoped and guard for v6 and will send the updated version soon.

Anyway, thanks for having a look at it.
Marcelo

> 
> J
> 
>