linux-kernel - [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 for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Apr 2020 11:24:55 +0300
From:   Alexandru Ardelean <alexandru.ardelean@...log.com>
To:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     <jic23@...nel.org>, <lars@...afoo.de>,
        <Eugen.Hristev@...rochip.com>, <Ludovic.Desroches@...rochip.com>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>
Subject: [RFC PATCH] iio: __iio_update_buffers: Update mode before preenable/after postdisable

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.
-------------------------------------------------------------------

 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