linux-kernel - Re: [PATCH v3 4/4] iio: adc: ad9467: check for backend capabilities

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: <4b84e4ec-7d42-4a77-b521-cb41a8623a24@vaisala.com>
Date: Wed, 14 Jan 2026 17:24:23 +0200
From: Tomas Melin <tomas.melin@...sala.com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>,
 Nuno Sa <nuno.sa@...log.com>, Lars-Peter Clausen <lars@...afoo.de>,
 Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
 Andy Shevchenko <andy@...nel.org>,
 Olivier Moysan <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] iio: adc: ad9467: check for backend capabilities

Hi,

On 14/01/2026 14:45, Jonathan Cameron wrote:
> On Wed, 14 Jan 2026 10:45:53 +0000
> Tomas Melin <tomas.melin@...sala.com> wrote:
> 
>> Add capability checks for operation with backends that do not support
>> full set of features, but are otherwise compatible with the device.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@...sala.com>
> Hi Tomas,
> 
> A few comments inline around when we should or should not return errors.
> 
>> ---
>>  drivers/iio/adc/ad9467.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
>> index 9cfe66425d4e91e215cccc40e24a92c5e99e9b87..349779a049ad68b4c9f72abfc40154b4a3f2e095 100644
>> --- a/drivers/iio/adc/ad9467.c
>> +++ b/drivers/iio/adc/ad9467.c
>> @@ -645,6 +645,9 @@ static int ad9467_backend_testmode_on(struct ad9467_state *st,
>>  	};
>>  	int ret;
>>  
>> +	if (!iio_backend_has_caps(st->back, IIO_BACKEND_CAP_CALIBRATION))
>> +		return 0;
> 
> If it isn't supported then isn't any attempt to turn test_mode on an error?
> I would return errors from all these calls and only decide it doesn't
> matter right at the top of the call stack.
I was considering this option too, I will change accordingly.

> 
> In this particular case that's either calibration mode stuff (covered by check
> below) or ad9467_chan_test_mod_write()  I'd just add a top level check there
> on ability to do calibration. Alternative would be to not register the debugfs
> at all.  Challenge is that is a small ABI break.  Do we care?  If we had
> been doing this from the start we would have just hidden the pointless
> interface.
> (Key is we can change userspace interfaces, as long no one notices!)

The testmodes from device side should still at least be there, only a
few of the modes currently have a backend mode, too.

> 
>> +
>>  	ret = iio_backend_data_format_set(st->back, chan, &data);
>>  	if (ret)
>>  		return ret;
>> @@ -665,6 +668,9 @@ static int ad9467_backend_testmode_off(struct ad9467_state *st,
>  	};
>>  	int ret;
>>  
>> +	if (!iio_backend_has_caps(st->back, IIO_BACKEND_CAP_CALIBRATION))
>> +		return 0;
>> +
>>  	ret = iio_backend_chan_disable(st->back, chan);
>>  	if (ret)
>>  		return ret;
>> @@ -807,6 +813,9 @@ static int ad9467_calibrate(struct ad9467_state *st)
>>  	bool invert = false, stat;
>>  	int ret;
>>  
>> +	if (!iio_backend_has_caps(st->back, IIO_BACKEND_CAP_CALIBRATION))
>> +		return 0;
> 
> So this the idea of a stub calibration bothers me a little, but I think
> it's more acceptable to do it for this top level call. So keep this one, but
> all the others look to me like they should be returning errors.
OK
> 
>> +
>>  	/* all points invalid */
>>  	bitmap_fill(st->calib_map, st->calib_map_size);
>>  
>> @@ -1357,7 +1366,7 @@ static int ad9467_probe(struct spi_device *spi)
>>  		return ret;
>>  
>>  	ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
>> -	if (ret)
>> +	if (ret && ret != -EOPNOTSUPP)
> 
> I don't see any modification of devm_iio_backend_request_buffer() in this
> set so why why is it now acceptable to return not supported?
> Given the capabilities stuff added, should this not be another capability
> we check before trying to request the buffer?
Basically because I reasoned that capabilities are more like features
rather than single operation. The single operation tells if the
capability is there. I reasoned a bit more about this in a related
answer replying to Nuno, hopefully you find time to read that
explanation and provide your thoughts.

And reason it would now be acceptable is that the original code does not
account for any optional features within the backends, but now it does.
It knows it can work without the buffering, and the return value can
tell if it is supported or not.

Thanks,
Tomas



> 
>>  		return ret;
>>  
>>  	ret = devm_iio_backend_enable(&spi->dev, st->back);
>>
>