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

[Paul Cercueil]

Hi Jonathan,

Le ven., août 19 2022 at 18:53:39 +0100, Jonathan Cameron 
<jic23@xxxxxxxxxx> a écrit :
> On Wed, 17 Aug 2022 12:56:43 +0200
> Artur Rojek <contact@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> >  Tested-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >  Signed-off-by: Artur Rojek <contact@xxxxxxxxxxxxxx>
> >  ---
> >   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;