iio: xilinx-ams: Don't include ams_ctrl_channels in scan_mask

Commit Message

Sean Anderson March 11, 2024, 4:28 p.m. UTC

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 <[e-mail]>
---

 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Jonathan Cameron March 14, 2024, 3:48 p.m. UTC | #1

On Mon, 11 Mar 2024 12:28:00 -0400
Sean Anderson <[e-mail]> 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 <[e-mail]>

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

Sean Anderson March 14, 2024, 5:30 p.m. UTC | #2

On 3/14/24 11:48, Jonathan Cameron wrote:
> On Mon, 11 Mar 2024 12:28:00 -0400
> Sean Anderson <[e-mail]> 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 <[e-mail]>
> 
> 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 */
>

O'Griofa, Conall March 15, 2024, 1:18 p.m. UTC | #3

[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 <[e-mail]>
> Sent: Thursday, March 14, 2024 5:30 PM
> To: Jonathan Cameron <[e-mail]>
> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
> [e-mail]; [e-mail]; Lars-Peter
> Clausen <[e-mail]>
> 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 <[e-mail]> 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 <[e-mail]>
> >
> > 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 */
> >

Sean Anderson March 15, 2024, 5:47 p.m. UTC | #4

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 <[e-mail]>
>> Sent: Thursday, March 14, 2024 5:30 PM
>> To: Jonathan Cameron <[e-mail]>
>> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
>> [e-mail]; [e-mail]; Lars-Peter
>> Clausen <[e-mail]>
>> 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 <[e-mail]> 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 <[e-mail]>
>> >
>> > 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 */
>> >

Jonathan Cameron March 16, 2024, 1:36 p.m. UTC | #5

On Fri, 15 Mar 2024 13:47:40 -0400
Sean Anderson <[e-mail]> 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 <[e-mail]>
> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> To: Jonathan Cameron <[e-mail]>
> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
> >> [e-mail]; [e-mail]; Lars-Peter
> >> Clausen <[e-mail]>
> >> 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 <[e-mail]> 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 <[e-mail]>  
> >> >
> >> > 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 */  
> >> >  
>

Sean Anderson March 18, 2024, 3:18 p.m. UTC | #6

On 3/16/24 09:36, Jonathan Cameron wrote:
> On Fri, 15 Mar 2024 13:47:40 -0400
> Sean Anderson <[e-mail]> 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 <[e-mail]>
>> >> Sent: Thursday, March 14, 2024 5:30 PM
>> >> To: Jonathan Cameron <[e-mail]>
>> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
>> >> [e-mail]; [e-mail]; Lars-Peter
>> >> Clausen <[e-mail]>
>> >> 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 <[e-mail]> 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 <[e-mail]>  
>> >> >
>> >> > 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 */  
>> >> >  
>> 
>

Jonathan Cameron March 18, 2024, 3:24 p.m. UTC | #7

On Mon, 18 Mar 2024 11:18:43 -0400
Sean Anderson <[e-mail]> wrote:

> On 3/16/24 09:36, Jonathan Cameron wrote:
> > On Fri, 15 Mar 2024 13:47:40 -0400
> > Sean Anderson <[e-mail]> 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 <[e-mail]>
> >> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> >> To: Jonathan Cameron <[e-mail]>
> >> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
> >> >> [e-mail]; [e-mail]; Lars-Peter
> >> >> Clausen <[e-mail]>
> >> >> 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 <[e-mail]> 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 <[e-mail]>    
> >> >> >
> >> >> > 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 */    
> >> >> >    
> >>   
> >   
> 
>

Sean Anderson March 18, 2024, 3:28 p.m. UTC | #8

On 3/18/24 11:24, Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 11:18:43 -0400
> Sean Anderson <[e-mail]> wrote:
> 
>> On 3/16/24 09:36, Jonathan Cameron wrote:
>> > On Fri, 15 Mar 2024 13:47:40 -0400
>> > Sean Anderson <[e-mail]> 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 <[e-mail]>
>> >> >> Sent: Thursday, March 14, 2024 5:30 PM
>> >> >> To: Jonathan Cameron <[e-mail]>
>> >> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
>> >> >> [e-mail]; [e-mail]; Lars-Peter
>> >> >> Clausen <[e-mail]>
>> >> >> 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 <[e-mail]> 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 <[e-mail]>    
>> >> >> >
>> >> >> > 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 */    
>> >> >> >    
>> >>   
>> >   
>> 
>> 
>

O'Griofa, Conall March 19, 2024, noon UTC | #9

[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 <[e-mail]>
> Sent: Friday, March 15, 2024 5:48 PM
> To: O'Griofa, Conall <[e-mail]>; Jonathan Cameron
> <[e-mail]>
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; Lars-Peter Clausen <[e-mail]>
> 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 <[e-mail]>
> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> To: Jonathan Cameron <[e-mail]>
> >> Cc: [e-mail]; O'Griofa, Conall
> >> <[e-mail]>; [e-mail];
> >> [e-mail]; Lars-Peter Clausen <[e-mail]>
> >> 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 <[e-mail]> 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 <[e-mail]>
> >> >
> >> > 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
> >> >> */
> >> >

Jonathan Cameron March 19, 2024, 1:42 p.m. UTC | #10

On Mon, 18 Mar 2024 11:28:49 -0400
Sean Anderson <[e-mail]> wrote:

> On 3/18/24 11:24, Jonathan Cameron wrote:
> > On Mon, 18 Mar 2024 11:18:43 -0400
> > Sean Anderson <[e-mail]> wrote:
> >   
> >> On 3/16/24 09:36, Jonathan Cameron wrote:  
> >> > On Fri, 15 Mar 2024 13:47:40 -0400
> >> > Sean Anderson <[e-mail]> 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 <[e-mail]>
> >> >> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> >> >> To: Jonathan Cameron <[e-mail]>
> >> >> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
> >> >> >> [e-mail]; [e-mail]; Lars-Peter
> >> >> >> Clausen <[e-mail]>
> >> >> >> 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 <[e-mail]> 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 <[e-mail]>      
> >> >> >> >
> >> >> >> > 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 */      
> >> >> >> >      
> >> >>     
> >> >     
> >> 
> >>   
> >   
>

Sean Anderson June 7, 2024, 5:42 p.m. UTC | #11

On 3/19/24 09:42, Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 11:28:49 -0400
> Sean Anderson <[e-mail]> wrote:
> 
>> On 3/18/24 11:24, Jonathan Cameron wrote:
>> > On Mon, 18 Mar 2024 11:18:43 -0400
>> > Sean Anderson <[e-mail]> wrote:
>> >   
>> >> On 3/16/24 09:36, Jonathan Cameron wrote:  
>> >> > On Fri, 15 Mar 2024 13:47:40 -0400
>> >> > Sean Anderson <[e-mail]> 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.

Upon further review, I think scan_index should remain the same, and this
patch should be applied as-is.

address is the only driver-private data in all of iio_chan_spec.
Unfortunately, it is suggestively named "address" and not "priv" or
"driver_id" or something similar. So the original author of this driver
went "Ah, I should put the channel address offsets in this register."
Except, because this driver has three address spaces, this is not enough
to uniquely identify the channel. So he then stuck an actual unique
identifier in scan_index. Now, you may object to this since the driver
doesn't actually support scans, but that is the current situation.

So there is really nothing wrong with scan_index semantically in the
context of the driver. We should not convert one address space's
channels to use -1 scan_index, since it is used as a unique identifier
elsewhere in the channel.

Future patches could convert scan_index to address, and store the
address offsets in an array. So e.g. reading a channel would go from
e.g.

	if (chan->scan_index >= AMS_PS_SEQ_MAX)
		*val = readl(ams->pl_base + chan->address);
	else
		*val = readl(ams->ps_base + chan->address);

to

	if (chan->address >= AMS_PS_SEQ_MAX)
		*val = readl(ams->pl_base + ams_chan_addr[chan->address]);
	else
		*val = readl(ams->ps_base + ams_chan_addr[chan->address]);

which while strictly less perfmant due to another level of indirection
does conform to existing semantics for scan_index. But TBH I don't see
much point in this.

But the above change would be pretty significant and has a chance of
causing bugs of its own. So I would rather this bug fix be applied as-is
and the scan_index semantics be modified at some other time.

--Sean

>> 
>> --Sean
>> 
>> >> 
>> >> --Sean
>> >>   
>> >> >> 
>> >> >> --Sean
>> >> >>     
>> >> >> >> -----Original Message-----
>> >> >> >> From: Sean Anderson <[e-mail]>
>> >> >> >> Sent: Thursday, March 14, 2024 5:30 PM
>> >> >> >> To: Jonathan Cameron <[e-mail]>
>> >> >> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
>> >> >> >> [e-mail]; [e-mail]; Lars-Peter
>> >> >> >> Clausen <[e-mail]>
>> >> >> >> 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 <[e-mail]> 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 <[e-mail]>      
>> >> >> >> >
>> >> >> >> > 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 */      
>> >> >> >> >      
>> >> >>     
>> >> >     
>> >> 
>> >>   
>> >   
>> 
>

Jonathan Cameron June 8, 2024, 2:03 p.m. UTC | #12

On Fri, 7 Jun 2024 13:42:19 -0400
Sean Anderson <[e-mail]> wrote:

> On 3/19/24 09:42, Jonathan Cameron wrote:
> > On Mon, 18 Mar 2024 11:28:49 -0400
> > Sean Anderson <[e-mail]> wrote:
> >   
> >> On 3/18/24 11:24, Jonathan Cameron wrote:  
> >> > On Mon, 18 Mar 2024 11:18:43 -0400
> >> > Sean Anderson <[e-mail]> wrote:
> >> >     
> >> >> On 3/16/24 09:36, Jonathan Cameron wrote:    
> >> >> > On Fri, 15 Mar 2024 13:47:40 -0400
> >> >> > Sean Anderson <[e-mail]> 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.  
> 
> Upon further review, I think scan_index should remain the same, and this
> patch should be applied as-is.
> 
> address is the only driver-private data in all of iio_chan_spec.
> Unfortunately, it is suggestively named "address" and not "priv" or
> "driver_id" or something similar. So the original author of this driver
> went "Ah, I should put the channel address offsets in this register."
> Except, because this driver has three address spaces, this is not enough
> to uniquely identify the channel. So he then stuck an actual unique
> identifier in scan_index. Now, you may object to this since the driver
> doesn't actually support scans, but that is the current situation.
> 
> So there is really nothing wrong with scan_index semantically in the
> context of the driver. We should not convert one address space's
> channels to use -1 scan_index, since it is used as a unique identifier
> elsewhere in the channel.
> 
> Future patches could convert scan_index to address, and store the
> address offsets in an array. So e.g. reading a channel would go from
> e.g.
> 
> 	if (chan->scan_index >= AMS_PS_SEQ_MAX)
> 		*val = readl(ams->pl_base + chan->address);
> 	else
> 		*val = readl(ams->ps_base + chan->address);
> 
> to
> 
> 	if (chan->address >= AMS_PS_SEQ_MAX)
> 		*val = readl(ams->pl_base + ams_chan_addr[chan->address]);
> 	else
> 		*val = readl(ams->ps_base + ams_chan_addr[chan->address]);
> 
> which while strictly less perfmant due to another level of indirection
> does conform to existing semantics for scan_index. But TBH I don't see
> much point in this.
> 
> But the above change would be pretty significant and has a chance of
> causing bugs of its own. So I would rather this bug fix be applied as-is
> and the scan_index semantics be modified at some other time.
Ok.  I'll pick this one up, but I'd either like the change you mention above
or maybe as a lesser nice but easier solution, a patch adding comments
on the use of scan_index in this driver.

Applied to the fixes-togreg branch of iio.git.  Thanks for figuring out
what happened here and the clear explanation.

Jonathan

> 
> --Sean
> 
> >> 
> >> --Sean
> >>   
> >> >> 
> >> >> --Sean
> >> >>     
> >> >> >> 
> >> >> >> --Sean
> >> >> >>       
> >> >> >> >> -----Original Message-----
> >> >> >> >> From: Sean Anderson <[e-mail]>
> >> >> >> >> Sent: Thursday, March 14, 2024 5:30 PM
> >> >> >> >> To: Jonathan Cameron <[e-mail]>
> >> >> >> >> Cc: [e-mail]; O'Griofa, Conall <[e-mail]>;
> >> >> >> >> [e-mail]; [e-mail]; Lars-Peter
> >> >> >> >> Clausen <[e-mail]>
> >> >> >> >> 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 <[e-mail]> 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 <[e-mail]>        
> >> >> >> >> >
> >> >> >> >> > 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 */        
> >> >> >> >> >        
> >> >> >>       
> >> >> >       
> >> >> 
> >> >>     
> >> >     
> >>   
> >

Patch

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