| Message ID | 20220621061234.3626638-4-david@protonic.nl (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1038165fbbf83967f29b3bb38872faa780b3a72 |
| Headers | show |
| Series | Optimize spi_sync path | expand |
On Tue, Jun 21, 2022 at 8:15 AM David Jander <david@protonic.nl> wrote: > > This way, the spi sync path does not need to deal with the idling > transition. ... > - 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(); In the similar way ret = ... if (ret) goto out_unlock; mutex_unlock(); cond_resched(); return; > + return; > + > +out_unlock: > + mutex_unlock(&ctlr->io_mutex);
On Tue, 21 Jun 2022 15:36:23 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 21, 2022 at 8:15 AM David Jander <david@protonic.nl> wrote: > > > > This way, the spi sync path does not need to deal with the idling > > transition. > > ... > > > - 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(); > > In the similar way > > > ret = ... > if (ret) > goto out_unlock; > > mutex_unlock(); > cond_resched(); > return; Trying to add this change as an incremental patch to the whole series now that it hit linux-next, I am not so sure about how to do this, since this code has changed: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi.c?id=dc3029056b02414c29b6627e3dd7b16624725ae9#n1729 I understand that the blank line at line 1730 should go after 1732, so that the ret = ... and the if (!ret) kthread... are not separated by a blank line. So far so good, but modifying the rest of the code into this "if (!ret) goto..." idiom will mess that up, since the code in lines 1733 and 1734 is now common to both paths, so the simplest way I see it is to move those two lines in between the "ret = ..." and "if (!ret)...". Is that more desirable than not having the "if (!ret) goto" idiom? Code would look like this: ret = __spi_pump_transfer_message(ctlr, msg, was_busy); ctlr->cur_msg = NULL; ctlr->fallback = false; if (!ret) { kthread_queue_work(ctlr->kworker, &ctlr->pump_messages); goto out_mutex; } mutex_unlock(&ctlr->io_mutex); /* Prod the scheduler in case transfer_one() was busy waiting */ cond_resched(); return; out_unlock: spin_unlock_irqrestore(&ctlr->queue_lock, flags); out_mutex: mutex_unlock(&ctlr->io_mutex); } Please advice: is this really more desirable to what it is now? Or can I better leave it as is and only move the blank line? > > + return; > > + > > +out_unlock: > > + mutex_unlock(&ctlr->io_mutex); > Best regards,
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); } /**
This way, the spi sync path does not need to deal with the idling transition. Signed-off-by: David Jander <david@protonic.nl> --- drivers/spi/spi.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)