On 02/12/2024 19:00, Christian Eggers wrote:
> Hi Jonathan, hi Javier,
>
> On Monday, 2 December 2024, 16:38:50 CET, Javier Carrasco wrote:
>> On 30/11/2024 21:49, Jonathan Cameron wrote:
>>> On Mon, 25 Nov 2024 22:16:18 +0100
>>> Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
>>>
>>>> The 'scan' local struct is used to push data to userspace from a
>>>> triggered buffer, but it leaves the first channel uninitialized if
>>>> AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel
>>>> readings.
>>>>
>>>> Set the temperature channel to zero if only color channels are
>>>> relevant to avoid pushing uninitialized information to userspace.
>>>>
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
>>> Huh.
>>>
>>> If the temperature channel is turned off the data should shift. So should be read
>>> into scan.chan[0] and [1] and [2], but not [3].
>>>
>>> Not skipping [0] as here.
>>>
>>> So this code path currently doesn't work as far as I can tell.
>
> I've just tested and you are right! In our application we never had the case that
> we didn't read the temperature channel. If I don't enable scan_elements/in_temp_en,
> I need to put the data into scan.chan[0..2] in order to get correct values in my
> application. This also means that the "Optimization for reading only color channel"
> (and the following saturation block) isn't correct at all, especially if reading only
> one or two of the available channels.
>
>>>
>>> Jonathan
>>>
>>>> ---
>>>> drivers/iio/light/as73211.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
>>>> index be0068081ebb..99679b686146 100644
>>>> --- a/drivers/iio/light/as73211.c
>>>> +++ b/drivers/iio/light/as73211.c
>>>> @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>>>> (char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
>>>> if (ret < 0)
>>>> goto done;
>>>> +
>>>> + /* Avoid leaking uninitialized data */
>>>> + scan.chan[0] = 0;
>>>> }
>>>>
>>>> if (data_result) {
>>>>
>>>
>>
>> Adding the driver maintainer (should have been added from the beginning)
>> to the conversation.
>>
>> @Christian, could you please confirm this?
>>
>> Apparently, the optimization to read the color channels without
>> temperature is not right. I don't have access to the AS7331 at the
>> moment, but I remember that you could test my patches on your hardware
>> with an AS73211, so maybe you can confirm whether wrong data is
>> delivered or not in that case.
>
> Yes, the delivered data is wrong (as already stated above).
>
> @Javier: If you like to rework this, I can test your patches (I have still
> access to the hardware). Otherwise I can also try to fix this on my own.
>
>>
>> Thanks and best regards,
>> Javier Carrasco
>
> Thanks for reporting this!
> Christian
>>
>>
>
Thanks for your prompt reply. I will rework it for v2, as the current
patch does not apply. For this path, scan.chan[0]..scan.chan[2] will be
read from the sensor, and scan.chan[3] will be set to zero.
Best regards,
Javier Carrasco
