| 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 |
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.
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;
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; >
> >> 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 --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;
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(-)