[PATCH] iio: common: st_sensors: fix channel data parsing

[PATCH] iio: common: st_sensors: fix channel data parsing

[Lorenzo Bianconi]

Using realbits as i2c/spi read len, when that value is not byte aligned
(e.g 12 bits), lead to skip msb part of out data registers.
Fix this using storagebits as read length

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index fe7775b..d01aa34 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 
 	for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {
 		const struct iio_chan_spec *channel = &indio_dev->channels[i];
-		unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
 		unsigned int storage_bytes =
 			channel->scan_type.storagebits >> 3;
 
 		buf = PTR_ALIGN(buf, storage_bytes);
 		if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
 						  channel->address,
-						  bytes_to_read, buf,
+						  storage_bytes, buf,
 						  sdata->multiread_bit) <
-		    bytes_to_read)
+		    storage_bytes)
 			return -EIO;
 
 		/* Advance the buffer pointer */
-- 
2.7.4

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Jonathan Cameron]

On 28/09/16 21:06, Lorenzo Bianconi wrote:
> Using realbits as i2c/spi read len, when that value is not byte aligned
> (e.g 12 bits), lead to skip msb part of out data registers.
> Fix this using storagebits as read length
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
Hi Lorenzo,

This would ideally have had a fixes tag.

I think it was
Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
Doing that would also have lead to you to the logic that lead to
this buggy change in the first place. Would also have shown
you that there is another place that probably suffers from the
same sort of issue.

Gregor can you take a look at this please.

If I recall the issue that lead to the original patch it was
that we were miss handling 24 bit values on pressure sensors and
this was intended to pad them?

I think the 'right' fix will be something along the lines of:
unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;

Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
24 bit read.

Thanks,

Jonathan
> ---
>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index fe7775b..d01aa34 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  
>  	for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
>  		const struct iio_chan_spec *channel = &indio_dev->channels[i];
> -		unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
>  		unsigned int storage_bytes =
>  			channel->scan_type.storagebits >> 3;
>  
>  		buf = PTR_ALIGN(buf, storage_bytes);
>  		if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
>  						  channel->address,
> -						  bytes_to_read, buf,
> +						  storage_bytes, buf,
>  						  sdata->multiread_bit) <
> -		    bytes_to_read)
> +		    storage_bytes)
>  			return -EIO;
>  
>  		/* Advance the buffer pointer */
> 

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Lorenzo Bianconi]

> On 28/09/16 21:06, Lorenzo Bianconi wrote:
>> Using realbits as i2c/spi read len, when that value is not byte aligned
>> (e.g 12 bits), lead to skip msb part of out data registers.
>> Fix this using storagebits as read length
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> Hi Lorenzo,

Hi Jonathan,

>
> This would ideally have had a fixes tag.
>
> I think it was
> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
> Doing that would also have lead to you to the logic that lead to
> this buggy change in the first place. Would also have shown
> you that there is another place that probably suffers from the
> same sort of issue.
>

Right. I missed the same issue in st_sensors_read_axis_data()

> Gregor can you take a look at this please.
>
> If I recall the issue that lead to the original patch it was
> that we were miss handling 24 bit values on pressure sensors and
> this was intended to pad them?
>
> I think the 'right' fix will be something along the lines of:
> unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
>

This should not be correct if you consider 8bit accel like LIS331DL.
Taking a look to lps22hb code I think storagebits should be 24 and not
32 for pressure channel. What do you think?

> Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
> 24 bit read.
>
> Thanks,
>

Thanks,
Lorenzo

> Jonathan
>> ---
>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> index fe7775b..d01aa34 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> @@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>
>>       for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
>>               const struct iio_chan_spec *channel = &indio_dev->channels[i];
>> -             unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
>>               unsigned int storage_bytes =
>>                       channel->scan_type.storagebits >> 3;
>>
>>               buf = PTR_ALIGN(buf, storage_bytes);
>>               if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
>>                                                 channel->address,
>> -                                               bytes_to_read, buf,
>> +                                               storage_bytes, buf,
>>                                                 sdata->multiread_bit) <
>> -                 bytes_to_read)
>> +                 storage_bytes)
>>                       return -EIO;
>>
>>               /* Advance the buffer pointer */
>>
>



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Jonathan Cameron]

