* [PATCH 1/4] staging:iio:ad5933: Fix NULL pointer deref when enabling buffer
@ 2014-09-23 18:04 Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 2/4] staging:iio:ad5933: Drop "raw" from channel names Lars-Peter Clausen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-09-23 18:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Peter Meerwald, linux-iio, Lars-Peter Clausen
In older versions of the IIO framework it was possible to pass a
completely different set of channels to iio_buffer_register() as the one
that is assigned to the IIO device. Commit 959d2952d124 ("staging:iio: make
iio_sw_buffer_preenable much more general.") introduced a restriction that
requires that the set of channels that is passed to iio_buffer_register() is
a subset of the channels assigned to the IIO device as the IIO core will use
the list of channels that is assigned to the device to lookup a channel by
scan index in iio_compute_scan_bytes(). If it can not find the channel the
function will crash. This patch fixes the issue by making sure that the same
set of channels is assigned to the IIO device and passed to
iio_buffer_register().
Fixes the follow NULL pointer derefernce kernel crash:
Unable to handle kernel NULL pointer dereference at virtual address 00000016
pgd = d53d0000
[00000016] *pgd=1534e831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 1626 Comm: bash Not tainted 3.15.0-19969-g2a180eb-dirty #9545
task: d6c124c0 ti: d539a000 task.ti: d539a000
PC is at iio_compute_scan_bytes+0x34/0xa8
LR is at iio_compute_scan_bytes+0x34/0xa8
pc : [<c03052e4>] lr : [<c03052e4>] psr: 60070013
sp : d539beb8 ip : 00000001 fp : 00000000
r10: 00000002 r9 : 00000000 r8 : 00000001
r7 : 00000000 r6 : d6dc8800 r5 : d7571000 r4 : 00000002
r3 : d7571000 r2 : 00000044 r1 : 00000001 r0 : 00000000
Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
Control: 18c5387d Table: 153d004a DAC: 00000015
Process bash (pid: 1626, stack limit = 0xd539a240)
Stack: (0xd539beb8 to 0xd539c000)
bea0: c02fc0e4 d7571000
bec0: d76c1640 d6dc8800 d757117c 00000000 d757112c c0305b04 d76c1690 d76c1640
bee0: d7571188 00000002 00000000 d7571000 d539a000 00000000 000dd1c8 c0305d54
bf00: d7571010 0160b868 00000002 c69d3900 d7573278 d7573308 c69d3900 c01ece90
bf20: 00000002 c0103fac c0103f6c d539bf88 00000002 c69d3b00 c69d3b0c c0103468
bf40: 00000000 00000000 d7694a00 00000002 000af408 d539bf88 c000dd84 c00b2f94
bf60: d7694a00 000af408 00000002 d7694a00 d7694a00 00000002 000af408 c000dd84
bf80: 00000000 c00b32d0 00000000 00000000 00000002 b6f1aa78 00000002 000af408
bfa0: 00000004 c000dc00 b6f1aa78 00000002 00000001 000af408 00000002 00000000
bfc0: b6f1aa78 00000002 000af408 00000004 be806a4c 000a6094 00000000 000dd1c8
bfe0: 00000000 be8069cc b6e8ab77 b6ec125c 40070010 00000001 22940489 154a5007
[<c03052e4>] (iio_compute_scan_bytes) from [<c0305b04>] (__iio_update_buffers+0x248/0x438)
[<c0305b04>] (__iio_update_buffers) from [<c0305d54>] (iio_buffer_store_enable+0x60/0x7c)
[<c0305d54>] (iio_buffer_store_enable) from [<c01ece90>] (dev_attr_store+0x18/0x24)
[<c01ece90>] (dev_attr_store) from [<c0103fac>] (sysfs_kf_write+0x40/0x4c)
[<c0103fac>] (sysfs_kf_write) from [<c0103468>] (kernfs_fop_write+0x110/0x154)
[<c0103468>] (kernfs_fop_write) from [<c00b2f94>] (vfs_write+0xd0/0x160)
[<c00b2f94>] (vfs_write) from [<c00b32d0>] (SyS_write+0x40/0x78)
[<c00b32d0>] (SyS_write) from [<c000dc00>] (ret_fast_syscall+0x0/0x30)
Code: ea00000e e1a01008 e1a00005 ebfff6fc (e5d0a016)
Fixes: 959d2952d124 ("staging:iio: make iio_sw_buffer_preenable much more general.")
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/staging/iio/impedance-analyzer/ad5933.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index d0c89d0..9a6665d 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -115,6 +115,7 @@ static const struct iio_chan_spec ad5933_channels[] = {
.channel = 0,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
.address = AD5933_REG_TEMP_DATA,
+ .scan_index = -1,
.scan_type = {
.sign = 's',
.realbits = 14,
@@ -125,8 +126,6 @@ static const struct iio_chan_spec ad5933_channels[] = {
.indexed = 1,
.channel = 0,
.extend_name = "real_raw",
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
- BIT(IIO_CHAN_INFO_SCALE),
.address = AD5933_REG_REAL_DATA,
.scan_index = 0,
.scan_type = {
@@ -139,8 +138,6 @@ static const struct iio_chan_spec ad5933_channels[] = {
.indexed = 1,
.channel = 0,
.extend_name = "imag_raw",
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
- BIT(IIO_CHAN_INFO_SCALE),
.address = AD5933_REG_IMAG_DATA,
.scan_index = 1,
.scan_type = {
@@ -749,14 +746,14 @@ static int ad5933_probe(struct i2c_client *client,
indio_dev->name = id->name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = ad5933_channels;
- indio_dev->num_channels = 1; /* only register temp0_input */
+ indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
ret = ad5933_register_ring_funcs_and_init(indio_dev);
if (ret)
goto error_disable_reg;
- /* skip temp0_input, register in0_(real|imag)_raw */
- ret = iio_buffer_register(indio_dev, &ad5933_channels[1], 2);
+ ret = iio_buffer_register(indio_dev, ad5933_channels,
+ ARRAY_SIZE(ad5933_channels));
if (ret)
goto error_unreg_ring;
--
1.8.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] staging:iio:ad5933: Drop "raw" from channel names
2014-09-23 18:04 [PATCH 1/4] staging:iio:ad5933: Fix NULL pointer deref when enabling buffer Lars-Peter Clausen
@ 2014-09-23 18:04 ` Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 4/4] staging:iio:ad5933: Remove platform data from state struct Lars-Peter Clausen
2 siblings, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-09-23 18:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Peter Meerwald, linux-iio, Lars-Peter Clausen
"raw" is the name of a channel property, but should not be part of the
channel name itself.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/staging/iio/impedance-analyzer/ad5933.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 9a6665d..b6bd609 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -125,7 +125,7 @@ static const struct iio_chan_spec ad5933_channels[] = {
.type = IIO_VOLTAGE,
.indexed = 1,
.channel = 0,
- .extend_name = "real_raw",
+ .extend_name = "real",
.address = AD5933_REG_REAL_DATA,
.scan_index = 0,
.scan_type = {
@@ -137,7 +137,7 @@ static const struct iio_chan_spec ad5933_channels[] = {
.type = IIO_VOLTAGE,
.indexed = 1,
.channel = 0,
- .extend_name = "imag_raw",
+ .extend_name = "imag",
.address = AD5933_REG_IMAG_DATA,
.scan_index = 1,
.scan_type = {
--
1.8.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value
2014-09-23 18:04 [PATCH 1/4] staging:iio:ad5933: Fix NULL pointer deref when enabling buffer Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 2/4] staging:iio:ad5933: Drop "raw" from channel names Lars-Peter Clausen
@ 2014-09-23 18:04 ` Lars-Peter Clausen
2014-09-24 8:02 ` Daniel Baluta
2014-09-23 18:04 ` [PATCH 4/4] staging:iio:ad5933: Remove platform data from state struct Lars-Peter Clausen
2 siblings, 1 reply; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-09-23 18:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Peter Meerwald, linux-iio, Lars-Peter Clausen
We shouldn't be doing the unit conversion in kernel space. Just report the
raw value for the property and the scale. Userspace can do the conversion if
necessary.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/staging/iio/impedance-analyzer/ad5933.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index b6bd609..ebff637 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -113,7 +113,8 @@ static const struct iio_chan_spec ad5933_channels[] = {
.type = IIO_TEMP,
.indexed = 1,
.channel = 0,
- .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
.address = AD5933_REG_TEMP_DATA,
.scan_index = -1,
.scan_type = {
@@ -522,10 +523,9 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
__be16 dat;
int ret = -EINVAL;
- mutex_lock(&indio_dev->mlock);
switch (m) {
case IIO_CHAN_INFO_RAW:
- case IIO_CHAN_INFO_PROCESSED:
+ mutex_lock(&indio_dev->mlock);
if (iio_buffer_enabled(indio_dev)) {
ret = -EBUSY;
goto out;
@@ -543,14 +543,13 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
if (ret < 0)
goto out;
mutex_unlock(&indio_dev->mlock);
- ret = be16_to_cpu(dat);
- /* Temp in Milli degrees Celsius */
- if (ret < 8192)
- *val = ret * 1000 / 32;
- else
- *val = (ret - 16384) * 1000 / 32;
+ *val = sign_extend32(be16_to_cpu(dat), 13);
return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ *val = 1000;
+ *val2 = 5;
+ return IIO_VAL_FRACTIONAL_LOG2;
}
out:
--
1.8.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] staging:iio:ad5933: Remove platform data from state struct
2014-09-23 18:04 [PATCH 1/4] staging:iio:ad5933: Fix NULL pointer deref when enabling buffer Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 2/4] staging:iio:ad5933: Drop "raw" from channel names Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value Lars-Peter Clausen
@ 2014-09-23 18:04 ` Lars-Peter Clausen
2 siblings, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-09-23 18:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Peter Meerwald, linux-iio, Lars-Peter Clausen
The platform data is only used in the probe function. No need to keep it
around.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/staging/iio/impedance-analyzer/ad5933.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index ebff637..77c77b0 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -89,7 +89,6 @@
struct ad5933_state {
struct i2c_client *client;
struct regulator *reg;
- struct ad5933_platform_data *pdata;
struct delayed_work work;
unsigned long mclk_hz;
unsigned char ctrl_hb;
@@ -711,9 +710,7 @@ static int ad5933_probe(struct i2c_client *client,
st->client = client;
if (!pdata)
- st->pdata = &ad5933_default_pdata;
- else
- st->pdata = pdata;
+ pdata = &ad5933_default_pdata;
st->reg = devm_regulator_get(&client->dev, "vcc");
if (!IS_ERR(st->reg)) {
@@ -726,10 +723,10 @@ static int ad5933_probe(struct i2c_client *client,
if (voltage_uv)
st->vref_mv = voltage_uv / 1000;
else
- st->vref_mv = st->pdata->vref_mv;
+ st->vref_mv = pdata->vref_mv;
- if (st->pdata->ext_clk_Hz) {
- st->mclk_hz = st->pdata->ext_clk_Hz;
+ if (pdata->ext_clk_Hz) {
+ st->mclk_hz = pdata->ext_clk_Hz;
st->ctrl_lb = AD5933_CTRL_EXT_SYSCLK;
} else {
st->mclk_hz = AD5933_INT_OSC_FREQ_Hz;
--
1.8.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value
2014-09-23 18:04 ` [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value Lars-Peter Clausen
@ 2014-09-24 8:02 ` Daniel Baluta
2014-09-24 10:46 ` Lars-Peter Clausen
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Baluta @ 2014-09-24 8:02 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio
On Tue, Sep 23, 2014 at 9:04 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> We shouldn't be doing the unit conversion in kernel space. Just report the
> raw value for the property and the scale. Userspace can do the conversion if
> necessary.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> drivers/staging/iio/impedance-analyzer/ad5933.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index b6bd609..ebff637 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -113,7 +113,8 @@ static const struct iio_chan_spec ad5933_channels[] = {
> .type = IIO_TEMP,
> .indexed = 1,
> .channel = 0,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> .address = AD5933_REG_TEMP_DATA,
> .scan_index = -1,
> .scan_type = {
> @@ -522,10 +523,9 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
> __be16 dat;
> int ret = -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> switch (m) {
> case IIO_CHAN_INFO_RAW:
> - case IIO_CHAN_INFO_PROCESSED:
> + mutex_lock(&indio_dev->mlock);
> if (iio_buffer_enabled(indio_dev)) {
> ret = -EBUSY;
> goto out;
> @@ -543,14 +543,13 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out;
> mutex_unlock(&indio_dev->mlock);
> - ret = be16_to_cpu(dat);
> - /* Temp in Milli degrees Celsius */
> - if (ret < 8192)
> - *val = ret * 1000 / 32;
> - else
> - *val = (ret - 16384) * 1000 / 32;
> + *val = sign_extend32(be16_to_cpu(dat), 13);
>
> return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = 1000;
> + *val2 = 5;
> + return IIO_VAL_FRACTIONAL_LOG2;
> }
>
> out:
Is it possible to reach this line with mutex not acquired?
out:
mutex_unlock(&indio_dev->mlock);
return ret;
Daniel.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value
2014-09-24 8:02 ` Daniel Baluta
@ 2014-09-24 10:46 ` Lars-Peter Clausen
0 siblings, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-09-24 10:46 UTC (permalink / raw)
To: Daniel Baluta; +Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio
On 09/24/2014 10:02 AM, Daniel Baluta wrote:
>> @@ -543,14 +543,13 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>> if (ret < 0)
>> goto out;
>> mutex_unlock(&indio_dev->mlock);
>> - ret = be16_to_cpu(dat);
>> - /* Temp in Milli degrees Celsius */
>> - if (ret < 8192)
>> - *val = ret * 1000 / 32;
>> - else
>> - *val = (ret - 16384) * 1000 / 32;
>> + *val = sign_extend32(be16_to_cpu(dat), 13);
>>
>> return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = 1000;
>> + *val2 = 5;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> }
>>
>> out:
>
> Is it possible to reach this line with mutex not acquired?
>
> out:
> mutex_unlock(&indio_dev->mlock);
> return ret;
>
It is not possible, but you are right the code should be restructured to
make this more clear.
Thanks.
- Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-24 10:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-23 18:04 [PATCH 1/4] staging:iio:ad5933: Fix NULL pointer deref when enabling buffer Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 2/4] staging:iio:ad5933: Drop "raw" from channel names Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 3/4] staging:iio:ad5933: Report temperature as raw value Lars-Peter Clausen
2014-09-24 8:02 ` Daniel Baluta
2014-09-24 10:46 ` Lars-Peter Clausen
2014-09-23 18:04 ` [PATCH 4/4] staging:iio:ad5933: Remove platform data from state struct Lars-Peter Clausen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.