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

[David Lechner <dlechner@baylibre.com>]

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@baylibre.com>
>> ---
>>
>> 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.

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

> 
>>
>> +struct spi_offload_trigger
>> +*devm_spi_offload_trigger_alloc(struct device *dev,
>> +				struct spi_offload_trigger_info *info)
>> +{
>> +	struct spi_offload_trigger *trigger;
>> +	int ret;
>> +
>> +	trigger = kzalloc(sizeof(*trigger), GFP_KERNEL);
>> +	if (!trigger)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	device_initialize(&trigger->dev);
> 
> Do we really need the full struct device and the overhead of adding it to the driver
> core? AFAICT, we're using it only for refcouting so we could use a plain kref for
> that matter. It would make things simpler. Or do you envision an future usecase as
> this might matter? Like allowing userspace to set some controls on the trigger (I
> would expect to be done through consumers though)?

Agreed. We should not need a device at this point.

> 
>> +	trigger->dev.parent = info->parent;
>> +	trigger->dev.fwnode = info->fwnode;
>> +	trigger->dev.of_node = of_node_get(to_of_node(trigger->dev.fwnode));
>> +	trigger->dev.of_node_reused = true;
>> +	trigger->dev.release = spi_offload_trigger_dev_release;
>> +
>> +	mutex_init(&trigger->lock);
>> +	trigger->ops = info->ops;
>> +
>> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_put, trigger);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	ret = dev_set_name(&trigger->dev, "%s-%d", info->name, info->id);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return trigger;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_alloc);
>> +
>> +static void spi_offload_trigger_unregister(void *data)
>> +{
>> +	struct spi_offload_trigger *trigger = data;
>> +
>> +	scoped_guard(mutex, &spi_offload_triggers_lock)
>> +		list_del(&trigger->list);
>> +
>> +	guard(mutex)(&trigger->lock);
>> +	trigger->priv = NULL;
> 
> nit: I guess this is a good as anything else but *ops could also be a good fit to
> nullify :)

I debated between the two. :-)

But if I change the priv handling like you suggest, I think ops will make
more sense here.