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

[Sean Anderson]

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.

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