linux-kernel - Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34ff08ef8b550ff2979dc50204fad500b9bb41e3.camel@gmail.com>
Date: Tue, 23 Jul 2024 09:53:50 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Mark Brown <broonie@...nel.org>, 
 Jonathan Cameron
	 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nuno
 Sá
	 <nuno.sa@...log.com>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Lars-Peter Clausen
	 <lars@...afoo.de>, David Jander <david@...tonic.nl>, Martin Sperl
	 <kernel@...tin.sperl.org>, linux-spi@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org
Subject: Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered
 offload

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> This extends the SPI framework to support hardware triggered offloading.
> This allows an arbitrary hardware trigger to be used to start a SPI
> transfer that was previously set up with spi_offload_prepare().
> 
> Since the hardware trigger can happen at any time, this means the SPI
> bus must be reserved for exclusive use as long as the hardware trigger
> is enabled. Since a hardware trigger could be enabled indefinitely,
> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
> otherwise this could cause deadlocks. So we introduce a new flag so that
> any attempt to lock or use the bus will fail with -EBUSY as long as the
> hardware trigger is enabled.
> 
> Peripheral drivers may need to control the trigger source as well. For
> this, we introduce a new spi_offload_hw_trigger_get_clk() function that
> can be used to get a clock trigger source. This is intended for used
> by ADC drivers that will use the clock to control the sample rate.
> Additional functions to get other types of trigger sources could be
> added in the future.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> 
> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
> check the return value. All callers will need to be updated first before
> this can be merged.
> 
> v3 changes:
> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
> * added spi_offload_hw_trigger_get_clk() function
> * fixed missing EXPORT_SYMBOL_GPL
> 
> v2 changes:
> 
> This is split out from "spi: add core support for controllers with
> offload capabilities".
> 
> Mark suggested that the standard SPI APIs should be aware that the
> hardware trigger is enabled. So I've added some locking for this. Nuno
> suggested that this might be overly strict though, and that we should
> let each individual controller driver decide what to do. For our use
> case though, I think we generally are going to have a single peripheral
> on the SPI bus, so this seems like a reasonable starting place anyway.
> ---

How explicitly do we want to be about returning errors? It seems that if the
trigger is enabled we can't anything else on the controller/offload_engine so we
could very well just hold the controller lock when enabling the trigger and 
release it when disabling it. Pretty much the same behavior as spi_bus_lock()...

...

> 
> +
> +/**
> + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
> + * @spi: SPI device
> + * @id: Function ID if SPI device uses more than one offload or NULL.
> + *
> + * The caller is responsible for calling clk_put() on the returned clock.
> + *
> + * Return: The clock for the offload trigger, or negative error code
> + */
> +static inline
> +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char
> *id)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +
> +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
> +}
> 

It would be nice if we could have some kind of spi abstraction...

- Nuno Sá