* [PATCH v3 0/5] iio: fix possible race condition during access of available info lists
@ 2024-10-15 11:06 Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 1/5] iio: core: add read_avail_release_resource callback to fix race Matteo Martelli
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Matteo Martelli @ 2024-10-15 11:06 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
Sebastian Reichel
Cc: linux-iio, linux-kernel, linux-mips, linux-pm, Matteo Martelli
Some iio drivers currently share an available info list buffer that
might be changed while iio core prints it to sysfs. This could cause the
buffer shared with iio core to be corrupted. However, note that I was
able to trigger the race condition only by adding a delay between each
sysfs_emit_at calls in the iio_format_list() to force the concurrent
access to the shared available list buffer.
This patch set extends the iio APIs and fixes some affected drivers.
Summary:
- Patch 1: iio core: introduce a iio info release callback to let
drivers share a copy of their available info list and later free it.
- Patch 2:
- inkern: make consumers copy and release the available info lists
of their producers, necessary after patch 1.
- iio-mux, iio-rescale, dpot-dac, ingenic-battery: adapt consumers
to inkern API change by freeing the now copied available lists of
their producers.
- Patch 3: pac1921: handle the current scale available info via the
read_avail+read_avail_release_resource APIs instead of using an ad-hoc
ext_info attribute. The latter was used to avoid the risk of a race in
the available list.
- Patch 4,5: ad7192, as73211: fix the possible race in the drivers by
copying/releasing the affected available lists.
Tested:
- pac1921: could not reproduce the race condition with the new APIs,
even with additional delays among the sysfs_emit_at calls during a
shunt resistor write. No new issue found after the change.
- iio-mux, iio-rescale, dpot-dac: tested with pac1921 as producer, which
was adapted to produce a mock raw available info list.
The tests did not cover the driver features but focused on assessing
the function call sequence. For example the following traced function
graph shows a read of the dpot mocked out voltage (with ftrace
filters: pac1921* iio* dpot* kmemdup_array* kfree*):
3) | iio_read_channel_info_avail [industrialio]() {
3) | dpot_dac_read_avail [dpot_dac]() {
3) | iio_read_avail_channel_raw [industrialio]() {
3) | iio_channel_read_avail [industrialio]() {
3) | pac1921_read_avail [pac1921]() {
3) 5.208 us | kmemdup_array();
3) + 11.459 us | }
3) 3.167 us | kmemdup_array();
3) | pac1921_read_avail_release_res [pac1921]() {
3) 1.709 us | kfree();
3) 4.458 us | }
3) + 25.750 us | }
3) + 31.792 us | }
3) + 35.000 us | }
3) + 37.083 us | iio_format_list [industrialio]();
3) | dpot_dac_read_avail_release_res [dpot_dac]() {
3) 1.583 us | kfree();
3) 4.250 us | }
3) + 84.292 us | }
- ingenic-battery: also tested with mock available info produced by the
pac1921 driver. Following the traced graph part that should correspond
to the ingenic_battery_set_scale() flow (which is not traceable with
the additional ingenic* ftrace filter for some reason):
2) | ingenic_battery_probe [ingenic_battery]() {
...
2) | iio_read_max_channel_raw [industrialio]() {
2) | iio_channel_read_avail [industrialio]() {
2) | pac1921_read_avail [pac1921]() {
2) 4.333 us | kmemdup_array();
2) + 10.834 us | }
2) 3.500 us | kmemdup_array();
2) | pac1921_read_avail_release_res [pac1921]() {
2) 1.791 us | kfree();
2) 4.625 us | }
2) + 26.291 us | }
2) 1.583 us | kfree();
2) + 35.750 us | }
2) | iio_read_avail_channel_attribute [industrialio]() {
2) | iio_channel_read_avail [industrialio]() {
2) | pac1921_read_avail [pac1921]() {
2) 3.250 us | kmemdup_array();
2) 8.209 us | }
2) 3.458 us | kmemdup_array();
2) | pac1921_read_avail_release_res [pac1921]() {
2) 1.542 us | kfree();
2) 4.292 us | }
2) + 21.417 us | }
2) + 26.333 us | }
2) | iio_write_channel_attribute [industrialio]() {
2) 4.375 us | pac1921_write_raw [pac1921]();
2) 9.625 us | }
2) 1.666 us | kfree();
2) * 47810.08 us | }
Not tested:
- ad7192, as73211
Link: https://lore.kernel.org/linux-iio/20240724-iio-pac1921-v4-0-723698e903a3@gmail.com/
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
Changes in v3:
- Rebased on top of iio-togreg
- Squash and reorder commits to allow bisection without memleaks
- Edit summary in cover letter to match new patch order
- Patch 2: inkern: add comment to clarify the need of the producer's buffer copy
- Patch 5: as73211: update comment on mutex declaration
- Link to v2: https://lore.kernel.org/r/20241007-iio-read-avail-release-v2-0-245002d5869e@gmail.com
Changes in v2:
- Patch 4: as73211: remove one blank line
- Patch 6: consumers: fix typo in commit message
- Patch 7: ingenic-battery: add missing header include
- Link to v1: https://lore.kernel.org/r/20241003-iio-read-avail-release-v1-0-c70cc7d9c2e0@gmail.com
---
Matteo Martelli (5):
iio: core: add read_avail_release_resource callback to fix race
iio: consumers: copy/release available info from producer to fix race
iio: pac1921: use read_avail+release APIs instead of custom ext_info
iio: ad7192: copy/release available filter frequencies to fix race
iio: as73211: copy/release available integration times to fix race
drivers/iio/adc/ad7192.c | 22 +++++-
drivers/iio/adc/pac1921.c | 128 ++++++++++++---------------------
drivers/iio/afe/iio-rescale.c | 8 +++
drivers/iio/dac/dpot-dac.c | 8 +++
drivers/iio/industrialio-core.c | 14 +++-
drivers/iio/inkern.c | 68 +++++++++++++-----
drivers/iio/light/as73211.c | 25 +++++--
drivers/iio/multiplexer/iio-mux.c | 8 +++
drivers/power/supply/ingenic-battery.c | 17 +++--
include/linux/iio/consumer.h | 4 +-
include/linux/iio/iio.h | 4 ++
11 files changed, 191 insertions(+), 115 deletions(-)
---
base-commit: c3e9df514041ec6c46be83801b1891392f4522f7
change-id: 20240802-iio-read-avail-release-cb3d2a1e1b98
Best regards,
--
Matteo Martelli <matteomartelli3@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/5] iio: core: add read_avail_release_resource callback to fix race
2024-10-15 11:06 [PATCH v3 0/5] iio: fix possible race condition during access of available info lists Matteo Martelli
@ 2024-10-15 11:06 ` Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 2/5] iio: consumers: copy/release available info from producer " Matteo Martelli
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Matteo Martelli @ 2024-10-15 11:06 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
Sebastian Reichel
Cc: linux-iio, linux-kernel, linux-mips, linux-pm, Matteo Martelli
Some iio drivers currently share an available info buffer that might be
changed while iio core prints it to sysfs. To avoid the buffer
corruption, add a release callback to let iio drivers allocate a copy of
the available info buffer and later free it in the release callback.
Such control is kept in the driver logic so that some driver that needs
a big available info buffer might also perform some check to keep the
copied buffer around in case no race has occurred.
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
drivers/iio/industrialio-core.c | 14 +++++++++++---
include/linux/iio/iio.h | 4 ++++
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 6a6568d4a2cb3a3f63381d5a6f25a2881b3ba2ed..4aea9de9f15a4d70f9d02fb3d47df49eef8c8423 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -859,12 +859,20 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
return ret;
switch (ret) {
case IIO_AVAIL_LIST:
- return iio_format_avail_list(buf, vals, type, length);
+ ret = iio_format_avail_list(buf, vals, type, length);
+ break;
case IIO_AVAIL_RANGE:
- return iio_format_avail_range(buf, vals, type);
+ ret = iio_format_avail_range(buf, vals, type);
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+
+ if (indio_dev->info->read_avail_release_resource)
+ indio_dev->info->read_avail_release_resource(
+ indio_dev, this_attr->c, vals, this_attr->address);
+
+ return ret;
}
/**
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 3a9b57187a958d6e65c699cf7814df5bac9a99e3..03bb765670a0c5f0129fc677c3a4a4cb38f4dad1 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -491,6 +491,10 @@ struct iio_info {
int *length,
long mask);
+ void (*read_avail_release_resource)(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask);
+
int (*write_raw)(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val,
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/5] iio: consumers: copy/release available info from producer to fix race
2024-10-15 11:06 [PATCH v3 0/5] iio: fix possible race condition during access of available info lists Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 1/5] iio: core: add read_avail_release_resource callback to fix race Matteo Martelli
@ 2024-10-15 11:06 ` Matteo Martelli
2024-10-16 21:08 ` Sebastian Reichel
2024-10-15 11:06 ` [PATCH v3 3/5] iio: pac1921: use read_avail+release APIs instead of custom ext_info Matteo Martelli
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Matteo Martelli @ 2024-10-15 11:06 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
Sebastian Reichel
Cc: linux-iio, linux-kernel, linux-mips, linux-pm, Matteo Martelli
Consumers need to call the producer's read_avail_release_resource()
callback after reading producer's available info. To avoid a race
condition with the producer unregistration, change inkern
iio_channel_read_avail() so that it copies the available info from the
producer and immediately calls its release callback with info_exists
locked.
Also, modify the users of iio_read_avail_channel_raw() and
iio_read_avail_channel_attribute() to free the copied available buffers
after calling these functions.
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
drivers/iio/afe/iio-rescale.c | 8 ++++
drivers/iio/dac/dpot-dac.c | 8 ++++
drivers/iio/inkern.c | 68 ++++++++++++++++++++++++++--------
drivers/iio/multiplexer/iio-mux.c | 8 ++++
drivers/power/supply/ingenic-battery.c | 17 ++++++---
include/linux/iio/consumer.h | 4 +-
6 files changed, 90 insertions(+), 23 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 56e5913ab82d1c045c9ca27012008a4495502cbf..78bb86c291706748b4072a484532ad20c415ff9f 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -249,9 +249,17 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
}
}
+static void rescale_read_avail_release_res(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask)
+{
+ kfree(vals);
+}
+
static const struct iio_info rescale_info = {
.read_raw = rescale_read_raw,
.read_avail = rescale_read_avail,
+ .read_avail_release_resource = rescale_read_avail_release_res,
};
static ssize_t rescale_read_ext_info(struct iio_dev *indio_dev,
diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
index f36f10bfb6be7863a56b911b5f58671ef530c977..43d68e17fc3a5fca59fad6ccf818eeadfecdb8c1 100644
--- a/drivers/iio/dac/dpot-dac.c
+++ b/drivers/iio/dac/dpot-dac.c
@@ -108,6 +108,13 @@ static int dpot_dac_read_avail(struct iio_dev *indio_dev,
return -EINVAL;
}
+static void dpot_dac_read_avail_release_res(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask)
+{
+ kfree(vals);
+}
+
static int dpot_dac_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val, int val2, long mask)
@@ -125,6 +132,7 @@ static int dpot_dac_write_raw(struct iio_dev *indio_dev,
static const struct iio_info dpot_dac_info = {
.read_raw = dpot_dac_read_raw,
.read_avail = dpot_dac_read_avail,
+ .read_avail_release_resource = dpot_dac_read_avail_release_res,
.write_raw = dpot_dac_write_raw,
};
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 7f325b3ed08fae6674245312cf8f57bb151006c0..7f50e33dc5084673aa66c25731add0c314cb477d 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -760,9 +760,29 @@ static int iio_channel_read_avail(struct iio_channel *chan,
if (!iio_channel_has_available(chan->channel, info))
return -EINVAL;
- if (iio_info->read_avail)
- return iio_info->read_avail(chan->indio_dev, chan->channel,
- vals, type, length, info);
+ if (iio_info->read_avail) {
+ const int *vals_tmp;
+ int ret;
+
+ ret = iio_info->read_avail(chan->indio_dev, chan->channel,
+ &vals_tmp, type, length, info);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Copy the producer's avail buffer with lock_exists locked to
+ * avoid possible race with producer unregistration.
+ */
+ *vals = kmemdup_array(vals_tmp, *length, sizeof(int), GFP_KERNEL);
+ if (!*vals)
+ return -ENOMEM;
+
+ if (iio_info->read_avail_release_resource)
+ iio_info->read_avail_release_resource(
+ chan->indio_dev, chan->channel, vals_tmp, info);
+
+ return ret;
+ }
return -EINVAL;
}
@@ -789,9 +809,11 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
ret = iio_read_avail_channel_attribute(chan, vals, &type, length,
IIO_CHAN_INFO_RAW);
- if (ret >= 0 && type != IIO_VAL_INT)
+ if (ret >= 0 && type != IIO_VAL_INT) {
/* raw values are assumed to be IIO_VAL_INT */
+ kfree(*vals);
ret = -EINVAL;
+ }
return ret;
}
@@ -820,24 +842,31 @@ static int iio_channel_read_max(struct iio_channel *chan,
if (val2)
*val2 = vals[5];
}
- return 0;
+ ret = 0;
+ break;
case IIO_AVAIL_LIST:
- if (length <= 0)
- return -EINVAL;
+ if (length <= 0) {
+ ret = -EINVAL;
+ goto out;
+ }
switch (*type) {
case IIO_VAL_INT:
*val = max_array(vals, length);
+ ret = 0;
break;
default:
/* TODO: learn about max for other iio values */
- return -EINVAL;
+ ret = -EINVAL;
}
- return 0;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+out:
+ kfree(vals);
+ return ret;
}
int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
@@ -876,24 +905,31 @@ static int iio_channel_read_min(struct iio_channel *chan,
if (val2)
*val2 = vals[1];
}
- return 0;
+ ret = 0;
+ break;
case IIO_AVAIL_LIST:
- if (length <= 0)
- return -EINVAL;
+ if (length <= 0) {
+ ret = -EINVAL;
+ goto out;
+ }
switch (*type) {
case IIO_VAL_INT:
*val = min_array(vals, length);
+ ret = 0;
break;
default:
/* TODO: learn about min for other iio values */
- return -EINVAL;
+ ret = -EINVAL;
}
- return 0;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+out:
+ kfree(vals);
+ return ret;
}
int iio_read_min_channel_raw(struct iio_channel *chan, int *val)
diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..31345437784b01c5d6f8ea70263f4c2574388e7a 100644
--- a/drivers/iio/multiplexer/iio-mux.c
+++ b/drivers/iio/multiplexer/iio-mux.c
@@ -142,6 +142,13 @@ static int mux_read_avail(struct iio_dev *indio_dev,
return ret;
}
+static void mux_read_avail_release_res(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask)
+{
+ kfree(vals);
+}
+
static int mux_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val, int val2, long mask)
@@ -171,6 +178,7 @@ static int mux_write_raw(struct iio_dev *indio_dev,
static const struct iio_info mux_info = {
.read_raw = mux_read_raw,
.read_avail = mux_read_avail,
+ .read_avail_release_resource = mux_read_avail_release_res,
.write_raw = mux_write_raw,
};
diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644
--- a/drivers/power/supply/ingenic-battery.c
+++ b/drivers/power/supply/ingenic-battery.c
@@ -12,6 +12,7 @@
#include <linux/platform_device.h>
#include <linux/power_supply.h>
#include <linux/property.h>
+#include <linux/slab.h>
struct ingenic_battery {
struct device *dev;
@@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
dev_err(bat->dev, "Unable to read channel avail scale\n");
return ret;
}
- if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
- return -EINVAL;
+ if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) {
+ ret = -EINVAL;
+ goto out;
+ }
max_mV = bat->info->voltage_max_design_uv / 1000;
@@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
if (best_idx < 0) {
dev_err(bat->dev, "Unable to find matching voltage scale\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
/* Only set scale if there is more than one (fractional) entry */
@@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
scale_raw[best_idx + 1],
IIO_CHAN_INFO_SCALE);
if (ret)
- return ret;
+ goto out;
}
- return 0;
+ ret = 0;
+out:
+ kfree(scale_raw);
+ return ret;
}
static enum power_supply_property ingenic_battery_properties[] = {
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 333d1d8ccb37f387fe531577ac5e0bfc7f752cec..e3e268d2574b3e01c9412449d90d627de7efcd84 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -316,7 +316,7 @@ int iio_read_min_channel_raw(struct iio_channel *chan, int *val);
/**
* iio_read_avail_channel_raw() - read available raw values from a given channel
* @chan: The channel being queried.
- * @vals: Available values read back.
+ * @vals: Available values read back. Must be freed after use.
* @length: Number of entries in vals.
*
* Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
@@ -334,7 +334,7 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
/**
* iio_read_avail_channel_attribute() - read available channel attribute values
* @chan: The channel being queried.
- * @vals: Available values read back.
+ * @vals: Available values read back. Must be freed after use.
* @type: Type of values read back.
* @length: Number of entries in vals.
* @attribute: info attribute to be read back.
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/5] iio: pac1921: use read_avail+release APIs instead of custom ext_info
2024-10-15 11:06 [PATCH v3 0/5] iio: fix possible race condition during access of available info lists Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 1/5] iio: core: add read_avail_release_resource callback to fix race Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 2/5] iio: consumers: copy/release available info from producer " Matteo Martelli
@ 2024-10-15 11:06 ` Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 4/5] iio: ad7192: copy/release available filter frequencies to fix race Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 5/5] iio: as73211: copy/release available integration times " Matteo Martelli
4 siblings, 0 replies; 9+ messages in thread
From: Matteo Martelli @ 2024-10-15 11:06 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
Sebastian Reichel
Cc: linux-iio, linux-kernel, linux-mips, linux-pm, Matteo Martelli
The pac1921 driver was exposing the available scale info via an ad-hoc
ext_info attribute instead of using the read_avail API. This to avoid a
possible race condition: while the available current scales were being
printed to sysfs by iio core (iio_read_channel_info_avail), the shunt
resistor might have been changed concurrently.
Switch to the read_avail+release APIs now that the race condition has
been addressed.
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
drivers/iio/adc/pac1921.c | 128 ++++++++++++++++------------------------------
1 file changed, 45 insertions(+), 83 deletions(-)
diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index a96fae546bc1e6d1bf3a0dbe67204c191d77a3ee..f6f8f9122a78d1b5e63d8184203eb3dae55eb560 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -444,11 +444,52 @@ static int pac1921_read_avail(struct iio_dev *indio_dev,
*vals = pac1921_int_num_samples;
*length = ARRAY_SIZE(pac1921_int_num_samples);
return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->channel) {
+ case PAC1921_CHAN_VBUS:
+ *vals = (const int *)pac1921_vbus_scales;
+ *length = ARRAY_SIZE(pac1921_vbus_scales) * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ return IIO_AVAIL_LIST;
+
+ case PAC1921_CHAN_VSENSE:
+ *vals = (const int *)pac1921_vsense_scales;
+ *length = ARRAY_SIZE(pac1921_vsense_scales) * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ return IIO_AVAIL_LIST;
+
+ case PAC1921_CHAN_CURRENT: {
+ struct pac1921_priv *priv = iio_priv(indio_dev);
+
+ *length = ARRAY_SIZE(priv->current_scales) * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+
+ guard(mutex)(&priv->lock);
+
+ *vals = kmemdup_array((int *)priv->current_scales,
+ *length, sizeof(int), GFP_KERNEL);
+ if (!*vals)
+ return -ENOMEM;
+
+ return IIO_AVAIL_LIST;
+ }
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
}
+static void pac1921_read_avail_release_res(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask)
+{
+ if (mask == IIO_CHAN_INFO_SCALE &&
+ chan->channel == PAC1921_CHAN_CURRENT)
+ kfree(vals);
+}
+
/*
* Perform configuration update sequence: set the device into read state, then
* write the config register and set the device back into integration state.
@@ -748,6 +789,7 @@ static int pac1921_read_event_value(struct iio_dev *indio_dev,
static const struct iio_info pac1921_iio = {
.read_raw = pac1921_read_raw,
.read_avail = pac1921_read_avail,
+ .read_avail_release_resource = pac1921_read_avail_release_res,
.write_raw = pac1921_write_raw,
.write_raw_get_fmt = pac1921_write_raw_get_fmt,
.read_label = pac1921_read_label,
@@ -805,88 +847,7 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
return len;
}
-/*
- * Emit on sysfs the list of available scales contained in scales_tbl
- *
- * TODO:: this function can be replaced with iio_format_avail_list() if the
- * latter will ever be exported.
- *
- * Must be called with lock held if the scales_tbl can change runtime (e.g. for
- * the current scales table)
- */
-static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2],
- size_t size, char *buf)
-{
- ssize_t len = 0;
-
- for (unsigned int i = 0; i < size; i++) {
- if (i != 0) {
- len += sysfs_emit_at(buf, len, " ");
- if (len >= PAGE_SIZE)
- return -EFBIG;
- }
- len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0],
- scales_tbl[i][1]);
- if (len >= PAGE_SIZE)
- return -EFBIG;
- }
-
- len += sysfs_emit_at(buf, len, "\n");
- return len;
-}
-
-/*
- * Read available scales for a specific channel
- *
- * NOTE: using extended info insted of iio.read_avail() because access to
- * current scales must be locked as they depend on shunt resistor which may
- * change runtime. Caller of iio.read_avail() would access the table unlocked
- * instead.
- */
-static ssize_t pac1921_read_scale_avail(struct iio_dev *indio_dev,
- uintptr_t private,
- const struct iio_chan_spec *chan,
- char *buf)
-{
- struct pac1921_priv *priv = iio_priv(indio_dev);
- const int (*scales_tbl)[2];
- size_t size;
-
- switch (chan->channel) {
- case PAC1921_CHAN_VBUS:
- scales_tbl = pac1921_vbus_scales;
- size = ARRAY_SIZE(pac1921_vbus_scales);
- return pac1921_format_scale_avail(scales_tbl, size, buf);
-
- case PAC1921_CHAN_VSENSE:
- scales_tbl = pac1921_vsense_scales;
- size = ARRAY_SIZE(pac1921_vsense_scales);
- return pac1921_format_scale_avail(scales_tbl, size, buf);
-
- case PAC1921_CHAN_CURRENT: {
- guard(mutex)(&priv->lock);
- scales_tbl = priv->current_scales;
- size = ARRAY_SIZE(priv->current_scales);
- return pac1921_format_scale_avail(scales_tbl, size, buf);
- }
- default:
- return -EINVAL;
- }
-}
-
-#define PAC1921_EXT_INFO_SCALE_AVAIL { \
- .name = "scale_available", \
- .read = pac1921_read_scale_avail, \
- .shared = IIO_SEPARATE, \
-}
-
-static const struct iio_chan_spec_ext_info pac1921_ext_info_voltage[] = {
- PAC1921_EXT_INFO_SCALE_AVAIL,
- {}
-};
-
static const struct iio_chan_spec_ext_info pac1921_ext_info_current[] = {
- PAC1921_EXT_INFO_SCALE_AVAIL,
{
.name = "shunt_resistor",
.read = pac1921_read_shunt_resistor,
@@ -910,6 +871,7 @@ static const struct iio_chan_spec pac1921_channels[] = {
.type = IIO_VOLTAGE,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all =
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
BIT(IIO_CHAN_INFO_SAMP_FREQ),
@@ -927,12 +889,12 @@ static const struct iio_chan_spec pac1921_channels[] = {
.indexed = 1,
.event_spec = pac1921_overflow_event,
.num_event_specs = ARRAY_SIZE(pac1921_overflow_event),
- .ext_info = pac1921_ext_info_voltage,
},
{
.type = IIO_VOLTAGE,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all =
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
BIT(IIO_CHAN_INFO_SAMP_FREQ),
@@ -950,12 +912,12 @@ static const struct iio_chan_spec pac1921_channels[] = {
.indexed = 1,
.event_spec = pac1921_overflow_event,
.num_event_specs = ARRAY_SIZE(pac1921_overflow_event),
- .ext_info = pac1921_ext_info_voltage,
},
{
.type = IIO_CURRENT,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all =
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
BIT(IIO_CHAN_INFO_SAMP_FREQ),
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 4/5] iio: ad7192: copy/release available filter frequencies to fix race
2024-10-15 11:06 [PATCH v3 0/5] iio: fix possible race condition during access of available info lists Matteo Martelli
` (2 preceding siblings ...)
2024-10-15 11:06 ` [PATCH v3 3/5] iio: pac1921: use read_avail+release APIs instead of custom ext_info Matteo Martelli
@ 2024-10-15 11:06 ` Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 5/5] iio: as73211: copy/release available integration times " Matteo Martelli
4 siblings, 0 replies; 9+ messages in thread
From: Matteo Martelli @ 2024-10-15 11:06 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
Sebastian Reichel
Cc: linux-iio, linux-kernel, linux-mips, linux-pm, Matteo Martelli
While available filter frequencies are being printed to sysfs by iio
core (iio_read_channel_info_avail), the sampling frequency might be
changed. This could cause the buffer shared with iio core to be
corrupted. To prevent it, make a copy of the filter frequencies buffer
and free it in the read_avail_release_resource callback.
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
drivers/iio/adc/ad7192.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 7042ddfdfc03ee5ea58ca07fb1943feb6538175b..acf625ced0b21db8d44f77929e8a875b3c10e1b1 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1056,12 +1056,19 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
*length = ARRAY_SIZE(st->scale_avail) * 2;
return IIO_AVAIL_LIST;
- case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
- *vals = (int *)st->filter_freq_avail;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
*type = IIO_VAL_FRACTIONAL;
*length = ARRAY_SIZE(st->filter_freq_avail) * 2;
+ guard(mutex)(&st->lock);
+
+ *vals = kmemdup_array((int *)st->filter_freq_avail, *length,
+ sizeof(int), GFP_KERNEL);
+ if (!*vals)
+ return -ENOMEM;
+
return IIO_AVAIL_LIST;
+ }
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
*vals = (int *)st->oversampling_ratio_avail;
*type = IIO_VAL_INT;
@@ -1073,6 +1080,14 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
return -EINVAL;
}
+static void ad7192_read_avail_release_res(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask)
+{
+ if (mask == IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY)
+ kfree(vals);
+}
+
static int ad7192_update_scan_mode(struct iio_dev *indio_dev, const unsigned long *scan_mask)
{
struct ad7192_state *st = iio_priv(indio_dev);
@@ -1098,6 +1113,7 @@ static const struct iio_info ad7192_info = {
.write_raw = ad7192_write_raw,
.write_raw_get_fmt = ad7192_write_raw_get_fmt,
.read_avail = ad7192_read_avail,
+ .read_avail_release_resource = ad7192_read_avail_release_res,
.attrs = &ad7192_attribute_group,
.validate_trigger = ad_sd_validate_trigger,
.update_scan_mode = ad7192_update_scan_mode,
@@ -1108,6 +1124,7 @@ static const struct iio_info ad7194_info = {
.write_raw = ad7192_write_raw,
.write_raw_get_fmt = ad7192_write_raw_get_fmt,
.read_avail = ad7192_read_avail,
+ .read_avail_release_resource = ad7192_read_avail_release_res,
.validate_trigger = ad_sd_validate_trigger,
};
@@ -1116,6 +1133,7 @@ static const struct iio_info ad7195_info = {
.write_raw = ad7192_write_raw,
.write_raw_get_fmt = ad7192_write_raw_get_fmt,
.read_avail = ad7192_read_avail,
+ .read_avail_release_resource = ad7192_read_avail_release_res,
.attrs = &ad7195_attribute_group,
.validate_trigger = ad_sd_validate_trigger,
.update_scan_mode = ad7192_update_scan_mode,
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 5/5] iio: as73211: copy/release available integration times to fix race
2024-10-15 11:06 [PATCH v3 0/5] iio: fix possible race condition during access of available info lists Matteo Martelli
` (3 preceding siblings ...)
2024-10-15 11:06 ` [PATCH v3 4/5] iio: ad7192: copy/release available filter frequencies to fix race Matteo Martelli
@ 2024-10-15 11:06 ` Matteo Martelli
4 siblings, 0 replies; 9+ messages in thread
From: Matteo Martelli @ 2024-10-15 11:06 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
Sebastian Reichel
Cc: linux-iio, linux-kernel, linux-mips, linux-pm, Matteo Martelli
While available integration times are being printed to sysfs by iio core
(iio_read_channel_info_avail), the sampling frequency might be changed.
This could cause the buffer shared with iio core to be corrupted. To
prevent it, make a copy of the integration times buffer and free it in
the read_avail_release_resource callback.
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
drivers/iio/light/as73211.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
index be0068081ebbbb37fdfb252b67a77b302ff725f6..c4c94873e6a1cc926cfb724d906b07222773c43f 100644
--- a/drivers/iio/light/as73211.c
+++ b/drivers/iio/light/as73211.c
@@ -108,7 +108,8 @@ struct as73211_spec_dev_data {
* @creg1: Cached Configuration Register 1.
* @creg2: Cached Configuration Register 2.
* @creg3: Cached Configuration Register 3.
- * @mutex: Keeps cached registers in sync with the device.
+ * @mutex: Keeps cached registers in sync with the device and protects
+ * int_time_avail concurrent access for updating and reading.
* @completion: Completion to wait for interrupt.
* @int_time_avail: Available integration times (depend on sampling frequency).
* @spec_dev: device-specific configuration.
@@ -493,17 +494,32 @@ static int as73211_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec co
*type = IIO_VAL_INT;
return IIO_AVAIL_LIST;
- case IIO_CHAN_INFO_INT_TIME:
+ case IIO_CHAN_INFO_INT_TIME: {
*length = ARRAY_SIZE(data->int_time_avail);
- *vals = data->int_time_avail;
*type = IIO_VAL_INT_PLUS_MICRO;
- return IIO_AVAIL_LIST;
+ guard(mutex)(&data->mutex);
+
+ *vals = kmemdup_array(data->int_time_avail, *length,
+ sizeof(int), GFP_KERNEL);
+ if (!*vals)
+ return -ENOMEM;
+
+ return IIO_AVAIL_LIST;
+ }
default:
return -EINVAL;
}
}
+static void as73211_read_avail_release_res(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int *vals, long mask)
+{
+ if (mask == IIO_CHAN_INFO_INT_TIME)
+ kfree(vals);
+}
+
static int _as73211_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan __always_unused,
int val, int val2, long mask)
@@ -699,6 +715,7 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
static const struct iio_info as73211_info = {
.read_raw = as73211_read_raw,
.read_avail = as73211_read_avail,
+ .read_avail_release_resource = as73211_read_avail_release_res,
.write_raw = as73211_write_raw,
};
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/5] iio: consumers: copy/release available info from producer to fix race
2024-10-15 11:06 ` [PATCH v3 2/5] iio: consumers: copy/release available info from producer " Matteo Martelli
@ 2024-10-16 21:08 ` Sebastian Reichel
2024-10-17 10:49 ` Matteo Martelli
0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2024-10-16 21:08 UTC (permalink / raw)
To: Matteo Martelli
Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
linux-iio, linux-kernel, linux-mips, linux-pm
[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]
Hi,
On Tue, Oct 15, 2024 at 01:06:35PM +0200, Matteo Martelli wrote:
> Consumers need to call the producer's read_avail_release_resource()
> callback after reading producer's available info. To avoid a race
> condition with the producer unregistration, change inkern
> iio_channel_read_avail() so that it copies the available info from the
> producer and immediately calls its release callback with info_exists
> locked.
>
> Also, modify the users of iio_read_avail_channel_raw() and
> iio_read_avail_channel_attribute() to free the copied available buffers
> after calling these functions.
>
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> ---
> diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644
> --- a/drivers/power/supply/ingenic-battery.c
> +++ b/drivers/power/supply/ingenic-battery.c
> @@ -12,6 +12,7 @@
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> #include <linux/property.h>
> +#include <linux/slab.h>
>
> struct ingenic_battery {
> struct device *dev;
> @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> dev_err(bat->dev, "Unable to read channel avail scale\n");
> return ret;
> }
> - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
> - return -EINVAL;
> + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> max_mV = bat->info->voltage_max_design_uv / 1000;
>
> @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
>
> if (best_idx < 0) {
> dev_err(bat->dev, "Unable to find matching voltage scale\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> /* Only set scale if there is more than one (fractional) entry */
> @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> scale_raw[best_idx + 1],
> IIO_CHAN_INFO_SCALE);
> if (ret)
> - return ret;
> + goto out;
> }
>
> - return 0;
> + ret = 0;
> +out:
> + kfree(scale_raw);
> + return ret;
> }
>
> static enum power_supply_property ingenic_battery_properties[] = {
It should be enough to declare scale_raw like this at the beginning
of the function and otherwise keep it as is when you include
<linux/cleanup.h>:
const int *scale_raw __free(kfree) = NULL;
Greetings,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/5] iio: consumers: copy/release available info from producer to fix race
2024-10-16 21:08 ` Sebastian Reichel
@ 2024-10-17 10:49 ` Matteo Martelli
2024-10-19 11:50 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Matteo Martelli @ 2024-10-17 10:49 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
linux-iio, linux-kernel, linux-mips, linux-pm
Quoting Sebastian Reichel (2024-10-16 23:08:30)
> Hi,
>
> On Tue, Oct 15, 2024 at 01:06:35PM +0200, Matteo Martelli wrote:
> > Consumers need to call the producer's read_avail_release_resource()
> > callback after reading producer's available info. To avoid a race
> > condition with the producer unregistration, change inkern
> > iio_channel_read_avail() so that it copies the available info from the
> > producer and immediately calls its release callback with info_exists
> > locked.
> >
> > Also, modify the users of iio_read_avail_channel_raw() and
> > iio_read_avail_channel_attribute() to free the copied available buffers
> > after calling these functions.
> >
> > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> > ---
> > diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> > index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644
> > --- a/drivers/power/supply/ingenic-battery.c
> > +++ b/drivers/power/supply/ingenic-battery.c
> > @@ -12,6 +12,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/power_supply.h>
> > #include <linux/property.h>
> > +#include <linux/slab.h>
> >
> > struct ingenic_battery {
> > struct device *dev;
> > @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> > dev_err(bat->dev, "Unable to read channel avail scale\n");
> > return ret;
> > }
> > - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
> > - return -EINVAL;
> > + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> >
> > max_mV = bat->info->voltage_max_design_uv / 1000;
> >
> > @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> >
> > if (best_idx < 0) {
> > dev_err(bat->dev, "Unable to find matching voltage scale\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > /* Only set scale if there is more than one (fractional) entry */
> > @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> > scale_raw[best_idx + 1],
> > IIO_CHAN_INFO_SCALE);
> > if (ret)
> > - return ret;
> > + goto out;
> > }
> >
> > - return 0;
> > + ret = 0;
> > +out:
> > + kfree(scale_raw);
> > + return ret;
> > }
> >
> > static enum power_supply_property ingenic_battery_properties[] = {
>
> It should be enough to declare scale_raw like this at the beginning
> of the function and otherwise keep it as is when you include
> <linux/cleanup.h>:
>
> const int *scale_raw __free(kfree) = NULL;
Nice! I wasn't aware of it, thanks! I'll try it and submit it in next version.
I think that also fits for the similar usage in iio_channel_read_min() and
iio_channel_read_max() as well.
>
> Greetings,
>
> -- Sebastian
Thanks,
Matteo Martelli
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/5] iio: consumers: copy/release available info from producer to fix race
2024-10-17 10:49 ` Matteo Martelli
@ 2024-10-19 11:50 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2024-10-19 11:50 UTC (permalink / raw)
To: Matteo Martelli
Cc: Sebastian Reichel, Lars-Peter Clausen, Michael Hennerich,
Alisa-Dariana Roman, Christian Eggers, Peter Rosin, Paul Cercueil,
linux-iio, linux-kernel, linux-mips, linux-pm
On Thu, 17 Oct 2024 12:49:23 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:
> Quoting Sebastian Reichel (2024-10-16 23:08:30)
> > Hi,
> >
> > On Tue, Oct 15, 2024 at 01:06:35PM +0200, Matteo Martelli wrote:
> > > Consumers need to call the producer's read_avail_release_resource()
> > > callback after reading producer's available info. To avoid a race
> > > condition with the producer unregistration, change inkern
> > > iio_channel_read_avail() so that it copies the available info from the
> > > producer and immediately calls its release callback with info_exists
> > > locked.
> > >
> > > Also, modify the users of iio_read_avail_channel_raw() and
> > > iio_read_avail_channel_attribute() to free the copied available buffers
> > > after calling these functions.
> > >
> > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> > > ---
> > > diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> > > index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644
> > > --- a/drivers/power/supply/ingenic-battery.c
> > > +++ b/drivers/power/supply/ingenic-battery.c
> > > @@ -12,6 +12,7 @@
> > > #include <linux/platform_device.h>
> > > #include <linux/power_supply.h>
> > > #include <linux/property.h>
> > > +#include <linux/slab.h>
> > >
> > > struct ingenic_battery {
> > > struct device *dev;
> > > @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> > > dev_err(bat->dev, "Unable to read channel avail scale\n");
> > > return ret;
> > > }
> > > - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
> > > - return -EINVAL;
> > > + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) {
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > >
> > > max_mV = bat->info->voltage_max_design_uv / 1000;
> > >
> > > @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> > >
> > > if (best_idx < 0) {
> > > dev_err(bat->dev, "Unable to find matching voltage scale\n");
> > > - return -EINVAL;
> > > + ret = -EINVAL;
> > > + goto out;
> > > }
> > >
> > > /* Only set scale if there is more than one (fractional) entry */
> > > @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> > > scale_raw[best_idx + 1],
> > > IIO_CHAN_INFO_SCALE);
> > > if (ret)
> > > - return ret;
> > > + goto out;
> > > }
> > >
> > > - return 0;
> > > + ret = 0;
> > > +out:
> > > + kfree(scale_raw);
> > > + return ret;
> > > }
> > >
> > > static enum power_supply_property ingenic_battery_properties[] = {
> >
> > It should be enough to declare scale_raw like this at the beginning
> > of the function and otherwise keep it as is when you include
> > <linux/cleanup.h>:
> >
> > const int *scale_raw __free(kfree) = NULL;
>
> Nice! I wasn't aware of it, thanks! I'll try it and submit it in next version.
>
> I think that also fits for the similar usage in iio_channel_read_min() and
> iio_channel_read_max() as well.
Take care with this + read the documents.
The constructor and destructor should be in one line.
https://lore.kernel.org/all/172294149613.2215.3274492813920223809.tip-bot2@tip-bot2/
specifically the second to last line.
It's a clever tool but use with care!
Jonathan
>
> >
> > Greetings,
> >
> > -- Sebastian
>
> Thanks,
> Matteo Martelli
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-19 11:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-15 11:06 [PATCH v3 0/5] iio: fix possible race condition during access of available info lists Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 1/5] iio: core: add read_avail_release_resource callback to fix race Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 2/5] iio: consumers: copy/release available info from producer " Matteo Martelli
2024-10-16 21:08 ` Sebastian Reichel
2024-10-17 10:49 ` Matteo Martelli
2024-10-19 11:50 ` Jonathan Cameron
2024-10-15 11:06 ` [PATCH v3 3/5] iio: pac1921: use read_avail+release APIs instead of custom ext_info Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 4/5] iio: ad7192: copy/release available filter frequencies to fix race Matteo Martelli
2024-10-15 11:06 ` [PATCH v3 5/5] iio: as73211: copy/release available integration times " Matteo Martelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).