[AMD Official Use Only - General] Hi, Apologies, I never thanked you for submitting the patch, so let me start this mail by saying thanks! Yes, you are correct, they are functionally the same. There is one small difference: AMS_CTRL_SEQ_BASE equates to 66, since we exclude the control channels I don’t think any overflow will actually occur (as I don’t think there are any ps or pl channels that actually have a scan index so high) but if we look at it in isolation it looks like there could still be potential for overflow. In the referenced patch PL_SEQ_MAX equates to 60 which just means that even in isolation we can see there can never be an overflow. Please see my other comment inline. Thanks &Best Regards, Conall. > -----Original Message----- > From: Sean Anderson <sean.anderson@xxxxxxxxx> > Sent: Friday, March 15, 2024 5:48 PM > To: O'Griofa, Conall <conall.ogriofa@xxxxxxx>; Jonathan Cameron > <jic23@xxxxxxxxxx> > Cc: linux-iio@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen <lars@xxxxxxxxxx> > Subject: Re: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in > scan_mask > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Conall, > > On 3/15/24 09:18, O'Griofa, Conall wrote: > > [AMD Official Use Only - General] > > > > Hi, > > > > I think there was a fix for this issue applied to the version that was running on > 5.15 that didn't seem to make it into the upstream driver. > > Please see link for reference > > https://github.com/Xilinx/linux-xlnx/commit/608426961f16ab149b1b699f1c > > 35f7ad244c0720 > > > > I think a similar fix to the above patch is may be beneficial? > > These patches look functionally identical to me. > > --Sean > > >> -----Original Message----- > >> From: Sean Anderson <sean.anderson@xxxxxxxxx> > >> Sent: Thursday, March 14, 2024 5:30 PM > >> To: Jonathan Cameron <jic23@xxxxxxxxxx> > >> Cc: linux-iio@xxxxxxxxxxxxxxx; O'Griofa, Conall > >> <conall.ogriofa@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > >> linux-kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen <lars@xxxxxxxxxx> > >> Subject: Re: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels > >> in scan_mask > >> > >> Caution: This message originated from an External Source. Use proper > >> caution when opening attachments, clicking links, or responding. > >> > >> > >> On 3/14/24 11:48, Jonathan Cameron wrote: > >> > On Mon, 11 Mar 2024 12:28:00 -0400 > >> > Sean Anderson <sean.anderson@xxxxxxxxx> wrote: > >> > > >> >> ams_enable_channel_sequence constructs a "scan_mask" for all the > >> >> PS and PL channels. This works out fine, since scan_index for > >> >> these channels is less than 64. However, it also includes the > >> >> ams_ctrl_channels, where scan_index is greater than 64, triggering > >> >> undefined behavior. Since we don't need these channels anyway, > >> >> just > >> exclude them. > >> >> > >> >> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver") > >> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx> > >> > > >> > Hi Sean, > >> > > >> > I'd ideally like to understand why we have channels with such large > >> > scan indexes. Those values should only be used for buffered capture. > >> > It feels like they are being abused here. Can we set them to -1 > >> > instead and check based on that? > >> > For a channel, a scan index of -1 means it can't be captured via > >> > the buffered interfaces but only accessed via sysfs reads. > >> > I think that's what we have here? > >> > >> From what I can tell, none of the channels support buffered reads. > >> And we can't naïvely convert the scan_index to -1, since that causes > >> sysfs naming conflicts (not to mention the compatibility break). > >> > >> > > >> > I just feel like if we leave these as things stand, we will get > >> > bitten by similar bugs in the future. At least with -1 it should be obvious > why! > >> > >> There are just as likely to be bugs confusing the PL/PS subdevices... > >> > >> FWIW I had no trouble identifying the channels involved with this bug. > >> > >> --Sean > >> > >> > Jonathan > >> > > >> > > >> >> --- > >> >> > >> >> drivers/iio/adc/xilinx-ams.c | 8 ++++++-- > >> >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/drivers/iio/adc/xilinx-ams.c > >> >> b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d > >> >> 100644 > >> >> --- a/drivers/iio/adc/xilinx-ams.c > >> >> +++ b/drivers/iio/adc/xilinx-ams.c > >> >> @@ -414,8 +414,12 @@ static void > >> >> ams_enable_channel_sequence(struct > >> >> iio_dev *indio_dev) > >> >> > >> >> /* Run calibration of PS & PL as part of the sequence */ > >> >> scan_mask = BIT(0) | BIT(AMS_PS_SEQ_MAX); > >> >> - for (i = 0; i < indio_dev->num_channels; i++) > >> >> - scan_mask |= BIT_ULL(indio_dev->channels[i].scan_index); > >> >> + for (i = 0; i < indio_dev->num_channels; i++) { > >> >> + const struct iio_chan_spec *chan = > >> >> + &indio_dev->channels[i]; [COG] I don't think there is a need for the above we can just keep using "indio_dev->channels[i].scan_index" > >> >> + > >> >> + if (chan->scan_index < AMS_CTRL_SEQ_BASE) > >> >> + scan_mask |= BIT_ULL(chan->scan_index); > >> >> + } > >> >> > >> >> if (ams->ps_base) { > >> >> /* put sysmon in a soft reset to change the sequence > >> >> */ > >> >
