Received-SPF: pass (google.com: domain of linux-kernel+bounces-161489-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 28 Apr 2024 17:29:23 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>, Nuno =?UTF-8?B?U8Oh?=
 <nuno.sa@analog.com>, Jonathan Corbet <corbet@lwn.net>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-doc@vger.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: ad7944: add support for chain mode
Message-ID: <20240428172923.7dfe00ff@jic23-huawei>
In-Reply-To: <20240425-iio-ad7944-chain-mode-v1-1-9d9220ff21e1@baylibre.com>
References: <20240425-iio-ad7944-chain-mode-v1-0-9d9220ff21e1@baylibre.com>
	<20240425-iio-ad7944-chain-mode-v1-1-9d9220ff21e1@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 25 Apr 2024 09:09:59 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This adds support for the chain mode of the AD7944 ADC. This mode allows
> multiple ADCs to be daisy-chained together. Data from all of the ADCs in
> is read by reading multiple words from the first ADC in the chain.
> 
> Each chip in the chain adds an extra IIO input voltage channel to the
> IIO device.
> 
> Only the wiring configuration where the SPI controller CS line is
> connected to the CNV pin of all of the ADCs in the chain is supported
> in this patch.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Looks good except for one minor tweak needed to ensure the allocated buffer
is zeroed as we don't necessarily overwrite the the whole thing.

Given that's all I found, I've just switched that to devm_kzalloc and
applied the series.

Applied to the togreg branch of iio.git and pushed out initially as testing
for 0-day to look at it.

Thanks,

Jonathan


>  
> +/**
> + * ad7944_chain_mode_alloc - allocate and initialize channel specs and buffers
> + *                           for daisy-chained devices
> + * @dev: The device for devm_ functions
> + * @chan_template: The channel template for the devices (array of 2 channels
> + *                 voltage and timestamp)
> + * @n_chain_dev: The number of devices in the chain
> + * @chain_chan: Pointer to receive the allocated channel specs
> + * @chain_mode_buf: Pointer to receive the allocated rx buffer
> + * @chain_scan_masks: Pointer to receive the allocated scan masks
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad7944_chain_mode_alloc(struct device *dev,
> +				   const struct iio_chan_spec *chan_template,
> +				   u32 n_chain_dev,
> +				   struct iio_chan_spec **chain_chan,
> +				   void **chain_mode_buf,
> +				   unsigned long **chain_scan_masks)
> +{
> +	struct iio_chan_spec *chan;
> +	size_t chain_mode_buf_size;
> +	unsigned long *scan_masks;
> +	void *buf;
> +	int i;
> +
> +	/* 1 channel for each device in chain plus 1 for soft timestamp */
> +
> +	chan = devm_kcalloc(dev, n_chain_dev + 1, sizeof(*chan), GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n_chain_dev; i++) {
> +		chan[i] = chan_template[0];
> +
> +		if (chan_template[0].differential) {
> +			chan[i].channel = 2 * i;
> +			chan[i].channel2 = 2 * i + 1;
> +		} else {
> +			chan[i].channel = i;
> +		}
> +
> +		chan[i].scan_index = i;
> +	}
> +
> +	/* soft timestamp */
> +	chan[i] = chan_template[1];
> +	chan[i].scan_index = i;
> +
> +	*chain_chan = chan;
> +
> +	/* 1 word for each voltage channel + aligned u64 for timestamp */
> +
> +	chain_mode_buf_size = ALIGN(n_chain_dev *
> +		BITS_TO_BYTES(chan[0].scan_type.storagebits), sizeof(u64))
> +		+ sizeof(u64);
> +	buf = devm_kmalloc(dev, chain_mode_buf_size, GFP_KERNEL);

Zero it - It's not a problem to leak stale ADC data or similar
into the gap between the data and the timestamp, but it is a problem
if it's general kernel data potentially leaking.

So play it safe and devm_kzalloc()
		
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	*chain_mode_buf = buf;
> +
> +	/*
> +	 * Have to limit n_chain_dev due to current implementation of
> +	 * available_scan_masks.
> +	 */
> +	if (n_chain_dev > BITS_PER_LONG)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "chain is limited to 32 devices\n");
> +
> +	scan_masks = devm_kcalloc(dev, 2, sizeof(*scan_masks), GFP_KERNEL);
> +	if (!scan_masks)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Scan mask is needed since we always have to read all devices in the
> +	 * chain in one SPI transfer.
> +	 */
> +	scan_masks[0] = GENMASK(n_chain_dev - 1, 0);
> +
> +	*chain_scan_masks = scan_masks;
> +
> +	return 0;
> +}