linux-kernel - Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

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: <248b009e-0401-4531-b9f0-56771e16bdef@baylibre.com>
Date: Mon, 1 Dec 2025 15:53:19 -0600
From: David Lechner <dlechner@...libre.com>
To: Kurt Borja <kuurtb@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Tobias Sperling <tobias.sperling@...ting.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

On 12/1/25 1:47 PM, Kurt Borja wrote:
> On Mon Dec 1, 2025 at 11:07 AM -05, David Lechner wrote:
> 
> ...
> 
>>>>> +	if (iio_device_claim_buffer_mode(indio_dev))
>>>>> +		goto out_notify_done;
>>>>> +
>>>>> +	if (iio_trigger_using_own(indio_dev)) {
>>>>> +		disable_irq(ads1018->drdy_irq);
>>>>> +		ret = ads1018_read_unlocked(ads1018, &scan.conv, true);
>>>>> +		enable_irq(ads1018->drdy_irq);
>>>>> +	} else {
>>>>> +		ret = spi_read(ads1018->spi, ads1018->rx_buf, sizeof(ads1018->rx_buf));
>>>>> +		scan.conv = ads1018->rx_buf[0];
>>>>> +	}
>>>>> +
>>>>> +	iio_device_release_buffer_mode(indio_dev);
>>>>> +
>>>>> +	if (ret)
>>>>> +		goto out_notify_done;
>>>>> +
>>>>> +	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
>>>>> +
>>>>> +out_notify_done:
>>>>> +	iio_trigger_notify_done(ads1018->indio_trig);
>>>>
>>>> Jonathan et al., maybe we need an ACQUIRE() class for this? It will solve
>>>> the conditional scoped guard case, no?
>>
>> No, ACQUIRE() is not scoped, just conditional. I don't think it
>> will improve anything here.
> 
> Maybe I'm not understanding the problem fully?
> 
> I interpreted "ACQUIRE() class" as a general GUARD class, i.e.
> 	
> 	guard(iio_trigger_notify)(indio_dev->trig);
> 
> This way drivers may use other cleanup.h helpers cleaner, because of the
> goto problem?
> 
> I do think it's a good idea, like a `defer` keyword. But it is a bit
> unorthodox using guard for non locks.
> 
> 

To take a simple example first:

static int
ads1018_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
		 int *val, int *val2, long mask)
{
	int ret;

	if (!iio_device_claim_direct(indio_dev))
		return -EBUSY;

	ret = ads1018_read_raw_unlocked(indio_dev, chan, val, val2, mask);

	iio_device_release_direct(indio_dev);

	return ret;
}

using ACQUIRE would look like:

static int
ads1018_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
		 int *val, int *val2, long mask)
{
	int ret;

	ACQUIRE(iio_device_claim_direct_mode, claim)(indio_dev);
	if ((ret = ACQUIRE_ERR(iio_device_claim_direct_mode, &claim)))
		return ret;

	return ads1018_read_raw_unlocked(indio_dev, chan, val, val2, mask);
}

It makes it quite more verbose IMHO with little benefit (the direct
return is nice, but comes at at an expense of the rest being less
readable).



And when we need it to be scoped, it adds indent and we have to do
some unusual things still to avoid using goto.

static irqreturn_t ads1018_trigger_handler(int irq, void *p)
{
	struct iio_poll_func *pf = p;
	struct iio_dev *indio_dev = pf->indio_dev;
	struct ads1018 *ads1018 = iio_priv(indio_dev);
	struct {
		__be16 conv;
		aligned_s64 ts;
	} scan = {};
	int ret;

	do {
		ACQUIRE(iio_device_claim_direct_mode, claim)(indio_dev);
		if ((ret = ACQUIRE_ERR(iio_device_claim_direct_mode, &claim)))
			break;

		if (iio_trigger_using_own(indio_dev)) {
			disable_irq(ads1018->drdy_irq);
			ret = ads1018_read_unlocked(ads1018, &scan.conv, true);
			enable_irq(ads1018->drdy_irq);
		} else {
			ret = spi_read(ads1018->spi, ads1018->rx_buf, sizeof(ads1018->rx_buf));
			scan.conv = ads1018->rx_buf[0];
		}
	} while (0);

	if (!ret)
		iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);

	iio_trigger_notify_done(ads1018->indio_trig);

	return IRQ_HANDLED;
}

So unless Jonathan says this is what he wants, I would avoid it.