On Fri, 7 Jun 2024 13:42:19 -0400 Sean Anderson <sean.anderson@xxxxxxxxx> wrote: > On 3/19/24 09:42, Jonathan Cameron wrote: > > On Mon, 18 Mar 2024 11:28:49 -0400 > > Sean Anderson <sean.anderson@xxxxxxxxx> wrote: > > > >> On 3/18/24 11:24, Jonathan Cameron wrote: > >> > On Mon, 18 Mar 2024 11:18:43 -0400 > >> > Sean Anderson <sean.anderson@xxxxxxxxx> wrote: > >> > > >> >> On 3/16/24 09:36, Jonathan Cameron wrote: > >> >> > On Fri, 15 Mar 2024 13:47:40 -0400 > >> >> > Sean Anderson <sean.anderson@xxxxxxxxx> 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 <sean.anderson@xxxxxxxxx> > >> >> >> >> Sent: Thursday, March 14, 2024 5:30 PM > >> >> >> >> To: Jonathan Cameron <jic23@xxxxxxxxxx> > >> >> >> >> Cc: linux-iio@xxxxxxxxxxxxxxx; O'Griofa, Conall <conall.ogriofa@xxxxxxx>; > >> >> >> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Lars-Peter > >> >> >> >> Clausen <lars@xxxxxxxxxx> > >> >> >> >> Subject: Re: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in > >> >> >> >> scan_mask > >> >> >> >> > >> >> >> >> Caution: This message originated from an External Source. Use proper caution > >> >> >> >> when opening attachments, clicking links, or responding. > >> >> >> >> > >> >> >> >> > >> >> >> >> On 3/14/24 11:48, Jonathan Cameron wrote: > >> >> >> >> > On Mon, 11 Mar 2024 12:28:00 -0400 > >> >> >> >> > Sean Anderson <sean.anderson@xxxxxxxxx> wrote: > >> >> >> >> > > >> >> >> >> >> ams_enable_channel_sequence constructs a "scan_mask" for all the PS > >> >> >> >> >> and PL channels. This works out fine, since scan_index for these > >> >> >> >> >> channels is less than 64. However, it also includes the > >> >> >> >> >> ams_ctrl_channels, where scan_index is greater than 64, triggering > >> >> >> >> >> undefined behavior. Since we don't need these channels anyway, just > >> >> >> >> exclude them. > >> >> >> >> >> > >> >> >> >> >> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver") > >> >> >> >> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx> > >> >> >> >> > > >> >> >> >> > Hi Sean, > >> >> >> >> > > >> >> >> >> > I'd ideally like to understand why we have channels with such large > >> >> >> >> > scan indexes. Those values should only be used for buffered capture. > >> >> >> >> > It feels like they are being abused here. Can we set them to -1 > >> >> >> >> > instead and check based on that? > >> >> >> >> > For a channel, a scan index of -1 means it can't be captured via the > >> >> >> >> > buffered interfaces but only accessed via sysfs reads. > >> >> >> >> > I think that's what we have here? > >> >> >> >> > >> >> >> >> From what I can tell, none of the channels support buffered reads. And we can't > >> >> >> >> naïvely convert the scan_index to -1, since that causes sysfs naming conflicts > >> >> >> >> (not to mention the compatibility break). > >> >> >> >> > >> >> >> >> > > >> >> >> >> > I just feel like if we leave these as things stand, we will get bitten > >> >> >> >> > by similar bugs in the future. At least with -1 it should be obvious why! > >> >> >> >> > >> >> >> >> There are just as likely to be bugs confusing the PL/PS subdevices... > >> >> >> >> > >> >> >> >> FWIW I had no trouble identifying the channels involved with this bug. > >> >> >> >> > >> >> >> >> --Sean > >> >> >> >> > >> >> >> >> > Jonathan > >> >> >> >> > > >> >> >> >> > > >> >> >> >> >> --- > >> >> >> >> >> > >> >> >> >> >> drivers/iio/adc/xilinx-ams.c | 8 ++++++-- > >> >> >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> >> >> >> >> > >> >> >> >> >> diff --git a/drivers/iio/adc/xilinx-ams.c > >> >> >> >> >> b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d > >> >> >> >> >> 100644 > >> >> >> >> >> --- a/drivers/iio/adc/xilinx-ams.c > >> >> >> >> >> +++ b/drivers/iio/adc/xilinx-ams.c > >> >> >> >> >> @@ -414,8 +414,12 @@ static void ams_enable_channel_sequence(struct > >> >> >> >> >> iio_dev *indio_dev) > >> >> >> >> >> > >> >> >> >> >> /* Run calibration of PS & PL as part of the sequence */ > >> >> >> >> >> scan_mask = BIT(0) | BIT(AMS_PS_SEQ_MAX); > >> >> >> >> >> - for (i = 0; i < indio_dev->num_channels; i++) > >> >> >> >> >> - scan_mask |= BIT_ULL(indio_dev->channels[i].scan_index); > >> >> >> >> >> + for (i = 0; i < indio_dev->num_channels; i++) { > >> >> >> >> >> + const struct iio_chan_spec *chan = > >> >> >> >> >> + &indio_dev->channels[i]; > >> >> >> >> >> + > >> >> >> >> >> + 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 */ > >> >> >> >> > > >> >> >> > >> >> > > >> >> > >> >> > >> > > >> > >
