linux-kernel - Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

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:   Wed, 12 Oct 2022 10:40:38 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jagath Jog J <jagathjog1996@...il.com>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A
 accelerometer

On 10/10/22 16:20, Vaittinen, Matti wrote:
> On 10/10/22 14:58, Andy Shevchenko wrote:
>> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
>> ...
>>
>>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
>>>>> +			       sizeof(s16));
>>
>>>> No endianess awareness (sizeof __le16 / __be16)
>>
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	*val = data->buffer[0];
>>>>
>>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
>>>
>>> I have probably misunderstood something but I don't see why we should use
>>> 'endianess awareness' in drivers? I thought the IIO framework code takes
>>> care of the endianes conversions based on scan_type so each individual
>>> driver does not need to do that. That however has been just my assumption. I
>>> will need to check this. Thanks for pointing it out.
>>
>> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.

Following is some hand waving and speculation after my quick code read. 
So, I may be utterly wrong in which case please do correct me...

Anyways, it seems to me that you're correct. The endianness field is 
only used by the IIO to build the channel information for user-space so 
that applications reading data can parse it. As far as I understand, the 
driver does not need to do the conversions for user-space, but the 
user-space tools should inspect the type information and do the 
conversion. I think it makes sense as user-space applications may be 
better equipped to do some maths. It also may be some applications do 
not want to spend cycles doing the conversion but the conversions can be 
done later "offline" for the captured raw data. So omitting conversion 
in the IIO driver kind of makes sense to me.

I haven't thoroughly looked (and I have never used) the in-kernel IIO 
APIs for getting the data. A quick look at the 
include/linux/iio/consumer.h allows me to assume the iio_chan_spec can 
be obtained by the consumer drivers. This should make the endianess 
information available for the consumer drivers as well. So, again, 
consumer drivers can parse the raw-format data themself.

I have this far only used the sysfs and iio_generic_buffer on a 
little-endian machine so I have had no issues with the little-endian 
data and I have only observed the code. Hence I can not really say if my 
reasoning is correct - or if it is how IIO has been designed to operate. 
But based on my quick study I don't see a need for the IIO driver to do 
endianess conversion to any other format but what is indicated by 
scan_type. Specifically for KX022A, the data is already 16B LE when read 
from the sensor. This is also advertised by scan_type so no conversion 
should be needed (unless, of course, I am mistaken :]).

>> And it does nothing with it. Maybe Jonathan can shed a light what is it for
>> (I mean the field)?
>>

I agree. It'd be great to listen to someone who actually knows what he 
is talking about and is not just guessing as I am ^_^;

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~