Re: [PATCH RFC v4 03/15] spi: offload: add support for hardware triggers

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Thu, 2024-10-24 at 10:02 -0500, David Lechner wrote:
> On 10/24/24 9:04 AM, Nuno Sá wrote:
> > On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote:
> > > Extend SPI offloading to support hardware triggers.
> > > 
> > > This allows an arbitrary hardware trigger to be used to start a SPI
> > > transfer that was previously set up with spi_optimize_message().
> > > 
> > > A new struct spi_offload_trigger is introduced that can be used to
> > > configure any type of trigger. It has a type discriminator and a union
> > > to allow it to be extended in the future. Two trigger types are defined
> > > to start with. One is a trigger that indicates that the SPI peripheral
> > > is ready to read or write data. The other is a periodic trigger to
> > > repeat a SPI message at a fixed rate.
> > > 
> > > There is also a spi_offload_hw_trigger_validate() function that works
> > > similar to clk_round_rate(). It basically asks the question of if we
> > > enabled the hardware trigger what would the actual parameters be. This
> > > can be used to test if the requested trigger type is actually supported
> > > by the hardware and for periodic triggers, it can be used to find the
> > > actual rate that the hardware is capable of.
> > > 
> > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > > ---
> > > 
> > > In previous versions, we locked the SPI bus when the hardware trigger
> > > was enabled, but we found this to be too restrictive. In one use case,
> > > to avoid a race condition, we need to enable the SPI offload via a
> > > hardware trigger, then write a SPI message to the peripheral to place
> > > it into a mode that will generate the trigger. If we did it the other
> > > way around, we could miss the first trigger.
> > > 
> > > Another likely use case will be enabling two offloads/triggers at one
> > > time on the same device, e.g. a read trigger and a write trigger. So
> > > the exclusive bus lock for a single trigger would be too restrictive in
> > > this case too.
> > > 
> > > So for now, I'm going with Nuno's suggestion to leave any locking up to
> > > the individual controller driver. If we do find we need something more
> > > generic in the future, we could add a new spi_bus_lock_exclusive() API
> > > that causes spi_bus_lock() to fail instead of waiting and add "locked"
> > > versions of trigger enable functions. This would allow a peripheral to
> > > claim exclusive use of the bus indefinitely while still being able to
> > > do any SPI messaging that it needs.
> > > 
> > > v4 changes:
> > > * Added new struct spi_offload_trigger that is a generic struct for any
> > >   hardware trigger rather than returning a struct clk.
> > > * Added new spi_offload_hw_trigger_validate() function.
> > > * Dropped extra locking since it was too restrictive.
> > > 
> > > 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".
> > > * Added locking for offload trigger to claim exclusive use of the SPI
> > >   bus.
> > > ---
> > >  drivers/spi/spi-offload.c       | 266 ++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/spi/spi-offload.h |  78 ++++++++++++
> > >  2 files changed, 344 insertions(+)
> > > 
> > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> > > index c344cbf50bdb..2a1f9587f27a 100644
> > > --- a/drivers/spi/spi-offload.c
> > > +++ b/drivers/spi/spi-offload.c
> > > @@ -9,12 +9,26 @@
> > >  #include <linux/cleanup.h>
> > >  #include <linux/device.h>
> > >  #include <linux/export.h>
> > > +#include <linux/list.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/of.h>
> > >  #include <linux/property.h>
> > >  #include <linux/spi/spi-offload.h>
> > >  #include <linux/spi/spi.h>
> > >  #include <linux/types.h>
> > >  
> > > +struct spi_offload_trigger {
> > > +	struct list_head list;
> > > +	struct device dev;
> > > +	/* synchronizes calling ops and driver registration */
> > > +	struct mutex lock;
> > > +	const struct spi_offload_trigger_ops *ops;
> > > +	void *priv;
> > > +};
> > > +
> > > +static LIST_HEAD(spi_offload_triggers);
> > > +static DEFINE_MUTEX(spi_offload_triggers_lock);
> > > +
> > >  /**
> > >   * devm_spi_offload_alloc() - Allocate offload instances
> > >   * @dev: Device for devm purposes
> > > @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device
> > > *dev,
> > >  	return offload;
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> > > +
> > > +static void spi_offload_trigger_release(void *data)
> > > +{
> > > +	struct spi_offload_trigger *trigger = data;
> > > +
> > > +	guard(mutex)(&trigger->lock);
> > > +	if (trigger->priv && trigger->ops->release)
> > > +		trigger->ops->release(trigger->priv);
> > > +
> > > +	put_device(&trigger->dev);
> > > +}
> > > +
> > > +struct spi_offload_trigger
> > > +*devm_spi_offload_trigger_get(struct device *dev,
> > > +			      struct spi_offload *offload,
> > > +			      enum spi_offload_trigger_type type)
> > > +{
> > > +	struct spi_offload_trigger *trigger;
> > > +	struct fwnode_reference_args args;
> > > +	bool match = false;
> > > +	int ret;
> > > +
> > > +	ret = fwnode_property_get_reference_args(dev_fwnode(offload-
> > > > provider_dev),
> > > +						 "trigger-sources",
> > > +						 "#trigger-source-cells", 0,
> > > 0,
> > > +						 &args);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) =
> > > args.fwnode;
> > > +
> > > +	guard(mutex)(&spi_offload_triggers_lock);
> > > +
> > > +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> > > +		if (trigger->dev.fwnode != args.fwnode)
> > > +			continue;
> > > +
> > > +		match = trigger->ops->match(trigger->priv, type, args.args,
> > > args.nargs);
> > > +		if (match)
> > > +			break;
> > > +	}
> > > +
> > > +	if (!match)
> > > +		return ERR_PTR(-EPROBE_DEFER);
> > > +
> > > +	guard(mutex)(&trigger->lock);
> > > +
> > > +	if (!trigger->priv)
> > > +		return ERR_PTR(-ENODEV);
> > 
> > This is a bit odd tbh. Not a real deal breaker for me but the typical pattern I
> > would
> > expect is for methods of the trigger to get a struct spi_offload_trigger opaque
> > pointer. Then we provide a get_private kind of API for the private data. I guess
> > you
> > want to avoid that but IMO it makes for neater API instead of getting void
> > pointers.
> 
> I was just trying to save a step of an extra call to get *priv
> in each callback implementation, but yeah, no problem to change
> it to something more "normal" looking.

Yeah, I figured that but I guess any of these paths are fastpaths anyways... 
> 
> > 
> > Another thing is, can the above actually happen? We have the
> > spi_offload_triggers_lock grabbed and we got a match so the trigger should not be
> > able to go away (should block on the same lock).
> 
> The problem is that it could have gone away before we took the lock.
> 
> It could happen like this:
> 
> * Trigger driver registers trigger - sets *priv.
> * SPI peripheral driver gets reference to trigger.
> * Trigger driver unregisters trigger - removes *priv.
> * SPI peripheral tries to call trigger function.
> 

Ah I see... we're using scoped_guard() in the unregister path.

- Nuno Sá
>