[PATCH 0/4] spi: A better solution for cros_ec_spi reliability

[PATCH 0/4] spi: A better solution for cros_ec_spi reliability

[Douglas Anderson]

This series is a much better solution for getting the Chrome OS EC to
talk reliably and replaces commit 37a186225a0c ("platform/chrome:
cros_ec_spi: Transfer messages at high priority").

Note that the cros_ec bits can't land until the SPI bits are
somewhere.  If the SPI bits look OK to land it might be convenient if
they could be placed somewhere with a stable git hash?

Special thanks to Guenter Roeck for pointing out the "realtime"
feature of the SPI framework so I didn't re-invent the wheel.  I have
no idea how I missed it.  :-/


Douglas Anderson (4):
  spi: For controllers that need realtime always use the pump thread
  spi: Allow SPI devices to specify that they are timing sensitive
  platform/chrome: cros_ec_spi: Set ourselves as timing sensitive
  Revert "platform/chrome: cros_ec_spi: Transfer messages at high
    priority"

 drivers/platform/chrome/cros_ec_spi.c | 81 +++------------------------
 drivers/spi/spi.c                     | 41 +++++++++++---
 include/linux/spi/spi.h               |  3 +
 3 files changed, 43 insertions(+), 82 deletions(-)

-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Douglas Anderson]

If a controller specifies that it needs high priority for sending
messages we should always schedule our transfers on the thread.  If we
don't do this we'll do the transfer in the caller's context which
might not be very high priority.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/spi/spi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8eb7460dd744..0597f7086de3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1230,8 +1230,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
 		return;
 	}
 
-	/* If another context is idling the device then defer */
-	if (ctlr->idling) {
+	/*
+	 * If another context is idling the device then defer.
+	 * If we are high priority then the thread should do the transfer.
+	 */
+	if (ctlr->idling || (ctlr->rt && !in_kthread)) {
 		kthread_queue_work(&ctlr->kworker, &ctlr->pump_messages);
 		spin_unlock_irqrestore(&ctlr->queue_lock, flags);
 		return;
-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 2/4] spi: Allow SPI devices to specify that they are timing sensitive

[Douglas Anderson]

If a device on the SPI bus is very sensitive to timing then it may be
necessary (for correctness) not to get interrupted during a transfer.
One example is the EC (Embedded Controller) on Chromebooks.  The
Chrome OS EC will drop a transfer if more than ~8ms passes between the
chip select being asserted and the transfer finishing.

The SPI framework already has code to handle the case where transfers
are timing senstive.  It can set its message pumping thread to
realtime to to minimize interruptions during the transfer.  However,
at the moment, this mode can only be requested by a SPI controller.
Let's allow the drivers for SPI devices to also request this mode.

NOTE: at the moment if a given device on a bus says that it's timing
sensitive then we'll pump all messages on that bus at high priority.
It is possible we might want to relax this in the future but it seems
like it should be fine for now.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/spi/spi.c       | 34 ++++++++++++++++++++++++++++------
 include/linux/spi/spi.h |  3 +++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 0597f7086de3..d117ab3adafa 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1367,10 +1367,30 @@ static void spi_pump_messages(struct kthread_work *work)
 	__spi_pump_messages(ctlr, true);
 }
 
-static int spi_init_queue(struct spi_controller *ctlr)
+/**
+ * spi_boost_thread_priority - set the controller to pump at realtime priority
+ * @ctlr: controller to boost priority of
+ *
+ * This can be called because the controller requested realtime priority
+ * (by setting the ->rt value before calling spi_register_controller()) or
+ * because a device on the bus said that its transfers were timing senstive.
+ *
+ * NOTE: at the moment if any device on a bus says it is timing sensitive then
+ * all the devices on this bus will do transfers at realtime priority.  If
+ * this eventually becomes a problem we may see if we can find a way to boost
+ * the priority only temporarily during relevant transfers.
+ */
+static void spi_boost_thread_priority(struct spi_controller *ctlr)
 {
 	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
 
+	dev_info(&ctlr->dev,
+		"will run message pump with realtime priority\n");
+	sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
+}
+
+static int spi_init_queue(struct spi_controller *ctlr)
+{
 	ctlr->running = false;
 	ctlr->busy = false;
 
@@ -1390,11 +1410,8 @@ static int spi_init_queue(struct spi_controller *ctlr)
 	 * request and the scheduling of the message pump thread. Without this
 	 * setting the message pump thread will remain at default priority.
 	 */
-	if (ctlr->rt) {
-		dev_info(&ctlr->dev,
-			"will run message pump with realtime priority\n");
-		sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
-	}
+	if (ctlr->rt)
+		spi_boost_thread_priority(ctlr);
 
 	return 0;
 }
@@ -2985,6 +3002,11 @@ int spi_setup(struct spi_device *spi)
 
 	spi_set_cs(spi, false);
 
+	if (spi->timing_sensitive && !spi->controller->rt) {
+		spi->controller->rt = true;
+		spi_boost_thread_priority(spi->controller);
+	}
+
 	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
 			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
 			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 053abd22ad31..ef6bdd4d25f2 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -109,6 +109,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
  *	This may be changed by the device's driver, or left at the
  *	default (0) indicating protocol words are eight bit bytes.
  *	The spi_transfer.bits_per_word can override this for each transfer.
+ * @timing_sensitive: Transfers for this device are senstive to timing
+ *	so we should do our transfer at high priority.
  * @irq: Negative, or the number passed to request_irq() to receive
  *	interrupts from this device.
  * @controller_state: Controller's runtime state
