Received-SPF: pass (google.com: domain of linux-kernel+bounces-104744-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <3b481539-0c9c-4110-ad03-bd252e80efb0@linux.dev>
Date: Fri, 15 Mar 2024 13:47:40 -0400
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in
 scan_mask
Content-Language: en-US
To: "O'Griofa, Conall" <conall.ogriofa@amd.com>,
 Jonathan Cameron <jic23@kernel.org>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
 "linux-arm-kernel@lists.infradead.org"
 <linux-arm-kernel@lists.infradead.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 Lars-Peter Clausen <lars@metafoo.de>
References: <20240311162800.11074-1-sean.anderson@linux.dev>
 <20240314154824.37150a54@jic23-huawei>
 <a9ed95ec-aafe-49f6-93dd-c94c73620de2@linux.dev>
 <DM6PR12MB4217EAA1049F815F234EE6D18B282@DM6PR12MB4217.namprd12.prod.outlook.com>
From: Sean Anderson <sean.anderson@linux.dev>
In-Reply-To: <DM6PR12MB4217EAA1049F815F234EE6D18B282@DM6PR12MB4217.namprd12.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <sean.anderson@linux.dev>
>> Sent: Thursday, March 14, 2024 5:30 PM
>> To: Jonathan Cameron <jic23@kernel.org>
>> Cc: linux-iio@vger.kernel.org; O'Griofa, Conall <conall.ogriofa@amd.com>;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Lars-Peter
>> Clausen <lars@metafoo.de>
>> 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@linux.dev> 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@linux.dev>
>> >
>> > 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
>> naï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 */
>> >