On 01/10/16 15:18, Lorenzo Bianconi wrote:
>> On 28/09/16 21:06, Lorenzo Bianconi wrote:
>>> Using realbits as i2c/spi read len, when that value is not byte aligned
>>> (e.g 12 bits), lead to skip msb part of out data registers.
>>> Fix this using storagebits as read length
>>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>> Hi Lorenzo,
> 
> Hi Jonathan,
> 
>>
>> This would ideally have had a fixes tag.
>>
>> I think it was
>> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
>> Doing that would also have lead to you to the logic that lead to
>> this buggy change in the first place. Would also have shown
>> you that there is another place that probably suffers from the
>> same sort of issue.
>>
> 
> Right. I missed the same issue in st_sensors_read_axis_data()
> 
>> Gregor can you take a look at this please.
>>
>> If I recall the issue that lead to the original patch it was
>> that we were miss handling 24 bit values on pressure sensors and
>> this was intended to pad them?
>>
>> I think the 'right' fix will be something along the lines of:
>> unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
>>
> 
> This should not be correct if you consider 8bit accel like LIS331DL.
8 + 7 = 15
15 >> 3 = 1 byte.

Would get interest if the data was shifted to run across two bytes, but
it's not.  Could handle that case as well by doing
(channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3

> Taking a look to lps22hb code I think storagebits should be 24 and not
> 32 for pressure channel. What do you think?
No. Storage bytes must always be a power of 2. It's assumed at various
points in the buffer handling code in the core and userspace code.
This is why Gregor's fix was needed.

These 24 bit packed readings are a pain, but it was much worse to try
and remove the aligned data assumptions that need them to be powers
of 2.  Just think about 3 bytes floating in a little endian integer
field vs the big endian version. It's a real pain to unwind.
> 
>> Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
>> 24 bit read.
>>
>> Thanks,
>>
> 
> Thanks,
> Lorenzo
> 
>> Jonathan
>>> ---
>>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>> index fe7775b..d01aa34 100644
>>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>> @@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>>
>>>       for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
>>>               const struct iio_chan_spec *channel = &indio_dev->channels[i];
>>> -             unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
>>>               unsigned int storage_bytes =
>>>                       channel->scan_type.storagebits >> 3;
>>>
>>>               buf = PTR_ALIGN(buf, storage_bytes);
>>>               if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
>>>                                                 channel->address,
>>> -                                               bytes_to_read, buf,
>>> +                                               storage_bytes, buf,
>>>                                                 sdata->multiread_bit) <
>>> -                 bytes_to_read)
>>> +                 storage_bytes)
>>>                       return -EIO;
>>>
>>>               /* Advance the buffer pointer */
>>>
>>
> 
> 
> 

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Lorenzo Bianconi]

On Oct 01, Jonathan Cameron wrote:
> On 01/10/16 15:18, Lorenzo Bianconi wrote:
> >> On 28/09/16 21:06, Lorenzo Bianconi wrote:
> >>> Using realbits as i2c/spi read len, when that value is not byte aligned
> >>> (e.g 12 bits), lead to skip msb part of out data registers.
> >>> Fix this using storagebits as read length
> >>>
> >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> >> Hi Lorenzo,
> > 
> > Hi Jonathan,
> > 
> >>
> >> This would ideally have had a fixes tag.
> >>
> >> I think it was
> >> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
> >> Doing that would also have lead to you to the logic that lead to
> >> this buggy change in the first place. Would also have shown
> >> you that there is another place that probably suffers from the
> >> same sort of issue.
> >>
> > 
> > Right. I missed the same issue in st_sensors_read_axis_data()
> > 
> >> Gregor can you take a look at this please.
> >>
> >> If I recall the issue that lead to the original patch it was
> >> that we were miss handling 24 bit values on pressure sensors and
> >> this was intended to pad them?
> >>
> >> I think the 'right' fix will be something along the lines of:
> >> unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
> >>
> > 
> > This should not be correct if you consider 8bit accel like LIS331DL.
> 8 + 7 = 15
> 15 >> 3 = 1 byte.