@@ -143,6 +145,7 @@ struct spi_device {
 	u32			max_speed_hz;
 	u8			chip_select;
 	u8			bits_per_word;
+	bool			timing_sensitive;
 	u32			mode;
 #define	SPI_CPHA	0x01			/* clock phase */
 #define	SPI_CPOL	0x02			/* clock polarity */
-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 3/4] platform/chrome: cros_ec_spi: Set ourselves as timing sensitive

[Douglas Anderson]

All currently known ECs in the wild are very sensitive to timing.
Specifically the ECs are known to drop a transfer if more than 8 ms
passes from the assertion of the chip select until the transfer
finishes.

Let's use the new feature introduced in the patch ("spi: Allow SPI
devices to specify that they are timing sensitive") to specify this
and increase the success rate of our transfers.

NOTE: if future Chrome OS ECs ever fix themselves to be less sensitive
then we could consider adding a property (or compatible string) to not
set this property.  For now we need it across the board.

With this change we can revert the commit 37a186225a0c
("platform/chrome: cros_ec_spi: Transfer messages at high priority").
...and, in fact, transfers are _even more_ reliable than they were
with that commit since the SPI framework will use a higher priority
(realtime) and we no longer lose our priority when we get shunted over
to the message pumping thread (because we now always get shunted and
the thread is high priority).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/platform/chrome/cros_ec_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8e9451720e73..757a115502ec 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -703,6 +703,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 
 	spi->bits_per_word = 8;
 	spi->mode = SPI_MODE_0;
+	spi->timing_sensitive = true;
 	err = spi_setup(spi);
 	if (err < 0)
 		return err;
-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Douglas Anderson]

This reverts commit 37a186225a0c020516bafad2727fdcdfc039a1e4.

We have a better solution in the patch ("platform/chrome: cros_ec_spi:
Set ourselves as timing sensitive").  Let's revert the uglier and less
reliable solution.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/platform/chrome/cros_ec_spi.c | 80 ++-------------------------
 1 file changed, 6 insertions(+), 74 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 757a115502ec..70ff1ad09012 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -75,27 +75,6 @@ struct cros_ec_spi {
 	unsigned int end_of_msg_delay;
 };
 
-typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
-				  struct cros_ec_command *ec_msg);
-
-/**
- * struct cros_ec_xfer_work_params - params for our high priority workers
- *
- * @work: The work_struct needed to queue work
- * @fn: The function to use to transfer
- * @ec_dev: ChromeOS EC device
- * @ec_msg: Message to transfer
- * @ret: The return value of the function
- */
-
-struct cros_ec_xfer_work_params {
-	struct work_struct work;
-	cros_ec_xfer_fn_t fn;
-	struct cros_ec_device *ec_dev;
-	struct cros_ec_command *ec_msg;
-	int ret;
-};
-
 static void debug_packet(struct device *dev, const char *name, u8 *ptr,
 			 int len)
 {
@@ -371,13 +350,13 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
 }
 
 /**
- * do_cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
+ * cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
  *
  * @ec_dev: ChromeOS EC device
  * @ec_msg: Message to transfer
  */
-static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
-				   struct cros_ec_command *ec_msg)
+static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
+				struct cros_ec_command *ec_msg)
 {
 	struct ec_host_response *response;
 	struct cros_ec_spi *ec_spi = ec_dev->priv;
@@ -514,13 +493,13 @@ static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
 }
 
 /**
- * do_cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
+ * cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
  *
  * @ec_dev: ChromeOS EC device
  * @ec_msg: Message to transfer
  */
-static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
-				   struct cros_ec_command *ec_msg)
+static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
+				struct cros_ec_command *ec_msg)
 {
 	struct cros_ec_spi *ec_spi = ec_dev->priv;
 	struct spi_transfer trans;
@@ -632,53 +611,6 @@ static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	return ret;
 }
 
-static void cros_ec_xfer_high_pri_work(struct work_struct *work)
-{
-	struct cros_ec_xfer_work_params *params;
-
-	params = container_of(work, struct cros_ec_xfer_work_params, work);
-	params->ret = params->fn(params->ec_dev, params->ec_msg);
-}
-
-static int cros_ec_xfer_high_pri(struct cros_ec_device *ec_dev,
-				 struct cros_ec_command *ec_msg,
-				 cros_ec_xfer_fn_t fn)
-{
-	struct cros_ec_xfer_work_params params;
-
-	INIT_WORK_ONSTACK(&params.work, cros_ec_xfer_high_pri_work);
-	params.ec_dev = ec_dev;
-	params.ec_msg = ec_msg;
-	params.fn = fn;
-
-	/*
-	 * This looks a bit ridiculous.  Why do the work on a
-	 * different thread if we're just going to block waiting for
-	 * the thread to finish?  The key here is that the thread is
-	 * running at high priority but the calling context might not
-	 * be.  We need to be at high priority to avoid getting
-	 * context switched out for too long and the EC giving up on
-	 * the transfer.
-	 */
-	queue_work(system_highpri_wq, &params.work);
-	flush_work(&params.work);
-	destroy_work_on_stack(&params.work);
-
-	return params.ret;
-}
-
-static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
-				struct cros_ec_command *ec_msg)
-{
-	return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_pkt_xfer_spi);
-}
-
-static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
-				struct cros_ec_command *ec_msg)
-{
-	return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
-}
-
 static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
 {
 	struct device_node *np = dev->of_node;
-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Douglas Anderson]

