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

PATCH Applied Sean Anderson 4 participants 13 messages Mar 11, 2024 – Jun 8, 2024

Subsystems (3)

IIO Arm/Zynq architecture Xilinx ams driver

lore

Share & tools

lore

Sean Anderson

author

2024-03-11 16:28 UTC #0 lore raw

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

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

drivers/iio/adc/xilinx-ams.c +6 -2

index a55396c1f8b2..4de7ce598e4d 100644
@@ -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 */

Signature · 1 line

2.35.1.1320.gc452695387.dirty

Jonathan Cameron

maintainer

2024-03-14 15:48 UTC #1 lore raw

Quoted 8 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…

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

Quoted 24 lines

---

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

drivers/iio/adc/xilinx-ams.c +6 -2

index a55396c1f8b2..4de7ce598e4d 100644
@@ -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

author

2024-03-14 17:30 UTC #2 lore raw

Quoted 14 lines · Jonathan Cameron

Quoted 8 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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,

Quoted 7 lines

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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).

Quoted 3 lines

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

Quoted 28 lines

Jonathan

Quoted 24 lines

---

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

drivers/iio/adc/xilinx-ams.c +6 -2

index a55396c1f8b2..4de7ce598e4d 100644
@@ -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

maintainer

2024-03-15 13:18 UTC #3 lore raw

[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.

Quoted 83 lines

-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

author

2024-03-15 17:47 UTC #4 lore raw

Hi Conall,

Quoted 8 lines · Conall

[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

Quoted 83 lines

Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

maintainer

2024-03-16 13:36 UTC #5 lore raw

Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…

Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]

Quoted 8 lines

Quoted 6 lines
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

Quoted 87 lines

--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

author

2024-03-18 15:18 UTC #6 lore raw

Quoted 23 lines · Jonathan Cameron

Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…
Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]
Quoted 8 lines
Quoted 6 lines
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.

Quoted 5 lines

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

Quoted 88 lines

Quoted 87 lines
--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

2024-03-18 15:24 UTC #7 lore raw

Quoted 24 lines · On Mon, 18 Mar 2024 11:18:43 -0400 Sean Anderson <[email protected]>…

Quoted 23 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…
Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]
Quoted 8 lines
Quoted 6 lines
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.

Quoted 7 lines

Quoted 5 lines
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

Quoted 93 lines

--Sean
Quoted 88 lines
Quoted 87 lines
--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

author

2024-03-18 15:28 UTC #8 lore raw

Quoted 33 lines · Jonathan Cameron

Quoted 30 lines · On Mon, 18 Mar 2024 11:18:43 -0400 Sean Anderson <[email protected]>…
Quoted 28 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…
Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]
Quoted 8 lines
Quoted 6 lines
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).

Quoted 6 lines

Quoted 1 line
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

Quoted 94 lines

Quoted 93 lines
--Sean
Quoted 88 lines
Quoted 87 lines
--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

2024-03-19 13:43 UTC #9 lore raw

Quoted 41 lines · On Mon, 18 Mar 2024 11:28:49 -0400 Sean Anderson <[email protected]>…

Quoted 33 lines · Jonathan Cameron
Quoted 30 lines · On Mon, 18 Mar 2024 11:18:43 -0400 Sean Anderson <[email protected]>…
Quoted 28 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…
Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]
Quoted 8 lines
Quoted 6 lines
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).
Quoted 6 lines
Quoted 1 line
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

Quoted 7 lines

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

Quoted 98 lines

--Sean
Quoted 94 lines
Quoted 93 lines
--Sean
Quoted 88 lines
Quoted 87 lines
--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

author

2024-06-07 17:42 UTC #10 lore raw

Quoted 52 lines · Jonathan Cameron

Quoted 41 lines · On Mon, 18 Mar 2024 11:28:49 -0400 Sean Anderson <[email protected]>…
Quoted 33 lines · Jonathan Cameron
Quoted 30 lines · On Mon, 18 Mar 2024 11:18:43 -0400 Sean Anderson <[email protected]>…
Quoted 28 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…
Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]
Quoted 8 lines
Quoted 6 lines
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).
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
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.

Quoted 7 lines

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

	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

Quoted 99 lines

Quoted 98 lines
--Sean
Quoted 94 lines
Quoted 93 lines
--Sean
Quoted 88 lines
Quoted 87 lines
--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

maintainer Applied

2024-06-08 14:03 UTC #11 lore raw

Quoted 94 lines · On Fri, 7 Jun 2024 13:42:19 -0400 Sean Anderson <[email protected]> wrote:

Quoted 52 lines · Jonathan Cameron
Quoted 41 lines · On Mon, 18 Mar 2024 11:28:49 -0400 Sean Anderson <[email protected]>…
Quoted 33 lines · Jonathan Cameron
Quoted 30 lines · On Mon, 18 Mar 2024 11:18:43 -0400 Sean Anderson <[email protected]>…
Quoted 28 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 15 Mar 2024 13:47:40 -0400 Sean Anderson <[email protected]>…
Hi Conall,
Quoted 2 lines · Conall
[AMD Official Use Only - General]
Quoted 8 lines
Quoted 6 lines
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).
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
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.
Quoted 7 lines
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]);

Quoted 7 lines

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

Quoted 102 lines

--Sean
Quoted 99 lines
Quoted 98 lines
--Sean
Quoted 94 lines
Quoted 93 lines
--Sean
Quoted 88 lines
Quoted 87 lines
--Sean
Quoted 83 lines
Quoted 83 lines
-----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.
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
Fixes d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by Sean Anderson <[email protected]>
Hi Sean,
Quoted 7 lines
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 naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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
Quoted 31 lines
Jonathan
Quoted 27 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +7 -2
@@ -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

maintainer

2024-03-19 12:00 UTC #12 lore raw

In reply to Sean Anderson

[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.

Quoted 104 lines

-----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,
Quoted 5 lines · Conall
[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.
Quoted 5 lines
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
Quoted 39 lines
Quoted 39 lines
-----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.
Quoted 9 lines · Jonathan Cameron
Quoted 6 lines · On Mon, 11 Mar 2024 12:28:00 -0400 Sean Anderson <[email protected]>…
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.
Quoted 13 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 7 lines
From what I can tell, none of the channels support buffered reads. And we can't naïvely convert the scan_index to -1, since that causes sysfs naming conflicts (not to mention the compatibility break).
Quoted 3 lines
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!
Quoted 24 lines
Quoted 24 lines
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
Quoted 17 lines
Jonathan
Quoted 14 lines
---
 drivers/iio/adc/xilinx-ams.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d 100644
drivers/iio/adc/xilinx-ams.c +0 -0
@@ -414,8 +414,12 @@ static void
ams_enable_channel_sequence(struct
iio_dev *indio_dev)

Quoted 7 lines

drivers/iio/adc/xilinx-ams.c static void ams_enable_channel_sequence(struct iio_dev *indio_dev)

Quoted 7 lines

     /* Run calibration of PS & PL as part of the sequence */
     scan_mask = BIT(0) | BIT(AMS_PS_SEQ_MAX);

drivers/iio/adc/xilinx-ams.c +3 -2

-    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"

Quoted 9 lines

Quoted 8 lines

drivers/iio/adc/xilinx-ams.c +4 -0

+
+            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