Don't try to access buffer data of a channel by its scan index. Instead,
use the newly introduced `iio_find_channel_offset_in_buffer` to get the
correct data offset.
The scan index of a channel does not represent its position in a buffer,
as the buffer will contain data for enabled channels only, affecting
data offsets and alignment.
Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick driver.")
Reported-by: Chris Morgan <[email protected]>
Tested-by: Paul Cercueil <[email protected]>
Signed-off-by: Artur Rojek <[email protected]>
---
drivers/input/joystick/adc-joystick.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..aed853ebe1d1 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -6,6 +6,7 @@
#include <linux/ctype.h>
#include <linux/input.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/consumer.h>
#include <linux/module.h>
#include <linux/platform_device.h>
@@ -46,36 +47,43 @@ static void adc_joystick_poll(struct input_dev *input)
static int adc_joystick_handle(const void *data, void *private)
{
struct adc_joystick *joy = private;
+ struct iio_buffer *buffer;
enum iio_endian endianness;
- int bytes, msb, val, idx, i;
- const u16 *data_u16;
+ int bytes, msb, val, off;
+ const u8 *chan_data;
+ unsigned int i;
bool sign;
bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
for (i = 0; i < joy->num_chans; ++i) {
- idx = joy->chans[i].channel->scan_index;
endianness = joy->chans[i].channel->scan_type.endianness;
msb = joy->chans[i].channel->scan_type.realbits - 1;
sign = tolower(joy->chans[i].channel->scan_type.sign) == 's';
+ buffer = iio_channel_cb_get_iio_buffer(joy->buffer);
+ off = iio_find_channel_offset_in_buffer(joy->chans[i].indio_dev,
+ joy->chans[i].channel,
+ buffer);
+ if (off < 0)
+ return off;
+
+ chan_data = (const u8 *)data + off;
switch (bytes) {
case 1:
- val = ((const u8 *)data)[idx];
+ val = *chan_data;
break;
case 2:
- data_u16 = (const u16 *)data + idx;
-
/*
* Data is aligned to the sample size by IIO core.
* Call `get_unaligned_xe16` to hide type casting.
*/
if (endianness == IIO_BE)
- val = get_unaligned_be16(data_u16);
+ val = get_unaligned_be16(chan_data);
else if (endianness == IIO_LE)
- val = get_unaligned_le16(data_u16);
+ val = get_unaligned_le16(chan_data);
else /* IIO_CPU */
- val = *data_u16;
+ val = *(const u16 *)chan_data;
break;
default:
return -EINVAL;
--
2.37.2
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <[email protected]> wrote:
>
> Don't try to access buffer data of a channel by its scan index. Instead,
> use the newly introduced `iio_find_channel_offset_in_buffer` to get the
> correct data offset.
>
> The scan index of a channel does not represent its position in a buffer,
> as the buffer will contain data for enabled channels only, affecting
> data offsets and alignment.
> Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick driver.")
You may not use Fixes here because it has dependencies. The possible
solutions are:
1/
a) create a real fix for the existing code;
b) refactor it.
2/
put the Fixes tag to all dependencies (which is questionable).
> Reported-by: Chris Morgan <[email protected]>
> Tested-by: Paul Cercueil <[email protected]>
> Signed-off-by: Artur Rojek <[email protected]>
Try to keep tags in chronological order, I do not believe the change
may be tested before it has been created.
--
With Best Regards,
Andy Shevchenko
On Wed, 17 Aug 2022 12:56:43 +0200
Artur Rojek <[email protected]> wrote:
> Don't try to access buffer data of a channel by its scan index. Instead,
> use the newly introduced `iio_find_channel_offset_in_buffer` to get the
> correct data offset.
>
> The scan index of a channel does not represent its position in a buffer,
> as the buffer will contain data for enabled channels only, affecting
> data offsets and alignment.
>
> Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick driver.")
> Reported-by: Chris Morgan <[email protected]>
> Tested-by: Paul Cercueil <[email protected]>
> Signed-off-by: Artur Rojek <[email protected]>
> ---
> drivers/input/joystick/adc-joystick.c | 26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
> index c0deff5d4282..aed853ebe1d1 100644
> --- a/drivers/input/joystick/adc-joystick.c
> +++ b/drivers/input/joystick/adc-joystick.c
> @@ -6,6 +6,7 @@
> #include <linux/ctype.h>
> #include <linux/input.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/consumer.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> @@ -46,36 +47,43 @@ static void adc_joystick_poll(struct input_dev *input)
> static int adc_joystick_handle(const void *data, void *private)
> {
> struct adc_joystick *joy = private;
> + struct iio_buffer *buffer;
> enum iio_endian endianness;
> - int bytes, msb, val, idx, i;
> - const u16 *data_u16;
> + int bytes, msb, val, off;
> + const u8 *chan_data;
> + unsigned int i;
> bool sign;
>
> bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
>
> for (i = 0; i < joy->num_chans; ++i) {
> - idx = joy->chans[i].channel->scan_index;
> endianness = joy->chans[i].channel->scan_type.endianness;
> msb = joy->chans[i].channel->scan_type.realbits - 1;
> sign = tolower(joy->chans[i].channel->scan_type.sign) == 's';
> + buffer = iio_channel_cb_get_iio_buffer(joy->buffer);
> + off = iio_find_channel_offset_in_buffer(joy->chans[i].indio_dev,
> + joy->chans[i].channel,
> + buffer);
With this call replaced with one that instead uses
off = iio_find_channel_offset_in_buffer(joy->chans, i);
which I'm fairly sure is enough via the info in chans[x]->channel to establish this offset.
All is good, though you should probably cache it as doing that maths every
time seems excessive.
> + if (off < 0)
> + return off;
> +
> + chan_data = (const u8 *)data + off;
>
> switch (bytes) {
> case 1:
> - val = ((const u8 *)data)[idx];
> + val = *chan_data;
> break;
> case 2:
> - data_u16 = (const u16 *)data + idx;
> -
> /*
> * Data is aligned to the sample size by IIO core.
> * Call `get_unaligned_xe16` to hide type casting.
> */
> if (endianness == IIO_BE)
> - val = get_unaligned_be16(data_u16);
> + val = get_unaligned_be16(chan_data);
I obviously missed this previously but these are aligned so we don't need the
unaligned form.
> else if (endianness == IIO_LE)
> - val = get_unaligned_le16(data_u16);
> + val = get_unaligned_le16(chan_data);
> else /* IIO_CPU */
> - val = *data_u16;
> + val = *(const u16 *)chan_data;
> break;
> default:
> return -EINVAL;
Hi Jonathan,
Le ven., ao?t 19 2022 at 18:53:39 +0100, Jonathan Cameron
<[email protected]> a ?crit :
> On Wed, 17 Aug 2022 12:56:43 +0200
> Artur Rojek <[email protected]> wrote:
>
>> Don't try to access buffer data of a channel by its scan index.
>> Instead,
>> use the newly introduced `iio_find_channel_offset_in_buffer` to get
>> the
>> correct data offset.
>>
>> The scan index of a channel does not represent its position in a
>> buffer,
>> as the buffer will contain data for enabled channels only, affecting
>> data offsets and alignment.
>>
>> Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick
>> driver.")
>> Reported-by: Chris Morgan <[email protected]>
>> Tested-by: Paul Cercueil <[email protected]>
>> Signed-off-by: Artur Rojek <[email protected]>
>> ---
>> drivers/input/joystick/adc-joystick.c | 26
>> +++++++++++++++++---------
>> 1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/input/joystick/adc-joystick.c
>> b/drivers/input/joystick/adc-joystick.c
>> index c0deff5d4282..aed853ebe1d1 100644
>> --- a/drivers/input/joystick/adc-joystick.c
>> +++ b/drivers/input/joystick/adc-joystick.c
>> @@ -6,6 +6,7 @@
>> #include <linux/ctype.h>
>> #include <linux/input.h>
>> #include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> #include <linux/iio/consumer.h>
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> @@ -46,36 +47,43 @@ static void adc_joystick_poll(struct input_dev
>> *input)
>> static int adc_joystick_handle(const void *data, void *private)
>> {
>> struct adc_joystick *joy = private;
>> + struct iio_buffer *buffer;
>> enum iio_endian endianness;
>> - int bytes, msb, val, idx, i;
>> - const u16 *data_u16;
>> + int bytes, msb, val, off;
>> + const u8 *chan_data;
>> + unsigned int i;
>> bool sign;
>>
>> bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
>>
>> for (i = 0; i < joy->num_chans; ++i) {
>> - idx = joy->chans[i].channel->scan_index;
>> endianness = joy->chans[i].channel->scan_type.endianness;
>> msb = joy->chans[i].channel->scan_type.realbits - 1;
>> sign = tolower(joy->chans[i].channel->scan_type.sign) == 's';
>> + buffer = iio_channel_cb_get_iio_buffer(joy->buffer);
>> + off = iio_find_channel_offset_in_buffer(joy->chans[i].indio_dev,
>> + joy->chans[i].channel,
>> + buffer);
>
> With this call replaced with one that instead uses
>
> off = iio_find_channel_offset_in_buffer(joy->chans, i);
>
> which I'm fairly sure is enough via the info in chans[x]->channel to
> establish this offset.
>
> All is good, though you should probably cache it as doing that maths
> every
> time seems excessive.
>
>
>> + if (off < 0)
>> + return off;
>> +
>> + chan_data = (const u8 *)data + off;
>>
>> switch (bytes) {
>> case 1:
>> - val = ((const u8 *)data)[idx];
>> + val = *chan_data;
>> break;
>> case 2:
>> - data_u16 = (const u16 *)data + idx;
>> -
>> /*
>> * Data is aligned to the sample size by IIO core.
>> * Call `get_unaligned_xe16` to hide type casting.
>> */
>> if (endianness == IIO_BE)
>> - val = get_unaligned_be16(data_u16);
>> + val = get_unaligned_be16(chan_data);
>
> I obviously missed this previously but these are aligned so we don't
> need the
> unaligned form.
Yes, the comment above says that it's used to hide type casting.
Cheers,
-Paul
>> else if (endianness == IIO_LE)
>> - val = get_unaligned_le16(data_u16);
>> + val = get_unaligned_le16(chan_data);
>> else /* IIO_CPU */
>> - val = *data_u16;
>> + val = *(const u16 *)chan_data;
>> break;
>> default:
>> return -EINVAL;
>
> >> case 2:
> >> - data_u16 = (const u16 *)data + idx;
> >> -
> >> /*
> >> * Data is aligned to the sample size by IIO core.
> >> * Call `get_unaligned_xe16` to hide type casting.
> >> */
> >> if (endianness == IIO_BE)
> >> - val = get_unaligned_be16(data_u16);
> >> + val = get_unaligned_be16(chan_data);
> >
> > I obviously missed this previously but these are aligned so we don't
> > need the
> > unaligned form.
>
> Yes, the comment above says that it's used to hide type casting.
oops :)
Thanks for pointing out my lack of observation!
Jonathan