This reverts commit 37a186225a0c020516bafad2727fdcdfc039a1e4.

We have a better solution in the patch ("platform/chrome: cros_ec_spi:
Set ourselves as timing sensitive").  Let's revert the uglier and less
reliable solution.

Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

 drivers/platform/chrome/cros_ec_spi.c | 80 ++-------------------------
 1 file changed, 6 insertions(+), 74 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 757a115502ec..70ff1ad09012 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -75,27 +75,6 @@ struct cros_ec_spi {
 	unsigned int end_of_msg_delay;
 };
 
-typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
-				  struct cros_ec_command *ec_msg);
-
-/**
- * struct cros_ec_xfer_work_params - params for our high priority workers
- *
- * @work: The work_struct needed to queue work
- * @fn: The function to use to transfer
- * @ec_dev: ChromeOS EC device
- * @ec_msg: Message to transfer
- * @ret: The return value of the function
- */
-
-struct cros_ec_xfer_work_params {
-	struct work_struct work;
-	cros_ec_xfer_fn_t fn;
-	struct cros_ec_device *ec_dev;
-	struct cros_ec_command *ec_msg;
-	int ret;
-};
-
 static void debug_packet(struct device *dev, const char *name, u8 *ptr,
 			 int len)
 {
@@ -371,13 +350,13 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
 }
 
 /**
- * do_cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
+ * cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
  *
  * @ec_dev: ChromeOS EC device
  * @ec_msg: Message to transfer
  */
-static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
-				   struct cros_ec_command *ec_msg)
+static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
+				struct cros_ec_command *ec_msg)
 {
 	struct ec_host_response *response;
 	struct cros_ec_spi *ec_spi = ec_dev->priv;
@@ -514,13 +493,13 @@ static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
 }
 
 /**
- * do_cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
+ * cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
  *
  * @ec_dev: ChromeOS EC device
  * @ec_msg: Message to transfer
  */
-static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
-				   struct cros_ec_command *ec_msg)
+static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
+				struct cros_ec_command *ec_msg)
 {
 	struct cros_ec_spi *ec_spi = ec_dev->priv;
 	struct spi_transfer trans;
@@ -632,53 +611,6 @@ static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	return ret;
 }
 
-static void cros_ec_xfer_high_pri_work(struct work_struct *work)
-{
-	struct cros_ec_xfer_work_params *params;
-
-	params = container_of(work, struct cros_ec_xfer_work_params, work);
-	params->ret = params->fn(params->ec_dev, params->ec_msg);
-}
-
-static int cros_ec_xfer_high_pri(struct cros_ec_device *ec_dev,
-				 struct cros_ec_command *ec_msg,
-				 cros_ec_xfer_fn_t fn)
-{
-	struct cros_ec_xfer_work_params params;
-
-	INIT_WORK_ONSTACK(&params.work, cros_ec_xfer_high_pri_work);
-	params.ec_dev = ec_dev;
-	params.ec_msg = ec_msg;
-	params.fn = fn;
-
-	/*
-	 * This looks a bit ridiculous.  Why do the work on a
-	 * different thread if we're just going to block waiting for
-	 * the thread to finish?  The key here is that the thread is
-	 * running at high priority but the calling context might not
-	 * be.  We need to be at high priority to avoid getting
-	 * context switched out for too long and the EC giving up on
-	 * the transfer.
-	 */
-	queue_work(system_highpri_wq, &params.work);
-	flush_work(&params.work);
-	destroy_work_on_stack(&params.work);
-
-	return params.ret;
-}
-
-static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
-				struct cros_ec_command *ec_msg)
-{
-	return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_pkt_xfer_spi);
-}
-
-static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
-				struct cros_ec_command *ec_msg)
-{
-	return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
-}
-
 static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
 {
 	struct device_node *np = dev->of_node;
-- 
2.21.0.1020.gf2820cf01a-goog

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Guenter Roeck]

From: Douglas Anderson <dianders@chromium.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip@lists.infradead.org>, <drinkcat@chromium.org>,
Guenter Roeck, <briannorris@chromium.org>, <mka@chromium.org>, Douglas
Anderson, <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>

