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 <[email protected]>
---
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];
+
+ 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 */
--
2.35.1.1320.gc452695387.dirty
On Mon, 11 Mar 2024 12:28:00 -0400
Sean Anderson <[email protected]> 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 <[email protected]>
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?
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!
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];
> +
> + 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 */
On 3/14/24 11:48, Jonathan Cameron wrote:
> On Mon, 11 Mar 2024 12:28:00 -0400
> Sean Anderson <[email protected]> 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 <[email protected]>
>
> 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];
>> +
>> + 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 */
>
[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/608426961f16ab149b1b699f1c35f7ad244c0720
I think a similar fix to the above patch is may be beneficial?
Cheers,
Conall.
> -----Original Message-----
> From: Sean Anderson <[email protected]>
> Sent: Thursday, March 14, 2024 5:30 PM
> To: Jonathan Cameron <[email protected]>
> Cc: [email protected]; O'Griofa, Conall <[email protected]>;
> [email protected]; [email protected]; Lars-Peter
> Clausen <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> >
> > 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];
> >> +
> >> + 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 */
> >
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/608426961f16ab149b1b699f1c35f7ad244c0720
>
> 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 <[email protected]>
>> Sent: Thursday, March 14, 2024 5:30 PM
>> To: Jonathan Cameron <[email protected]>
>> Cc: [email protected]; O'Griofa, Conall <[email protected]>;
>> [email protected]; [email protected]; Lars-Peter
>> Clausen <[email protected]>
>> 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 <[email protected]> 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 <[email protected]>
>> >
>> > 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];
>> >> +
>> >> + 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 */
>> >
On Fri, 15 Mar 2024 13:47:40 -0400
Sean Anderson <[email protected]> wrote:
> 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/608426961f16ab149b1b699f1c35f7ad244c0720
> >
> > I think a similar fix to the above patch is may be beneficial?
>
> These patches look functionally identical to me.
Because there are no channels with scan index between
22 * 2 + 16 (that patch) and 22 * 3 (your patch) that is
the effect is indeed the same. But given the issues is the
64 limit on maximum scan index, 22 * 3 = 66 is an ugly value
to compare with.
I'm still very against the use of scan_index for anything other
than scan indices (which is why partly how this bug wasn't noticed
in the first palce). So the check should be scan_index != -1
and uses of those values elsewhere in the driver should be fixed
(which looks simple to do from a quick glance at the code).
Jonathan
>
> --Sean
>
> >> -----Original Message-----
> >> From: Sean Anderson <[email protected]>
> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> To: Jonathan Cameron <[email protected]>
> >> Cc: [email protected]; O'Griofa, Conall <[email protected]>;
> >> [email protected]; [email protected]; Lars-Peter
> >> Clausen <[email protected]>
> >> 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 <[email protected]> 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 <[email protected]>
> >> >
> >> > 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];
> >> >> +
> >> >> + 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 */
> >> >
>
On 3/16/24 09:36, Jonathan Cameron wrote:
> On Fri, 15 Mar 2024 13:47:40 -0400
> Sean Anderson <[email protected]> wrote:
>
>> 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/608426961f16ab149b1b699f1c35f7ad244c0720
>> >
>> > I think a similar fix to the above patch is may be beneficial?
>>
>> These patches look functionally identical to me.
>
> Because there are no channels with scan index between
> 22 * 2 + 16 (that patch) and 22 * 3 (your patch) that is
> the effect is indeed the same. But given the issues is the
> 64 limit on maximum scan index, 22 * 3 = 66 is an ugly value
> to compare with.
>
> I'm still very against the use of scan_index for anything other
> than scan indices (which is why partly how this bug wasn't noticed
> in the first palce). So the check should be scan_index != -1
> and uses of those values elsewhere in the driver should be fixed
> (which looks simple to do from a quick glance at the code).
OK, so how do the sysfs files get named then?
--Sean
>>
>> --Sean
>>
>> >> -----Original Message-----
>> >> From: Sean Anderson <[email protected]>
>> >> Sent: Thursday, March 14, 2024 5:30 PM
>> >> To: Jonathan Cameron <[email protected]>
>> >> Cc: [email protected]; O'Griofa, Conall <[email protected]>;
>> >> [email protected]; [email protected]; Lars-Peter
>> >> Clausen <[email protected]>
>> >> 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 <[email protected]> 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 <[email protected]>
>> >> >
>> >> > 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];
>> >> >> +
>> >> >> + 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 */
>> >> >
>>
>
On Mon, 18 Mar 2024 11:18:43 -0400
Sean Anderson <[email protected]> wrote:
> On 3/16/24 09:36, Jonathan Cameron wrote:
> > On Fri, 15 Mar 2024 13:47:40 -0400
> > Sean Anderson <[email protected]> wrote:
> >
> >> 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/608426961f16ab149b1b699f1c35f7ad244c0720
> >> >
> >> > I think a similar fix to the above patch is may be beneficial?
> >>
> >> These patches look functionally identical to me.
> >
> > Because there are no channels with scan index between
> > 22 * 2 + 16 (that patch) and 22 * 3 (your patch) that is
> > the effect is indeed the same. But given the issues is the
> > 64 limit on maximum scan index, 22 * 3 = 66 is an ugly value
> > to compare with.
> >
> > I'm still very against the use of scan_index for anything other
> > than scan indices (which is why partly how this bug wasn't noticed
> > in the first palce). So the check should be scan_index != -1
> > and uses of those values elsewhere in the driver should be fixed
> > (which looks simple to do from a quick glance at the code).
>
> OK, so how do the sysfs files get named then?
Using channel and channel2 as appropriate (+ index and modified
which change the meaning of channel2) - scan_index never had
anything to do with sysfs file names - just the value in
bufferX/in_xyz_scan_index
>
> --Sean
>
> >>
> >> --Sean
> >>
> >> >> -----Original Message-----
> >> >> From: Sean Anderson <[email protected]>
> >> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> >> To: Jonathan Cameron <[email protected]>
> >> >> Cc: [email protected]; O'Griofa, Conall <conall.ogriofa@amdcom>;
> >> >> [email protected]; [email protected]; Lars-Peter
> >> >> Clausen <[email protected]>
> >> >> 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 <[email protected]> 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 <[email protected]>
> >> >> >
> >> >> > 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];
> >> >> >> +
> >> >> >> + 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 */
> >> >> >
> >>
> >
>
>
On 3/18/24 11:24, Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 11:18:43 -0400
> Sean Anderson <[email protected]> wrote:
>
>> On 3/16/24 09:36, Jonathan Cameron wrote:
>> > On Fri, 15 Mar 2024 13:47:40 -0400
>> > Sean Anderson <[email protected]> wrote:
>> >
>> >> 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/608426961f16ab149b1b699f1c35f7ad244c0720
>> >> >
>> >> > I think a similar fix to the above patch is may be beneficial?
>> >>
>> >> These patches look functionally identical to me.
>> >
>> > Because there are no channels with scan index between
>> > 22 * 2 + 16 (that patch) and 22 * 3 (your patch) that is
>> > the effect is indeed the same. But given the issues is the
>> > 64 limit on maximum scan index, 22 * 3 = 66 is an ugly value
>> > to compare with.
>> >
>> > I'm still very against the use of scan_index for anything other
>> > than scan indices (which is why partly how this bug wasn't noticed
>> > in the first palce). So the check should be scan_index != -1
>> > and uses of those values elsewhere in the driver should be fixed
>> > (which looks simple to do from a quick glance at the code).
>>
>> OK, so how do the sysfs files get named then?
>
> Using channel and channel2 as appropriate (+ index and modified
> which change the meaning of channel2) - scan_index never had
> anything to do with sysfs file names - just the value in
> bufferX/in_xyz_scan_index
I tried to prototype setting scan_index to -1, but when registering channels I saw
[ 1.637049] iio iio:device0: tried to double register : in_voltage_raw
[ 1.637245] xilinx-ams ffa50000.ams: Failed to register sysfs interfaces
[ 1.637433] xilinx-ams: probe of ffa50000.ams failed with error -16
And AIUI .channel is filled in by ams_parse_firmware.
--Sean
>>
>> --Sean
>>
>> >>
>> >> --Sean
>> >>
>> >> >> -----Original Message-----
>> >> >> From: Sean Anderson <[email protected]>
>> >> >> Sent: Thursday, March 14, 2024 5:30 PM
>> >> >> To: Jonathan Cameron <[email protected]>
>> >> >> Cc: [email protected]; O'Griofa, Conall <[email protected]>;
>> >> >> [email protected]; [email protected]; Lars-Peter
>> >> >> Clausen <[email protected]>
>> >> >> 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 <[email protected]> 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 <[email protected]>
>> >> >> >
>> >> >> > 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];
>> >> >> >> +
>> >> >> >> + 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 */
>> >> >> >
>> >>
>> >
>>
>>
>
[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 <[email protected]>
> Sent: Friday, March 15, 2024 5:48 PM
> To: O'Griofa, Conall <[email protected]>; Jonathan Cameron
> <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Lars-Peter Clausen <[email protected]>
> 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 <[email protected]>
> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> To: Jonathan Cameron <[email protected]>
> >> Cc: [email protected]; O'Griofa, Conall
> >> <[email protected]>; [email protected];
> >> [email protected]; Lars-Peter Clausen <[email protected]>
> >> 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 <[email protected]> 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 <[email protected]>
> >> >
> >> > 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
> >> >> */
> >> >
On Mon, 18 Mar 2024 11:28:49 -0400
Sean Anderson <[email protected]> wrote:
> On 3/18/24 11:24, Jonathan Cameron wrote:
> > On Mon, 18 Mar 2024 11:18:43 -0400
> > Sean Anderson <[email protected]> wrote:
> >
> >> On 3/16/24 09:36, Jonathan Cameron wrote:
> >> > On Fri, 15 Mar 2024 13:47:40 -0400
> >> > Sean Anderson <[email protected]> wrote:
> >> >
> >> >> 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/608426961f16ab149b1b699f1c35f7ad244c0720
> >> >> >
> >> >> > I think a similar fix to the above patch is may be beneficial?
> >> >>
> >> >> These patches look functionally identical to me.
> >> >
> >> > Because there are no channels with scan index between
> >> > 22 * 2 + 16 (that patch) and 22 * 3 (your patch) that is
> >> > the effect is indeed the same. But given the issues is the
> >> > 64 limit on maximum scan index, 22 * 3 = 66 is an ugly value
> >> > to compare with.
> >> >
> >> > I'm still very against the use of scan_index for anything other
> >> > than scan indices (which is why partly how this bug wasn't noticed
> >> > in the first palce). So the check should be scan_index != -1
> >> > and uses of those values elsewhere in the driver should be fixed
> >> > (which looks simple to do from a quick glance at the code).
> >>
> >> OK, so how do the sysfs files get named then?
> >
> > Using channel and channel2 as appropriate (+ index and modified
> > which change the meaning of channel2) - scan_index never had
> > anything to do with sysfs file names - just the value in
> > bufferX/in_xyz_scan_index
>
> I tried to prototype setting scan_index to -1, but when registering channels I saw
>
> [ 1.637049] iio iio:device0: tried to double register : in_voltage_raw
> [ 1.637245] xilinx-ams ffa50000.ams: Failed to register sysfs interfaces
> [ 1.637433] xilinx-ams: probe of ffa50000.ams failed with error -16
>
> And AIUI .channel is filled in by ams_parse_firmware.
Is indexed set for the channel? Check it at the point of calling
devm_iio_device_register() as the code that builds the channels in this
driver is complex, so maybe it's getting overwritten?
There might be a core bug somewhere, but there are other drivers using
-1 scan index without hitting this problem so my first instinct is
something is getting wrongly set in the driver.
Jonathan
>
> --Sean
>
> >>
> >> --Sean
> >>
> >> >>
> >> >> --Sean
> >> >>
> >> >> >> -----Original Message-----
> >> >> >> From: Sean Anderson <[email protected]>
> >> >> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> >> >> To: Jonathan Cameron <[email protected]>
> >> >> >> Cc: [email protected]; O'Griofa, Conall <[email protected]>;
> >> >> >> [email protected]; [email protected]; Lars-Peter
> >> >> >> Clausen <[email protected]>
> >> >> >> 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 <[email protected]> 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 <[email protected]>
> >> >> >> >
> >> >> >> > 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];
> >> >> >> >> +
> >> >> >> >> + 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 */
> >> >> >> >
> >> >>
> >> >
> >>
> >>
> >
>