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

Date: Sat, 29 Jun 2024 19:05:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <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>, <marcelo.schmitt1@...il.com>,
 <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 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. 

J