> If a controller specifies that it needs high priority for sending
> messages we should always schedule our transfers on the thread.  If we
> don't do this we'll do the transfer in the caller's context which
> might not be very high priority.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
>  drivers/spi/spi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8eb7460dd744..0597f7086de3 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1230,8 +1230,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>                 return;
>         }
>
> -       /* If another context is idling the device then defer */
> -       if (ctlr->idling) {
> +       /*
> +        * If another context is idling the device then defer.
> +        * If we are high priority then the thread should do the transfer.
> +        */
> +       if (ctlr->idling || (ctlr->rt && !in_kthread)) {
>                 kthread_queue_work(&ctlr->kworker, &ctlr->pump_messages);
>                 spin_unlock_irqrestore(&ctlr->queue_lock, flags);
>                 return;
> --
> 2.21.0.1020.gf2820cf01a-goog
>

Re: [PATCH 2/4] spi: Allow SPI devices to specify that they are timing sensitive

[Guenter Roeck]

From: Douglas Anderson <dianders@chromium.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip@lists.infradead.org>, <drinkcat@chromium.org>,
Guenter Roeck, <briannorris@chromium.org>, <mka@chromium.org>, Douglas
Anderson, <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>

> If a device on the SPI bus is very sensitive to timing then it may be
> necessary (for correctness) not to get interrupted during a transfer.
> One example is the EC (Embedded Controller) on Chromebooks.  The
> Chrome OS EC will drop a transfer if more than ~8ms passes between the
> chip select being asserted and the transfer finishing.
>
> The SPI framework already has code to handle the case where transfers
> are timing senstive.  It can set its message pumping thread to
> realtime to to minimize interruptions during the transfer.  However,
> at the moment, this mode can only be requested by a SPI controller.
> Let's allow the drivers for SPI devices to also request this mode.
>
> NOTE: at the moment if a given device on a bus says that it's timing
> sensitive then we'll pump all messages on that bus at high priority.
> It is possible we might want to relax this in the future but it seems
> like it should be fine for now.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Nitpick: I would use 'rt' instead of 'timing_sensitive' as name for the
new variable.

Otherwise:

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
>  drivers/spi/spi.c       | 34 ++++++++++++++++++++++++++++------
>  include/linux/spi/spi.h |  3 +++
>  2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 0597f7086de3..d117ab3adafa 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1367,10 +1367,30 @@ static void spi_pump_messages(struct kthread_work *work)
>         __spi_pump_messages(ctlr, true);
>  }
>
> -static int spi_init_queue(struct spi_controller *ctlr)
> +/**
> + * spi_boost_thread_priority - set the controller to pump at realtime priority
> + * @ctlr: controller to boost priority of
> + *
> + * This can be called because the controller requested realtime priority
> + * (by setting the ->rt value before calling spi_register_controller()) or
> + * because a device on the bus said that its transfers were timing senstive.
> + *
> + * NOTE: at the moment if any device on a bus says it is timing sensitive then
> + * all the devices on this bus will do transfers at realtime priority.  If
> + * this eventually becomes a problem we may see if we can find a way to boost
> + * the priority only temporarily during relevant transfers.
> + */
> +static void spi_boost_thread_priority(struct spi_controller *ctlr)
>  {
>         struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
>
> +       dev_info(&ctlr->dev,
> +               "will run message pump with realtime priority\n");
> +       sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> +}
> +
> +static int spi_init_queue(struct spi_controller *ctlr)
> +{
>         ctlr->running = false;
>         ctlr->busy = false;
>
> @@ -1390,11 +1410,8 @@ static int spi_init_queue(struct spi_controller *ctlr)
>          * request and the scheduling of the message pump thread. Without this
>          * setting the message pump thread will remain at default priority.
>          */
> -       if (ctlr->rt) {
> -               dev_info(&ctlr->dev,
> -                       "will run message pump with realtime priority\n");
> -               sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> -       }
> +       if (ctlr->rt)
> +               spi_boost_thread_priority(ctlr);
>
>         return 0;
>  }
> @@ -2985,6 +3002,11 @@ int spi_setup(struct spi_device *spi)
>
>         spi_set_cs(spi, false);
>
> +       if (spi->timing_sensitive && !spi->controller->rt) {
> +               spi->controller->rt = true;
> +               spi_boost_thread_priority(spi->controller);
> +       }
> +
>         dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
>                         (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>                         (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 053abd22ad31..ef6bdd4d25f2 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -109,6 +109,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
>   *     This may be changed by the device's driver, or left at the
>   *     default (0) indicating protocol words are eight bit bytes.
>   *     The spi_transfer.bits_per_word can override this for each transfer.
> + * @timing_sensitive: Transfers for this device are senstive to timing
> + *     so we should do our transfer at high priority.
>   * @irq: Negative, or the number passed to request_irq() to receive
>   *     interrupts from this device.
>   * @controller_state: Controller's runtime state
> @@ -143,6 +145,7 @@ struct spi_device {
>         u32                     max_speed_hz;
>         u8                      chip_select;
>         u8                      bits_per_word;
> +       bool                    timing_sensitive;
>         u32                     mode;
>  #define        SPI_CPHA        0x01                    /* clock phase */
>  #define        SPI_CPOL        0x02                    /* clock polarity */
> --
> 2.21.0.1020.gf2820cf01a-goog
>

Re: [PATCH 2/4] spi: Allow SPI devices to specify that they are timing sensitive

[Guenter Roeck]

From: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, <drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
Guenter Roeck, <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Douglas
Anderson, <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, <linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

> If a device on the SPI bus is very sensitive to timing then it may be
> necessary (for correctness) not to get interrupted during a transfer.
> One example is the EC (Embedded Controller) on Chromebooks.  The
> Chrome OS EC will drop a transfer if more than ~8ms passes between the
> chip select being asserted and the transfer finishing.
>
> The SPI framework already has code to handle the case where transfers
> are timing senstive.  It can set its message pumping thread to
> realtime to to minimize interruptions during the transfer.  However,
> at the moment, this mode can only be requested by a SPI controller.
> Let's allow the drivers for SPI devices to also request this mode.
>
> NOTE: at the moment if a given device on a bus says that it's timing
> sensitive then we'll pump all messages on that bus at high priority.
> It is possible we might want to relax this in the future but it seems
> like it should be fine for now.
>
> Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Nitpick: I would use 'rt' instead of 'timing_sensitive' as name for the
new variable.

Otherwise:

Reviewed-by: Guenter Roeck <groeck-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

> ---
>
>  drivers/spi/spi.c       | 34 ++++++++++++++++++++++++++++------
>  include/linux/spi/spi.h |  3 +++
>  2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 0597f7086de3..d117ab3adafa 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1367,10 +1367,30 @@ static void spi_pump_messages(struct kthread_work *work)
>         __spi_pump_messages(ctlr, true);
>  }
>
> -static int spi_init_queue(struct spi_controller *ctlr)
> +/**
> + * spi_boost_thread_priority - set the controller to pump at realtime priority
> + * @ctlr: controller to boost priority of
> + *
> + * This can be called because the controller requested realtime priority
> + * (by setting the ->rt value before calling spi_register_controller()) or
> + * because a device on the bus said that its transfers were timing senstive.
> + *
> + * NOTE: at the moment if any device on a bus says it is timing sensitive then
> + * all the devices on this bus will do transfers at realtime priority.  If
> + * this eventually becomes a problem we may see if we can find a way to boost
> + * the priority only temporarily during relevant transfers.
> + */
> +static void spi_boost_thread_priority(struct spi_controller *ctlr)
>  {
>         struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
>
> +       dev_info(&ctlr->dev,
> +               "will run message pump with realtime priority\n");
> +       sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> +}
> +
> +static int spi_init_queue(struct spi_controller *ctlr)
> +{
>         ctlr->running = false;
>         ctlr->busy = false;
>
> @@ -1390,11 +1410,8 @@ static int spi_init_queue(struct spi_controller *ctlr)
>          * request and the scheduling of the message pump thread. Without this
>          * setting the message pump thread will remain at default priority.
>          */
> -       if (ctlr->rt) {
> -               dev_info(&ctlr->dev,
> -                       "will run message pump with realtime priority\n");
> -               sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> -       }
> +       if (ctlr->rt)
> +               spi_boost_thread_priority(ctlr);
>
>         return 0;
>  }
> @@ -2985,6 +3002,11 @@ int spi_setup(struct spi_device *spi)
>
>         spi_set_cs(spi, false);
>
> +       if (spi->timing_sensitive && !spi->controller->rt) {
> +               spi->controller->rt = true;
> +               spi_boost_thread_priority(spi->controller);
> +       }
> +
>         dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
>                         (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>                         (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 053abd22ad31..ef6bdd4d25f2 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -109,6 +109,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
>   *     This may be changed by the device's driver, or left at the
>   *     default (0) indicating protocol words are eight bit bytes.
>   *     The spi_transfer.bits_per_word can override this for each transfer.
> + * @timing_sensitive: Transfers for this device are senstive to timing
> + *     so we should do our transfer at high priority.
>   * @irq: Negative, or the number passed to request_irq() to receive
>   *     interrupts from this device.
>   * @controller_state: Controller's runtime state
> @@ -143,6 +145,7 @@ struct spi_device {
>         u32                     max_speed_hz;
>         u8                      chip_select;
>         u8                      bits_per_word;
> +       bool                    timing_sensitive;
>         u32                     mode;
>  #define        SPI_CPHA        0x01                    /* clock phase */
>  #define        SPI_CPOL        0x02                    /* clock polarity */
> --
> 2.21.0.1020.gf2820cf01a-goog
>

Re: [PATCH 3/4] platform/chrome: cros_ec_spi: Set ourselves as timing sensitive

[Guenter Roeck]

From: Douglas Anderson <dianders@chromium.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip@lists.infradead.org>, <drinkcat@chromium.org>,
Guenter Roeck, <briannorris@chromium.org>, <mka@chromium.org>, Douglas
Anderson, <linux-kernel@vger.kernel.org>

> All currently known ECs in the wild are very sensitive to timing.
> Specifically the ECs are known to drop a transfer if more than 8 ms
> passes from the assertion of the chip select until the transfer
> finishes.
>
> Let's use the new feature introduced in the patch ("spi: Allow SPI
> devices to specify that they are timing sensitive") to specify this
> and increase the success rate of our transfers.
>
> NOTE: if future Chrome OS ECs ever fix themselves to be less sensitive
> then we could consider adding a property (or compatible string) to not
> set this property.  For now we need it across the board.
>
> With this change we can revert the commit 37a186225a0c
> ("platform/chrome: cros_ec_spi: Transfer messages at high priority").
> ...and, in fact, transfers are _even more_ reliable than they were
> with that commit since the SPI framework will use a higher priority
> (realtime) and we no longer lose our priority when we get shunted over
> to the message pumping thread (because we now always get shunted and
> the thread is high priority).
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
>  drivers/platform/chrome/cros_ec_spi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 8e9451720e73..757a115502ec 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -703,6 +703,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
>
>         spi->bits_per_word = 8;
>         spi->mode = SPI_MODE_0;
> +       spi->timing_sensitive = true;
>         err = spi_setup(spi);
>         if (err < 0)
>                 return err;
> --
> 2.21.0.1020.gf2820cf01a-goog
>

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Guenter Roeck]

From: Douglas Anderson <dianders@chromium.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip@lists.infradead.org>, <drinkcat@chromium.org>,
Guenter Roeck, <briannorris@chromium.org>, <mka@chromium.org>, Douglas
Anderson, <linux-kernel@vger.kernel.org>

> This reverts commit 37a186225a0c020516bafad2727fdcdfc039a1e4.
>
> We have a better solution in the patch ("platform/chrome: cros_ec_spi:
> Set ourselves as timing sensitive").  Let's revert the uglier and less
> reliable solution.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
>  drivers/platform/chrome/cros_ec_spi.c | 80 ++-------------------------
>  1 file changed, 6 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 757a115502ec..70ff1ad09012 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -75,27 +75,6 @@ struct cros_ec_spi {
>         unsigned int end_of_msg_delay;
>  };
>
> -typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
> -                                 struct cros_ec_command *ec_msg);
> -
> -/**
> - * struct cros_ec_xfer_work_params - params for our high priority workers
> - *
> - * @work: The work_struct needed to queue work
> - * @fn: The function to use to transfer
> - * @ec_dev: ChromeOS EC device
> - * @ec_msg: Message to transfer
> - * @ret: The return value of the function
> - */
> -
> -struct cros_ec_xfer_work_params {
> -       struct work_struct work;
> -       cros_ec_xfer_fn_t fn;
> -       struct cros_ec_device *ec_dev;
> -       struct cros_ec_command *ec_msg;
> -       int ret;
> -};
> -
>  static void debug_packet(struct device *dev, const char *name, u8 *ptr,
>                          int len)
>  {
> @@ -371,13 +350,13 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
>  }
>
>  /**
> - * do_cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
> + * cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
>   *
>   * @ec_dev: ChromeOS EC device
>   * @ec_msg: Message to transfer
>   */
> -static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
> -                                  struct cros_ec_command *ec_msg)
> +static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
> +                               struct cros_ec_command *ec_msg)
>  {
>         struct ec_host_response *response;
>         struct cros_ec_spi *ec_spi = ec_dev->priv;
> @@ -514,13 +493,13 @@ static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
>  }
>
>  /**
> - * do_cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
> + * cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
>   *
>   * @ec_dev: ChromeOS EC device
>   * @ec_msg: Message to transfer
>   */
> -static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> -                                  struct cros_ec_command *ec_msg)
> +static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> +                               struct cros_ec_command *ec_msg)
>  {
>         struct cros_ec_spi *ec_spi = ec_dev->priv;
>         struct spi_transfer trans;
> @@ -632,53 +611,6 @@ static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>         return ret;
>  }
>
> -static void cros_ec_xfer_high_pri_work(struct work_struct *work)
> -{
> -       struct cros_ec_xfer_work_params *params;
> -
> -       params = container_of(work, struct cros_ec_xfer_work_params, work);
> -       params->ret = params->fn(params->ec_dev, params->ec_msg);
> -}
> -
> -static int cros_ec_xfer_high_pri(struct cros_ec_device *ec_dev,
> -                                struct cros_ec_command *ec_msg,
> -                                cros_ec_xfer_fn_t fn)
> -{
> -       struct cros_ec_xfer_work_params params;
> -
> -       INIT_WORK_ONSTACK(&params.work, cros_ec_xfer_high_pri_work);
> -       params.ec_dev = ec_dev;
> -       params.ec_msg = ec_msg;
> -       params.fn = fn;
> -
> -       /*
> -        * This looks a bit ridiculous.  Why do the work on a
> -        * different thread if we're just going to block waiting for
> -        * the thread to finish?  The key here is that the thread is
> -        * running at high priority but the calling context might not
> -        * be.  We need to be at high priority to avoid getting
> -        * context switched out for too long and the EC giving up on
> -        * the transfer.
> -        */
> -       queue_work(system_highpri_wq, &params.work);
> -       flush_work(&params.work);
> -       destroy_work_on_stack(&params.work);
> -
> -       return params.ret;
> -}
> -
> -static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
> -                               struct cros_ec_command *ec_msg)
> -{
> -       return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_pkt_xfer_spi);
> -}
> -
> -static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> -                               struct cros_ec_command *ec_msg)
> -{
> -       return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
> -}
> -
>  static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
>  {
>         struct device_node *np = dev->of_node;
> --
> 2.21.0.1020.gf2820cf01a-goog
>

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Mark Brown]

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

