| Message ID | 20220901120732.49245-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b30f7c8eb0780e1479a9882526e838664271f4c9 |
| Headers | show |
| Series | [v1] spi: mux: Fix mux interaction with fast path optimisations | expand |
Hi, This does indeed solve my problems. Thank you! Tested-by: Casper Andersson <casper.casan@gmail.com> 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@gmail.com> > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > 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 >
On Thu, 1 Sep 2022 13:07:32 +0100, 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. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: mux: Fix mux interaction with fast path optimisations commit: b30f7c8eb0780e1479a9882526e838664271f4c9 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
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)
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@gmail.com> Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/spi/spi-mux.c | 1 + drivers/spi/spi.c | 2 +- include/linux/spi/spi.h | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-) base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5