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: <20240727142652.45942b5c@jic23-huawei>
Date: Sat, 27 Jul 2024 14:26:52 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Mark Brown <broonie@...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>, 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, 22 Jul 2024 16:57:10 -0500
David Lechner <dlechner@...libre.com> 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.

If it's going to fail sometimes, probably needs a name that indicates
that. I'm not sure spi_bus_try_lock() is appropriate though.

> 
> 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.

...

> +int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_enable)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&ctlr->bus_lock_mutex);
> +
> +	if (ctlr->offload_hw_trigger_mode_enabled) {
> +		mutex_unlock(&ctlr->bus_lock_mutex);
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
> +	ctlr->offload_hw_trigger_mode_enabled = true;
Why do you need to take the spinlock when setting this to true, but not when
setting it to fast (in the error path below)?

> +	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
> +
> +	/* TODO: how to wait for empty message queue? */
> +
> +	mutex_lock(&ctlr->io_mutex);
> +	ret = ctlr->offload_ops->hw_trigger_mode_enable(spi, id);
> +	mutex_unlock(&ctlr->io_mutex);
> +
> +	if (ret) {
> +		ctlr->offload_hw_trigger_mode_enabled = false;
> +		mutex_unlock(&ctlr->bus_lock_mutex);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&ctlr->bus_lock_mutex);
> +
> +	return 0;
> +}
>