| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Jan 2019 15:10:09 +0000
From: Mark Brown <broonie@...nel.org>
To: Jon Hunter <jonathanh@...dia.com>
Cc: Martin Sperl <kernel@...tin.sperl.org>,
linux-tegra <linux-tegra@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-spi@...r.kernel.org
Subject: Re: Regression: spi: core: avoid waking pump thread from spi_sync
instead run teardown delayed
On Tue, Jan 15, 2019 at 02:26:02PM +0000, Jon Hunter wrote:
> It seems that __spi_pump_messages() is getting called several times
> during boot when registering the spi-flash, then after the spi-flash has
> been registered, about a 1 sec later spi_pump_idle_teardown() is called
> (as expected), but exits because 'ctlr->running' is true. However,
> spi_pump_idle_teardown() is never called again and when we suspend we
> are stuck in the busy/running state. In this case should something be
> scheduling spi_pump_idle_teardown() again? Although even if it does I
> don't see where the busy flag would be cleared in this path?
Right, I think with the current code we just shouldn't be checking for
busy in teardown, since there's now a fairly big delay between idle and
actually turning the hardware off the name is just super misleading and
the logic confused. I don't have time to test right now but does
something like the below which changes it to a flag for the hardware
being powered up work:
diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c
index 3b2a9a6b990d..0170b0ef5d37 100644
--- a/drivers/spi/spi-stm32-qspi.c
+++ b/drivers/spi/spi-stm32-qspi.c
@@ -358,9 +358,6 @@ static int stm32_qspi_setup(struct spi_device *spi)
struct stm32_qspi_flash *flash;
u32 cr, presc;
- if (ctrl->busy)
- return -EBUSY;
-
if (!spi->max_speed_hz)
return -EINVAL;
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 5f19016bbf10..3d746a0782eb 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -152,11 +152,6 @@ static int ti_qspi_setup(struct spi_device *spi)
int clk_div = 0, ret;
u32 clk_ctrl_reg, clk_rate, clk_mask;
- if (spi->master->busy) {
- dev_dbg(qspi->dev, "master busy doing other transfers\n");
- return -EBUSY;
- }
-
if (!qspi->spi_max_frequency) {
dev_err(qspi->dev, "spi max frequency not defined\n");
return -EINVAL;
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 9f83e1b17aa1..a173a3114813 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -487,22 +487,6 @@ static int zynqmp_qspi_setup_transfer(struct spi_device *qspi,
return 0;
}
-/**
- * zynqmp_qspi_setup: Configure the QSPI controller
- * @qspi: Pointer to the spi_device structure
- *
- * Sets the operational mode of QSPI controller for the next QSPI transfer,
- * baud rate and divisor value to setup the requested qspi clock.
- *
- * Return: 0 on success; error value otherwise.
- */
-static int zynqmp_qspi_setup(struct spi_device *qspi)
-{
- if (qspi->master->busy)
- return -EBUSY;
- return 0;
-}
-
/**
* zynqmp_qspi_filltxfifo: Fills the TX FIFO as long as there is room in
* the FIFO or the bytes required to be
@@ -1088,7 +1072,6 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
master->num_chipselect = GQSPI_DEFAULT_NUM_CS;
- master->setup = zynqmp_qspi_setup;
master->set_cs = zynqmp_qspi_chipselect;
master->transfer_one = zynqmp_qspi_start_transfer;
master->prepare_transfer_hardware = zynqmp_prepare_transfer_hardware;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 06b9139664a3..6d10ad9d768f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1234,7 +1234,7 @@ 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) {
+ if (!ctlr->hw_active) {
spin_unlock_irqrestore(&ctlr->queue_lock, flags);
return;
}
@@ -1252,10 +1252,10 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
list_first_entry(&ctlr->queue, struct spi_message, queue);
list_del_init(&ctlr->cur_msg->queue);
- if (ctlr->busy)
+ if (ctlr->hw_active)
was_busy = true;
else
- ctlr->busy = true;
+ ctlr->hw_active = true;
spin_unlock_irqrestore(&ctlr->queue_lock, flags);
mutex_lock(&ctlr->io_mutex);
@@ -1362,10 +1362,6 @@ static void spi_pump_idle_teardown(struct kthread_work *work)
if (ctlr->running)
goto out;
- /* if the controller is busy then exit */
- if (ctlr->busy)
- goto out;
-
/* if the controller is idling then exit
* this is actually a bit strange and would indicate that
* this function is scheduled twice, which should not happen
@@ -1374,7 +1370,6 @@ static void spi_pump_idle_teardown(struct kthread_work *work)
goto out;
/* set up the initial states */
- ctlr->busy = false;
ctlr->idling = true;
spin_unlock_irqrestore(&ctlr->queue_lock, flags);
@@ -1384,15 +1379,17 @@ static void spi_pump_idle_teardown(struct kthread_work *work)
kfree(ctlr->dummy_tx);
ctlr->dummy_tx = NULL;
- /* unprepare hardware */
- if (ctlr->unprepare_transfer_hardware &&
- ctlr->unprepare_transfer_hardware(ctlr))
- dev_err(&ctlr->dev,
- "failed to unprepare transfer hardware\n");
- /* handle pm */
- if (ctlr->auto_runtime_pm) {
- pm_runtime_mark_last_busy(ctlr->dev.parent);
- pm_runtime_put_autosuspend(ctlr->dev.parent);
+ if (ctlr->hw_active) {
+ /* unprepare hardware */
+ if (ctlr->unprepare_transfer_hardware &&
+ ctlr->unprepare_transfer_hardware(ctlr))
+ dev_err(&ctlr->dev,
+ "failed to unprepare transfer hardware\n");
+ /* handle pm */
+ if (ctlr->auto_runtime_pm) {
+ pm_runtime_mark_last_busy(ctlr->dev.parent);
+ pm_runtime_put_autosuspend(ctlr->dev.parent);
+ }
}
/* mark controller as idle */
@@ -1401,6 +1398,7 @@ static void spi_pump_idle_teardown(struct kthread_work *work)
/* finally put us from idling into stopped */
spin_lock_irqsave(&ctlr->queue_lock, flags);
ctlr->idling = false;
+ ctlr->hw_active = false;
out:
spin_unlock_irqrestore(&ctlr->queue_lock, flags);
@@ -1411,7 +1409,7 @@ static int spi_init_queue(struct spi_controller *ctlr)
struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
ctlr->running = false;
- ctlr->busy = false;
+ ctlr->hw_active = false;
kthread_init_worker(&ctlr->kworker);
ctlr->kworker_task = kthread_run(kthread_worker_fn, &ctlr->kworker,
@@ -1520,7 +1518,7 @@ static int spi_start_queue(struct spi_controller *ctlr)
spin_lock_irqsave(&ctlr->queue_lock, flags);
- if (ctlr->running || ctlr->busy) {
+ if (ctlr->running || ctlr->hw_active) {
spin_unlock_irqrestore(&ctlr->queue_lock, flags);
return -EBUSY;
}
@@ -1543,18 +1541,19 @@ static int spi_stop_queue(struct spi_controller *ctlr)
spin_lock_irqsave(&ctlr->queue_lock, flags);
/*
- * This is a bit lame, but is optimized for the common execution path.
- * A wait_queue on the ctlr->busy could be used, but then the common
- * execution path (pump_messages) would be required to call wake_up or
- * friends on every SPI message. Do this instead.
+ * This is a bit sad, but is optimized for the common execution path.
+ * A wait_queue on the ctlr->hw_active could be used, but then
+ * the common execution path (pump_messages) would be required
+ * to call wake_up or friends on every SPI message. Do this
+ * instead.
*/
- while ((!list_empty(&ctlr->queue) || ctlr->busy) && limit--) {
+ while ((!list_empty(&ctlr->queue) || ctlr->hw_active) && limit--) {
spin_unlock_irqrestore(&ctlr->queue_lock, flags);
usleep_range(10000, 11000);
spin_lock_irqsave(&ctlr->queue_lock, flags);
}
- if (!list_empty(&ctlr->queue) || ctlr->busy)
+ if (!list_empty(&ctlr->queue) || ctlr->hw_active)
ret = -EBUSY;
else
ctlr->running = false;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 79ad62e2487c..d9b7be89e50b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -343,7 +343,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
* in-flight message
* @cur_msg_mapped: message has been mapped for DMA
* @xfer_completion: used by core transfer_one_message()
- * @busy: message pump is busy
+ * @hw_active: message pump has hardware powered up
* @running: message pump is running
* @rt: whether this queue is set to run as a realtime task
* @auto_runtime_pm: the core should ensure a runtime PM reference is held
@@ -538,7 +538,7 @@ struct spi_controller {
struct list_head queue;
struct spi_message *cur_msg;
bool idling;
- bool busy;
+ bool hw_active;
bool running;
bool rt;
bool auto_runtime_pm;
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists