linux-kernel - Re: [RFC PATCH] iio: __iio_update_buffers: Update mode before preenable/after postdisable

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: <8551295e-c166-674e-c449-61ccb5bbef76@microchip.com>
Date:   Thu, 14 May 2020 10:56:16 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <jic23@...nel.org>
CC:     <alexandru.ardelean@...log.com>, <linux-iio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <lars@...afoo.de>,
        <Ludovic.Desroches@...rochip.com>
Subject: Re: [RFC PATCH] iio: __iio_update_buffers: Update mode before
 preenable/after postdisable

On 04.05.2020 15:27, Eugen Hristev - M18282 wrote:
> On 30.04.2020 11:42, Eugen Hristev - M18282 wrote:
>> On 30.04.2020 11:24, Alexandru Ardelean wrote:
>>> From: Lars-Peter Clausen <lars@...afoo.de>
>>>
>>> It is clear that we transition to INDIO_DIRECT_MODE when disabling the
>>> buffer(s) and it is also clear that we transition from INDIO_DIRECT_MODE
>>> when enabling the buffer(s). So leaving the currentmode field
>>> INDIO_DIRECT_MODE until after the preenable() callback and updating it to
>>> INDIO_DIRECT_MODE before the postdisable() callback doesn't add additional
>>> value. On the other hand some drivers will need to perform different
>>> actions depending on which mode the device is going to operate in/was
>>> operating in.
>>>
>>> Moving the update of currentmode before preenable() and after postdisable()
>>> enables us to have drivers which perform mode dependent actions in those
>>> callbacks.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
>>> ---
>>>
>>> This patch is also a V2 of this older patch from a while ago:
>>>     https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-lars@metafoo.de/
>>>
>>> However, in this recent context, it comes to fix this:
>>>     https://lore.kernel.org/linux-iio/b9ab676489de3575984dac5610fcf05fd8742a38.camel@analog.com/T/#mc09284c8f79250b92a52fd5b8d1f541d1c02c0c0
>>>
>>> At this point, I don't have a clear idea if this approach is good or
>>> not; since the motivation is to fix the at91 adc.
>>> Hence the RFC.
>>>
>>> Some excerpt from the AT91 discussion:
>>> -------------------------------------------------------------------
>>> I decided to do a bit of shell magic for this:
>>>
>>> get_files() {
>>> git grep -w iio_buffer_setup_ops  | grep drivers | cut -d: -f1 | sort | uniq
>>> }
>>>
>>> for file in $(get_files) ; do
>>>        if grep -q currentmode $file ; then
>>>            echo $file
>>>        fi
>>> done
>>>
>>> It finds 4 drivers.
>>> Though, `get_files()` will return 56 files.
>>>
>>> drivers/iio/accel/bmc150-accel-core.c
>>> drivers/iio/adc/at91-sama5d2_adc.c
>>> drivers/iio/adc/stm32-dfsdm-adc.c
>>> drivers/iio/magnetometer/rm3100-core.c
>>>
>>> The rm3100 driver doesn't do any checks in the setup_ops for 'currentmode' as
>>> far as I could see.
>>>
>>> So, Lars' patch could work nicely to fix this current case and not break others.
>>>
>>> Semantically though, it would sound nicer to have a 'nextmode' parameter
>>> somewhere; maybe on the setup_ops(indio_dev, nextmode)?
>>> Though, only those 3 drivers would really ever use it; so doing it like that
>>> sounds like overkill.
>>>
>>> So, we're left with Lars' patch or we could add an 'indio_dev->nextmode' field,
>>> that may be used in just these 3 drivers [which again: sounds overkill at this
>>> point in time].
>>>
>>> Alternatively, this 'indio_dev->currentmode' could be removed from all these 3
>>> drivers somehow. But that needs testing and a thorough understanding of all 3
>>> drivers and what they're doing, to do properly.
>>> -------------------------------------------------------------------
>>
>> Hi Alex,
>>
>> Thanks for finding this. I will test this with the at91-sama5d2_adc
>> driver on Jonathan's testing branch. I will let you know of the results.
>>
>> Eugen
> 
> Hi,
> 
> For sama5d2-xplained, hw trigger testing, at91-sama5d2_adc driver,
> Tested-by: Eugen Hristev <eugen.hristev@...rochip.com>
> 
> I did not get my hands on the touchscreen yet, but hopefully soon.
> 
> Thanks again,
> Eugen

Hello Jonathan,

Without this patch, your current testing branch is broken for hardware 
trigger for at91-sama5d2_adc (including for-5.8b tag)

You want me to create a patch that fixes that in the driver, or you will 
take this patch maybe ?

Thanks !
Eugen
>>
>>>
>>>     drivers/iio/industrialio-buffer.c | 8 +++-----
>>>     1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index 30af8af8f312..efcc44b62946 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -989,6 +989,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>>>            indio_dev->active_scan_mask = config->scan_mask;
>>>            indio_dev->scan_timestamp = config->scan_timestamp;
>>>            indio_dev->scan_bytes = config->scan_bytes;
>>> +       indio_dev->currentmode = config->mode;
>>>
>>>            iio_update_demux(indio_dev);
>>>
>>> @@ -1024,8 +1025,6 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>>>                            goto err_disable_buffers;
>>>            }
>>>
>>> -       indio_dev->currentmode = config->mode;
>>> -
>>>            if (indio_dev->setup_ops->postenable) {
>>>                    ret = indio_dev->setup_ops->postenable(indio_dev);
>>>                    if (ret) {
>>> @@ -1042,10 +1041,10 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>>>                                                 buffer_list)
>>>                    iio_buffer_disable(buffer, indio_dev);
>>>     err_run_postdisable:
>>> -       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>>            if (indio_dev->setup_ops->postdisable)
>>>                    indio_dev->setup_ops->postdisable(indio_dev);
>>>     err_undo_config:
>>> +       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>>            indio_dev->active_scan_mask = NULL;
>>>
>>>            return ret;
>>> @@ -1080,8 +1079,6 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>>>                            ret = ret2;
>>>            }
>>>
>>> -       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>> -
>>>            if (indio_dev->setup_ops->postdisable) {
>>>                    ret2 = indio_dev->setup_ops->postdisable(indio_dev);
>>>                    if (ret2 && !ret)
>>> @@ -1090,6 +1087,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>>>
>>>            iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
>>>            indio_dev->active_scan_mask = NULL;
>>> +       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>>
>>>            return ret;
>>>     }
>>> --
>>> 2.17.1
>>>
>>
>