On Fri, May 10, 2019 at 03:34:34PM -0700, Douglas Anderson wrote:
> If a controller specifies that it needs high priority for sending
> messages we should always schedule our transfers on the thread.  If we
> don't do this we'll do the transfer in the caller's context which
> might not be very high priority.

If performance is important you probably also want to avoid the context
thrashing - executing in the calling context is generally a substantial
performance boost.  I can see this causing problems further down the
line when someone else turns up with a different requirement, perhaps in
an application where the caller does actually have a raised priority
themselves and just wanted to make sure that the thread wasn't lower
than they are.  I guess it'd be nice if we could check what priority the
calling thread has and make a decision based on that but there don't
seem to be any facilities for doing that which I can see right now.

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

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 945 bytes --]

On Fri, May 10, 2019 at 03:34:34PM -0700, Douglas Anderson wrote:
> If a controller specifies that it needs high priority for sending
> messages we should always schedule our transfers on the thread.  If we
> don't do this we'll do the transfer in the caller's context which
> might not be very high priority.

If performance is important you probably also want to avoid the context
thrashing - executing in the calling context is generally a substantial
performance boost.  I can see this causing problems further down the
line when someone else turns up with a different requirement, perhaps in
an application where the caller does actually have a raised priority
themselves and just wanted to make sure that the thread wasn't lower
than they are.  I guess it'd be nice if we could check what priority the
calling thread has and make a decision based on that but there don't
seem to be any facilities for doing that which I can see right now.

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/4] spi: Allow SPI devices to specify that they are timing sensitive

