RE: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in scan_mask

[O'Griofa, Conall]

[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
> >> >> */
> >> >