[4/4] Revert "platform/chrome: cros_ec_spi: Transfer messages at high priority"
diff mbox series

Message ID 20190510223437.84368-5-dianders@chromium.org
State New
Headers show
Series
  • spi: A better solution for cros_ec_spi reliability
Related show

Commit Message

Doug Anderson May 10, 2019, 10:34 p.m. UTC
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(-)

Comments

Guenter Roeck May 11, 2019, 12:32 a.m. UTC | #1
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
>
Mark Brown May 12, 2019, 7:45 a.m. UTC | #2
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.
Doug Anderson May 13, 2019, 3:57 p.m. UTC | #3
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
Mark Brown May 13, 2019, 4:01 p.m. UTC | #4
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.
Doug Anderson May 13, 2019, 4:03 p.m. UTC | #5
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
Mark Brown May 13, 2019, 4:47 p.m. UTC | #6
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.
Doug Anderson May 13, 2019, 8:21 p.m. UTC | #7
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

Patch
diff mbox series

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;