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