Re: Regression: spi: core: avoid waking pump thread from spi_sync instead run teardown delayed

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8567 bytes --]

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;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]