Right sorry :)

> 
> Would get interest if the data was shifted to run across two bytes, but
> it's not.  Could handle that case as well by doing
> (channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
> 

Ack. I will send a v2 afer testing on multiple devices (8, 12, 24 realbits)

> > Taking a look to lps22hb code I think storagebits should be 24 and not
> > 32 for pressure channel. What do you think?
> No. Storage bytes must always be a power of 2. It's assumed at various
> points in the buffer handling code in the core and userspace code.
> This is why Gregor's fix was needed.
> 
> These 24 bit packed readings are a pain, but it was much worse to try
> and remove the aligned data assumptions that need them to be powers
> of 2.  Just think about 3 bytes floating in a little endian integer
> field vs the big endian version. It's a real pain to unwind.

Right :)

Regards,
Lorenzo

> > 
> >> Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
> >> 24 bit read.
> >>
> >> Thanks,
> >>
> > 
> > Thanks,
> > Lorenzo
> > 
> >> Jonathan
> >>> ---
> >>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
> >>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >>> index fe7775b..d01aa34 100644
> >>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >>> @@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> >>>
> >>>       for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
> >>>               const struct iio_chan_spec *channel = &indio_dev->channels[i];
> >>> -             unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
> >>>               unsigned int storage_bytes =
> >>>                       channel->scan_type.storagebits >> 3;
> >>>
> >>>               buf = PTR_ALIGN(buf, storage_bytes);
> >>>               if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
> >>>                                                 channel->address,
> >>> -                                               bytes_to_read, buf,
> >>> +                                               storage_bytes, buf,
> >>>                                                 sdata->multiread_bit) <
> >>> -                 bytes_to_read)
> >>> +                 storage_bytes)
> >>>                       return -EIO;
> >>>
> >>>               /* Advance the buffer pointer */
> >>>
> >>
> > 
> > 
> > 
> 

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch; unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp; umount; make clean; sleep

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Gregor Boirie]

Sorry for the delay (vacations). Answers inline...


On 10/01/2016 04:58 PM, Jonathan Cameron wrote:
> On 01/10/16 15:18, Lorenzo Bianconi wrote:
>>> On 28/09/16 21:06, Lorenzo Bianconi wrote:
>>>> Using realbits as i2c/spi read len, when that value is not byte aligned
>>>> (e.g 12 bits), lead to skip msb part of out data registers.
>>>> Fix this using storagebits as read length
>>>>
>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>>> Hi Lorenzo,
>> Hi Jonathan,
>>
>>> This would ideally have had a fixes tag.
>>>
>>> I think it was
>>> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
>>> Doing that would also have lead to you to the logic that lead to
>>> this buggy change in the first place. Would also have shown
>>> you that there is another place that probably suffers from the
>>> same sort of issue.
>>>
>> Right. I missed the same issue in st_sensors_read_axis_data()
>>
>>> Gregor can you take a look at this please.
>>>
>>> If I recall the issue that lead to the original patch it was
>>> that we were miss handling 24 bit values on pressure sensors and
>>> this was intended to pad them?
>>>
>>> I think the 'right' fix will be something along the lines of:
>>> unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
>>>
>> This should not be correct if you consider 8bit accel like LIS331DL.
> 8 + 7 = 15
> 15 >> 3 = 1 byte.
>
> Would get interest if the data was shifted to run across two bytes, but
> it's not.  Could handle that case as well by doing
> (channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
Agreed.Maybe using DIV_ROUND_UP() would make ita bit clearer ? Something
like :
DIV_ROUND_UP(channel->scan_type.realbits + channel->scan_type.shift, 8)

Should alsobe applied to st_sensors_get_buffer_element() and
st_sensors_read_axis_data() functions as told earlier.


>> Taking a look to lps22hb code I think storagebits should be 24 and not
>> 32 for pressure channel. What do you think?
> No. Storage bytes must always be a power of 2. It's assumed at various
> points in the buffer handling code in the core and userspace code.
> This is why Gregor's fix was needed.
That was it.

I'll be happy to perform some testing with my setup (LPS22HB).

Regards,
Grégor.

>
> These 24 bit packed readings are a pain, but it was much worse to try
> and remove the aligned data assumptions that need them to be powers
> of 2.  Just think about 3 bytes floating in a little endian integer
> field vs the big endian version. It's a real pain to unwind.
>>> Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
>>> 24 bit read.
>>>
>>> Thanks,
>>>
>> Thanks,
>> Lorenzo
>>
>>> Jonathan
>>>> ---
>>>>   drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>> index fe7775b..d01aa34 100644
>>>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>> @@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>>>
>>>>        for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
>>>>                const struct iio_chan_spec *channel = &indio_dev->channels[i];
>>>> -             unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
>>>>                unsigned int storage_bytes =
>>>>                        channel->scan_type.storagebits >> 3;
>>>>
>>>>                buf = PTR_ALIGN(buf, storage_bytes);
>>>>                if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
>>>>                                                  channel->address,
>>>> -                                               bytes_to_read, buf,
>>>> +                                               storage_bytes, buf,
>>>>                                                  sdata->multiread_bit) <
>>>> -                 bytes_to_read)
>>>> +                 storage_bytes)
>>>>                        return -EIO;
>>>>
>>>>                /* Advance the buffer pointer */
>>>>
>>
>>

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Lorenzo Bianconi]

> Sorry for the delay (vacations). Answers inline...
>
>
>
> On 10/01/2016 04:58 PM, Jonathan Cameron wrote:
>>
>> On 01/10/16 15:18, Lorenzo Bianconi wrote:
>>>>
>>>> On 28/09/16 21:06, Lorenzo Bianconi wrote:
>>>>>
>>>>> Using realbits as i2c/spi read len, when that value is not byte align=
ed
>>>>> (e.g 12 bits), lead to skip msb part of out data registers.
>>>>> Fix this using storagebits as read length
>>>>>
>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>>>>
>>>> Hi Lorenzo,
>>>
>>> Hi Jonathan,
>>>
>>>> This would ideally have had a fixes tag.
>>>>
>>>> I think it was
>>>> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
>>>> Doing that would also have lead to you to the logic that lead to
>>>> this buggy change in the first place. Would also have shown
>>>> you that there is another place that probably suffers from the
>>>> same sort of issue.
>>>>
>>> Right. I missed the same issue in st_sensors_read_axis_data()
>>>
>>>> Gregor can you take a look at this please.
>>>>
>>>> If I recall the issue that lead to the original patch it was
>>>> that we were miss handling 24 bit values on pressure sensors and
>>>> this was intended to pad them?
>>>>
>>>> I think the 'right' fix will be something along the lines of:
>>>> unsigned int bytes_to_read =3D (channel->scan_type.realbits + 7) >> 3;
>>>>
>>> This should not be correct if you consider 8bit accel like LIS331DL.
>>
>> 8 + 7 =3D 15
>> 15 >> 3 =3D 1 byte.
>>
>> Would get interest if the data was shifted to run across two bytes, but
>> it's not.  Could handle that case as well by doing
>> (channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
>
> Agreed.Maybe using DIV_ROUND_UP() would make ita bit clearer ? Something
> like :
> DIV_ROUND_UP(channel->scan_type.realbits + channel->scan_type.shift, 8)
>
> Should alsobe applied to st_sensors_get_buffer_element() and
> st_sensors_read_axis_data() functions as told earlier.
>
>
>>> Taking a look to lps22hb code I think storagebits should be 24 and not
>>> 32 for pressure channel. What do you think?
>>
>> No. Storage bytes must always be a power of 2. It's assumed at various
>> points in the buffer handling code in the core and userspace code.
>> This is why Gregor's fix was needed.
>
> That was it.
>
> I'll be happy to perform some testing with my setup (LPS22HB).
>

I will perform some tests on multiple devices later today and I will
let you know

Regards,
Lorenzo

> Regards,
> Gr=C3=A9gor.
>
>
>>
>> These 24 bit packed readings are a pain, but it was much worse to try
>> and remove the aligned data assumptions that need them to be powers
>> of 2.  Just think about 3 bytes floating in a little endian integer
>> field vs the big endian version. It's a real pain to unwind.
>>>>
>>>> Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
>>>> 24 bit read.
>>>>
>>>> Thanks,
>>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> Jonathan
>>>>>
>>>>> ---
>>>>>   drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
>>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>>> b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>>> index fe7775b..d01aa34 100644
>>>>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>>>>> @@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct
>>>>> iio_dev *indio_dev, u8 *buf)
>>>>>
>>>>>        for_each_set_bit(i, indio_dev->active_scan_mask,
>>>>> num_data_channels) {q
>>>>>                const struct iio_chan_spec *channel =3D
>>>>> &indio_dev->channels[i];
>>>>> -             unsigned int bytes_to_read =3D channel->scan_type.realb=
its
>>>>> >> 3;
>>>>>                unsigned int storage_bytes =3D
>>>>>                        channel->scan_type.storagebits >> 3;
>>>>>
>>>>>                buf =3D PTR_ALIGN(buf, storage_bytes);
>>>>>                if (sdata->tf->read_multiple_byte(&sdata->tb,
>>>>> sdata->dev,
>>>>>                                                  channel->address,
>>>>> -                                               bytes_to_read, buf,
>>>>> +                                               storage_bytes, buf,
>>>>>                                                  sdata->multiread_bit=
)
>>>>> <
>>>>> -                 bytes_to_read)
>>>>> +                 storage_bytes)
>>>>>                        return -EIO;
>>>>>
>>>>>                /* Advance the buffer pointer */
>>>>>
>>>
>>>
>



--=20
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Lorenzo Bianconi]

> Sorry for the delay (vacations). Answers inline...

Hi Gregor,

> 
> 
> On 10/01/2016 04:58 PM, Jonathan Cameron wrote:
> >On 01/10/16 15:18, Lorenzo Bianconi wrote:
> >>>On 28/09/16 21:06, Lorenzo Bianconi wrote:
> >>>>Using realbits as i2c/spi read len, when that value is not byte aligned
> >>>>(e.g 12 bits), lead to skip msb part of out data registers.
> >>>>Fix this using storagebits as read length
> >>>>
> >>>>Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> >>>Hi Lorenzo,
> >>Hi Jonathan,
> >>
> >>>This would ideally have had a fixes tag.
> >>>
> >>>I think it was
> >>>Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
> >>>Doing that would also have lead to you to the logic that lead to
> >>>this buggy change in the first place. Would also have shown
> >>>you that there is another place that probably suffers from the
> >>>same sort of issue.
> >>>
> >>Right. I missed the same issue in st_sensors_read_axis_data()
> >>
> >>>Gregor can you take a look at this please.
> >>>
> >>>If I recall the issue that lead to the original patch it was
> >>>that we were miss handling 24 bit values on pressure sensors and
> >>>this was intended to pad them?
> >>>
> >>>I think the 'right' fix will be something along the lines of:
> >>>unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
> >>>
> >>This should not be correct if you consider 8bit accel like LIS331DL.
> >8 + 7 = 15
> >15 >> 3 = 1 byte.
> >
> >Would get interest if the data was shifted to run across two bytes, but
> >it's not.  Could handle that case as well by doing
> >(channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
> Agreed.Maybe using DIV_ROUND_UP() would make ita bit clearer ? Something
> like :
> DIV_ROUND_UP(channel->scan_type.realbits + channel->scan_type.shift, 8)
> 
> Should alsobe applied to st_sensors_get_buffer_element() and
> st_sensors_read_axis_data() functions as told earlier.
> 
> 
> >>Taking a look to lps22hb code I think storagebits should be 24 and not
> >>32 for pressure channel. What do you think?
> >No. Storage bytes must always be a power of 2. It's assumed at various
> >points in the buffer handling code in the core and userspace code.
> >This is why Gregor's fix was needed.
> That was it.
> 
> I'll be happy to perform some testing with my setup (LPS22HB).

Today I tested the patch inline on 8,12,16 bit device and it worked properly.
I had no the chance to test it on 24 bit device (lps22hb). Can you give it a
whirl?

Thanks,
Lorenzo

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c | 4 +++-
 drivers/iio/common/st_sensors/st_sensors_core.c   | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index fe7775b..df40452 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -30,7 +30,9 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
 
 	for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {
 		const struct iio_chan_spec *channel = &indio_dev->channels[i];
-		unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
+		unsigned int bytes_to_read =
+			DIV_ROUND_UP(channel->scan_type.realbits +
+				     channel->scan_type.shift, 8);
 		unsigned int storage_bytes =
 			channel->scan_type.storagebits >> 3;
 
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 285a64a..afd60a6 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -483,8 +483,10 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
 	int err;
 	u8 *outdata;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
-	unsigned int byte_for_channel = ch->scan_type.realbits >> 3;
+	unsigned int byte_for_channel;
 
+	byte_for_channel = DIV_ROUND_UP(ch->scan_type.realbits +
+					ch->scan_type.shift, 8);
 	outdata = kmalloc(byte_for_channel, GFP_KERNEL);
 	if (!outdata)
 		return -ENOMEM;
-- 
2.7.4

> 
> Regards,
> Grégor.
> 
> >
> >These 24 bit packed readings are a pain, but it was much worse to try
> >and remove the aligned data assumptions that need them to be powers
> >of 2.  Just think about 3 bytes floating in a little endian integer
> >field vs the big endian version. It's a real pain to unwind.
> >>>Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
> >>>24 bit read.
> >>>
> >>>Thanks,
> >>>
> >>Thanks,
> >>Lorenzo
> >>
> >>>Jonathan
> >>>>---
> >>>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
> >>>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >>>>index fe7775b..d01aa34 100644
> >>>>--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >>>>+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> >>>>@@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> >>>>
> >>>>       for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
> >>>>               const struct iio_chan_spec *channel = &indio_dev->channels[i];
> >>>>-             unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
> >>>>               unsigned int storage_bytes =
> >>>>                       channel->scan_type.storagebits >> 3;
> >>>>
> >>>>               buf = PTR_ALIGN(buf, storage_bytes);
> >>>>               if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
> >>>>                                                 channel->address,
> >>>>-                                               bytes_to_read, buf,
> >>>>+                                               storage_bytes, buf,
> >>>>                                                 sdata->multiread_bit) <
> >>>>-                 bytes_to_read)
> >>>>+                 storage_bytes)
> >>>>                       return -EIO;
> >>>>
> >>>>               /* Advance the buffer pointer */
> >>>>
> >>
> >>
> 

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch; unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp; umount; make clean; sleep

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Jonathan Cameron]

