Received-SPF: pass (google.com: domain of linux-kernel+bounces-103628-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <a9ed95ec-aafe-49f6-93dd-c94c73620de2@linux.dev>
Date: Thu, 14 Mar 2024 13:30:11 -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: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio@vger.kernel.org, Conall O'Griofa <conall.ogriofa@amd.com>,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
 Lars-Peter Clausen <lars@metafoo.de>
References: <20240311162800.11074-1-sean.anderson@linux.dev>
 <20240314154824.37150a54@jic23-huawei>
From: Sean Anderson <sean.anderson@linux.dev>
In-Reply-To: <20240314154824.37150a54@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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