[Mark Brown]

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

On Fri, May 10, 2019 at 03:34:35PM -0700, Douglas Anderson wrote:

> + * @timing_sensitive: Transfers for this device are senstive to timing
> + *	so we should do our transfer at high priority.

Not sure this is the best name.  Every device is timing sensitive to
some extent and it's a bit wooly about what it's trying to accomplish,
it's specifically about the timing involved in ensuring that the entire
message goes out as quickly as possible AIUI.  I think if anything I'd
just have the caller specifying a RT priority for the thread, but that's
awkward as we might want to switch over to deadline at some point.  How
about just calling the parameter rt the same as it is when the
controller does the same configuration?

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

Re: [PATCH 2/4] spi: Allow SPI devices to specify that they are timing sensitive

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 726 bytes --]

On Fri, May 10, 2019 at 03:34:35PM -0700, Douglas Anderson wrote:

> + * @timing_sensitive: Transfers for this device are senstive to timing
> + *	so we should do our transfer at high priority.

Not sure this is the best name.  Every device is timing sensitive to
some extent and it's a bit wooly about what it's trying to accomplish,
it's specifically about the timing involved in ensuring that the entire
message goes out as quickly as possible AIUI.  I think if anything I'd
just have the caller specifying a RT priority for the thread, but that's
awkward as we might want to switch over to deadline at some point.  How
about just calling the parameter rt the same as it is when the
controller does the same configuration?

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Mark Brown]

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

