Re: [PATCH v1] spi: mux: Fix mux interaction with fast path optimisations

[Casper Andersson <casper.casan@xxxxxxxxx>]

Hi,

This does indeed solve my problems. Thank you!

Tested-by: Casper Andersson <casper.casan@xxxxxxxxx>

On 2022-09-01 13:07, Mark Brown wrote:
> The spi-mux driver is rather too clever and attempts to resubmit any
> message that is submitted to it to the parent controller with some
> adjusted callbacks.  This does not play at all nicely with the fast
> path which now sets flags on the message indicating that it's being
> handled through the fast path, we see async messages flagged as being on
> the fast path.  Ideally the spi-mux code would duplicate the message but
> that's rather invasive and a bit fragile in that it relies on the mux
> knowing which fields in the message to copy.  Instead teach the core
> that there are controllers which can't cope with the fast path and have
> the mux flag itself as being such a controller, ensuring that messages
> going via the mux don't get partially handled via the fast path.
> 
> This will reduce the performance of any spi-mux connected device since
> we'll now always use the thread for both the actual controller and the
> mux controller instead of just the actual controller but given that we
> were always hitting the slow path anyway it's hopefully not too much of
> an additional cost and it allows us to keep the fast path.
> 
> Fixes: ae7d2346dc89 ("spi: Don't use the message queue if possible in spi_sync")
> Reported-by: Casper Andersson <casper.casan@xxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> ---
>  drivers/spi/spi-mux.c   | 1 +
>  drivers/spi/spi.c       | 2 +-
>  include/linux/spi/spi.h | 2 ++
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
> index f5d32ec4634e..0709e987bd5a 100644
> --- a/drivers/spi/spi-mux.c
> +++ b/drivers/spi/spi-mux.c
> @@ -161,6 +161,7 @@ static int spi_mux_probe(struct spi_device *spi)
>  	ctlr->num_chipselect = mux_control_states(priv->mux);
>  	ctlr->bus_num = -1;
>  	ctlr->dev.of_node = spi->dev.of_node;
> +	ctlr->must_async = true;
>  
>  	ret = devm_spi_register_controller(&spi->dev, ctlr);
>  	if (ret)
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 83da8862b8f2..c4ccd45ebc1a 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -4033,7 +4033,7 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
>  	 * guard against reentrancy from a different context. The io_mutex
>  	 * will catch those cases.
>  	 */
> -	if (READ_ONCE(ctlr->queue_empty)) {
> +	if (READ_ONCE(ctlr->queue_empty) && !ctlr->must_async) {
>  		message->actual_length = 0;
>  		message->status = -EINPROGRESS;
>  
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index e6c73d5ff1a8..f089ee1ead58 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -469,6 +469,7 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch
>   *	SPI_TRANS_FAIL_NO_START.
>   * @queue_empty: signal green light for opportunistically skipping the queue
>   *	for spi_sync transfers.
> + * @must_async: disable all fast paths in the core
>   *
>   * Each SPI controller can communicate with one or more @spi_device
>   * children.  These make a small bus, sharing MOSI, MISO and SCK signals
> @@ -690,6 +691,7 @@ struct spi_controller {
>  
>  	/* Flag for enabling opportunistic skipping of the queue in spi_sync */
>  	bool			queue_empty;
> +	bool			must_async;
>  };
>  
>  static inline void *spi_controller_get_devdata(struct spi_controller *ctlr)
> 
> base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
> -- 
> 2.30.2
>