Re: [PATCH 1/5] spi: add spi_optimize_message() APIs

[David Lechner <dlechner@xxxxxxxxxxxx>]

On Tue, Feb 13, 2024 at 3:50 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Mon, 2024-02-12 at 17:26 -0600, David Lechner wrote:
> > This adds a new spi_optimize_message() function that can be used to
> > optimize SPI messages that are used more than once. Peripheral drivers
> > that use the same message multiple times can use this API to perform SPI
> > message validation and controller-specific optimizations once and then
> > reuse the message while avoiding the overhead of revalidating the
> > message on each spi_(a)sync() call.
> >
> > Internally, the SPI core will also call this function for each message
> > if the peripheral driver did not explicitly call it. This is done to so
> > that controller drivers don't have to have multiple code paths for
> > optimized and non-optimized messages.
> >
> > A hook is provided for controller drivers to perform controller-specific
> > optimizations.
> >
> > Suggested-by: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> > Link:
> > https://lore.kernel.org/linux-spi/39DEC004-10A1-47EF-9D77-276188D2580C@xxxxxxxxxxxxxxxx/
> > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > ---
> >  drivers/spi/spi.c       | 145 ++++++++++++++++++++++++++++++++++++++++++++++-
> > -
> >  include/linux/spi/spi.h |  19 +++++++
> >  2 files changed, 160 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index c2b10e2c75f0..5bac215d7009 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -2106,6 +2106,41 @@ struct spi_message *spi_get_next_queued_message(struct
> > spi_controller *ctlr)
> >  }
> >  EXPORT_SYMBOL_GPL(spi_get_next_queued_message);
> >
> > +/**
> > + * __spi_unoptimize_message - shared implementation of
> > spi_unoptimize_message()
> > + *                            and spi_maybe_unoptimize_message()
> > + * @msg: the message to unoptimize
> > + *
> > + * Periperhal drivers should use spi_unoptimize_message() and callers inside
> > + * core should use spi_maybe_unoptimize_message() rather than calling this
> > + * function directly.
> > + *
> > + * It is not valid to call this on a message that is not currently optimized.
> > + */
> > +static void __spi_unoptimize_message(struct spi_message *msg)
> > +{
> > +     struct spi_controller *ctlr = msg->spi->controller;
> > +
> > +     if (ctlr->unoptimize_message)
> > +             ctlr->unoptimize_message(msg);
> > +
> > +     msg->optimized = false;
> > +     msg->opt_state = NULL;
> > +}
> > +
> > +/**
> > + * spi_maybe_unoptimize_message - unoptimize msg not managed by a peripheral
> > + * @msg: the message to unoptimize
> > + *
> > + * This function is used to unoptimize a message if and only if it was
> > + * optimized by the core (via spi_maybe_optimize_message()).
> > + */
> > +static void spi_maybe_unoptimize_message(struct spi_message *msg)
> > +{
> > +     if (!msg->pre_optimized && msg->optimized)
> > +             __spi_unoptimize_message(msg);
> > +}
> > +
> >  /**
> >   * spi_finalize_current_message() - the current message is complete
> >   * @ctlr: the controller to return the message to
> > @@ -2153,6 +2188,8 @@ void spi_finalize_current_message(struct spi_controller
> > *ctlr)
> >
> >       mesg->prepared = false;
> >
> > +     spi_maybe_unoptimize_message(mesg);
> > +
> >       WRITE_ONCE(ctlr->cur_msg_incomplete, false);
> >       smp_mb(); /* See __spi_pump_transfer_message()... */
> >       if (READ_ONCE(ctlr->cur_msg_need_completion))
> > @@ -4194,6 +4231,99 @@ static int __spi_validate(struct spi_device *spi,
> > struct spi_message *message)
> >       return 0;
> >  }
> >
> > +/**
> > + * __spi_optimize_message - shared implementation for spi_optimize_message()
> > + *                          and spi_maybe_optimize_message()
> > + * @spi: the device that will be used for the message
> > + * @msg: the message to optimize
> > + * @pre_optimized: whether the message is considered pre-optimized or not
> > + *
> > + * Peripheral drivers will call spi_optimize_message() and the spi core will
> > + * call spi_maybe_optimize_message() instead of calling this directly.
> > + *
> > + * It is not valid to call this on a message that has already been optimized.
> > + *
> > + * Return: zero on success, else a negative error code
> > + */
> > +static int __spi_optimize_message(struct spi_device *spi,
> > +                               struct spi_message *msg,
> > +                               bool pre_optimized)
> > +{
> > +     struct spi_controller *ctlr = spi->controller;
> > +     int ret;
> > +
> > +     ret = __spi_validate(spi, msg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (ctlr->optimize_message) {
> > +             ret = ctlr->optimize_message(msg);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> Not really sure what are the spi core guarantees or what controllers should be
> expecting but I'll still ask :). Do we need to care about locking in here?
> Mainly on the controller callback? For spi device related data I guess it's up
> to the peripheral driver not to do anything weird or to properly protect the spi
> message?
>

Currently, it is expected that this operates only on the message
struct and doesn't poke any hardware so no locking is currently
required. And, yes, it is up to peripheral drivers that opt in to
pre-optimization to follow the rules of not touching the message while
it is in the optimized state. For peripheral drivers that don't call
spi_optimized_message(), nothing has really changed.