On Fri, May 10, 2019 at 03:34:37PM -0700, Douglas Anderson wrote:
> This reverts commit 37a186225a0c020516bafad2727fdcdfc039a1e4.
> 
> We have a better solution in the patch ("platform/chrome: cros_ec_spi:
> Set ourselves as timing sensitive").  Let's revert the uglier and less
> reliable solution.

It isn't clear to me that it's a bad thing to have this even with the
SPI thread at realime priority.

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

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 412 bytes --]

On Fri, May 10, 2019 at 03:34:37PM -0700, Douglas Anderson wrote:
> This reverts commit 37a186225a0c020516bafad2727fdcdfc039a1e4.
> 
> We have a better solution in the patch ("platform/chrome: cros_ec_spi:
> Set ourselves as timing sensitive").  Let's revert the uglier and less
> reliable solution.

It isn't clear to me that it's a bad thing to have this even with the
SPI thread at realime priority.

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

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Doug Anderson]

Hi,

On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, May 10, 2019 at 03:34:37PM -0700, Douglas Anderson wrote:
> > This reverts commit 37a186225a0c020516bafad2727fdcdfc039a1e4.
> >
> > We have a better solution in the patch ("platform/chrome: cros_ec_spi:
> > Set ourselves as timing sensitive").  Let's revert the uglier and less
> > reliable solution.
>
> It isn't clear to me that it's a bad thing to have this even with the
> SPI thread at realime priority.

The code that's there right now isn't enough.  As per the description
in the original patch, it didn't solve all problems but just made
things an order of magnitude better.  So if I don't do this revert I
instead need a patch to bump cros_ec SPI up to realtime to get SPI
transfers _truly_ reliable.  I actually have a patch coded up to do
just that.  ...but then Guenter pointed out that I was effectively
duplicating the work that the SPI framework could already do for me if
I could use the pumping thread at real time priority.

My current plan is parameterize things so that cros_ec_spi can request
a forced transition to the realtime pump thread without breaking
existing users.  I'll code that up this morning and send out a v2 soon
so you can see what you think of it.  :-)

NOTE: I actually tracked down one reason why the high priority thread
wasn't enough and I needed something like real time.  I found that
commit a1b89132dc4f ("dm crypt: use WQ_HIGHPRI for the IO and crypt
workqueues") was making dm-crypt preempt me.  I'll start a separate
discussion about that, but in the end it still seems better to use
something like a real time priority for cros_ec.


-Doug

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Mark Brown]

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

On Mon, May 13, 2019 at 08:57:12AM -0700, Doug Anderson wrote:
> On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:

> > It isn't clear to me that it's a bad thing to have this even with the
> > SPI thread at realime priority.

> The code that's there right now isn't enough.  As per the description
> in the original patch, it didn't solve all problems but just made
> things an order of magnitude better.  So if I don't do this revert I

I'm not saying the other changes aren't helping, I'm saying that it's
not clear that this revert is improving things.

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

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Doug Anderson]

Hi,

On Mon, May 13, 2019 at 9:02 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, May 13, 2019 at 08:57:12AM -0700, Doug Anderson wrote:
> > On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > It isn't clear to me that it's a bad thing to have this even with the
> > > SPI thread at realime priority.
>
> > The code that's there right now isn't enough.  As per the description
> > in the original patch, it didn't solve all problems but just made
> > things an order of magnitude better.  So if I don't do this revert I
>
> I'm not saying the other changes aren't helping, I'm saying that it's
> not clear that this revert is improving things.

If I add a call to force the pumping to happen on the SPI thread then
the commit I'm reverting here is useless though, isn't it?

-Doug

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Mark Brown]

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

On Mon, May 13, 2019 at 09:03:28AM -0700, Doug Anderson wrote:
> On Mon, May 13, 2019 at 9:02 AM Mark Brown <broonie@kernel.org> wrote:

> > I'm not saying the other changes aren't helping, I'm saying that it's
> > not clear that this revert is improving things.

> If I add a call to force the pumping to happen on the SPI thread then
> the commit I'm reverting here is useless though, isn't it?

Well, I'm not convinced that that change is ideal anyway and it does
leave you vulnerable to further changes in the SPI core pushing things
out to calling context which feels like it isn't going to be helping
robustness.

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