On 03/10/16 20:35, Lorenzo Bianconi wrote:
>> Sorry for the delay (vacations). Answers inline...
> 
> Hi Gregor,
> 
>>
>>
>> On 10/01/2016 04:58 PM, Jonathan Cameron wrote:
>>> On 01/10/16 15:18, Lorenzo Bianconi wrote:
>>>>> On 28/09/16 21:06, Lorenzo Bianconi wrote:
>>>>>> Using realbits as i2c/spi read len, when that value is not byte aligned
>>>>>> (e.g 12 bits), lead to skip msb part of out data registers.
>>>>>> Fix this using storagebits as read length
>>>>>>
>>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>>>>> Hi Lorenzo,
>>>> Hi Jonathan,
>>>>
>>>>> This would ideally have had a fixes tag.
>>>>>
>>>>> I think it was
>>>>> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
>>>>> Doing that would also have lead to you to the logic that lead to
>>>>> this buggy change in the first place. Would also have shown
>>>>> you that there is another place that probably suffers from the
>>>>> same sort of issue.
>>>>>
>>>> Right. I missed the same issue in st_sensors_read_axis_data()
>>>>
>>>>> Gregor can you take a look at this please.
>>>>>
>>>>> If I recall the issue that lead to the original patch it was
>>>>> that we were miss handling 24 bit values on pressure sensors and
>>>>> this was intended to pad them?
>>>>>
>>>>> I think the 'right' fix will be something along the lines of:
>>>>> unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
>>>>>
>>>> This should not be correct if you consider 8bit accel like LIS331DL.
>>> 8 + 7 = 15
>>> 15 >> 3 = 1 byte.
>>>
>>> Would get interest if the data was shifted to run across two bytes, but
>>> it's not.  Could handle that case as well by doing
>>> (channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
>> Agreed.Maybe using DIV_ROUND_UP() would make ita bit clearer ? Something
>> like :
>> DIV_ROUND_UP(channel->scan_type.realbits + channel->scan_type.shift, 8)
>>
>> Should alsobe applied to st_sensors_get_buffer_element() and
>> st_sensors_read_axis_data() functions as told earlier.
>>
>>
>>>> Taking a look to lps22hb code I think storagebits should be 24 and not
>>>> 32 for pressure channel. What do you think?
>>> No. Storage bytes must always be a power of 2. It's assumed at various
>>> points in the buffer handling code in the core and userspace code.
>>> This is why Gregor's fix was needed.
>> That was it.
>>
>> I'll be happy to perform some testing with my setup (LPS22HB).
> 
> Today I tested the patch inline on 8,12,16 bit device and it worked properly.
> I had no the chance to test it on 24 bit device (lps22hb). Can you give it a
> whirl?
> 
> Thanks,
> Lorenzo
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
Much nicer than my 'bodge' ;)

