linux-kernel - Re: [PATCH 1/2] iio: light: ltr390: Add sysfs attribute to report data freshness

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: <8f924da6-c5f6-4f88-9cb1-3e7e1aae491b@baylibre.com>
Date: Tue, 22 Jul 2025 16:15:55 -0500
From: David Lechner <dlechner@...libre.com>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: anshulusr@...il.com, jic23@...nel.org, nuno.sa@...log.com,
 andy@...nel.org, shuah@...nel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: light: ltr390: Add sysfs attribute to report
 data freshness

On 7/21/25 10:55 PM, Akshay Jindal wrote:
> Thanks David for the swift and valuable feedback. Please find replies inline.
> 
> On Tue, Jul 22, 2025 at 1:55 AM David Lechner <dlechner@...libre.com> wrote:
>>
>> For debugging, using debugfs would make more sense.
> Will it make sense if I simply give register access via debugfs?
> Something like this:
> static const struct iio_info ad7606_info_sw_mode = {
>     .read_raw = &ad7606_read_raw,
>     .write_raw = &ad7606_write_raw,
>     .debugfs_reg_access = &ad7606_reg_access,<---------
>     .validate_trigger = &ad7606_validate_trigger,
>     .update_scan_mode = &ad7606_update_scan_mode,
> };
> This way the information about data status will be available on demand,
> without exposing any special sysfs attribute.

Yes, this would be fine.

>>
>> For application level filtering, why does it matter if we have
>> read the same value before or not? The sampling_frequency is
>> available, so the application should be able to deduce when
>> fresh data should be available based on time.
> Agreed.
>>
>> I could see maybe polling this in the kernel in order to implement
>> a buffered capture mode, but not sure passing this on to userspace
>> is the best way to go about it.
> I had sent 2 patches. This was my PATCH 1 of 2. In patch 2 of 2,
> I have done something similar to what you mentioned, not exactly polling,

Here is what the IIO docs have to say about it (reading the in_light_raw
attribute is considered INDIO_DIRECT_MODE):

 * @INDIO_DIRECT_MODE: There is an access to either:
 * a) The last single value available for devices that do not provide
 *    on-demand reads.
 * b) A new value after performing an on-demand read otherwise.
 * On most devices, this is a single-shot read. On some devices with data
 * streams without an 'on-demand' function, this might also be the 'last value'
 * feature. Above all, this mode internally means that we are not in any of the
 * other modes, and sysfs reads should work.
 * Device drivers should inform the core if they support this mode.

So I don't think the current implementation is really in violation
of this.

Additionally, changes you proposed in patch 2/2 would break existing
users by returning an error when it didn't used to, so we couldn't
accept that.

If this was a new driver, we might consider having the raw read
poll for fresh data so that each read was guaranteed to be new
data, but even that change in timing could break existing users,
so probably not something we would want to do now.

I still think there is a reasonable workaround of just having userspace
wait for the sample period between reads to guarantee fresh data
without changing the driver at all.

> but reading the data status bit before reading actual data.
> Both patches are independent. Please have a look at that as well.
> 
> Thanks,
> Akshay