Re: [PATCH 4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"

[Doug Anderson]

Hi,

On Mon, May 13, 2019 at 9:47 AM, Mark Brown <broonie@kernel.org> wrote:

> On Mon, May 13, 2019 at 09:03:28AM -0700, Doug Anderson wrote:
> > On Mon, May 13, 2019 at 9:02 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > I'm not saying the other changes aren't helping, I'm saying that it's
> > > not clear that this revert is improving things.
>
> > If I add a call to force the pumping to happen on the SPI thread then
> > the commit I'm reverting here is useless though, isn't it?
>
> Well, I'm not convinced that that change is ideal anyway and it does
> leave you vulnerable to further changes in the SPI core pushing things
> out to calling context which feels like it isn't going to be helping
> robustness.

OK.  Here's my plan: in v2 I've still included this revert and you can
see how things look.  If you hate it as much as you think you will
then let me know and I'll send a v3 that avoids to forcing and re-adds
the realtime thread to cros_ec.

One note just so you're aware: For my particular device I'm not nearly
as concerned with latency / throughput as I am concerned with
transfers not getting interrupted once started.  I've added this
explicitly in the commit message now, too.  :-)


-Doug

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Doug Anderson]

Hi,

On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:

> On Fri, May 10, 2019 at 03:34:34PM -0700, Douglas Anderson wrote:
> > If a controller specifies that it needs high priority for sending
> > messages we should always schedule our transfers on the thread.  If we
> > don't do this we'll do the transfer in the caller's context which
> > might not be very high priority.
>
> If performance is important you probably also want to avoid the context
> thrashing - executing in the calling context is generally a substantial
> performance boost.  I can see this causing problems further down the
> line when someone else turns up with a different requirement, perhaps in
> an application where the caller does actually have a raised priority
> themselves and just wanted to make sure that the thread wasn't lower
> than they are.  I guess it'd be nice if we could check what priority the
> calling thread has and make a decision based on that but there don't
> seem to be any facilities for doing that which I can see right now.

In my case performance is 2nd place to a transfer not getting
interrupted once started (so we don't break the 8ms rule of the EC).
My solution in v2 of my series is to take out the forcing in the case
that the controller wanted "rt" priority and then to add "force" to
the parameter name.  If someone wants rt priority for the thread but
doesn't want to force all transfers to the thread we can later add a
different parameter for that?

-Doug

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Mark Brown]

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

On Mon, May 13, 2019 at 01:24:57PM -0700, Doug Anderson wrote:
> On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:

> > If performance is important you probably also want to avoid the context
> > thrashing - executing in the calling context is generally a substantial
> > performance boost.  I can see this causing problems further down the
> > line when someone else turns up with a different requirement, perhaps in
> > an application where the caller does actually have a raised priority
> > themselves and just wanted to make sure that the thread wasn't lower
> > than they are.  I guess it'd be nice if we could check what priority the
> > calling thread has and make a decision based on that but there don't
> > seem to be any facilities for doing that which I can see right now.

> In my case performance is 2nd place to a transfer not getting
> interrupted once started (so we don't break the 8ms rule of the EC).

That's great but other users do care very much about performance and are
also interested in both priority control and avoiding context thrashing.

> My solution in v2 of my series is to take out the forcing in the case
> that the controller wanted "rt" priority and then to add "force" to
> the parameter name.  If someone wants rt priority for the thread but
> doesn't want to force all transfers to the thread we can later add a
> different parameter for that?

I think that's going to be the common case for this.  Forcing context
thrashing is really not something anyone else is asking for.

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

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Doug Anderson]

Hi,

On Tue, May 14, 2019 at 2:30 AM Mark Brown <broonie@kernel.org> wrote:

> On Mon, May 13, 2019 at 01:24:57PM -0700, Doug Anderson wrote:
> > On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:
>
> > In my case performance is 2nd place to a transfer not getting
> > interrupted once started (so we don't break the 8ms rule of the EC).
>
> That's great but other users do care very much about performance and are
> also interested in both priority control and avoiding context thrashing.
>
> > My solution in v2 of my series is to take out the forcing in the case
> > that the controller wanted "rt" priority and then to add "force" to
> > the parameter name.  If someone wants rt priority for the thread but
> > doesn't want to force all transfers to the thread we can later add a
> > different parameter for that?
>
> I think that's going to be the common case for this.  Forcing context
> thrashing is really not something anyone else is asking for.

OK, that's fair.  Even if nobody else is asking for it, the solution
I've coded up for v2 still allows cros_ec to use the SPI core's thread
in a pretty clean way and saves a bunch of code in cros_ec.  It
shouldn't penalize any other SPI users.

...but I guess you're saying that you don't want to guarantee that the
SPI core will happen to have this thread sitting around in the future
so you'd rather add the extra complexity to cros_ec so the core can
evolve more freely?

-Doug

Re: [PATCH 1/4] spi: For controllers that need realtime always use the pump thread

[Mark Brown]

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

On Tue, May 14, 2019 at 07:42:38AM -0700, Doug Anderson wrote:

> ...but I guess you're saying that you don't want to guarantee that the
> SPI core will happen to have this thread sitting around in the future
> so you'd rather add the extra complexity to cros_ec so the core can
> evolve more freely?

We need something to support spi_async() but what you're asking for is
fairly specific implementation details about how things are currently
structured, and we do need to be able to continue to make improvements
for users who are interested in performance.  Ensuring that the calling
context is also less likely to be preempted is going to make it much
less likely that any other work is going to cause some timing change
that creates problems for you.

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