[PATCH 0/2] iio: pressure: do not rely on structure field ordering

[PATCH 0/2] iio: pressure: do not rely on structure field ordering

[Peter Rosin]

Hi!

These two patches are needed to fix some fallout from adding the
available attribute.

I'm sending this before getting a verification that it work from Ken Lin
in the hopes that these mails will reach other recipients swifter than my
original mail appears to do [1]. It looks like my mails are held up
somewhere, because I don't seem them in archives (yet). I don't know
what is going on with that, but as I said hopefully git send-email is
more effective???

Cheers,
peda

[1] Message-ID: <28916f18-da40-cf0a-5bfa-2c8d53a7d83c@axentia.se>

Peter Rosin (2):
  iio: pressure: mpl3115: do not rely on structure field ordering
  iio: pressure: mpl115: do not rely on structure field ordering

 drivers/iio/pressure/mpl115.c  | 1 +
 drivers/iio/pressure/mpl3115.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Rosin]

Fixes a regression triggered by a change in the layout of
struct iio_chan_spec, but the real bug is in the driver which assumed
a specific structure layout in the first place.

# cat /sys/bus/iio/devices/iio\:device1/in_pressure_scale_available
Segmentation fault

Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = ecc3c000
[00000000] *pgd=87f91831
Internal error: Oops: 80000007 [#1] SMP ARM
Modules linked in:
CPU: 1 PID: 1051 Comm: cat Not tainted 4.10.0-rc5-00009-gffd8858-dirty #3
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
task: ed54ec00 task.stack: ee2bc000
PC is at 0x0
LR is at iio_read_channel_info_avail+0x40/0x280
pc : [<00000000>]    lr : [<c06fbc1c>]    psr: a0070013
sp : ee2bdda8  ip : 00000000  fp : ee2bddf4
r10: c0a53c74  r9 : ed79f000  r8 : ee8d1018
r7 : 00001000  r6 : 00000fff  r5 : ee8b9a00  r4 : ed79f000
r3 : ee2bddc4  r2 : ee2bddbc  r1 : c0a86dcc  r0 : ee8d1000
Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 3cc3c04a  DAC: 00000051
Process cat (pid: 1051, stack limit = 0xee2bc210)
Stack: (0xee2bdda8 to 0xee2be000)
dda0:                   ee2bddc0 00000002 c016d720 c016d394 ed54ec00 00000000
ddc0: 60070013 ed413780 00000001 edffd480 ee8b9a00 00000fff 00001000 ee8d1018
dde0: ed79f000 c0a53c74 ee2bde0c ee2bddf8 c0513c58 c06fbbe8 edffd480 edffd540
de00: ee2bde3c ee2bde10 c0293474 c0513c40 c02933e4 ee2bde60 00000001 ed413780
de20: 00000001 ed413780 00000000 edffd480 ee2bde4c ee2bde40 c0291d00 c02933f0
de40: ee2bde9c ee2bde50 c024679c c0291ce0 edffd4b0 b6e37000 00020000 ee2bdf78
de60: 00000000 00000000 ed54ec00 ed013200 00000817 c0a111fc edffd540 ed413780
de80: b6e37000 00020000 00020000 ee2bdf78 ee2bded4 ee2bdea0 c0292890 c0246604
dea0: c0117940 c016ba50 00000025 c0a111fc b6e37000 ed413780 ee2bdf78 00020000
dec0: ee2bc000 b6e37000 ee2bdf44 ee2bded8 c021d158 c0292770 c0117764 b6e36004
dee0: c0f0d7c4 ee2bdfb0 b6f89228 00021008 ee2bdfac ee2bdf00 c0101374 c0117770
df00: 00000000 00000000 ee2bc000 00000000 ee2bdf34 ee2bdf20 c016ba04 c0171080
df20: 00000000 00020000 ed413780 b6e37000 00000000 ee2bdf78 ee2bdf74 ee2bdf48
df40: c021e7a0 c021d130 c023e300 c023e280 ee2bdf74 00000000 00000000 ed413780
df60: ed413780 00020000 ee2bdfa4 ee2bdf78 c021e870 c021e71c 00000000 00000000
df80: 00020000 00020000 b6e37000 00000003 c0108084 00000000 00000000 ee2bdfa8
dfa0: c0107ee0 c021e838 00020000 00020000 00000003 b6e37000 00020000 0001a2b4
dfc0: 00020000 00020000 b6e37000 00000003 7fffe000 00000000 00000000 00020000
dfe0: 00000000 be98eb4c 0000c740 b6f1985c 60070010 00000003 00000000 00000000
Backtrace:
[<c06fbbdc>] (iio_read_channel_info_avail) from [<c0513c58>] (dev_attr_show+0x24/0x50)
 r10:c0a53c74 r9:ed79f000 r8:ee8d1018 r7:00001000 r6:00000fff r5:ee8b9a00
 r4:edffd480
[<c0513c34>] (dev_attr_show) from [<c0293474>] (sysfs_kf_seq_show+0x90/0x110)
 r5:edffd540 r4:edffd480
[<c02933e4>] (sysfs_kf_seq_show) from [<c0291d00>] (kernfs_seq_show+0x2c/0x30)
 r10:edffd480 r9:00000000 r8:ed413780 r7:00000001 r6:ed413780 r5:00000001
 r4:ee2bde60 r3:c02933e4
[<c0291cd4>] (kernfs_seq_show) from [<c024679c>] (seq_read+0x1a4/0x4e0)
[<c02465f8>] (seq_read) from [<c0292890>] (kernfs_fop_read+0x12c/0x1cc)
 r10:ee2bdf78 r9:00020000 r8:00020000 r7:b6e37000 r6:ed413780 r5:edffd540
 r4:c0a111fc
[<c0292764>] (kernfs_fop_read) from [<c021d158>] (__vfs_read+0x34/0x118)
 r10:b6e37000 r9:ee2bc000 r8:00020000 r7:ee2bdf78 r6:ed413780 r5:b6e37000
 r4:c0a111fc
[<c021d124>] (__vfs_read) from [<c021e7a0>] (vfs_read+0x90/0x11c)
 r8:ee2bdf78 r7:00000000 r6:b6e37000 r5:ed413780 r4:00020000
[<c021e710>] (vfs_read) from [<c021e870>] (SyS_read+0x44/0x90)
 r8:00020000 r7:ed413780 r6:ed413780 r5:00000000 r4:00000000
[<c021e82c>] (SyS_read) from [<c0107ee0>] (ret_fast_syscall+0x0/0x1c)
 r10:00000000 r8:c0108084 r7:00000003 r6:b6e37000 r5:00020000 r4:00020000
Code: bad PC value
---[ end trace 9c4938ccd0389004 ]---

Fixes: cc26ad455f57 ("iio: Add Freescale MPL3115A2 pressure / temperature sensor driver")
Fixes: 51239600074b ("iio:core: add a callback to allow drivers to provide _available attributes")
Reported-by: Ken Lin <ken.lin@advantech.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/pressure/mpl3115.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index cc3f84139157..525644a7442d 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 	{
 		.type = IIO_PRESSURE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 		.scan_index = 0,
 		.scan_type = {
 			.sign = 'u',
@@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 		.scan_index = 1,
 		.scan_type = {
 			.sign = 's',
-- 
2.1.4

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Meerwald-Stadler]

Hello,

> Fixes a regression triggered by a change in the layout of
> struct iio_chan_spec, but the real bug is in the driver which assumed
> a specific structure layout in the first place.

I don't think that this is a proper fix

maybe the driver is unique in that it uses mask_separate for INFO_SCALE 
and not by_type, but since there is just one PRESSURE channel, it should 
be equivalent

what do you mean by 'driver which assumed a specific structure'?

thanks, p.

> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index cc3f84139157..525644a7442d 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>  	{
>  		.type = IIO_PRESSURE,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  		.scan_index = 0,
>  		.scan_type = {
>  			.sign = 'u',
> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  		.scan_index = 1,
>  		.scan_type = {
>  			.sign = 's',
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Rosin]

On 2017-02-01 10:10, Peter Meerwald-Stadler wrote:
> Hello,
> 
>> Fixes a regression triggered by a change in the layout of
>> struct iio_chan_spec, but the real bug is in the driver which assumed
>> a specific structure layout in the first place.
> 
> I don't think that this is a proper fix
> 
> maybe the driver is unique in that it uses mask_separate for INFO_SCALE 
> and not by_type, but since there is just one PRESSURE channel, it should 
> be equivalent
> 
> what do you mean by 'driver which assumed a specific structure'?

Look again, the two bits are not OR:ed together as implied by the
indentation. There is a comma between them, which put the ..._SCALE
bit in the next field. That next field was .info_mask_shared_by_type
before the patch adding the available attribute that triggered the
regression and .info_mask_separate_available after it.

Cheers,
peda

> thanks, p.
> 
>> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
>> index cc3f84139157..525644a7442d 100644
>> --- a/drivers/iio/pressure/mpl3115.c
>> +++ b/drivers/iio/pressure/mpl3115.c
>> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>>  	{
>>  		.type = IIO_PRESSURE,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> -			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>  		.scan_index = 0,
>>  		.scan_type = {
>>  			.sign = 'u',
>> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>>  	{
>>  		.type = IIO_TEMP,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> -			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>  		.scan_index = 1,
>>  		.scan_type = {
>>  			.sign = 's',
>>
> 

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Meerwald-Stadler]

> >> Fixes a regression triggered by a change in the layout of
> >> struct iio_chan_spec, but the real bug is in the driver which assumed
> >> a specific structure layout in the first place.

> > what do you mean by 'driver which assumed a specific structure'?
> 
> Look again, the two bits are not OR:ed together as implied by the
> indentation. There is a comma between them, which put the ..._SCALE
> bit in the next field. That next field was .info_mask_shared_by_type
> before the patch adding the available attribute that triggered the
> regression and .info_mask_separate_available after it.

wow, now that you say it :)

since I wrote these drivers I can assure you that this is just a typo, use
of the 'specific structure' is a coincident

maybe adding your explanation regarding not ORing to the patch description
would be a good idea

I was confused by the change from 
.info_mask_separate to .info_mask_shared_by_type
a fix could have just changed
-  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
			BIT(IIO_CHAN_INFO_SCALE),
+  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
			BIT(IIO_CHAN_INFO_SCALE),
as originally intended

> Cheers,
> peda
> 
> > thanks, p.
> > 
> >> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> >> index cc3f84139157..525644a7442d 100644
> >> --- a/drivers/iio/pressure/mpl3115.c
> >> +++ b/drivers/iio/pressure/mpl3115.c
> >> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >>  	{
> >>  		.type = IIO_PRESSURE,
> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> -			BIT(IIO_CHAN_INFO_SCALE),
> >> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>  		.scan_index = 0,
> >>  		.scan_type = {
> >>  			.sign = 'u',
> >> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >>  	{
> >>  		.type = IIO_TEMP,
> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> -			BIT(IIO_CHAN_INFO_SCALE),
> >> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>  		.scan_index = 1,
> >>  		.scan_type = {
> >>  			.sign = 's',
> >>
> > 
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Rosin]

On 2017-02-01 10:31, Peter Meerwald-Stadler wrote:
> 
>>>> Fixes a regression triggered by a change in the layout of
>>>> struct iio_chan_spec, but the real bug is in the driver which assumed
>>>> a specific structure layout in the first place.
> 
>>> what do you mean by 'driver which assumed a specific structure'?
>>
>> Look again, the two bits are not OR:ed together as implied by the
>> indentation. There is a comma between them, which put the ..._SCALE
>> bit in the next field. That next field was .info_mask_shared_by_type
>> before the patch adding the available attribute that triggered the
>> regression and .info_mask_separate_available after it.
> 
> wow, now that you say it :)
> 
> since I wrote these drivers I can assure you that this is just a typo, use
> of the 'specific structure' is a coincident
> 
> maybe adding your explanation regarding not ORing to the patch description
> would be a good idea
> 
> I was confused by the change from 
> .info_mask_separate to .info_mask_shared_by_type
> a fix could have just changed
> -  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> 			BIT(IIO_CHAN_INFO_SCALE),
> +  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 			BIT(IIO_CHAN_INFO_SCALE),
> as originally intended

I considered that option, but the code in mpl3115_read_raw (and
mpl115_read_raw for that matter) return constants fro these values which
to me indicated that they were not "separate" and as that would also be
the change which replicated the exact behavior from before the regression
I went with that. But I don't care either way, so I can re-spin if you
want me to? (But don't blame me if that regresses in some other
interesting way).

Cheers,
peda

>> Cheers,
>> peda
>>
>>> thanks, p.
>>>
>>>> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
>>>> index cc3f84139157..525644a7442d 100644
>>>> --- a/drivers/iio/pressure/mpl3115.c
>>>> +++ b/drivers/iio/pressure/mpl3115.c
>>>> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>>>>  	{
>>>>  		.type = IIO_PRESSURE,
>>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> -			BIT(IIO_CHAN_INFO_SCALE),
>>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.scan_index = 0,
>>>>  		.scan_type = {
>>>>  			.sign = 'u',
>>>> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>>>>  	{
>>>>  		.type = IIO_TEMP,
>>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> -			BIT(IIO_CHAN_INFO_SCALE),
>>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.scan_index = 1,
>>>>  		.scan_type = {
>>>>  			.sign = 's',
>>>>
>>>
>>
> 

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Meerwald-Stadler]

> > -  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > 			BIT(IIO_CHAN_INFO_SCALE),
> > +  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > 			BIT(IIO_CHAN_INFO_SCALE),
> > as originally intended
> 
> I considered that option, but the code in mpl3115_read_raw (and
> mpl115_read_raw for that matter) return constants fro these values which
> to me indicated that they were not "separate" and as that would also be
> the change which replicated the exact behavior from before the regression
> I went with that. But I don't care either way, so I can re-spin if you
> want me to? (But don't blame me if that regresses in some other
> interesting way).

no, all good; shared_by_type is the way to go
I'd rather respin for the not ORed comment in the patch

> >> Cheers,
> >> peda
> >>
> >>> thanks, p.
> >>>
> >>>> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> >>>> index cc3f84139157..525644a7442d 100644
> >>>> --- a/drivers/iio/pressure/mpl3115.c
> >>>> +++ b/drivers/iio/pressure/mpl3115.c
> >>>> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >>>>  	{
> >>>>  		.type = IIO_PRESSURE,
> >>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >>>> -			BIT(IIO_CHAN_INFO_SCALE),
> >>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>>>  		.scan_index = 0,
> >>>>  		.scan_type = {
> >>>>  			.sign = 'u',
> >>>> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >>>>  	{
> >>>>  		.type = IIO_TEMP,
> >>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >>>> -			BIT(IIO_CHAN_INFO_SCALE),
> >>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>>>  		.scan_index = 1,
> >>>>  		.scan_type = {
> >>>>  			.sign = 's',
> >>>>
> >>>
> >>
> > 
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Peter Rosin]

On 2017-02-01 10:57, Peter Meerwald-Stadler wrote:
>>> -  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> 			BIT(IIO_CHAN_INFO_SCALE),
>>> +  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> 			BIT(IIO_CHAN_INFO_SCALE),
>>> as originally intended
>>
>> I considered that option, but the code in mpl3115_read_raw (and
>> mpl115_read_raw for that matter) return constants fro these values which
>> to me indicated that they were not "separate" and as that would also be
>> the change which replicated the exact behavior from before the regression
>> I went with that. But I don't care either way, so I can re-spin if you
>> want me to? (But don't blame me if that regresses in some other
>> interesting way).
> 
> no, all good; shared_by_type is the way to go
> I'd rather respin for the not ORed comment in the patch

Ok, but I think I'll wait a bit so that Ken Lin gets some time to verify
that it actually solves the original problem. It should, but...

Cheers,
peda

RE: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Ken.Lin]

> -----Original Message-----
> From: Peter Rosin [mailto:peda@axentia.se]
> Sent: Wednesday, February 1, 2017 2:18 AM
> To: Peter Meerwald-Stadler
> Cc: linux-kernel@vger.kernel.org; Jonathan Cameron; Hartmut Knaack; Lars-
> Peter Clausen; Alison Schofield; Gregor Boirie; Sanchayan Maity; Ken.Lin; linux-
> iio@vger.kernel.org
> Subject: Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field
> ordering
> 
> On 2017-02-01 10:57, Peter Meerwald-Stadler wrote:
> >>> -  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >>> 			BIT(IIO_CHAN_INFO_SCALE),
> >>> +  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >>> 			BIT(IIO_CHAN_INFO_SCALE),
> >>> as originally intended
> >>
> >> I considered that option, but the code in mpl3115_read_raw (and
> >> mpl115_read_raw for that matter) return constants fro these values
> >> which to me indicated that they were not "separate" and as that would
> >> also be the change which replicated the exact behavior from before
> >> the regression I went with that. But I don't care either way, so I
> >> can re-spin if you want me to? (But don't blame me if that regresses
> >> in some other interesting way).
> >
> > no, all good; shared_by_type is the way to go I'd rather respin for
> > the not ORed comment in the patch
> 
> Ok, but I think I'll wait a bit so that Ken Lin gets some time to verify that it
> actually solves the original problem. It should, but...
> 


The patch test result looks good to me and functions well as before.

# ls /sys/bus/iio/devices/iio\:device1/
buffer  current_timestamp_clock  dev  in_pressure_raw  in_pressure_scale  in_temp_raw  in_temp_scale  name  of_node  power  scan_elements  subsystem  trigger  uevent
# cat /sys/bus/iio/devices/iio\:device1/in_pressure_raw
403668
# cat /sys/bus/iio/devices/iio\:device1/in_pressure_scale
0.000250
# cat /sys/bus/iio/devices/iio\:device1/in_temp_scale
0.062500
# cat /sys/bus/iio/devices/iio\:device1/in_temp_raw
480

Thanks

> Cheers,
> peda
> 
> 
> --
> This message has been scanned for viruses and dangerous content by
> MailScanner, and is believed to be clean.


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

[Jonathan Cameron]

Ouch=2E I should have actually read that driver!

Will probably pick up this evening=2E

On 1 February 2017 08:09:26 GMT+00:00, Peter Rosin <peda@axentia=2Ese> wro=
te:
>Fixes a regression triggered by a change in the layout of
>struct iio_chan_spec, but the real bug is in the driver which assumed
>a specific structure layout in the first place=2E
>
># cat /sys/bus/iio/devices/iio\:device1/in_pressure_scale_available
>Segmentation fault
>
>Unable to handle kernel NULL pointer dereference at virtual address
>00000000
>pgd =3D ecc3c000
>[00000000] *pgd=3D87f91831
>Internal error: Oops: 80000007 [#1] SMP ARM
>Modules linked in:
>CPU: 1 PID: 1051 Comm: cat Not tainted 4=2E10=2E0-rc5-00009-gffd8858-dirt=
y
>#3
>Hardware name: Freescale i=2EMX6 Quad/DualLite (Device Tree)
>task: ed54ec00 task=2Estack: ee2bc000
>PC is at 0x0
>LR is at iio_read_channel_info_avail+0x40/0x280
>pc : [<00000000>]    lr : [<c06fbc1c>]    psr: a0070013
>sp : ee2bdda8  ip : 00000000  fp : ee2bddf4
>r10: c0a53c74  r9 : ed79f000  r8 : ee8d1018
>r7 : 00001000  r6 : 00000fff  r5 : ee8b9a00  r4 : ed79f000
>r3 : ee2bddc4  r2 : ee2bddbc  r1 : c0a86dcc  r0 : ee8d1000
>Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>Control: 10c5387d  Table: 3cc3c04a  DAC: 00000051
>Process cat (pid: 1051, stack limit =3D 0xee2bc210)
>Stack: (0xee2bdda8 to 0xee2be000)
>dda0:                   ee2bddc0 00000002 c016d720 c016d394 ed54ec00
>00000000
>ddc0: 60070013 ed413780 00000001 edffd480 ee8b9a00 00000fff 00001000
>ee8d1018
>dde0: ed79f000 c0a53c74 ee2bde0c ee2bddf8 c0513c58 c06fbbe8 edffd480
>edffd540
>de00: ee2bde3c ee2bde10 c0293474 c0513c40 c02933e4 ee2bde60 00000001
>ed413780
>de20: 00000001 ed413780 00000000 edffd480 ee2bde4c ee2bde40 c0291d00
>c02933f0
>de40: ee2bde9c ee2bde50 c024679c c0291ce0 edffd4b0 b6e37000 00020000
>ee2bdf78
>de60: 00000000 00000000 ed54ec00 ed013200 00000817 c0a111fc edffd540
>ed413780
>de80: b6e37000 00020000 00020000 ee2bdf78 ee2bded4 ee2bdea0 c0292890
>c0246604
>dea0: c0117940 c016ba50 00000025 c0a111fc b6e37000 ed413780 ee2bdf78
>00020000
>dec0: ee2bc000 b6e37000 ee2bdf44 ee2bded8 c021d158 c0292770 c0117764
>b6e36004
>dee0: c0f0d7c4 ee2bdfb0 b6f89228 00021008 ee2bdfac ee2bdf00 c0101374
>c0117770
>df00: 00000000 00000000 ee2bc000 00000000 ee2bdf34 ee2bdf20 c016ba04
>c0171080
>df20: 00000000 00020000 ed413780 b6e37000 00000000 ee2bdf78 ee2bdf74
>ee2bdf48
>df40: c021e7a0 c021d130 c023e300 c023e280 ee2bdf74 00000000 00000000
>ed413780
>df60: ed413780 00020000 ee2bdfa4 ee2bdf78 c021e870 c021e71c 00000000
>00000000
>df80: 00020000 00020000 b6e37000 00000003 c0108084 00000000 00000000
>ee2bdfa8
>dfa0: c0107ee0 c021e838 00020000 00020000 00000003 b6e37000 00020000
>0001a2b4
>dfc0: 00020000 00020000 b6e37000 00000003 7fffe000 00000000 00000000
>00020000
>dfe0: 00000000 be98eb4c 0000c740 b6f1985c 60070010 00000003 00000000
>00000000
>Backtrace:
>[<c06fbbdc>] (iio_read_channel_info_avail) from [<c0513c58>]
>(dev_attr_show+0x24/0x50)
>r10:c0a53c74 r9:ed79f000 r8:ee8d1018 r7:00001000 r6:00000fff
>r5:ee8b9a00
> r4:edffd480
>[<c0513c34>] (dev_attr_show) from [<c0293474>]
>(sysfs_kf_seq_show+0x90/0x110)
> r5:edffd540 r4:edffd480
>[<c02933e4>] (sysfs_kf_seq_show) from [<c0291d00>]
>(kernfs_seq_show+0x2c/0x30)
>r10:edffd480 r9:00000000 r8:ed413780 r7:00000001 r6:ed413780
>r5:00000001
> r4:ee2bde60 r3:c02933e4
>[<c0291cd4>] (kernfs_seq_show) from [<c024679c>] (seq_read+0x1a4/0x4e0)
>[<c02465f8>] (seq_read) from [<c0292890>] (kernfs_fop_read+0x12c/0x1cc)
>r10:ee2bdf78 r9:00020000 r8:00020000 r7:b6e37000 r6:ed413780
>r5:edffd540
> r4:c0a111fc
>[<c0292764>] (kernfs_fop_read) from [<c021d158>]
>(__vfs_read+0x34/0x118)
>r10:b6e37000 r9:ee2bc000 r8:00020000 r7:ee2bdf78 r6:ed413780
>r5:b6e37000
> r4:c0a111fc
>[<c021d124>] (__vfs_read) from [<c021e7a0>] (vfs_read+0x90/0x11c)
> r8:ee2bdf78 r7:00000000 r6:b6e37000 r5:ed413780 r4:00020000
>[<c021e710>] (vfs_read) from [<c021e870>] (SyS_read+0x44/0x90)
> r8:00020000 r7:ed413780 r6:ed413780 r5:00000000 r4:00000000
>[<c021e82c>] (SyS_read) from [<c0107ee0>] (ret_fast_syscall+0x0/0x1c)
>r10:00000000 r8:c0108084 r7:00000003 r6:b6e37000 r5:00020000
>r4:00020000
>Code: bad PC value
>---[ end trace 9c4938ccd0389004 ]---
>
>Fixes: cc26ad455f57 ("iio: Add Freescale MPL3115A2 pressure /
>temperature sensor driver")
>Fixes: 51239600074b ("iio:core: add a callback to allow drivers to
>provide _available attributes")
>Reported-by: Ken Lin <ken=2Elin@advantech=2Ecom>
>Signed-off-by: Peter Rosin <peda@axentia=2Ese>
>---
> drivers/iio/pressure/mpl3115=2Ec | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/iio/pressure/mpl3115=2Ec
>b/drivers/iio/pressure/mpl3115=2Ec
>index cc3f84139157=2E=2E525644a7442d 100644
>--- a/drivers/iio/pressure/mpl3115=2Ec
>+++ b/drivers/iio/pressure/mpl3115=2Ec
>@@ -190,7 +190,7 @@ static const struct iio_chan_spec
>mpl3115_channels[] =3D {
> 	{
> 		=2Etype =3D IIO_PRESSURE,
> 		=2Einfo_mask_separate =3D BIT(IIO_CHAN_INFO_RAW),
>-			BIT(IIO_CHAN_INFO_SCALE),
>+		=2Einfo_mask_shared_by_type =3D BIT(IIO_CHAN_INFO_SCALE),
> 		=2Escan_index =3D 0,
> 		=2Escan_type =3D {
> 			=2Esign =3D 'u',
>@@ -203,7 +203,7 @@ static const struct iio_chan_spec
>mpl3115_channels[] =3D {
> 	{
> 		=2Etype =3D IIO_TEMP,
> 		=2Einfo_mask_separate =3D BIT(IIO_CHAN_INFO_RAW),
>-			BIT(IIO_CHAN_INFO_SCALE),
>+		=2Einfo_mask_shared_by_type =3D BIT(IIO_CHAN_INFO_SCALE),
> 		=2Escan_index =3D 1,
> 		=2Escan_type =3D {
> 			=2Esign =3D 's',

--=20
Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E

[PATCH 2/2] iio: pressure: mpl115: do not rely on structure field ordering

[Peter Rosin]

Fixes a regression triggered by a change in the layout of
struct iio_chan_spec, but the real bug is in the driver which assumed
a specific structure layout in the first place.

Fixes: 3017d90e8931 ("iio: Add Freescale MPL115A2 pressure / temperature sensor driver")
Fixes: 51239600074b ("iio:core: add a callback to allow drivers to provide _available attributes")
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/pressure/mpl115.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/pressure/mpl115.c b/drivers/iio/pressure/mpl115.c
index 73f2f0c46e62..8f2bce213248 100644
--- a/drivers/iio/pressure/mpl115.c
+++ b/drivers/iio/pressure/mpl115.c
@@ -137,6 +137,7 @@ static const struct iio_chan_spec mpl115_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type =
 			BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_SCALE),
 	},
 };
-- 
2.1.4

Re: [PATCH 0/2] iio: pressure: do not rely on structure field ordering

[Lars-Peter Clausen]

On 02/01/2017 09:09 AM, Peter Rosin wrote:
> Hi!
> 
> These two patches are needed to fix some fallout from adding the
> available attribute.

I put together a quick cocci script to check for other occurrences of this
issue and these were the only two drivers affected by the issue.