Once Gregor's had a chance to test, please send me a fresh copy of this with
his tested-by etc so I don't have to fiddle with pulling it out of this thread.

Thanks,

Jonathan
> ---
>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 4 +++-
>  drivers/iio/common/st_sensors/st_sensors_core.c   | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index fe7775b..df40452 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -30,7 +30,9 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>  
>  	for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {
>  		const struct iio_chan_spec *channel = &indio_dev->channels[i];
> -		unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
> +		unsigned int bytes_to_read =
> +			DIV_ROUND_UP(channel->scan_type.realbits +
> +				     channel->scan_type.shift, 8);
>  		unsigned int storage_bytes =
>  			channel->scan_type.storagebits >> 3;
>  
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 285a64a..afd60a6 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -483,8 +483,10 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
>  	int err;
>  	u8 *outdata;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
> -	unsigned int byte_for_channel = ch->scan_type.realbits >> 3;
> +	unsigned int byte_for_channel;
>  
> +	byte_for_channel = DIV_ROUND_UP(ch->scan_type.realbits +
> +					ch->scan_type.shift, 8);
>  	outdata = kmalloc(byte_for_channel, GFP_KERNEL);
>  	if (!outdata)
>  		return -ENOMEM;
> 

Re: [PATCH] iio: common: st_sensors: fix channel data parsing

[Lorenzo Bianconi]

Hi Gregor,

> On 03/10/16 20:35, Lorenzo Bianconi wrote:
>>> Sorry for the delay (vacations). Answers inline...
>>
>> Hi Gregor,
>>
>>>
>>>
>>> On 10/01/2016 04:58 PM, Jonathan Cameron wrote:
>>>> On 01/10/16 15:18, Lorenzo Bianconi wrote:
>>>>>> On 28/09/16 21:06, Lorenzo Bianconi wrote:
>>>>>>> Using realbits as i2c/spi read len, when that value is not byte aligned
>>>>>>> (e.g 12 bits), lead to skip msb part of out data registers.
>>>>>>> Fix this using storagebits as read length
>>>>>>>
>>>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>>>>>> Hi Lorenzo,
>>>>> Hi Jonathan,
>>>>>
>>>>>> This would ideally have had a fixes tag.
>>>>>>
>>>>>> I think it was
>>>>>> Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
>>>>>> Doing that would also have lead to you to the logic that lead to
>>>>>> this buggy change in the first place. Would also have shown
>>>>>> you that there is another place that probably suffers from the
>>>>>> same sort of issue.
>>>>>>
>>>>> Right. I missed the same issue in st_sensors_read_axis_data()
>>>>>
>>>>>> Gregor can you take a look at this please.
>>>>>>
>>>>>> If I recall the issue that lead to the original patch it was
>>>>>> that we were miss handling 24 bit values on pressure sensors and
>>>>>> this was intended to pad them?
>>>>>>
>>>>>> I think the 'right' fix will be something along the lines of:
>>>>>> unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
>>>>>>
>>>>> This should not be correct if you consider 8bit accel like LIS331DL.
>>>> 8 + 7 = 15
>>>> 15 >> 3 = 1 byte.
>>>>
>>>> Would get interest if the data was shifted to run across two bytes, but
>>>> it's not.  Could handle that case as well by doing
>>>> (channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
>>> Agreed.Maybe using DIV_ROUND_UP() would make ita bit clearer ? Something
>>> like :
>>> DIV_ROUND_UP(channel->scan_type.realbits + channel->scan_type.shift, 8)
>>>
>>> Should alsobe applied to st_sensors_get_buffer_element() and
>>> st_sensors_read_axis_data() functions as told earlier.
>>>
>>>
>>>>> Taking a look to lps22hb code I think storagebits should be 24 and not
>>>>> 32 for pressure channel. What do you think?
>>>> No. Storage bytes must always be a power of 2. It's assumed at various
>>>> points in the buffer handling code in the core and userspace code.
>>>> This is why Gregor's fix was needed.
>>> That was it.
>>>
>>> I'll be happy to perform some testing with my setup (LPS22HB).
>>
>> Today I tested the patch inline on 8,12,16 bit device and it worked properly.
>> I had no the chance to test it on 24 bit device (lps22hb). Can you give it a
>> whirl?
>>
>> Thanks,
>> Lorenzo
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> Much nicer than my 'bodge' ;)
>
> Once Gregor's had a chance to test, please send me a fresh copy of this with
> his tested-by etc so I don't have to fiddle with pulling it out of this thread.
>
> Thanks,
>
> Jonathan

have you got the chance to test that patch on lps22hb?

Regards,
Lorenzo

>> ---
>>  drivers/iio/common/st_sensors/st_sensors_buffer.c | 4 +++-
>>  drivers/iio/common/st_sensors/st_sensors_core.c   | 4 +++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> index fe7775b..df40452 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> @@ -30,7 +30,9 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>>
>>       for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {
>>               const struct iio_chan_spec *channel = &indio_dev->channels[i];
>> -             unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
>> +             unsigned int bytes_to_read =
>> +                     DIV_ROUND_UP(channel->scan_type.realbits +
>> +                                  channel->scan_type.shift, 8);
>>               unsigned int storage_bytes =
>>                       channel->scan_type.storagebits >> 3;
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
>> index 285a64a..afd60a6 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
>> @@ -483,8 +483,10 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
>>       int err;
>>       u8 *outdata;
>>       struct st_sensor_data *sdata = iio_priv(indio_dev);
>> -     unsigned int byte_for_channel = ch->scan_type.realbits >> 3;
>> +     unsigned int byte_for_channel;
>>
>> +     byte_for_channel = DIV_ROUND_UP(ch->scan_type.realbits +
>> +                                     ch->scan_type.shift, 8);
>>       outdata = kmalloc(byte_for_channel, GFP_KERNEL);
>>       if (!outdata)
>>               return -ENOMEM;
>>
>



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep