From 1714582a3a087eda8786d5a1b32b2ec86ca8a303 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:24 +0200 Subject: spi: Move ctlr->cur_msg_prepared to struct spi_message This enables the possibility to transfer a message that is not at the current tip of the async message queue. This is in preparation of the next patch(es) which enable spi_sync messages to skip the queue altogether. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-2-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index c78d1ceeaa42..eb6360153fa1 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1684,7 +1684,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) spi_finalize_current_message(ctlr); goto out; } - ctlr->cur_msg_prepared = true; + msg->prepared = true; } ret = spi_map_msg(ctlr, msg); @@ -1926,7 +1926,7 @@ void spi_finalize_current_message(struct spi_controller *ctlr) */ spi_res_release(ctlr, mesg); - if (ctlr->cur_msg_prepared && ctlr->unprepare_message) { + if (mesg->prepared && ctlr->unprepare_message) { ret = ctlr->unprepare_message(ctlr, mesg); if (ret) { dev_err(&ctlr->dev, "failed to unprepare message: %d\n", @@ -1934,9 +1934,10 @@ void spi_finalize_current_message(struct spi_controller *ctlr) } } + mesg->prepared = false; + spin_lock_irqsave(&ctlr->queue_lock, flags); ctlr->cur_msg = NULL; - ctlr->cur_msg_prepared = false; ctlr->fallback = false; kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); spin_unlock_irqrestore(&ctlr->queue_lock, flags); -- cgit v1.2.3-70-g09d2 From ae7d2346dc89ae89a6e0aabe6037591a11e593c0 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:25 +0200 Subject: spi: Don't use the message queue if possible in spi_sync The interaction with the controller message queue and its corresponding auxiliary flags and variables requires the use of the queue_lock which is costly. Since spi_sync will transfer the complete message anyway, and not return until it is finished, there is no need to put the message into the queue if the queue is empty. This can save a lot of overhead. As an example of how significant this is, when using the MCP2518FD SPI CAN controller on a i.MX8MM SoC, the time during which the interrupt line stays active (during 3 relatively short spi_sync messages), is reduced from 98us to 72us by this patch. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-3-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 246 +++++++++++++++++++++++++++++------------------- include/linux/spi/spi.h | 11 ++- 2 files changed, 159 insertions(+), 98 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index eb6360153fa1..2d057d03c4f7 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1549,6 +1549,80 @@ static void spi_idle_runtime_pm(struct spi_controller *ctlr) } } +static int __spi_pump_transfer_message(struct spi_controller *ctlr, + struct spi_message *msg, bool was_busy) +{ + struct spi_transfer *xfer; + int ret; + + if (!was_busy && ctlr->auto_runtime_pm) { + ret = pm_runtime_get_sync(ctlr->dev.parent); + if (ret < 0) { + pm_runtime_put_noidle(ctlr->dev.parent); + dev_err(&ctlr->dev, "Failed to power device: %d\n", + ret); + return ret; + } + } + + if (!was_busy) + trace_spi_controller_busy(ctlr); + + if (!was_busy && ctlr->prepare_transfer_hardware) { + ret = ctlr->prepare_transfer_hardware(ctlr); + if (ret) { + dev_err(&ctlr->dev, + "failed to prepare transfer hardware: %d\n", + ret); + + if (ctlr->auto_runtime_pm) + pm_runtime_put(ctlr->dev.parent); + + msg->status = ret; + spi_finalize_current_message(ctlr); + + return ret; + } + } + + trace_spi_message_start(msg); + + if (ctlr->prepare_message) { + ret = ctlr->prepare_message(ctlr, msg); + if (ret) { + dev_err(&ctlr->dev, "failed to prepare message: %d\n", + ret); + msg->status = ret; + spi_finalize_current_message(ctlr); + return ret; + } + msg->prepared = true; + } + + ret = spi_map_msg(ctlr, msg); + if (ret) { + msg->status = ret; + spi_finalize_current_message(ctlr); + return ret; + } + + if (!ctlr->ptp_sts_supported && !ctlr->transfer_one) { + list_for_each_entry(xfer, &msg->transfers, transfer_list) { + xfer->ptp_sts_word_pre = 0; + ptp_read_system_prets(xfer->ptp_sts); + } + } + + ret = ctlr->transfer_one_message(ctlr, msg); + if (ret) { + dev_err(&ctlr->dev, + "failed to transfer one message from queue\n"); + return ret; + } + + return 0; +} + /** * __spi_pump_messages - function which processes spi message queue * @ctlr: controller to process queue for @@ -1564,7 +1638,6 @@ static void spi_idle_runtime_pm(struct spi_controller *ctlr) */ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) { - struct spi_transfer *xfer; struct spi_message *msg; bool was_busy = false; unsigned long flags; @@ -1599,6 +1672,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) !ctlr->unprepare_transfer_hardware) { spi_idle_runtime_pm(ctlr); ctlr->busy = false; + ctlr->queue_empty = true; trace_spi_controller_idle(ctlr); } else { kthread_queue_work(ctlr->kworker, @@ -1625,6 +1699,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) spin_lock_irqsave(&ctlr->queue_lock, flags); ctlr->idling = false; + ctlr->queue_empty = true; spin_unlock_irqrestore(&ctlr->queue_lock, flags); return; } @@ -1641,75 +1716,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) spin_unlock_irqrestore(&ctlr->queue_lock, flags); mutex_lock(&ctlr->io_mutex); - - if (!was_busy && ctlr->auto_runtime_pm) { - ret = pm_runtime_resume_and_get(ctlr->dev.parent); - if (ret < 0) { - dev_err(&ctlr->dev, "Failed to power device: %d\n", - ret); - mutex_unlock(&ctlr->io_mutex); - return; - } - } - - if (!was_busy) - trace_spi_controller_busy(ctlr); - - if (!was_busy && ctlr->prepare_transfer_hardware) { - ret = ctlr->prepare_transfer_hardware(ctlr); - if (ret) { - dev_err(&ctlr->dev, - "failed to prepare transfer hardware: %d\n", - ret); - - if (ctlr->auto_runtime_pm) - pm_runtime_put(ctlr->dev.parent); - - msg->status = ret; - spi_finalize_current_message(ctlr); - - mutex_unlock(&ctlr->io_mutex); - return; - } - } - - trace_spi_message_start(msg); - - if (ctlr->prepare_message) { - ret = ctlr->prepare_message(ctlr, msg); - if (ret) { - dev_err(&ctlr->dev, "failed to prepare message: %d\n", - ret); - msg->status = ret; - spi_finalize_current_message(ctlr); - goto out; - } - msg->prepared = true; - } - - ret = spi_map_msg(ctlr, msg); - if (ret) { - msg->status = ret; - spi_finalize_current_message(ctlr); - goto out; - } - - if (!ctlr->ptp_sts_supported && !ctlr->transfer_one) { - list_for_each_entry(xfer, &msg->transfers, transfer_list) { - xfer->ptp_sts_word_pre = 0; - ptp_read_system_prets(xfer->ptp_sts); - } - } - - ret = ctlr->transfer_one_message(ctlr, msg); - if (ret) { - dev_err(&ctlr->dev, - "failed to transfer one message from queue: %d\n", - ret); - goto out; - } - -out: + ret = __spi_pump_transfer_message(ctlr, msg, was_busy); mutex_unlock(&ctlr->io_mutex); /* Prod the scheduler in case transfer_one() was busy waiting */ @@ -1839,6 +1846,7 @@ static int spi_init_queue(struct spi_controller *ctlr) { ctlr->running = false; ctlr->busy = false; + ctlr->queue_empty = true; ctlr->kworker = kthread_create_worker(0, dev_name(&ctlr->dev)); if (IS_ERR(ctlr->kworker)) { @@ -1936,11 +1944,20 @@ void spi_finalize_current_message(struct spi_controller *ctlr) mesg->prepared = false; - spin_lock_irqsave(&ctlr->queue_lock, flags); - ctlr->cur_msg = NULL; - ctlr->fallback = false; - kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); - spin_unlock_irqrestore(&ctlr->queue_lock, flags); + if (!mesg->sync) { + /* + * This message was sent via the async message queue. Handle + * the queue and kick the worker thread to do the + * idling/shutdown or send the next message if needed. + */ + spin_lock_irqsave(&ctlr->queue_lock, flags); + WARN(ctlr->cur_msg != mesg, + "Finalizing queued message that is not the current head of queue!"); + ctlr->cur_msg = NULL; + ctlr->fallback = false; + kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); + spin_unlock_irqrestore(&ctlr->queue_lock, flags); + } trace_spi_message_done(mesg); @@ -2043,6 +2060,7 @@ static int __spi_queued_transfer(struct spi_device *spi, msg->status = -EINPROGRESS; list_add_tail(&msg->queue, &ctlr->queue); + ctlr->queue_empty = false; if (!ctlr->busy && need_pump) kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); @@ -3938,6 +3956,39 @@ static int spi_async_locked(struct spi_device *spi, struct spi_message *message) } +static void __spi_transfer_message_noqueue(struct spi_controller *ctlr, struct spi_message *msg) +{ + bool was_busy; + int ret; + + mutex_lock(&ctlr->io_mutex); + + /* If another context is idling the device then wait */ + while (ctlr->idling) + usleep_range(10000, 11000); + + was_busy = READ_ONCE(ctlr->busy); + + ret = __spi_pump_transfer_message(ctlr, msg, was_busy); + if (ret) + goto out; + + if (!was_busy) { + kfree(ctlr->dummy_rx); + ctlr->dummy_rx = NULL; + kfree(ctlr->dummy_tx); + ctlr->dummy_tx = NULL; + if (ctlr->unprepare_transfer_hardware && + ctlr->unprepare_transfer_hardware(ctlr)) + dev_err(&ctlr->dev, + "failed to unprepare transfer hardware\n"); + spi_idle_runtime_pm(ctlr); + } + +out: + mutex_unlock(&ctlr->io_mutex); +} + /*-------------------------------------------------------------------------*/ /* @@ -3956,51 +4007,52 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message) DECLARE_COMPLETION_ONSTACK(done); int status; struct spi_controller *ctlr = spi->controller; - unsigned long flags; status = __spi_validate(spi, message); if (status != 0) return status; - message->complete = spi_complete; - message->context = &done; message->spi = spi; SPI_STATISTICS_INCREMENT_FIELD(ctlr->pcpu_statistics, spi_sync); SPI_STATISTICS_INCREMENT_FIELD(spi->pcpu_statistics, spi_sync); /* - * If we're not using the legacy transfer method then we will - * try to transfer in the calling context so special case. - * This code would be less tricky if we could remove the - * support for driver implemented message queues. + * Checking queue_empty here only guarantees async/sync message + * ordering when coming from the same context. It does not need to + * guard against reentrancy from a different context. The io_mutex + * will catch those cases. */ - if (ctlr->transfer == spi_queued_transfer) { - spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags); + if (READ_ONCE(ctlr->queue_empty)) { + message->sync = true; + message->actual_length = 0; + message->status = -EINPROGRESS; trace_spi_message_submit(message); - status = __spi_queued_transfer(spi, message, false); + SPI_STATISTICS_INCREMENT_FIELD(ctlr->pcpu_statistics, spi_sync_immediate); + SPI_STATISTICS_INCREMENT_FIELD(spi->pcpu_statistics, spi_sync_immediate); - spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags); - } else { - status = spi_async_locked(spi, message); + __spi_transfer_message_noqueue(ctlr, message); + + return message->status; } + /* + * There are messages in the async queue that could have originated + * from the same context, so we need to preserve ordering. + * Therefor we send the message to the async queue and wait until they + * are completed. + */ + message->complete = spi_complete; + message->context = &done; + status = spi_async_locked(spi, message); if (status == 0) { - /* Push out the messages in the calling context if we can */ - if (ctlr->transfer == spi_queued_transfer) { - SPI_STATISTICS_INCREMENT_FIELD(ctlr->pcpu_statistics, - spi_sync_immediate); - SPI_STATISTICS_INCREMENT_FIELD(spi->pcpu_statistics, - spi_sync_immediate); - __spi_pump_messages(ctlr, false); - } - wait_for_completion(&done); status = message->status; } message->context = NULL; + return status; } diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 1a75c26742f2..74261a83b5fa 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -461,6 +461,8 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @irq_flags: Interrupt enable state during PTP system timestamping * @fallback: fallback to pio if dma transfer return failure with * SPI_TRANS_FAIL_NO_START. + * @queue_empty: signal green light for opportunistically skipping the queue + * for spi_sync transfers. * * Each SPI controller can communicate with one or more @spi_device * children. These make a small bus, sharing MOSI, MISO and SCK signals @@ -677,6 +679,9 @@ struct spi_controller { /* Interrupt enable state during PTP system timestamping */ unsigned long irq_flags; + + /* Flag for enabling opportunistic skipping of the queue in spi_sync */ + bool queue_empty; }; static inline void *spi_controller_get_devdata(struct spi_controller *ctlr) @@ -986,6 +991,7 @@ struct spi_transfer { * @state: for use by whichever driver currently owns the message * @resources: for resource management when the spi message is processed * @prepared: spi_prepare_message was called for the this message + * @sync: this message took the direct sync path skipping the async queue * * A @spi_message is used to execute an atomic sequence of data transfers, * each represented by a struct spi_transfer. The sequence is "atomic" @@ -1037,7 +1043,10 @@ struct spi_message { struct list_head resources; /* spi_prepare_message was called for this message */ - bool prepared; + bool prepared; + + /* this message is skipping the async queue */ + bool sync; }; static inline void spi_message_init_no_memset(struct spi_message *m) -- cgit v1.2.3-70-g09d2 From c1038165fbbf83967f29b3bb38872faa780b3a72 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:26 +0200 Subject: spi: Lock controller idling transition inside the io_mutex This way, the spi sync path does not need to deal with the idling transition. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-4-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 2d057d03c4f7..cfff2ff96fa0 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1643,27 +1643,30 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) unsigned long flags; int ret; + /* Take the IO mutex */ + mutex_lock(&ctlr->io_mutex); + /* Lock queue */ spin_lock_irqsave(&ctlr->queue_lock, flags); /* Make sure we are not already running a message */ if (ctlr->cur_msg) { spin_unlock_irqrestore(&ctlr->queue_lock, flags); - return; + goto out_unlock; } /* If another context is idling the device then defer */ if (ctlr->idling) { kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); spin_unlock_irqrestore(&ctlr->queue_lock, flags); - return; + goto out_unlock; } /* Check if the queue is idle */ if (list_empty(&ctlr->queue) || !ctlr->running) { if (!ctlr->busy) { spin_unlock_irqrestore(&ctlr->queue_lock, flags); - return; + goto out_unlock; } /* Defer any non-atomic teardown to the thread */ @@ -1679,7 +1682,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) &ctlr->pump_messages); } spin_unlock_irqrestore(&ctlr->queue_lock, flags); - return; + goto out_unlock; } ctlr->busy = false; @@ -1701,7 +1704,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) ctlr->idling = false; ctlr->queue_empty = true; spin_unlock_irqrestore(&ctlr->queue_lock, flags); - return; + goto out_unlock; } /* Extract head of queue */ @@ -1715,13 +1718,16 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) ctlr->busy = true; spin_unlock_irqrestore(&ctlr->queue_lock, flags); - mutex_lock(&ctlr->io_mutex); ret = __spi_pump_transfer_message(ctlr, msg, was_busy); mutex_unlock(&ctlr->io_mutex); /* Prod the scheduler in case transfer_one() was busy waiting */ if (!ret) cond_resched(); + return; + +out_unlock: + mutex_unlock(&ctlr->io_mutex); } /** -- cgit v1.2.3-70-g09d2 From 8711a2ab51dd47b2bcb3880403add25dd7fc7c13 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:27 +0200 Subject: spi: __spi_pump_messages: Consolidate spin_unlocks to goto target Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-5-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index cfff2ff96fa0..fa2d091d2854 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1650,10 +1650,8 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) spin_lock_irqsave(&ctlr->queue_lock, flags); /* Make sure we are not already running a message */ - if (ctlr->cur_msg) { - spin_unlock_irqrestore(&ctlr->queue_lock, flags); + if (ctlr->cur_msg) goto out_unlock; - } /* If another context is idling the device then defer */ if (ctlr->idling) { @@ -1664,10 +1662,8 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) /* Check if the queue is idle */ if (list_empty(&ctlr->queue) || !ctlr->running) { - if (!ctlr->busy) { - spin_unlock_irqrestore(&ctlr->queue_lock, flags); + if (!ctlr->busy) goto out_unlock; - } /* Defer any non-atomic teardown to the thread */ if (!in_kthread) { @@ -1681,7 +1677,6 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); } - spin_unlock_irqrestore(&ctlr->queue_lock, flags); goto out_unlock; } @@ -1703,7 +1698,6 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) spin_lock_irqsave(&ctlr->queue_lock, flags); ctlr->idling = false; ctlr->queue_empty = true; - spin_unlock_irqrestore(&ctlr->queue_lock, flags); goto out_unlock; } @@ -1727,6 +1721,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) return; out_unlock: + spin_unlock_irqrestore(&ctlr->queue_lock, flags); mutex_unlock(&ctlr->io_mutex); } -- cgit v1.2.3-70-g09d2 From d5256cce1f50ff4c8fad6b8eb7b4ec9e47d38925 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:28 +0200 Subject: spi: Remove check for controller idling in spi sync path Now that the idling flag is wholly behind the io_mutex, this broken piece of code can be safely removed. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-6-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 4 ---- 1 file changed, 4 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index fa2d091d2854..d8d2b7ac78f2 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3964,10 +3964,6 @@ static void __spi_transfer_message_noqueue(struct spi_controller *ctlr, struct s mutex_lock(&ctlr->io_mutex); - /* If another context is idling the device then wait */ - while (ctlr->idling) - usleep_range(10000, 11000); - was_busy = READ_ONCE(ctlr->busy); ret = __spi_pump_transfer_message(ctlr, msg, was_busy); -- cgit v1.2.3-70-g09d2 From 049d6ccc4da8d34f382949ebec6d4fb318a9c7c0 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:29 +0200 Subject: spi: Remove check for idling in __spi_pump_messages() Since the whole idling transition is locked by the io_mutex now, there is no need to check this flag anymore. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-7-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 7 ------- 1 file changed, 7 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index d8d2b7ac78f2..71b767a9ad77 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1653,13 +1653,6 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) if (ctlr->cur_msg) goto out_unlock; - /* If another context is idling the device then defer */ - if (ctlr->idling) { - kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); - spin_unlock_irqrestore(&ctlr->queue_lock, flags); - goto out_unlock; - } - /* Check if the queue is idle */ if (list_empty(&ctlr->queue) || !ctlr->running) { if (!ctlr->busy) -- cgit v1.2.3-70-g09d2 From 66a221593cb26dd6aabba63bcd18173f4e69c7ab Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:30 +0200 Subject: spi: Remove the now unused ctlr->idling flag The ctlr->idling flag is never checked now, so we don't need to set it either. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-8-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 2 -- include/linux/spi/spi.h | 2 -- 2 files changed, 4 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 71b767a9ad77..52736e339645 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1674,7 +1674,6 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) } ctlr->busy = false; - ctlr->idling = true; spin_unlock_irqrestore(&ctlr->queue_lock, flags); kfree(ctlr->dummy_rx); @@ -1689,7 +1688,6 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) trace_spi_controller_idle(ctlr); spin_lock_irqsave(&ctlr->queue_lock, flags); - ctlr->idling = false; ctlr->queue_empty = true; goto out_unlock; } diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 74261a83b5fa..c58f46be762f 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -383,7 +383,6 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @pump_messages: work struct for scheduling work to the message pump * @queue_lock: spinlock to syncronise access to message queue * @queue: message queue - * @idling: the device is entering idle state * @cur_msg: the currently in-flight message * @cur_msg_mapped: message has been mapped for DMA * @last_cs: the last chip_select that is recorded by set_cs, -1 on non chip @@ -616,7 +615,6 @@ struct spi_controller { spinlock_t queue_lock; struct list_head queue; struct spi_message *cur_msg; - bool idling; bool busy; bool running; bool rt; -- cgit v1.2.3-70-g09d2 From 1a9cafcb57b70fc1439d4a5cb28963122568967a Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:31 +0200 Subject: spi: Remove unneeded READ_ONCE for ctlr->busy flag Now this flag is written entirely in the mutex, so no need for READ_ONCE Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-9-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 52736e339645..29f42753ef0f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3955,7 +3955,7 @@ static void __spi_transfer_message_noqueue(struct spi_controller *ctlr, struct s mutex_lock(&ctlr->io_mutex); - was_busy = READ_ONCE(ctlr->busy); + was_busy = ctlr->busy; ret = __spi_pump_transfer_message(ctlr, msg, was_busy); if (ret) -- cgit v1.2.3-70-g09d2 From 72c5c59b659d54d0c824d0333a211f373316361d Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:32 +0200 Subject: spi: Set ctlr->cur_msg also in the sync transfer case Some drivers rely on this to point to the currently processed message, so set this here also. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-10-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 29f42753ef0f..3df84f43918c 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3957,6 +3957,7 @@ static void __spi_transfer_message_noqueue(struct spi_controller *ctlr, struct s was_busy = ctlr->busy; + ctlr->cur_msg = msg; ret = __spi_pump_transfer_message(ctlr, msg, was_busy); if (ret) goto out; -- cgit v1.2.3-70-g09d2 From 69fa95905d40846756d22402690ddf5361a9d13b Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:33 +0200 Subject: spi: Ensure the io_mutex is held until spi_finalize_current_message() This patch introduces a completion that is completed in spi_finalize_current_message() and waited for in __spi_pump_transfer_message(). This way all manipulation of ctlr->cur_msg is done with the io_mutex held and strictly ordered: __spi_pump_transfer_message() will not return until spi_finalize_current_message() is done using ctlr->cur_msg, and its calling context is only touching ctlr->cur_msg after returning. Due to this, we can safely drop the spin-locks around ctlr->cur_msg. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-11-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 32 ++++++++++++++------------------ include/linux/spi/spi.h | 6 ++---- 2 files changed, 16 insertions(+), 22 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 3df84f43918c..db08cb868652 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1613,11 +1613,14 @@ static int __spi_pump_transfer_message(struct spi_controller *ctlr, } } + reinit_completion(&ctlr->cur_msg_completion); ret = ctlr->transfer_one_message(ctlr, msg); if (ret) { dev_err(&ctlr->dev, "failed to transfer one message from queue\n"); return ret; + } else { + wait_for_completion(&ctlr->cur_msg_completion); } return 0; @@ -1704,6 +1707,12 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) spin_unlock_irqrestore(&ctlr->queue_lock, flags); ret = __spi_pump_transfer_message(ctlr, msg, was_busy); + + if (!ret) + kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); + ctlr->cur_msg = NULL; + ctlr->fallback = false; + mutex_unlock(&ctlr->io_mutex); /* Prod the scheduler in case transfer_one() was busy waiting */ @@ -1897,12 +1906,9 @@ void spi_finalize_current_message(struct spi_controller *ctlr) { struct spi_transfer *xfer; struct spi_message *mesg; - unsigned long flags; int ret; - spin_lock_irqsave(&ctlr->queue_lock, flags); mesg = ctlr->cur_msg; - spin_unlock_irqrestore(&ctlr->queue_lock, flags); if (!ctlr->ptp_sts_supported && !ctlr->transfer_one) { list_for_each_entry(xfer, &mesg->transfers, transfer_list) { @@ -1936,20 +1942,7 @@ void spi_finalize_current_message(struct spi_controller *ctlr) mesg->prepared = false; - if (!mesg->sync) { - /* - * This message was sent via the async message queue. Handle - * the queue and kick the worker thread to do the - * idling/shutdown or send the next message if needed. - */ - spin_lock_irqsave(&ctlr->queue_lock, flags); - WARN(ctlr->cur_msg != mesg, - "Finalizing queued message that is not the current head of queue!"); - ctlr->cur_msg = NULL; - ctlr->fallback = false; - kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); - spin_unlock_irqrestore(&ctlr->queue_lock, flags); - } + complete(&ctlr->cur_msg_completion); trace_spi_message_done(mesg); @@ -3036,6 +3029,7 @@ int spi_register_controller(struct spi_controller *ctlr) } ctlr->bus_lock_flag = 0; init_completion(&ctlr->xfer_completion); + init_completion(&ctlr->cur_msg_completion); if (!ctlr->max_dma_len) ctlr->max_dma_len = INT_MAX; @@ -3962,6 +3956,9 @@ static void __spi_transfer_message_noqueue(struct spi_controller *ctlr, struct s if (ret) goto out; + ctlr->cur_msg = NULL; + ctlr->fallback = false; + if (!was_busy) { kfree(ctlr->dummy_rx); ctlr->dummy_rx = NULL; @@ -4013,7 +4010,6 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message) * will catch those cases. */ if (READ_ONCE(ctlr->queue_empty)) { - message->sync = true; message->actual_length = 0; message->status = -EINPROGRESS; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index c58f46be762f..c56e0d240a58 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -384,6 +384,7 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @queue_lock: spinlock to syncronise access to message queue * @queue: message queue * @cur_msg: the currently in-flight message + * @cur_msg_completion: a completion for the current in-flight message * @cur_msg_mapped: message has been mapped for DMA * @last_cs: the last chip_select that is recorded by set_cs, -1 on non chip * selected @@ -615,6 +616,7 @@ struct spi_controller { spinlock_t queue_lock; struct list_head queue; struct spi_message *cur_msg; + struct completion cur_msg_completion; bool busy; bool running; bool rt; @@ -989,7 +991,6 @@ struct spi_transfer { * @state: for use by whichever driver currently owns the message * @resources: for resource management when the spi message is processed * @prepared: spi_prepare_message was called for the this message - * @sync: this message took the direct sync path skipping the async queue * * A @spi_message is used to execute an atomic sequence of data transfers, * each represented by a struct spi_transfer. The sequence is "atomic" @@ -1042,9 +1043,6 @@ struct spi_message { /* spi_prepare_message was called for this message */ bool prepared; - - /* this message is skipping the async queue */ - bool sync; }; static inline void spi_message_init_no_memset(struct spi_message *m) -- cgit v1.2.3-70-g09d2 From dc3029056b02414c29b6627e3dd7b16624725ae9 Mon Sep 17 00:00:00 2001 From: David Jander Date: Tue, 21 Jun 2022 08:12:34 +0200 Subject: spi: opportunistically skip ctlr->cur_msg_completion There are only a few drivers that do not call spi_finalize_current_message() in the context of transfer_one_message(), and even for those cases the completion ctlr->cur_msg_completion is not needed always. The calls to complete() and wait_for_completion() each take a spin-lock, which is costly. This patch makes it possible to avoid those calls in the big majority of cases, by introducing two flags that with the help of ordering via barriers can avoid using the completion safely. In case of a race with the context calling spi_finalize_current_message(), the scheme errs on the safe side and takes the completion. The impact of this patch is worth the effort: On a i.MX8MM SoC, the time the SPI bus is idle between two consecutive calls to spi_sync(), is reduced from 19.6us to 16.8us... roughly 15%. Signed-off-by: David Jander Link: https://lore.kernel.org/r/20220621061234.3626638-12-david@protonic.nl Signed-off-by: Mark Brown --- drivers/spi/spi.c | 27 +++++++++++++++++++++++++-- include/linux/spi/spi.h | 8 ++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index db08cb868652..ef37f043fd17 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1613,14 +1613,34 @@ static int __spi_pump_transfer_message(struct spi_controller *ctlr, } } + /* + * Drivers implementation of transfer_one_message() must arrange for + * spi_finalize_current_message() to get called. Most drivers will do + * this in the calling context, but some don't. For those cases, a + * completion is used to guarantee that this function does not return + * until spi_finalize_current_message() is done accessing + * ctlr->cur_msg. + * Use of the following two flags enable to opportunistically skip the + * use of the completion since its use involves expensive spin locks. + * In case of a race with the context that calls + * spi_finalize_current_message() the completion will always be used, + * due to strict ordering of these flags using barriers. + */ + WRITE_ONCE(ctlr->cur_msg_incomplete, true); + WRITE_ONCE(ctlr->cur_msg_need_completion, false); reinit_completion(&ctlr->cur_msg_completion); + smp_wmb(); /* make these available to spi_finalize_current_message */ + ret = ctlr->transfer_one_message(ctlr, msg); if (ret) { dev_err(&ctlr->dev, "failed to transfer one message from queue\n"); return ret; } else { - wait_for_completion(&ctlr->cur_msg_completion); + WRITE_ONCE(ctlr->cur_msg_need_completion, true); + smp_mb(); /* see spi_finalize_current_message()... */ + if (READ_ONCE(ctlr->cur_msg_incomplete)) + wait_for_completion(&ctlr->cur_msg_completion); } return 0; @@ -1942,7 +1962,10 @@ void spi_finalize_current_message(struct spi_controller *ctlr) mesg->prepared = false; - complete(&ctlr->cur_msg_completion); + WRITE_ONCE(ctlr->cur_msg_incomplete, false); + smp_mb(); /* See __spi_pump_transfer_message()... */ + if (READ_ONCE(ctlr->cur_msg_need_completion)) + complete(&ctlr->cur_msg_completion); trace_spi_message_done(mesg); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index c56e0d240a58..eb0d316e3c36 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -385,6 +385,12 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @queue: message queue * @cur_msg: the currently in-flight message * @cur_msg_completion: a completion for the current in-flight message + * @cur_msg_incomplete: Flag used internally to opportunistically skip + * the @cur_msg_completion. This flag is used to check if the driver has + * already called spi_finalize_current_message(). + * @cur_msg_need_completion: Flag used internally to opportunistically skip + * the @cur_msg_completion. This flag is used to signal the context that + * is running spi_finalize_current_message() that it needs to complete() * @cur_msg_mapped: message has been mapped for DMA * @last_cs: the last chip_select that is recorded by set_cs, -1 on non chip * selected @@ -617,6 +623,8 @@ struct spi_controller { struct list_head queue; struct spi_message *cur_msg; struct completion cur_msg_completion; + bool cur_msg_incomplete; + bool cur_msg_need_completion; bool busy; bool running; bool rt; -- cgit v1.2.3-70-g09d2