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

[Sean Anderson]

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.

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