diff mbox series

[4/4] input: joystick: Fix buffer data parsing

Message ID 20220817105643.95710-5-contact@artur-rojek.eu (mailing list archive)
State Changes Requested
Headers show
Series iio/adc-joystick: buffer data parsing fixes | expand

Commit Message

Artur Rojek Aug. 17, 2022, 10:56 a.m. UTC
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 <macromorgan@hotmail.com>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/input/joystick/adc-joystick.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko Aug. 19, 2022, 8:21 a.m. UTC | #1
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> 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 <macromorgan@hotmail.com>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>

Try to keep tags in chronological order, I do not believe the change
may be tested before it has been created.
Jonathan Cameron Aug. 19, 2022, 5:53 p.m. UTC | #2
On Wed, 17 Aug 2022 12:56:43 +0200
Artur Rojek <contact@artur-rojek.eu> 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 <macromorgan@hotmail.com>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  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;
Paul Cercueil Aug. 22, 2022, 9:03 a.m. UTC | #3
Hi Jonathan,

Le ven., août 19 2022 at 18:53:39 +0100, Jonathan Cameron 
<jic23@kernel.org> a écrit :
> On Wed, 17 Aug 2022 12:56:43 +0200
> Artur Rojek <contact@artur-rojek.eu> 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 <macromorgan@hotmail.com>
>>  Tested-by: Paul Cercueil <paul@crapouillou.net>
>>  Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>>   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;
>
Jonathan Cameron Aug. 22, 2022, 7:01 p.m. UTC | #4
> >>   		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
diff mbox series

Patch

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;