[PATCH linux dev-4.13 00/16] Locking fixes for FSI, SBEFIFO, OCC

[PATCH linux dev-4.13 00/16] Locking fixes for FSI, SBEFIFO, OCC

[Andrew Jeffery]

Hello,

This is a forward-port to dev-4.13 of my dev-4.10 series aimed at fixing the
locking and work scheduling for the OCC stack:

Cover Letter: https://lists.ozlabs.org/pipermail/openbmc/2018-February/010793.html
Patch Series: http://patchwork.ozlabs.org/project/openbmc/list/?series=28746&state=%2A&archive=both

The dev-4.13 series adds several new patches:

0001-dts-fsi-Make-OCC-description-match-expected-hierarch.patch

dev-4.13 as it stood failed to get the OCC driver stack up and running due to
broken descriptions in the devicetree with respect to the drivers'
implementations. Patch 1 fixes this by making the devicetree reflect what the
drivers expect, but the fact remains that this is an abuse of the devicetree.
We need to have a discussion about how best to instantiate the hwmon driver and
fix what we're doing here.

0002-hwmon-p9_sbe-Initialise-device-spin-lock.patch

Patch 2 fixes what appears to be an oversight in lock initialisation in the
p9_sbe OCC hwmon implementation.

0011-hwmon-p9_sbe-Add-static-key-to-satisfy-lockdep.patch

Finally, patch 11 shuts up lockdep in the p9_sbe OCC hwmon implementation,
which was whinging due to dynamic mutex allocation. Kernel locking experts
should feel free to chime in as I don't know that this is actually the right
fix.

Beyond that, the series has been lightly tested on a Witherspoon system and
nothing was obviously broken.

Please review!

Cheers,

Andrew

Andrew Jeffery (12):
  dts: fsi: Make OCC description match expected hierarchy
  hwmon (p9_sbe): Initialise device spin lock
  fsi: sbefifo: Rename sbefifo_release_client() for consistency
  fsi: sbefifo: Add tracing of transfers
  fsi: gpio: Trace busy count
  fsi: occ: Add tracepoints
  hwmon (p9_sbe): Rename context variable
  hwmon (p9_sbe): Rename lock member of struct p9_sbe_occ
  hwmon (p9_sbe): Convert client_lock from a spinlock to a mutex
  hwmon (p9_sbe): Add static key to satisfy lockdep
  fsi: sbefifo: Switch list_lock from spinlock to mutex
  fsi: gpio: Remove unused 'id' variable

Eddie James (3):
  fsi: sbefifo: don't delete canceled xfrs in write
  fsi: sbefifo: Avoid using a timer to drive FIFO transfers
  fsi: sbefifo: Switch to mutex in work function

Jeremy Kerr (1):
  fsi: gpio: Use a mutex to protect transfers

 arch/arm/boot/dts/ibm-power9-cfam.dtsi |  12 +++-
 drivers/fsi/fsi-master-gpio.c          |  94 +++++++++++++++++++-------
 drivers/fsi/fsi-occ.c                  |   9 +++
 drivers/fsi/fsi-sbefifo.c              | 119 +++++++++++++++++++++++----------
 drivers/hwmon/occ/p9_sbe.c             |  54 ++++++++-------
 include/trace/events/fsi_master_gpio.h |  16 +++++
 include/trace/events/fsi_occ.h         |  86 ++++++++++++++++++++++++
 include/trace/events/sbefifo.h         |  93 ++++++++++++++++++++++++++
 8 files changed, 394 insertions(+), 89 deletions(-)
 create mode 100644 include/trace/events/fsi_occ.h
 create mode 100644 include/trace/events/sbefifo.h

-- 
2.14.1

[PATCH linux dev-4.13 01/16] dts: fsi: Make OCC description match expected hierarchy

[Andrew Jeffery]

ibm,p9-occ-hwmon describes a driver, not hardware, but this compatible is
what's required to get the driver to probe.

While we discuss how to resolve that problem, lets at least make the current
configuration work.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/ibm-power9-cfam.dtsi | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/ibm-power9-cfam.dtsi b/arch/arm/boot/dts/ibm-power9-cfam.dtsi
index e0e738f9e539..215cc8182ca5 100644
--- a/arch/arm/boot/dts/ibm-power9-cfam.dtsi
+++ b/arch/arm/boot/dts/ibm-power9-cfam.dtsi
@@ -113,8 +113,12 @@
 			#size-cells = <0>;
 
 			occ@1 {
-				compatible = "ibm,p9-occ-hwmon";
+				compatible = "ibm,p9-occ";
 				reg = <1>;
+
+				occ-hwmon@1 {
+					compatible = "ibm,p9-occ-hwmon";
+				};
 			};
 		};
 
@@ -205,8 +209,12 @@
 					#size-cells = <0>;
 
 					occ@2 {
-						compatible = "ibm,p9-occ-hwmon";
+						compatible = "ibm,p9-occ";
 						reg = <2>;
+
+						occ-hwmon@2 {
+							compatible = "ibm,p9-occ-hwmon";
+						};
 					};
 				};
 			};
-- 
2.14.1

Re: [PATCH linux dev-4.13 01/16] dts: fsi: Make OCC description match expected hierarchy

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> ibm,p9-occ-hwmon describes a driver, not hardware, but this compatible is
> what's required to get the driver to probe.
>
> While we discuss how to resolve that problem, lets at least make the current
> configuration work.

I already have a possible solution in my patch "drivers: occ: Create 
hwmon platform device directly"
that does away with dts entries for the hwmon driver. Open to feedback, 
but it will obviously conflict with this patch.

Thanks,
Eddie

>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   arch/arm/boot/dts/ibm-power9-cfam.dtsi | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/ibm-power9-cfam.dtsi b/arch/arm/boot/dts/ibm-power9-cfam.dtsi
> index e0e738f9e539..215cc8182ca5 100644
> --- a/arch/arm/boot/dts/ibm-power9-cfam.dtsi
> +++ b/arch/arm/boot/dts/ibm-power9-cfam.dtsi
> @@ -113,8 +113,12 @@
>   			#size-cells = <0>;
>
>   			occ@1 {
> -				compatible = "ibm,p9-occ-hwmon";
> +				compatible = "ibm,p9-occ";
>   				reg = <1>;
> +
> +				occ-hwmon@1 {
> +					compatible = "ibm,p9-occ-hwmon";
> +				};
>   			};
>   		};
>
> @@ -205,8 +209,12 @@
>   					#size-cells = <0>;
>
>   					occ@2 {
> -						compatible = "ibm,p9-occ-hwmon";
> +						compatible = "ibm,p9-occ";
>   						reg = <2>;
> +
> +						occ-hwmon@2 {
> +							compatible = "ibm,p9-occ-hwmon";
> +						};
>   					};
>   				};
>   			};

Re: [PATCH linux dev-4.13 01/16] dts: fsi: Make OCC description match expected hierarchy

[Andrew Jeffery]

On Fri, 23 Feb 2018, at 06:55, Eddie James wrote:
> 
> 
> On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> > ibm,p9-occ-hwmon describes a driver, not hardware, but this compatible is
> > what's required to get the driver to probe.
> >
> > While we discuss how to resolve that problem, lets at least make the current
> > configuration work.
> 
> I already have a possible solution in my patch "drivers: occ: Create 
> hwmon platform device directly"
> that does away with dts entries for the hwmon driver. Open to feedback, 
> but it will obviously conflict with this patch.
> 

Yep, I need to review your patches. Happy to drop this for yours pending the outcome.

Cheers,

Andrew

[PATCH linux dev-4.13 02/16] hwmon (p9_sbe): Initialise device spin lock

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/p9_sbe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 7dbe4d5bcd61..cda29f47f8ff 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -116,6 +116,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	if (!p9_sbe_occ)
 		return -ENOMEM;
 
+	spin_lock_init(&p9_sbe_occ->lock);
 	p9_sbe_occ->sbe = pdev->dev.parent;
 	occ = &p9_sbe_occ->occ;
 	occ->bus_dev = &pdev->dev;
-- 
2.14.1

Re: [PATCH linux dev-4.13 02/16] hwmon (p9_sbe): Initialise device spin lock

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

> ---
>   drivers/hwmon/occ/p9_sbe.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 7dbe4d5bcd61..cda29f47f8ff 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -116,6 +116,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>   	if (!p9_sbe_occ)
>   		return -ENOMEM;
>
> +	spin_lock_init(&p9_sbe_occ->lock);
>   	p9_sbe_occ->sbe = pdev->dev.parent;
>   	occ = &p9_sbe_occ->occ;
>   	occ->bus_dev = &pdev->dev;

[PATCH linux dev-4.13 03/16] fsi: sbefifo: Rename sbefifo_release_client() for consistency

[Andrew Jeffery]

We have sbefifo_new_client(), which feels unmatched by the currently named
sbefifo_client_release().

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-sbefifo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 92947b0fa279..699de6b9a4eb 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -314,7 +314,7 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
 	return client;
 }
 
-static void sbefifo_client_release(struct kref *kref)
+static void sbefifo_release_client(struct kref *kref)
 {
 	struct sbefifo *sbefifo;
 	struct sbefifo_client *client;
@@ -353,7 +353,7 @@ static void sbefifo_get_client(struct sbefifo_client *client)
 
 static void sbefifo_put_client(struct sbefifo_client *client)
 {
-	kref_put(&client->kref, sbefifo_client_release);
+	kref_put(&client->kref, sbefifo_release_client);
 }
 
 static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
-- 
2.14.1

Re: [PATCH linux dev-4.13 03/16] fsi: sbefifo: Rename sbefifo_release_client() for consistency

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> We have sbefifo_new_client(), which feels unmatched by the currently named
> sbefifo_client_release().
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

> ---
>   drivers/fsi/fsi-sbefifo.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 92947b0fa279..699de6b9a4eb 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -314,7 +314,7 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
>   	return client;
>   }
>
> -static void sbefifo_client_release(struct kref *kref)
> +static void sbefifo_release_client(struct kref *kref)
>   {
>   	struct sbefifo *sbefifo;
>   	struct sbefifo_client *client;
> @@ -353,7 +353,7 @@ static void sbefifo_get_client(struct sbefifo_client *client)
>
>   static void sbefifo_put_client(struct sbefifo_client *client)
>   {
> -	kref_put(&client->kref, sbefifo_client_release);
> +	kref_put(&client->kref, sbefifo_release_client);
>   }
>
>   static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)

[PATCH linux dev-4.13 04/16] fsi: sbefifo: Add tracing of transfers

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-sbefifo.c      | 16 ++++++++
 include/trace/events/sbefifo.h | 93 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+)
 create mode 100644 include/trace/events/sbefifo.h

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 699de6b9a4eb..ef4a141dee2a 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -33,6 +33,9 @@
 #include <linux/uaccess.h>
 #include <linux/wait.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/sbefifo.h>
+
 /*
  * The SBEFIFO is a pipe-like FSI device for communicating with
  * the self boot engine on POWER processors.
@@ -275,6 +278,8 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
 	if (!xfr)
 		return ERR_PTR(-ENOMEM);
 
+	trace_sbefifo_enq_xfer(client, xfr);
+
 	xfr->rbuf = &client->rbuf;
 	xfr->wbuf = &client->wbuf;
 	list_add_tail(&xfr->xfrs, &sbefifo->xfrs);
@@ -303,6 +308,8 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
 	if (!client)
 		return NULL;
 
+	trace_sbefifo_new_client(client);
+
 	kref_init(&client->kref);
 	client->dev = sbefifo;
 	sbefifo_buf_init(&client->rbuf);
@@ -343,6 +350,7 @@ static void sbefifo_release_client(struct kref *kref)
 	}
 
 	sbefifo_put(sbefifo);
+	trace_sbefifo_release_client(client);
 	kfree(client);
 }
 
@@ -393,6 +401,8 @@ static void sbefifo_poll_timer(unsigned long data)
 	if (!xfr)
 		goto out_unlock;
 
+	trace_sbefifo_begin_xfer(xfr);
+
 	rbuf = xfr->rbuf;
 	wbuf = xfr->wbuf;
 
@@ -506,6 +516,8 @@ static void sbefifo_poll_timer(unsigned long data)
 	}
 
 out:
+	trace_sbefifo_end_xfer(xfr, ret);
+
 	if (unlikely(ret)) {
 		sbefifo->rc = ret;
 		dev_err(&sbefifo->fsi_dev->dev,
@@ -612,6 +624,10 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		goto out;
 	}
 
+	trace_sbefifo_deq_xfer(client, list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client));
+
 	n = min_t(size_t, n, len);
 
 	if (ubuf) {
diff --git a/include/trace/events/sbefifo.h b/include/trace/events/sbefifo.h
new file mode 100644
index 000000000000..4755fcb23e56
--- /dev/null
+++ b/include/trace/events/sbefifo.h
@@ -0,0 +1,93 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM sbefifo
+
+#if !defined(_TRACE_TIMER_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SBEFIFO_H
+
+#include <linux/tracepoint.h>
+#include <linux/fsi-sbefifo.h>
+
+TRACE_EVENT(sbefifo_new_client,
+	TP_PROTO(const void *client),
+	TP_ARGS(client),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+	),
+	TP_printk("New client: %p", __entry->client)
+);
+
+TRACE_EVENT(sbefifo_release_client,
+	TP_PROTO(const void *client),
+	TP_ARGS(client),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+	),
+	TP_printk("Released client: %p", __entry->client)
+);
+
+TRACE_EVENT(sbefifo_enq_xfer,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("Client %p enqueued transfer %p",
+		  __entry->client, __entry->xfer)
+);
+
+TRACE_EVENT(sbefifo_begin_xfer,
+	TP_PROTO(const void *xfer),
+	TP_ARGS(xfer),
+	TP_STRUCT__entry(
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->xfer = xfer;
+	),
+	TP_printk("Began transfer %p",
+		  __entry->xfer)
+);
+
+TRACE_EVENT(sbefifo_end_xfer,
+	TP_PROTO(const void *xfer, int ret),
+	TP_ARGS(xfer, ret),
+	TP_STRUCT__entry(
+		__field(const void *, xfer)
+		__field(int, ret)
+	),
+	TP_fast_assign(
+		__entry->xfer = xfer;
+		__entry->ret = ret;
+	),
+	TP_printk("Completed transfer %p: %d",
+		  __entry->xfer, __entry->ret)
+);
+
+TRACE_EVENT(sbefifo_deq_xfer,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("Client %p dequeueing transfer %p",
+		  __entry->client, __entry->xfer)
+);
+#endif
+
+#include <trace/define_trace.h>
-- 
2.14.1

Re: [PATCH linux dev-4.13 04/16] fsi: sbefifo: Add tracing of transfers

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

> ---
>   drivers/fsi/fsi-sbefifo.c      | 16 ++++++++
>   include/trace/events/sbefifo.h | 93 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 109 insertions(+)
>   create mode 100644 include/trace/events/sbefifo.h
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 699de6b9a4eb..ef4a141dee2a 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -33,6 +33,9 @@
>   #include <linux/uaccess.h>
>   #include <linux/wait.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/sbefifo.h>
> +
>   /*
>    * The SBEFIFO is a pipe-like FSI device for communicating with
>    * the self boot engine on POWER processors.
> @@ -275,6 +278,8 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>   	if (!xfr)
>   		return ERR_PTR(-ENOMEM);
>
> +	trace_sbefifo_enq_xfer(client, xfr);
> +
>   	xfr->rbuf = &client->rbuf;
>   	xfr->wbuf = &client->wbuf;
>   	list_add_tail(&xfr->xfrs, &sbefifo->xfrs);
> @@ -303,6 +308,8 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
>   	if (!client)
>   		return NULL;
>
> +	trace_sbefifo_new_client(client);
> +
>   	kref_init(&client->kref);
>   	client->dev = sbefifo;
>   	sbefifo_buf_init(&client->rbuf);
> @@ -343,6 +350,7 @@ static void sbefifo_release_client(struct kref *kref)
>   	}
>
>   	sbefifo_put(sbefifo);
> +	trace_sbefifo_release_client(client);
>   	kfree(client);
>   }
>
> @@ -393,6 +401,8 @@ static void sbefifo_poll_timer(unsigned long data)
>   	if (!xfr)
>   		goto out_unlock;
>
> +	trace_sbefifo_begin_xfer(xfr);
> +
>   	rbuf = xfr->rbuf;
>   	wbuf = xfr->wbuf;
>
> @@ -506,6 +516,8 @@ static void sbefifo_poll_timer(unsigned long data)
>   	}
>
>   out:
> +	trace_sbefifo_end_xfer(xfr, ret);
> +
>   	if (unlikely(ret)) {
>   		sbefifo->rc = ret;
>   		dev_err(&sbefifo->fsi_dev->dev,
> @@ -612,6 +624,10 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>   		goto out;
>   	}
>
> +	trace_sbefifo_deq_xfer(client, list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client));
> +
>   	n = min_t(size_t, n, len);
>
>   	if (ubuf) {
> diff --git a/include/trace/events/sbefifo.h b/include/trace/events/sbefifo.h
> new file mode 100644
> index 000000000000..4755fcb23e56
> --- /dev/null
> +++ b/include/trace/events/sbefifo.h
> @@ -0,0 +1,93 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM sbefifo
> +
> +#if !defined(_TRACE_TIMER_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_SBEFIFO_H
> +
> +#include <linux/tracepoint.h>
> +#include <linux/fsi-sbefifo.h>
> +
> +TRACE_EVENT(sbefifo_new_client,
> +	TP_PROTO(const void *client),
> +	TP_ARGS(client),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +	),
> +	TP_printk("New client: %p", __entry->client)
> +);
> +
> +TRACE_EVENT(sbefifo_release_client,
> +	TP_PROTO(const void *client),
> +	TP_ARGS(client),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +	),
> +	TP_printk("Released client: %p", __entry->client)
> +);
> +
> +TRACE_EVENT(sbefifo_enq_xfer,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("Client %p enqueued transfer %p",
> +		  __entry->client, __entry->xfer)
> +);
> +
> +TRACE_EVENT(sbefifo_begin_xfer,
> +	TP_PROTO(const void *xfer),
> +	TP_ARGS(xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("Began transfer %p",
> +		  __entry->xfer)
> +);
> +
> +TRACE_EVENT(sbefifo_end_xfer,
> +	TP_PROTO(const void *xfer, int ret),
> +	TP_ARGS(xfer, ret),
> +	TP_STRUCT__entry(
> +		__field(const void *, xfer)
> +		__field(int, ret)
> +	),
> +	TP_fast_assign(
> +		__entry->xfer = xfer;
> +		__entry->ret = ret;
> +	),
> +	TP_printk("Completed transfer %p: %d",
> +		  __entry->xfer, __entry->ret)
> +);
> +
> +TRACE_EVENT(sbefifo_deq_xfer,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("Client %p dequeueing transfer %p",
> +		  __entry->client, __entry->xfer)
> +);
> +#endif
> +
> +#include <trace/define_trace.h>

[PATCH linux dev-4.13 05/16] fsi: gpio: Trace busy count

[Andrew Jeffery]

An observation from trace output of the existing FSI tracepoints was that the
remote device was sometimes reporting as busy. Add a new tracepoint reporting
the busy count in order to get a better grip on how often this is the case,

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-master-gpio.c          |  3 +++
 include/trace/events/fsi_master_gpio.h | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 3f487449a277..2a49b167effe 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -401,6 +401,9 @@ static int poll_for_response(struct fsi_master_gpio *master,
 		break;
 	}
 
+	if (busy_count > 0)
+		trace_fsi_master_gpio_poll_response_busy(master, busy_count);
+
 	/* Clock the slave enough to be ready for next operation */
 	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
 	return rc;
diff --git a/include/trace/events/fsi_master_gpio.h b/include/trace/events/fsi_master_gpio.h
index 11b36c119048..48e83e5755f4 100644
--- a/include/trace/events/fsi_master_gpio.h
+++ b/include/trace/events/fsi_master_gpio.h
@@ -63,6 +63,22 @@ TRACE_EVENT(fsi_master_gpio_break,
 	)
 );
 
+
+TRACE_EVENT(fsi_master_gpio_poll_response_busy,
+	TP_PROTO(const struct fsi_master_gpio *master, int busy),
+	TP_ARGS(master, busy),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	busy)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+		__entry->busy = busy;
+	),
+	TP_printk("fsi-gpio%d: device reported busy %d times",
+		__entry->master_idx, __entry->busy)
+);
+
 #endif /* _TRACE_FSI_MASTER_GPIO_H */
 
 #include <trace/define_trace.h>
-- 
2.14.1

Re: [PATCH linux dev-4.13 05/16] fsi: gpio: Trace busy count

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> An observation from trace output of the existing FSI tracepoints was that the
> remote device was sometimes reporting as busy. Add a new tracepoint reporting
> the busy count in order to get a better grip on how often this is the case,

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/fsi/fsi-master-gpio.c          |  3 +++
>   include/trace/events/fsi_master_gpio.h | 16 ++++++++++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 3f487449a277..2a49b167effe 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -401,6 +401,9 @@ static int poll_for_response(struct fsi_master_gpio *master,
>   		break;
>   	}
>
> +	if (busy_count > 0)
> +		trace_fsi_master_gpio_poll_response_busy(master, busy_count);
> +
>   	/* Clock the slave enough to be ready for next operation */
>   	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
>   	return rc;
> diff --git a/include/trace/events/fsi_master_gpio.h b/include/trace/events/fsi_master_gpio.h
> index 11b36c119048..48e83e5755f4 100644
> --- a/include/trace/events/fsi_master_gpio.h
> +++ b/include/trace/events/fsi_master_gpio.h
> @@ -63,6 +63,22 @@ TRACE_EVENT(fsi_master_gpio_break,
>   	)
>   );
>
> +
> +TRACE_EVENT(fsi_master_gpio_poll_response_busy,
> +	TP_PROTO(const struct fsi_master_gpio *master, int busy),
> +	TP_ARGS(master, busy),
> +	TP_STRUCT__entry(
> +		__field(int,	master_idx)
> +		__field(int,	busy)
> +	),
> +	TP_fast_assign(
> +		__entry->master_idx = master->master.idx;
> +		__entry->busy = busy;
> +	),
> +	TP_printk("fsi-gpio%d: device reported busy %d times",
> +		__entry->master_idx, __entry->busy)
> +);
> +
>   #endif /* _TRACE_FSI_MASTER_GPIO_H */
>
>   #include <trace/define_trace.h>

[PATCH linux dev-4.13 06/16] fsi: occ: Add tracepoints

[Andrew Jeffery]

The OCC driver uses a workqueue to manage calls through to the SBEFIFO, which
in turn uses a timer callback to execute FIFO transfers. To ease observation of
end-to-end interactions e.g. from userspace `cat`ing an OCC hwmon attribute,
add tracing to book-end SBEFIFO transfers. This provides some perspective on
the time taken for a single OCC operation to take place.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-occ.c          |  9 +++++
 include/trace/events/fsi_occ.h | 86 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)
 create mode 100644 include/trace/events/fsi_occ.h

diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
index 171355023aec..7e47466d5f55 100644
--- a/drivers/fsi/fsi-occ.c
+++ b/drivers/fsi/fsi-occ.c
@@ -30,6 +30,9 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/fsi_occ.h>
+
 #define OCC_SRAM_BYTES		4096
 #define OCC_CMD_DATA_BYTES	4090
 #define OCC_RESP_DATA_BYTES	4089
@@ -132,6 +135,8 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	spin_unlock_irqrestore(&occ->list_lock, flags);
 
+	trace_occ_enq_xfer(client, xfr);
+
 	if (empty)
 		queue_work(occ_wq, &occ->work);
 
@@ -277,6 +282,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 done:
 	spin_unlock_irqrestore(&client->lock, flags);
+	trace_occ_read_complete(client, xfr);
 	occ_put_client(client);
 	return rc;
 }
@@ -308,6 +314,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	occ_get_client(client);
 	xfr = &client->xfr;
 
+	trace_occ_write_begin(client, xfr);
 	spin_lock_irqsave(&client->lock, flags);
 
 	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -638,6 +645,7 @@ static void occ_worker(struct work_struct *work)
 	set_bit(XFR_IN_PROGRESS, &xfr->flags);
 
 	spin_unlock_irqrestore(&occ->list_lock, flags);
+	trace_occ_worker_xfer_begin(client, xfr);
 	mutex_lock(&occ->occ_lock);
 
 	start = jiffies;
@@ -700,6 +708,7 @@ static void occ_worker(struct work_struct *work)
 	spin_unlock_irqrestore(&occ->list_lock, flags);
 
 	wake_up_interruptible(&client->wait);
+	trace_occ_worker_xfer_complete(client, xfr);
 	occ_put_client(client);
 
 	if (!empty)
diff --git a/include/trace/events/fsi_occ.h b/include/trace/events/fsi_occ.h
new file mode 100644
index 000000000000..89562436de86
--- /dev/null
+++ b/include/trace/events/fsi_occ.h
@@ -0,0 +1,86 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM fsi_occ
+
+#if !defined(_TRACE_TIMER_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_OCC_H
+
+#include <linux/tracepoint.h>
+#include <linux/fsi-occ.h>
+
+TRACE_EVENT(occ_enq_xfer,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("Client %p enqueued xfer %p", __entry->client, __entry->xfer)
+);
+
+TRACE_EVENT(occ_read_complete,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("Client %p completed read for xfer %p",
+		__entry->client, __entry->xfer)
+);
+
+TRACE_EVENT(occ_write_begin,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("Client %p began write for xfer %p",
+		__entry->client, __entry->xfer)
+);
+
+TRACE_EVENT(occ_worker_xfer_begin,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("OCC worker began client %p xfer %p",
+		__entry->client, __entry->xfer)
+);
+
+TRACE_EVENT(occ_worker_xfer_complete,
+	TP_PROTO(const void *client, const void *xfer),
+	TP_ARGS(client, xfer),
+	TP_STRUCT__entry(
+		__field(const void *, client)
+		__field(const void *, xfer)
+	),
+	TP_fast_assign(
+		__entry->client = client;
+		__entry->xfer = xfer;
+	),
+	TP_printk("OCC worker completed client %p xfer %p",
+		__entry->client, __entry->xfer)
+);
+
+#endif
+
+#include <trace/define_trace.h>
-- 
2.14.1

Re: [PATCH linux dev-4.13 06/16] fsi: occ: Add tracepoints

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> The OCC driver uses a workqueue to manage calls through to the SBEFIFO, which
> in turn uses a timer callback to execute FIFO transfers. To ease observation of
> end-to-end interactions e.g. from userspace `cat`ing an OCC hwmon attribute,
> add tracing to book-end SBEFIFO transfers. This provides some perspective on
> the time taken for a single OCC operation to take place.

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/fsi/fsi-occ.c          |  9 +++++
>   include/trace/events/fsi_occ.h | 86 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 95 insertions(+)
>   create mode 100644 include/trace/events/fsi_occ.h
>
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index 171355023aec..7e47466d5f55 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -30,6 +30,9 @@
>   #include <linux/wait.h>
>   #include <linux/workqueue.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/fsi_occ.h>
> +
>   #define OCC_SRAM_BYTES		4096
>   #define OCC_CMD_DATA_BYTES	4090
>   #define OCC_RESP_DATA_BYTES	4089
> @@ -132,6 +135,8 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
>
>   	spin_unlock_irqrestore(&occ->list_lock, flags);
>
> +	trace_occ_enq_xfer(client, xfr);
> +
>   	if (empty)
>   		queue_work(occ_wq, &occ->work);
>
> @@ -277,6 +282,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>
>   done:
>   	spin_unlock_irqrestore(&client->lock, flags);
> +	trace_occ_read_complete(client, xfr);
>   	occ_put_client(client);
>   	return rc;
>   }
> @@ -308,6 +314,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>   	occ_get_client(client);
>   	xfr = &client->xfr;
>
> +	trace_occ_write_begin(client, xfr);
>   	spin_lock_irqsave(&client->lock, flags);
>
>   	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> @@ -638,6 +645,7 @@ static void occ_worker(struct work_struct *work)
>   	set_bit(XFR_IN_PROGRESS, &xfr->flags);
>
>   	spin_unlock_irqrestore(&occ->list_lock, flags);
> +	trace_occ_worker_xfer_begin(client, xfr);
>   	mutex_lock(&occ->occ_lock);
>
>   	start = jiffies;
> @@ -700,6 +708,7 @@ static void occ_worker(struct work_struct *work)
>   	spin_unlock_irqrestore(&occ->list_lock, flags);
>
>   	wake_up_interruptible(&client->wait);
> +	trace_occ_worker_xfer_complete(client, xfr);
>   	occ_put_client(client);
>
>   	if (!empty)
> diff --git a/include/trace/events/fsi_occ.h b/include/trace/events/fsi_occ.h
> new file mode 100644
> index 000000000000..89562436de86
> --- /dev/null
> +++ b/include/trace/events/fsi_occ.h
> @@ -0,0 +1,86 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM fsi_occ
> +
> +#if !defined(_TRACE_TIMER_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_OCC_H
> +
> +#include <linux/tracepoint.h>
> +#include <linux/fsi-occ.h>
> +
> +TRACE_EVENT(occ_enq_xfer,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("Client %p enqueued xfer %p", __entry->client, __entry->xfer)
> +);
> +
> +TRACE_EVENT(occ_read_complete,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("Client %p completed read for xfer %p",
> +		__entry->client, __entry->xfer)
> +);
> +
> +TRACE_EVENT(occ_write_begin,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("Client %p began write for xfer %p",
> +		__entry->client, __entry->xfer)
> +);
> +
> +TRACE_EVENT(occ_worker_xfer_begin,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("OCC worker began client %p xfer %p",
> +		__entry->client, __entry->xfer)
> +);
> +
> +TRACE_EVENT(occ_worker_xfer_complete,
> +	TP_PROTO(const void *client, const void *xfer),
> +	TP_ARGS(client, xfer),
> +	TP_STRUCT__entry(
> +		__field(const void *, client)
> +		__field(const void *, xfer)
> +	),
> +	TP_fast_assign(
> +		__entry->client = client;
> +		__entry->xfer = xfer;
> +	),
> +	TP_printk("OCC worker completed client %p xfer %p",
> +		__entry->client, __entry->xfer)
> +);
> +
> +#endif
> +
> +#include <trace/define_trace.h>

[PATCH linux dev-4.13 07/16] fsi: sbefifo: don't delete canceled xfrs in write

[Andrew Jeffery]

From: Eddie James <eajames@linux.vnet.ibm.com>

The write function was deleting canceled transfers before they had a
change to run and complete. This results in not acking the EOT and not
reading the rest of the data left in the FIFO for the canceled transfer.
This is the cause of the EBADMSG errors found in the OCC driver, as the
next op reads the data left over.

Fix it by just checking the first entry of the list instead of
iterating and canceling.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index ef4a141dee2a..041704806ae4 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -707,7 +707,10 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	n = sbefifo_buf_nbwriteable(&client->wbuf);
 
 	spin_lock_irq(&sbefifo->lock);
-	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
+ 
+	/* next xfr to be executed */
+	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
+				       xfrs);
 
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
 		spin_unlock_irq(&sbefifo->lock);
-- 
2.14.1

[PATCH linux dev-4.13 08/16] hwmon (p9_sbe): Rename context variable

[Andrew Jeffery]

Using 'occ' as the context variable caused naming conflicts in some instances.
Instead use 'ctx' which should make it clear it's the associated drvdata and
make way for calling other object pointers 'occ'.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/p9_sbe.c | 47 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index cda29f47f8ff..9b8f2f650895 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -39,16 +39,16 @@ struct p9_sbe_occ {
 
 #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
-static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
+static void p9_sbe_occ_close_client(struct p9_sbe_occ *ctx)
 {
 	unsigned long flags;
 	struct occ_client *tmp_client;
 
-	spin_lock_irqsave(&occ->lock, flags);
-	tmp_client = occ->client;
-	occ->client = NULL;
+	spin_lock_irqsave(&ctx->lock, flags);
+	tmp_client = ctx->client;
+	ctx->client = NULL;
 	occ_drv_release(tmp_client);
-	spin_unlock_irqrestore(&occ->lock, flags);
+	spin_unlock_irqrestore(&ctx->lock, flags);
 }
 
 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
@@ -56,26 +56,25 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 	int rc;
 	unsigned long flags;
 	struct occ_response *resp = &occ->resp;
-	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
+	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
 
-	spin_lock_irqsave(&p9_sbe_occ->lock, flags);
-	if (p9_sbe_occ->sbe)
-		p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
-	spin_unlock_irqrestore(&p9_sbe_occ->lock, flags);
+	spin_lock_irqsave(&ctx->lock, flags);
+	if (ctx->sbe)
+		ctx->client = occ_drv_open(ctx->sbe, 0);
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
-	if (!p9_sbe_occ->client)
+	if (!ctx->client)
 		return -ENODEV;
 
 	/* skip first byte (sequence number), OCC driver handles it */
-	rc = occ_drv_write(p9_sbe_occ->client, (const char *)&cmd[1], 7);
+	rc = occ_drv_write(ctx->client, (const char *)&cmd[1], 7);
 	if (rc < 0)
 		goto err;
 
-	rc = occ_drv_read(p9_sbe_occ->client, (char *)resp, sizeof(*resp));
+	rc = occ_drv_read(ctx->client, (char *)resp, sizeof(*resp));
 	if (rc < 0)
 		goto err;
 
-	/* check the OCC response */
 	switch (resp->return_status) {
 	case OCC_RESP_CMD_IN_PRG:
 		rc = -ETIMEDOUT;
@@ -103,22 +102,22 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 	}
 
 err:
-	p9_sbe_occ_close_client(p9_sbe_occ);
+	p9_sbe_occ_close_client(ctx);
 	return rc;
 }
 
 static int p9_sbe_occ_probe(struct platform_device *pdev)
 {
 	struct occ *occ;
-	struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev,
-						     sizeof(*p9_sbe_occ),
+	struct p9_sbe_occ *ctx = devm_kzalloc(&pdev->dev,
+						     sizeof(*ctx),
 						     GFP_KERNEL);
-	if (!p9_sbe_occ)
+	if (!ctx)
 		return -ENOMEM;
 
-	spin_lock_init(&p9_sbe_occ->lock);
-	p9_sbe_occ->sbe = pdev->dev.parent;
-	occ = &p9_sbe_occ->occ;
+	spin_lock_init(&ctx->lock);
+	ctx->sbe = pdev->dev.parent;
+	occ = &ctx->occ;
 	occ->bus_dev = &pdev->dev;
 	platform_set_drvdata(pdev, occ);
 
@@ -131,10 +130,10 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 static int p9_sbe_occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
-	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
+	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
 
-	p9_sbe_occ->sbe = NULL;
-	p9_sbe_occ_close_client(p9_sbe_occ);
+	ctx->sbe = NULL;
+	p9_sbe_occ_close_client(ctx);
 
 	occ_shutdown(occ);
 
-- 
2.14.1

Re: [PATCH linux dev-4.13 08/16] hwmon (p9_sbe): Rename context variable

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> Using 'occ' as the context variable caused naming conflicts in some instances.
> Instead use 'ctx' which should make it clear it's the associated drvdata and
> make way for calling other object pointers 'occ'.

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/occ/p9_sbe.c | 47 +++++++++++++++++++++++-----------------------
>   1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index cda29f47f8ff..9b8f2f650895 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -39,16 +39,16 @@ struct p9_sbe_occ {
>
>   #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
>
> -static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
> +static void p9_sbe_occ_close_client(struct p9_sbe_occ *ctx)
>   {
>   	unsigned long flags;
>   	struct occ_client *tmp_client;
>
> -	spin_lock_irqsave(&occ->lock, flags);
> -	tmp_client = occ->client;
> -	occ->client = NULL;
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	tmp_client = ctx->client;
> +	ctx->client = NULL;
>   	occ_drv_release(tmp_client);
> -	spin_unlock_irqrestore(&occ->lock, flags);
> +	spin_unlock_irqrestore(&ctx->lock, flags);
>   }
>
>   static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> @@ -56,26 +56,25 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>   	int rc;
>   	unsigned long flags;
>   	struct occ_response *resp = &occ->resp;
> -	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
> +	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>
> -	spin_lock_irqsave(&p9_sbe_occ->lock, flags);
> -	if (p9_sbe_occ->sbe)
> -		p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
> -	spin_unlock_irqrestore(&p9_sbe_occ->lock, flags);
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	if (ctx->sbe)
> +		ctx->client = occ_drv_open(ctx->sbe, 0);
> +	spin_unlock_irqrestore(&ctx->lock, flags);
>
> -	if (!p9_sbe_occ->client)
> +	if (!ctx->client)
>   		return -ENODEV;
>
>   	/* skip first byte (sequence number), OCC driver handles it */
> -	rc = occ_drv_write(p9_sbe_occ->client, (const char *)&cmd[1], 7);
> +	rc = occ_drv_write(ctx->client, (const char *)&cmd[1], 7);
>   	if (rc < 0)
>   		goto err;
>
> -	rc = occ_drv_read(p9_sbe_occ->client, (char *)resp, sizeof(*resp));
> +	rc = occ_drv_read(ctx->client, (char *)resp, sizeof(*resp));
>   	if (rc < 0)
>   		goto err;
>
> -	/* check the OCC response */
>   	switch (resp->return_status) {
>   	case OCC_RESP_CMD_IN_PRG:
>   		rc = -ETIMEDOUT;
> @@ -103,22 +102,22 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>   	}
>
>   err:
> -	p9_sbe_occ_close_client(p9_sbe_occ);
> +	p9_sbe_occ_close_client(ctx);
>   	return rc;
>   }
>
>   static int p9_sbe_occ_probe(struct platform_device *pdev)
>   {
>   	struct occ *occ;
> -	struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev,
> -						     sizeof(*p9_sbe_occ),
> +	struct p9_sbe_occ *ctx = devm_kzalloc(&pdev->dev,
> +						     sizeof(*ctx),
>   						     GFP_KERNEL);
> -	if (!p9_sbe_occ)
> +	if (!ctx)
>   		return -ENOMEM;
>
> -	spin_lock_init(&p9_sbe_occ->lock);
> -	p9_sbe_occ->sbe = pdev->dev.parent;
> -	occ = &p9_sbe_occ->occ;
> +	spin_lock_init(&ctx->lock);
> +	ctx->sbe = pdev->dev.parent;
> +	occ = &ctx->occ;
>   	occ->bus_dev = &pdev->dev;
>   	platform_set_drvdata(pdev, occ);
>
> @@ -131,10 +130,10 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>   static int p9_sbe_occ_remove(struct platform_device *pdev)
>   {
>   	struct occ *occ = platform_get_drvdata(pdev);
> -	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
> +	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>
> -	p9_sbe_occ->sbe = NULL;
> -	p9_sbe_occ_close_client(p9_sbe_occ);
> +	ctx->sbe = NULL;
> +	p9_sbe_occ_close_client(ctx);
>
>   	occ_shutdown(occ);
>

[PATCH linux dev-4.13 09/16] hwmon (p9_sbe): Rename lock member of struct p9_sbe_occ

[Andrew Jeffery]

Improve code clarity (and searchability) by renaming the lock to client_lock.
The end result is it is easier to search for uses in the source file.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/p9_sbe.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 9b8f2f650895..51bdfa89f1a6 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -34,7 +34,7 @@ struct p9_sbe_occ {
 	 * open, close and NULL assignment. This prevents simultaneous opening
 	 * and closing of the client, or closing multiple times.
 	 */
-	spinlock_t lock;
+	spinlock_t client_lock;
 };
 
 #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
@@ -44,11 +44,11 @@ static void p9_sbe_occ_close_client(struct p9_sbe_occ *ctx)
 	unsigned long flags;
 	struct occ_client *tmp_client;
 
-	spin_lock_irqsave(&ctx->lock, flags);
+	spin_lock_irqsave(&ctx->client_lock, flags);
 	tmp_client = ctx->client;
 	ctx->client = NULL;
 	occ_drv_release(tmp_client);
-	spin_unlock_irqrestore(&ctx->lock, flags);
+	spin_unlock_irqrestore(&ctx->client_lock, flags);
 }
 
 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
@@ -58,10 +58,10 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 	struct occ_response *resp = &occ->resp;
 	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
 
-	spin_lock_irqsave(&ctx->lock, flags);
+	spin_lock_irqsave(&ctx->client_lock, flags);
 	if (ctx->sbe)
 		ctx->client = occ_drv_open(ctx->sbe, 0);
-	spin_unlock_irqrestore(&ctx->lock, flags);
+	spin_unlock_irqrestore(&ctx->client_lock, flags);
 
 	if (!ctx->client)
 		return -ENODEV;
-- 
2.14.1

Re: [PATCH linux dev-4.13 09/16] hwmon (p9_sbe): Rename lock member of struct p9_sbe_occ

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> Improve code clarity (and searchability) by renaming the lock to client_lock.
> The end result is it is easier to search for uses in the source file.

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/occ/p9_sbe.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 9b8f2f650895..51bdfa89f1a6 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -34,7 +34,7 @@ struct p9_sbe_occ {
>   	 * open, close and NULL assignment. This prevents simultaneous opening
>   	 * and closing of the client, or closing multiple times.
>   	 */
> -	spinlock_t lock;
> +	spinlock_t client_lock;
>   };
>
>   #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
> @@ -44,11 +44,11 @@ static void p9_sbe_occ_close_client(struct p9_sbe_occ *ctx)
>   	unsigned long flags;
>   	struct occ_client *tmp_client;
>
> -	spin_lock_irqsave(&ctx->lock, flags);
> +	spin_lock_irqsave(&ctx->client_lock, flags);
>   	tmp_client = ctx->client;
>   	ctx->client = NULL;
>   	occ_drv_release(tmp_client);
> -	spin_unlock_irqrestore(&ctx->lock, flags);
> +	spin_unlock_irqrestore(&ctx->client_lock, flags);
>   }
>
>   static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> @@ -58,10 +58,10 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>   	struct occ_response *resp = &occ->resp;
>   	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>
> -	spin_lock_irqsave(&ctx->lock, flags);
> +	spin_lock_irqsave(&ctx->client_lock, flags);
>   	if (ctx->sbe)
>   		ctx->client = occ_drv_open(ctx->sbe, 0);
> -	spin_unlock_irqrestore(&ctx->lock, flags);
> +	spin_unlock_irqrestore(&ctx->client_lock, flags);
>
>   	if (!ctx->client)
>   		return -ENODEV;

[PATCH linux dev-4.13 10/16] hwmon (p9_sbe): Convert client_lock from a spinlock to a mutex

[Andrew Jeffery]

The hwmon occ implementation allocates GFP_KERNEL memory in occ_open_common(),
therefore we cannot call it under a spinlock as it may sleep. Compiling with
lock debugging enabled gives us the following warning:

[    4.420000] WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:2879 lockdep_trace_alloc+0xf0/0x124
[    4.420000] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
[    4.420000] CPU: 0 PID: 1 Comm: swapper Not tainted 4.10.17-00526-g1cbacc6bd3a1 #2334
[    4.420000] Hardware name: ASpeed SoC
[    4.420000] [<80010eec>] (unwind_backtrace) from [<8000e664>] (show_stack+0x20/0x24)
[    4.420000] [<8000e664>] (show_stack) from [<80249160>] (dump_stack+0x20/0x28)
[    4.420000] [<80249160>] (dump_stack) from [<8001d3a0>] (__warn+0xe0/0x108)
[    4.420000] [<8001d3a0>] (__warn) from [<8001d40c>] (warn_slowpath_fmt+0x44/0x4c)
[    4.420000] [<8001d40c>] (warn_slowpath_fmt) from [<8005d2cc>] (lockdep_trace_alloc+0xf0/0x124)
[    4.420000] [<8005d2cc>] (lockdep_trace_alloc) from [<8012bb1c>] (kmem_cache_alloc_trace+0x3c/0x284)
[    4.420000] [<8012bb1c>] (kmem_cache_alloc_trace) from [<8034fa2c>] (occ_open_common+0x30/0xac)
[    4.420000] [<8034fa2c>] (occ_open_common) from [<80350bac>] (occ_drv_open+0x24/0x28)
[    4.420000] [<80350bac>] (occ_drv_open) from [<803638fc>] (p9_sbe_occ_send_cmd+0x44/0x13c)
[    4.420000] [<803638fc>] (p9_sbe_occ_send_cmd) from [<803615b8>] (occ_poll+0x6c/0x1c0)
[    4.420000] [<803615b8>] (occ_poll) from [<80363a84>] (p9_sbe_occ_probe+0x90/0x178)
[    4.420000] [<80363a84>] (p9_sbe_occ_probe) from [<802b9850>] (platform_drv_probe+0x60/0xbc)
[    4.420000] [<802b9850>] (platform_drv_probe) from [<802b76b0>] (driver_probe_device+0x114/0x430)
[    4.420000] [<802b76b0>] (driver_probe_device) from [<802b7a98>] (__driver_attach+0xcc/0x10c)
[    4.420000] [<802b7a98>] (__driver_attach) from [<802b5770>] (bus_for_each_dev+0x5c/0xac)
[    4.420000] [<802b5770>] (bus_for_each_dev) from [<802b7c74>] (driver_attach+0x28/0x30)
[    4.420000] [<802b7c74>] (driver_attach) from [<802b6268>] (bus_add_driver+0x19c/0x25c)
[    4.420000] [<802b6268>] (bus_add_driver) from [<802b8abc>] (driver_register+0x88/0x104)
[    4.420000] [<802b8abc>] (driver_register) from [<802ba468>] (__platform_driver_register+0x3c/0x50)
[    4.420000] [<802ba468>] (__platform_driver_register) from [<8066d804>] (p9_sbe_occ_driver_init+0x18/0x20)
[    4.420000] [<8066d804>] (p9_sbe_occ_driver_init) from [<8064ae7c>] (do_one_initcall+0xa8/0x168)
[    4.420000] [<8064ae7c>] (do_one_initcall) from [<8064b04c>] (kernel_init_freeable+0x110/0x1c8)
[    4.420000] [<8064b04c>] (kernel_init_freeable) from [<804a9034>] (kernel_init+0x18/0x104)
[    4.420000] [<804a9034>] (kernel_init) from [<8000a888>] (ret_from_fork+0x14/0x2c)

Avoid the warning (or the need for GFP_ATOMIC) and allow for reduced system
latency by using a mutex instead. Mutual exclusion is the only requirement, we
do not need to block pre-emption.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/p9_sbe.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 51bdfa89f1a6..38cf57ad1bb8 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -34,34 +34,32 @@ struct p9_sbe_occ {
 	 * open, close and NULL assignment. This prevents simultaneous opening
 	 * and closing of the client, or closing multiple times.
 	 */
-	spinlock_t client_lock;
+	struct mutex client_lock;
 };
 
 #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
 static void p9_sbe_occ_close_client(struct p9_sbe_occ *ctx)
 {
-	unsigned long flags;
 	struct occ_client *tmp_client;
 
-	spin_lock_irqsave(&ctx->client_lock, flags);
+	mutex_lock(&ctx->client_lock);
 	tmp_client = ctx->client;
 	ctx->client = NULL;
 	occ_drv_release(tmp_client);
-	spin_unlock_irqrestore(&ctx->client_lock, flags);
+	mutex_unlock(&ctx->client_lock);
 }
 
 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 {
 	int rc;
-	unsigned long flags;
 	struct occ_response *resp = &occ->resp;
 	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
 
-	spin_lock_irqsave(&ctx->client_lock, flags);
+	mutex_lock(&ctx->client_lock);
 	if (ctx->sbe)
 		ctx->client = occ_drv_open(ctx->sbe, 0);
-	spin_unlock_irqrestore(&ctx->client_lock, flags);
+	mutex_unlock(&ctx->client_lock);
 
 	if (!ctx->client)
 		return -ENODEV;
@@ -115,7 +113,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	if (!ctx)
 		return -ENOMEM;
 
-	spin_lock_init(&ctx->lock);
+	mutex_init(&ctx->client_lock);
 	ctx->sbe = pdev->dev.parent;
 	occ = &ctx->occ;
 	occ->bus_dev = &pdev->dev;
-- 
2.14.1

Re: [PATCH linux dev-4.13 10/16] hwmon (p9_sbe): Convert client_lock from a spinlock to a mutex

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> The hwmon occ implementation allocates GFP_KERNEL memory in occ_open_common(),
> therefore we cannot call it under a spinlock as it may sleep. Compiling with
> lock debugging enabled gives us the following warning:

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> [    4.420000] WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:2879 lockdep_trace_alloc+0xf0/0x124
> [    4.420000] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
> [    4.420000] CPU: 0 PID: 1 Comm: swapper Not tainted 4.10.17-00526-g1cbacc6bd3a1 #2334
> [    4.420000] Hardware name: ASpeed SoC
> [    4.420000] [<80010eec>] (unwind_backtrace) from [<8000e664>] (show_stack+0x20/0x24)
> [    4.420000] [<8000e664>] (show_stack) from [<80249160>] (dump_stack+0x20/0x28)
> [    4.420000] [<80249160>] (dump_stack) from [<8001d3a0>] (__warn+0xe0/0x108)
> [    4.420000] [<8001d3a0>] (__warn) from [<8001d40c>] (warn_slowpath_fmt+0x44/0x4c)
> [    4.420000] [<8001d40c>] (warn_slowpath_fmt) from [<8005d2cc>] (lockdep_trace_alloc+0xf0/0x124)
> [    4.420000] [<8005d2cc>] (lockdep_trace_alloc) from [<8012bb1c>] (kmem_cache_alloc_trace+0x3c/0x284)
> [    4.420000] [<8012bb1c>] (kmem_cache_alloc_trace) from [<8034fa2c>] (occ_open_common+0x30/0xac)
> [    4.420000] [<8034fa2c>] (occ_open_common) from [<80350bac>] (occ_drv_open+0x24/0x28)
> [    4.420000] [<80350bac>] (occ_drv_open) from [<803638fc>] (p9_sbe_occ_send_cmd+0x44/0x13c)
> [    4.420000] [<803638fc>] (p9_sbe_occ_send_cmd) from [<803615b8>] (occ_poll+0x6c/0x1c0)
> [    4.420000] [<803615b8>] (occ_poll) from [<80363a84>] (p9_sbe_occ_probe+0x90/0x178)
> [    4.420000] [<80363a84>] (p9_sbe_occ_probe) from [<802b9850>] (platform_drv_probe+0x60/0xbc)
> [    4.420000] [<802b9850>] (platform_drv_probe) from [<802b76b0>] (driver_probe_device+0x114/0x430)
> [    4.420000] [<802b76b0>] (driver_probe_device) from [<802b7a98>] (__driver_attach+0xcc/0x10c)
> [    4.420000] [<802b7a98>] (__driver_attach) from [<802b5770>] (bus_for_each_dev+0x5c/0xac)
> [    4.420000] [<802b5770>] (bus_for_each_dev) from [<802b7c74>] (driver_attach+0x28/0x30)
> [    4.420000] [<802b7c74>] (driver_attach) from [<802b6268>] (bus_add_driver+0x19c/0x25c)
> [    4.420000] [<802b6268>] (bus_add_driver) from [<802b8abc>] (driver_register+0x88/0x104)
> [    4.420000] [<802b8abc>] (driver_register) from [<802ba468>] (__platform_driver_register+0x3c/0x50)
> [    4.420000] [<802ba468>] (__platform_driver_register) from [<8066d804>] (p9_sbe_occ_driver_init+0x18/0x20)
> [    4.420000] [<8066d804>] (p9_sbe_occ_driver_init) from [<8064ae7c>] (do_one_initcall+0xa8/0x168)
> [    4.420000] [<8064ae7c>] (do_one_initcall) from [<8064b04c>] (kernel_init_freeable+0x110/0x1c8)
> [    4.420000] [<8064b04c>] (kernel_init_freeable) from [<804a9034>] (kernel_init+0x18/0x104)
> [    4.420000] [<804a9034>] (kernel_init) from [<8000a888>] (ret_from_fork+0x14/0x2c)
>
> Avoid the warning (or the need for GFP_ATOMIC) and allow for reduced system
> latency by using a mutex instead. Mutual exclusion is the only requirement, we
> do not need to block pre-emption.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/occ/p9_sbe.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 51bdfa89f1a6..38cf57ad1bb8 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -34,34 +34,32 @@ struct p9_sbe_occ {
>   	 * open, close and NULL assignment. This prevents simultaneous opening
>   	 * and closing of the client, or closing multiple times.
>   	 */
> -	spinlock_t client_lock;
> +	struct mutex client_lock;
>   };
>
>   #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
>
>   static void p9_sbe_occ_close_client(struct p9_sbe_occ *ctx)
>   {
> -	unsigned long flags;
>   	struct occ_client *tmp_client;
>
> -	spin_lock_irqsave(&ctx->client_lock, flags);
> +	mutex_lock(&ctx->client_lock);
>   	tmp_client = ctx->client;
>   	ctx->client = NULL;
>   	occ_drv_release(tmp_client);
> -	spin_unlock_irqrestore(&ctx->client_lock, flags);
> +	mutex_unlock(&ctx->client_lock);
>   }
>
>   static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>   {
>   	int rc;
> -	unsigned long flags;
>   	struct occ_response *resp = &occ->resp;
>   	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>
> -	spin_lock_irqsave(&ctx->client_lock, flags);
> +	mutex_lock(&ctx->client_lock);
>   	if (ctx->sbe)
>   		ctx->client = occ_drv_open(ctx->sbe, 0);
> -	spin_unlock_irqrestore(&ctx->client_lock, flags);
> +	mutex_unlock(&ctx->client_lock);
>
>   	if (!ctx->client)
>   		return -ENODEV;
> @@ -115,7 +113,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>   	if (!ctx)
>   		return -ENOMEM;
>
> -	spin_lock_init(&ctx->lock);
> +	mutex_init(&ctx->client_lock);
>   	ctx->sbe = pdev->dev.parent;
>   	occ = &ctx->occ;
>   	occ->bus_dev = &pdev->dev;

[PATCH linux dev-4.13 11/16] hwmon (p9_sbe): Add static key to satisfy lockdep

[Andrew Jeffery]

Previously we received the following warning:

[    7.221161] INFO: trying to register non-static key.
[    7.226163] the code is fine but needs lockdep annotation.
[    7.231650] turning off the locking correctness validator.
[    7.237152] CPU: 0 PID: 1 Comm: swapper Tainted: G        W       4.13.16-00190-g78a200cb21ab #2393
[    7.246194] Hardware name: Generic DT based system
[    7.251040] [<8001141c>] (unwind_backtrace) from [<8000e87c>] (show_stack+0x20/0x24)
[    7.258827] [<8000e87c>] (show_stack) from [<804a65ec>] (dump_stack+0x20/0x28)
[    7.266098] [<804a65ec>] (dump_stack) from [<80054b08>] (register_lock_class+0x270/0x614)
[    7.274308] [<80054b08>] (register_lock_class) from [<800589f0>] (__lock_acquire+0x7c/0x17f4)
[    7.282857] [<800589f0>] (__lock_acquire) from [<8005ac18>] (lock_acquire+0xd8/0x204)
[    7.290719] [<8005ac18>] (lock_acquire) from [<804c0dcc>] (_raw_spin_lock_irqsave+0x60/0x74)
[    7.299200] [<804c0dcc>] (_raw_spin_lock_irqsave) from [<8035ecc0>] (p9_sbe_occ_send_cmd+0x30/0x100)
[    7.308363] [<8035ecc0>] (p9_sbe_occ_send_cmd) from [<8035c9ac>] (occ_poll+0x60/0x210)
[    7.316309] [<8035c9ac>] (occ_poll) from [<8035d9c8>] (occ_setup+0x50/0x1260)
[    7.323475] [<8035d9c8>] (occ_setup) from [<8035edf0>] (p9_sbe_occ_probe+0x60/0x7c)
[    7.331167] [<8035edf0>] (p9_sbe_occ_probe) from [<802b97c8>] (platform_drv_probe+0x60/0xbc)
[    7.339629] [<802b97c8>] (platform_drv_probe) from [<802b78f0>] (driver_probe_device+0x2e4/0x450)
[    7.348516] [<802b78f0>] (driver_probe_device) from [<802b7c0c>] (__device_attach_driver+0xa4/0x10c)
[    7.357668] [<802b7c0c>] (__device_attach_driver) from [<802b5de0>] (bus_for_each_drv+0x54/0xa4)
[    7.366477] [<802b5de0>] (bus_for_each_drv) from [<802b7454>] (__device_attach+0xc0/0x148)
[    7.374758] [<802b7454>] (__device_attach) from [<802b7cd8>] (device_initial_probe+0x1c/0x20)
[    7.383301] [<802b7cd8>] (device_initial_probe) from [<802b5ff8>] (bus_probe_device+0x98/0xa0)
[    7.391943] [<802b5ff8>] (bus_probe_device) from [<802b426c>] (device_add+0x398/0x5c4)
[    7.399895] [<802b426c>] (device_add) from [<8037ab10>] (of_device_add+0x40/0x48)
[    7.407405] [<8037ab10>] (of_device_add) from [<8037b618>] (of_platform_device_create_pdata+0x88/0xc8)
[    7.416735] [<8037b618>] (of_platform_device_create_pdata) from [<8037b94c>] (of_platform_device_create+0x20/0x24)
[    7.427109] [<8037b94c>] (of_platform_device_create) from [<80394894>] (occ_probe+0x19c/0x238)
[    7.435746] [<80394894>] (occ_probe) from [<802b97c8>] (platform_drv_probe+0x60/0xbc)
[    7.443604] [<802b97c8>] (platform_drv_probe) from [<802b78f0>] (driver_probe_device+0x2e4/0x450)
[    7.452497] [<802b78f0>] (driver_probe_device) from [<802b7b28>] (__driver_attach+0xcc/0x10c)
[    7.461044] [<802b7b28>] (__driver_attach) from [<802b58a4>] (bus_for_each_dev+0x5c/0xac)
[    7.469240] [<802b58a4>] (bus_for_each_dev) from [<802b7d04>] (driver_attach+0x28/0x30)
[    7.477258] [<802b7d04>] (driver_attach) from [<802b62dc>] (bus_add_driver+0x194/0x254)
[    7.485284] [<802b62dc>] (bus_add_driver) from [<802b8b54>] (driver_register+0x88/0x104)
[    7.493398] [<802b8b54>] (driver_register) from [<802ba42c>] (__platform_driver_register+0x3c/0x50)
[    7.502469] [<802ba42c>] (__platform_driver_register) from [<803943f0>] (occ_init+0x58/0x80)
[    7.510945] [<803943f0>] (occ_init) from [<80669f1c>] (do_one_initcall+0xb0/0x170)
[    7.518545] [<80669f1c>] (do_one_initcall) from [<8066a0f4>] (kernel_init_freeable+0x118/0x1d0)
[    7.527280] [<8066a0f4>] (kernel_init_freeable) from [<804b9b94>] (kernel_init+0x18/0x104)
[    7.535579] [<804b9b94>] (kernel_init) from [<8000a868>] (ret_from_fork+0x14/0x2c)

Provide a static key to lockdep to help with this dynamically allocated mutex.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/p9_sbe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 38cf57ad1bb8..b84cb1733fdf 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -16,6 +16,9 @@
 
 #include "common.h"
 
+/* Satisfy lockdep's need for static keys */
+static struct lock_class_key p9_sbe_occ_client_lock_key;
+
 struct p9_sbe_occ {
 	struct occ occ;
 	struct device *sbe;
@@ -114,6 +117,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mutex_init(&ctx->client_lock);
+	lockdep_set_class(&ctx->client_lock, &p9_sbe_occ_client_lock_key);
 	ctx->sbe = pdev->dev.parent;
 	occ = &ctx->occ;
 	occ->bus_dev = &pdev->dev;
-- 
2.14.1

Re: [PATCH linux dev-4.13 11/16] hwmon (p9_sbe): Add static key to satisfy lockdep

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> Previously we received the following warning:

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> [    7.221161] INFO: trying to register non-static key.
> [    7.226163] the code is fine but needs lockdep annotation.
> [    7.231650] turning off the locking correctness validator.
> [    7.237152] CPU: 0 PID: 1 Comm: swapper Tainted: G        W       4.13.16-00190-g78a200cb21ab #2393
> [    7.246194] Hardware name: Generic DT based system
> [    7.251040] [<8001141c>] (unwind_backtrace) from [<8000e87c>] (show_stack+0x20/0x24)
> [    7.258827] [<8000e87c>] (show_stack) from [<804a65ec>] (dump_stack+0x20/0x28)
> [    7.266098] [<804a65ec>] (dump_stack) from [<80054b08>] (register_lock_class+0x270/0x614)
> [    7.274308] [<80054b08>] (register_lock_class) from [<800589f0>] (__lock_acquire+0x7c/0x17f4)
> [    7.282857] [<800589f0>] (__lock_acquire) from [<8005ac18>] (lock_acquire+0xd8/0x204)
> [    7.290719] [<8005ac18>] (lock_acquire) from [<804c0dcc>] (_raw_spin_lock_irqsave+0x60/0x74)
> [    7.299200] [<804c0dcc>] (_raw_spin_lock_irqsave) from [<8035ecc0>] (p9_sbe_occ_send_cmd+0x30/0x100)
> [    7.308363] [<8035ecc0>] (p9_sbe_occ_send_cmd) from [<8035c9ac>] (occ_poll+0x60/0x210)
> [    7.316309] [<8035c9ac>] (occ_poll) from [<8035d9c8>] (occ_setup+0x50/0x1260)
> [    7.323475] [<8035d9c8>] (occ_setup) from [<8035edf0>] (p9_sbe_occ_probe+0x60/0x7c)
> [    7.331167] [<8035edf0>] (p9_sbe_occ_probe) from [<802b97c8>] (platform_drv_probe+0x60/0xbc)
> [    7.339629] [<802b97c8>] (platform_drv_probe) from [<802b78f0>] (driver_probe_device+0x2e4/0x450)
> [    7.348516] [<802b78f0>] (driver_probe_device) from [<802b7c0c>] (__device_attach_driver+0xa4/0x10c)
> [    7.357668] [<802b7c0c>] (__device_attach_driver) from [<802b5de0>] (bus_for_each_drv+0x54/0xa4)
> [    7.366477] [<802b5de0>] (bus_for_each_drv) from [<802b7454>] (__device_attach+0xc0/0x148)
> [    7.374758] [<802b7454>] (__device_attach) from [<802b7cd8>] (device_initial_probe+0x1c/0x20)
> [    7.383301] [<802b7cd8>] (device_initial_probe) from [<802b5ff8>] (bus_probe_device+0x98/0xa0)
> [    7.391943] [<802b5ff8>] (bus_probe_device) from [<802b426c>] (device_add+0x398/0x5c4)
> [    7.399895] [<802b426c>] (device_add) from [<8037ab10>] (of_device_add+0x40/0x48)
> [    7.407405] [<8037ab10>] (of_device_add) from [<8037b618>] (of_platform_device_create_pdata+0x88/0xc8)
> [    7.416735] [<8037b618>] (of_platform_device_create_pdata) from [<8037b94c>] (of_platform_device_create+0x20/0x24)
> [    7.427109] [<8037b94c>] (of_platform_device_create) from [<80394894>] (occ_probe+0x19c/0x238)
> [    7.435746] [<80394894>] (occ_probe) from [<802b97c8>] (platform_drv_probe+0x60/0xbc)
> [    7.443604] [<802b97c8>] (platform_drv_probe) from [<802b78f0>] (driver_probe_device+0x2e4/0x450)
> [    7.452497] [<802b78f0>] (driver_probe_device) from [<802b7b28>] (__driver_attach+0xcc/0x10c)
> [    7.461044] [<802b7b28>] (__driver_attach) from [<802b58a4>] (bus_for_each_dev+0x5c/0xac)
> [    7.469240] [<802b58a4>] (bus_for_each_dev) from [<802b7d04>] (driver_attach+0x28/0x30)
> [    7.477258] [<802b7d04>] (driver_attach) from [<802b62dc>] (bus_add_driver+0x194/0x254)
> [    7.485284] [<802b62dc>] (bus_add_driver) from [<802b8b54>] (driver_register+0x88/0x104)
> [    7.493398] [<802b8b54>] (driver_register) from [<802ba42c>] (__platform_driver_register+0x3c/0x50)
> [    7.502469] [<802ba42c>] (__platform_driver_register) from [<803943f0>] (occ_init+0x58/0x80)
> [    7.510945] [<803943f0>] (occ_init) from [<80669f1c>] (do_one_initcall+0xb0/0x170)
> [    7.518545] [<80669f1c>] (do_one_initcall) from [<8066a0f4>] (kernel_init_freeable+0x118/0x1d0)
> [    7.527280] [<8066a0f4>] (kernel_init_freeable) from [<804b9b94>] (kernel_init+0x18/0x104)
> [    7.535579] [<804b9b94>] (kernel_init) from [<8000a868>] (ret_from_fork+0x14/0x2c)
>
> Provide a static key to lockdep to help with this dynamically allocated mutex.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/occ/p9_sbe.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 38cf57ad1bb8..b84cb1733fdf 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -16,6 +16,9 @@
>
>   #include "common.h"
>
> +/* Satisfy lockdep's need for static keys */
> +static struct lock_class_key p9_sbe_occ_client_lock_key;
> +
>   struct p9_sbe_occ {
>   	struct occ occ;
>   	struct device *sbe;
> @@ -114,6 +117,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>
>   	mutex_init(&ctx->client_lock);
> +	lockdep_set_class(&ctx->client_lock, &p9_sbe_occ_client_lock_key);
>   	ctx->sbe = pdev->dev.parent;
>   	occ = &ctx->occ;
>   	occ->bus_dev = &pdev->dev;

[PATCH linux dev-4.13 12/16] fsi: sbefifo: Avoid using a timer to drive FIFO transfers

[Andrew Jeffery]

From: Eddie James <eajames@linux.vnet.ibm.com>

The timer subsystem appears to misbehave when requesting an immediate expiry
via `mod_timer(..., jiffies)`. The SBEFIFO driver uses this pattern for
scheduling work when it either adds a new transfer to an empty queue or the
timer callback finds the transfer queue is not empty. In both cases work should
be scheduled immediately, however the current approach is not optimal:

The comment [0] outlines an issue where OCC sensor polling on the BMC can come
at remarkable cost. Delays of up to and above 10 seconds can be experienced
when reading an OCC attribute.

Tracing of the subsystems involved (FSI, the SBEFIFO, the OCC, the scheduler
and the timer subsystems) showed that the SBEFIFO timer callbacks had a
saw-tooth pattern of execution latency in jiffies relative to their scheduled
expiry time. In the measurements the typical latency of an immediate reschedule
was 15 jiffies, though this could be as few as 1 or greater than 100 in the
pathological case. Surprisingly, these delays coincided in length with system
idle time between the SBEFIFO transfers - it appears timer subsystem is not
properly accounting for the immediacy of the request[1].

Querying sensors on the OCC incurs multiple SBEFIFO transfers, each with its
own (increasing) expiry latency, which leads to the poor responsiveness
observed above.

Stop misusing the timer infrastructure for work queue management and incurring
associated penalties (even if they appear to be a bug in the timer core).
Instead, use a workqueue and use queue_work() and queue_delayed_work() as
required. With this change, reading an OCC sensor typically takes ~0.5 seconds
wall-clock in the un-cached case and rarely exceeds 1 second. In the cached
case the read completes in around 0.01 seconds.

Finally, using a workqueue enables the use of mutexes in place of spinlocks,
making way for future changes down the stack in the FSI core.

[0] https://github.com/openbmc/openbmc/issues/2696#issuecomment-351207046
[1] It seems scheduling a timer with `mod_timer(..., jiffies)` is a
    pathological case: Changing all such call-sites in the SBEFIFO driver to
    use `mod_timer(..., jiffies + 1)` yielded a similar improvement in
    performance to the workqueue strategy.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
[arj: Rework the commit message]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-sbefifo.c | 54 ++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 041704806ae4..645ae00cc6ac 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -29,9 +29,9 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
-#include <linux/timer.h>
 #include <linux/uaccess.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/sbefifo.h>
@@ -60,7 +60,7 @@
 #define SBEFIFO_MAX_RESCHDULE	msecs_to_jiffies(5000)
 
 struct sbefifo {
-	struct timer_list poll_timer;
+	struct delayed_work work;
 	struct fsi_device *fsi_dev;
 	struct miscdevice mdev;
 	wait_queue_head_t wait;
@@ -102,6 +102,8 @@ struct sbefifo_client {
 	unsigned long f_flags;
 };
 
+static struct workqueue_struct *sbefifo_wq;
+
 static DEFINE_IDA(sbefifo_ida);
 
 static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
@@ -344,7 +346,7 @@ static void sbefifo_release_client(struct kref *kref)
 			 */
 			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 			sbefifo_get(sbefifo);
-			if (mod_timer(&client->dev->poll_timer, jiffies))
+			if (!queue_work(sbefifo_wq, &sbefifo->work.work))
 				sbefifo_put(sbefifo);
 		}
 	}
@@ -382,10 +384,11 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
 	return NULL;
 }
 
-static void sbefifo_poll_timer(unsigned long data)
+static void sbefifo_worker(struct work_struct *work)
 {
 	static const unsigned long EOT_MASK = 0x000000ff;
-	struct sbefifo *sbefifo = (void *)data;
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct sbefifo *sbefifo = container_of(dwork, struct sbefifo, work);
 	struct sbefifo_buf *rbuf, *wbuf;
 	struct sbefifo_xfr *xfr, *tmp;
 	struct sbefifo_buf drain;
@@ -403,6 +406,7 @@ static void sbefifo_poll_timer(unsigned long data)
 
 	trace_sbefifo_begin_xfer(xfr);
 
+again:
 	rbuf = xfr->rbuf;
 	wbuf = xfr->wbuf;
 
@@ -424,9 +428,8 @@ static void sbefifo_poll_timer(unsigned long data)
 		devn = sbefifo_dev_nwwriteable(sts);
 		if (devn == 0) {
 			/* No open slot for write.  Reschedule. */
-			sbefifo->poll_timer.expires = jiffies +
-				SBEFIFO_RESCHEDULE;
-			add_timer(&sbefifo->poll_timer);
+			queue_delayed_work(sbefifo_wq, &sbefifo->work,
+					   SBEFIFO_RESCHEDULE);
 			goto out_unlock;
 		}
 
@@ -478,9 +481,8 @@ static void sbefifo_poll_timer(unsigned long data)
 			}
 
 			/* No data yet.  Reschedule. */
-			sbefifo->poll_timer.expires = jiffies +
-				SBEFIFO_RESCHEDULE;
-			add_timer(&sbefifo->poll_timer);
+			queue_delayed_work(sbefifo_wq, &sbefifo->work,
+					   SBEFIFO_RESCHEDULE);
 			goto out_unlock;
 		} else {
 			xfr->wait_data_timeout = 0;
@@ -528,10 +530,12 @@ static void sbefifo_poll_timer(unsigned long data)
 		}
 		INIT_LIST_HEAD(&sbefifo->xfrs);
 
-	} else if (eot && sbefifo_next_xfr(sbefifo)) {
-		sbefifo_get(sbefifo);
-		sbefifo->poll_timer.expires = jiffies;
-		add_timer(&sbefifo->poll_timer);
+	} else if (eot) {
+		xfr = sbefifo_next_xfr(sbefifo);
+		if (xfr) {
+			wake_up_interruptible(&sbefifo->wait);
+			goto again;
+		}
 	}
 
 	sbefifo_put(sbefifo);
@@ -652,7 +656,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
 			/* Fill the read buffer back up. */
 			sbefifo_get(sbefifo);
-			if (mod_timer(&client->dev->poll_timer, jiffies))
+			if (!queue_work(sbefifo_wq, &sbefifo->work.work))
 				sbefifo_put(sbefifo);
 		} else {
 			list_del(&xfr->client);
@@ -738,7 +742,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 								 &n))) {
 			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 			sbefifo_get(sbefifo);
-			if (mod_timer(&sbefifo->poll_timer, jiffies))
+			if (!queue_work(sbefifo_wq, &sbefifo->work.work))
 				sbefifo_put(sbefifo);
 
 			ret = -ERESTARTSYS;
@@ -758,7 +762,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 			    n)) {
 				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 				sbefifo_get(sbefifo);
-				if (mod_timer(&sbefifo->poll_timer, jiffies))
+				if (!queue_work(sbefifo_wq,
+						&sbefifo->work.work))
 					sbefifo_put(sbefifo);
 				ret = -EFAULT;
 				goto out;
@@ -783,7 +788,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 
 		/* Drain the write buffer. */
 		sbefifo_get(sbefifo);
-		if (mod_timer(&client->dev->poll_timer, jiffies))
+		if (!queue_work(sbefifo_wq, &sbefifo->work.work))
 			sbefifo_put(sbefifo);
 	}
 
@@ -943,8 +948,7 @@ static int sbefifo_probe(struct device *dev)
 		 sbefifo->idx);
 
 	/* This bit of silicon doesn't offer any interrupts... */
-	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
-		    (unsigned long)sbefifo);
+	INIT_DELAYED_WORK(&sbefifo->work, sbefifo_worker);
 
 	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
 	sbefifo->mdev.fops = &sbefifo_fops;
@@ -997,7 +1001,7 @@ static int sbefifo_remove(struct device *dev)
 
 	ida_simple_remove(&sbefifo_ida, sbefifo->idx);
 
-	if (del_timer_sync(&sbefifo->poll_timer))
+	if (cancel_delayed_work_sync(&sbefifo->work))
 		sbefifo_put(sbefifo);
 
 	sbefifo_put(sbefifo);
@@ -1025,11 +1029,17 @@ static struct fsi_driver sbefifo_drv = {
 
 static int sbefifo_init(void)
 {
+	sbefifo_wq = create_singlethread_workqueue("sbefifo");
+	if (!sbefifo_wq)
+		return -ENOMEM;
+
 	return fsi_driver_register(&sbefifo_drv);
 }
 
 static void sbefifo_exit(void)
 {
+	destroy_workqueue(sbefifo_wq);
+
 	fsi_driver_unregister(&sbefifo_drv);
 
 	ida_destroy(&sbefifo_ida);
-- 
2.14.1

[PATCH linux dev-4.13 13/16] fsi: sbefifo: Switch to mutex in work function

[Andrew Jeffery]

From: Eddie James <eajames@linux.vnet.ibm.com>

Use a mutex instead of a spinlock to improve system latency while waiting on
FSI operations. This enables similar changes to be done down the stack in the
FSI core.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
[arj: Rework the commit message]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-sbefifo.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 645ae00cc6ac..5b12eec2602b 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -22,6 +22,7 @@
 #include <linux/list.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
@@ -66,7 +67,8 @@ struct sbefifo {
 	wait_queue_head_t wait;
 	struct list_head xfrs;
 	struct kref kref;
-	spinlock_t lock;
+	spinlock_t list_lock;		/* lock access to the xfrs list */
+	struct mutex sbefifo_lock;	/* lock access to the hardware */
 	char name[32];
 	int idx;
 	int rc;
@@ -397,12 +399,16 @@ static void sbefifo_worker(struct work_struct *work)
 	int ret = 0;
 	u32 sts;
 	int i;
+	unsigned long flags;
 
-	spin_lock(&sbefifo->lock);
+	spin_lock_irqsave(&sbefifo->list_lock, flags);
 	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
 				       xfrs);
+	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
 	if (!xfr)
-		goto out_unlock;
+		return;
+
+	mutex_lock(&sbefifo->sbefifo_lock);
 
 	trace_sbefifo_begin_xfer(xfr);
 
@@ -509,7 +515,11 @@ static void sbefifo_worker(struct work_struct *work)
 				goto out;
 
 			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
+
+			spin_lock_irqsave(&sbefifo->list_lock, flags);
 			list_del(&xfr->xfrs);
+			spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+
 			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
 					      &xfr->flags)))
 				kfree(xfr);
@@ -524,14 +534,19 @@ static void sbefifo_worker(struct work_struct *work)
 		sbefifo->rc = ret;
 		dev_err(&sbefifo->fsi_dev->dev,
 			"Fatal bus access failure: %d\n", ret);
+
+		spin_lock_irqsave(&sbefifo->list_lock, flags);
 		list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
 			list_del(&xfr->xfrs);
 			kfree(xfr);
 		}
 		INIT_LIST_HEAD(&sbefifo->xfrs);
-
+		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
 	} else if (eot) {
+		spin_lock_irqsave(&sbefifo->list_lock, flags);
 		xfr = sbefifo_next_xfr(sbefifo);
+		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+
 		if (xfr) {
 			wake_up_interruptible(&sbefifo->wait);
 			goto again;
@@ -542,7 +557,7 @@ static void sbefifo_worker(struct work_struct *work)
 	wake_up_interruptible(&sbefifo->wait);
 
 out_unlock:
-	spin_unlock(&sbefifo->lock);
+	mutex_unlock(&sbefifo->sbefifo_lock);
 }
 
 static int sbefifo_open(struct inode *inode, struct file *file)
@@ -700,6 +715,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	struct sbefifo_xfr *xfr;
 	ssize_t ret = 0;
 	size_t n;
+	unsigned long flags;
 
 	if ((len >> 2) << 2 != len)
 		return -EINVAL;
@@ -710,26 +726,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	sbefifo_get_client(client);
 	n = sbefifo_buf_nbwriteable(&client->wbuf);
 
-	spin_lock_irq(&sbefifo->lock);
+	spin_lock_irqsave(&sbefifo->list_lock, flags);
  
 	/* next xfr to be executed */
 	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
 				       xfrs);
 
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
-		spin_unlock_irq(&sbefifo->lock);
+		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
 		ret = -EAGAIN;
 		goto out;
 	}
 
 	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
 	if (IS_ERR(xfr)) {
-		spin_unlock_irq(&sbefifo->lock);
+		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
 		ret = PTR_ERR(xfr);
 		goto out;
 	}
 
-	spin_unlock_irq(&sbefifo->lock);
+	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
 
 	/*
 	 * Partial writes are not really allowed in that EOT is sent exactly
@@ -938,7 +954,8 @@ static int sbefifo_probe(struct device *dev)
 		}
 	}
 
-	spin_lock_init(&sbefifo->lock);
+	spin_lock_init(&sbefifo->list_lock);
+	mutex_init(&sbefifo->sbefifo_lock);
 	kref_init(&sbefifo->kref);
 	init_waitqueue_head(&sbefifo->wait);
 	INIT_LIST_HEAD(&sbefifo->xfrs);
@@ -981,8 +998,11 @@ static int sbefifo_remove(struct device *dev)
 {
 	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 	struct sbefifo_xfr *xfr, *tmp;
+	unsigned long flags;
 
-	spin_lock(&sbefifo->lock);
+	/* lock the sbefifo so to prevent deleting an ongoing xfr */
+	mutex_lock(&sbefifo->sbefifo_lock);
+	spin_lock_irqsave(&sbefifo->list_lock, flags);
 
 	WRITE_ONCE(sbefifo->rc, -ENODEV);
 	list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
@@ -992,7 +1012,8 @@ static int sbefifo_remove(struct device *dev)
 
 	INIT_LIST_HEAD(&sbefifo->xfrs);
 
-	spin_unlock(&sbefifo->lock);
+	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+	mutex_unlock(&sbefifo->sbefifo_lock);
 
 	wake_up_all(&sbefifo->wait);
 
-- 
2.14.1

[PATCH linux dev-4.13 14/16] fsi: sbefifo: Switch list_lock from spinlock to mutex

[Andrew Jeffery]

We allocate GFP_KERNEL memory for a list element in sbefifo_enq_xfr() (elided
from the stack trace presumably by inlining) but do so under list_lock, which
was previously a spin lock. Enabling lock debugging gives the following
warning:

[  188.830000] BUG: sleeping function called from invalid context at mm/slab.h:408
[  188.830000] in_atomic(): 1, irqs_disabled(): 128, pid: 110, name: kworker/u2:9
[  188.830000] INFO: lockdep is turned off.
[  188.830000] irq event stamp: 9002
[  188.830000] hardirqs last  enabled at (9001): [<801105bc>] kmem_cache_alloc_trace+0x8c/0x284
[  188.830000] hardirqs last disabled at (9002): [<8049458c>] _raw_spin_lock_irqsave+0x2c/0xa0
[  188.830000] softirqs last  enabled at (5370): [<8000987c>] __do_softirq+0x38c/0x510
[  188.830000] softirqs last disabled at (5363): [<8001f154>] irq_exit+0x120/0x16c
[  188.830000] CPU: 0 PID: 110 Comm: kworker/u2:9 Tainted: G        W       4.10.17-00534-gb846201e7a2d #2277
[  188.830000] Hardware name: ASpeed SoC
[  188.830000] Workqueue: occ occ_worker
[  188.830000] [<800108f8>] (unwind_backtrace) from [<8000e02c>] (show_stack+0x20/0x24)
[  188.830000] [<8000e02c>] (show_stack) from [<8022d1c0>] (dump_stack+0x20/0x28)
[  188.830000] [<8022d1c0>] (dump_stack) from [<80048bf8>] (___might_sleep+0x174/0x2ac)
[  188.830000] [<80048bf8>] (___might_sleep) from [<80048da0>] (__might_sleep+0x70/0xb0)
[  188.830000] [<80048da0>] (__might_sleep) from [<801106f8>] (kmem_cache_alloc_trace+0x1c8/0x284)
[  188.830000] [<801106f8>] (kmem_cache_alloc_trace) from [<803330e8>] (sbefifo_write_common+0x228/0x4b4)
[  188.830000] [<803330e8>] (sbefifo_write_common) from [<80333740>] (sbefifo_drv_write+0x24/0x28)
[  188.830000] [<80333740>] (sbefifo_drv_write) from [<80333b58>] (occ_write_sbefifo+0x44/0x60)
[  188.830000] [<80333b58>] (occ_write_sbefifo) from [<80334530>] (occ_worker+0x38/0x4a0)
[  188.830000] [<80334530>] (occ_worker) from [<80034a2c>] (process_one_work+0x23c/0x6d4)
[  188.830000] [<80034a2c>] (process_one_work) from [<80034f08>] (worker_thread+0x44/0x528)
[  188.830000] [<80034f08>] (worker_thread) from [<8003c8c0>] (kthread+0x160/0x17c)
[  188.830000] [<8003c8c0>] (kthread) from [<8000a888>] (ret_from_fork+0x14/0x2c)

Avoid the warning (or using GFP_ATOMIC memory) by switching to a mutex, as we
only require mutual exclusion, not pre-emption be disabled (occ_worker() is
executed on a workqueue). This should also reduce the latency impact of calling
into the OCC.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-sbefifo.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 5b12eec2602b..cc9b9e36ac72 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -67,7 +67,7 @@ struct sbefifo {
 	wait_queue_head_t wait;
 	struct list_head xfrs;
 	struct kref kref;
-	spinlock_t list_lock;		/* lock access to the xfrs list */
+	struct mutex list_lock;		/* lock access to the xfrs list */
 	struct mutex sbefifo_lock;	/* lock access to the hardware */
 	char name[32];
 	int idx;
@@ -399,12 +399,11 @@ static void sbefifo_worker(struct work_struct *work)
 	int ret = 0;
 	u32 sts;
 	int i;
-	unsigned long flags;
 
-	spin_lock_irqsave(&sbefifo->list_lock, flags);
+	mutex_lock(&sbefifo->list_lock);
 	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
 				       xfrs);
-	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+	mutex_unlock(&sbefifo->list_lock);
 	if (!xfr)
 		return;
 
@@ -516,9 +515,9 @@ static void sbefifo_worker(struct work_struct *work)
 
 			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
 
-			spin_lock_irqsave(&sbefifo->list_lock, flags);
+			mutex_lock(&sbefifo->list_lock);
 			list_del(&xfr->xfrs);
-			spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+			mutex_unlock(&sbefifo->list_lock);
 
 			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
 					      &xfr->flags)))
@@ -535,17 +534,17 @@ static void sbefifo_worker(struct work_struct *work)
 		dev_err(&sbefifo->fsi_dev->dev,
 			"Fatal bus access failure: %d\n", ret);
 
-		spin_lock_irqsave(&sbefifo->list_lock, flags);
+		mutex_lock(&sbefifo->list_lock);
 		list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
 			list_del(&xfr->xfrs);
 			kfree(xfr);
 		}
 		INIT_LIST_HEAD(&sbefifo->xfrs);
-		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+		mutex_unlock(&sbefifo->list_lock);
 	} else if (eot) {
-		spin_lock_irqsave(&sbefifo->list_lock, flags);
+		mutex_lock(&sbefifo->list_lock);
 		xfr = sbefifo_next_xfr(sbefifo);
-		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+		mutex_unlock(&sbefifo->list_lock);
 
 		if (xfr) {
 			wake_up_interruptible(&sbefifo->wait);
@@ -715,7 +714,6 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	struct sbefifo_xfr *xfr;
 	ssize_t ret = 0;
 	size_t n;
-	unsigned long flags;
 
 	if ((len >> 2) << 2 != len)
 		return -EINVAL;
@@ -726,26 +724,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	sbefifo_get_client(client);
 	n = sbefifo_buf_nbwriteable(&client->wbuf);
 
-	spin_lock_irqsave(&sbefifo->list_lock, flags);
+	mutex_lock(&sbefifo->list_lock);
  
 	/* next xfr to be executed */
 	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
 				       xfrs);
 
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
-		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+		mutex_unlock(&sbefifo->list_lock);
 		ret = -EAGAIN;
 		goto out;
 	}
 
 	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
 	if (IS_ERR(xfr)) {
-		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+		mutex_unlock(&sbefifo->list_lock);
 		ret = PTR_ERR(xfr);
 		goto out;
 	}
 
-	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+	mutex_unlock(&sbefifo->list_lock);
 
 	/*
 	 * Partial writes are not really allowed in that EOT is sent exactly
@@ -954,7 +952,7 @@ static int sbefifo_probe(struct device *dev)
 		}
 	}
 
-	spin_lock_init(&sbefifo->list_lock);
+	mutex_init(&sbefifo->list_lock);
 	mutex_init(&sbefifo->sbefifo_lock);
 	kref_init(&sbefifo->kref);
 	init_waitqueue_head(&sbefifo->wait);
@@ -998,11 +996,10 @@ static int sbefifo_remove(struct device *dev)
 {
 	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 	struct sbefifo_xfr *xfr, *tmp;
-	unsigned long flags;
 
 	/* lock the sbefifo so to prevent deleting an ongoing xfr */
 	mutex_lock(&sbefifo->sbefifo_lock);
-	spin_lock_irqsave(&sbefifo->list_lock, flags);
+	mutex_lock(&sbefifo->list_lock);
 
 	WRITE_ONCE(sbefifo->rc, -ENODEV);
 	list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
@@ -1012,7 +1009,7 @@ static int sbefifo_remove(struct device *dev)
 
 	INIT_LIST_HEAD(&sbefifo->xfrs);
 
-	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+	mutex_unlock(&sbefifo->list_lock);
 	mutex_unlock(&sbefifo->sbefifo_lock);
 
 	wake_up_all(&sbefifo->wait);
-- 
2.14.1

Re: [PATCH linux dev-4.13 14/16] fsi: sbefifo: Switch list_lock from spinlock to mutex

[Eddie James]

On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> We allocate GFP_KERNEL memory for a list element in sbefifo_enq_xfr() (elided
> from the stack trace presumably by inlining) but do so under list_lock, which
> was previously a spin lock. Enabling lock debugging gives the following
> warning:

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> [  188.830000] BUG: sleeping function called from invalid context at mm/slab.h:408
> [  188.830000] in_atomic(): 1, irqs_disabled(): 128, pid: 110, name: kworker/u2:9
> [  188.830000] INFO: lockdep is turned off.
> [  188.830000] irq event stamp: 9002
> [  188.830000] hardirqs last  enabled at (9001): [<801105bc>] kmem_cache_alloc_trace+0x8c/0x284
> [  188.830000] hardirqs last disabled at (9002): [<8049458c>] _raw_spin_lock_irqsave+0x2c/0xa0
> [  188.830000] softirqs last  enabled at (5370): [<8000987c>] __do_softirq+0x38c/0x510
> [  188.830000] softirqs last disabled at (5363): [<8001f154>] irq_exit+0x120/0x16c
> [  188.830000] CPU: 0 PID: 110 Comm: kworker/u2:9 Tainted: G        W       4.10.17-00534-gb846201e7a2d #2277
> [  188.830000] Hardware name: ASpeed SoC
> [  188.830000] Workqueue: occ occ_worker
> [  188.830000] [<800108f8>] (unwind_backtrace) from [<8000e02c>] (show_stack+0x20/0x24)
> [  188.830000] [<8000e02c>] (show_stack) from [<8022d1c0>] (dump_stack+0x20/0x28)
> [  188.830000] [<8022d1c0>] (dump_stack) from [<80048bf8>] (___might_sleep+0x174/0x2ac)
> [  188.830000] [<80048bf8>] (___might_sleep) from [<80048da0>] (__might_sleep+0x70/0xb0)
> [  188.830000] [<80048da0>] (__might_sleep) from [<801106f8>] (kmem_cache_alloc_trace+0x1c8/0x284)
> [  188.830000] [<801106f8>] (kmem_cache_alloc_trace) from [<803330e8>] (sbefifo_write_common+0x228/0x4b4)
> [  188.830000] [<803330e8>] (sbefifo_write_common) from [<80333740>] (sbefifo_drv_write+0x24/0x28)
> [  188.830000] [<80333740>] (sbefifo_drv_write) from [<80333b58>] (occ_write_sbefifo+0x44/0x60)
> [  188.830000] [<80333b58>] (occ_write_sbefifo) from [<80334530>] (occ_worker+0x38/0x4a0)
> [  188.830000] [<80334530>] (occ_worker) from [<80034a2c>] (process_one_work+0x23c/0x6d4)
> [  188.830000] [<80034a2c>] (process_one_work) from [<80034f08>] (worker_thread+0x44/0x528)
> [  188.830000] [<80034f08>] (worker_thread) from [<8003c8c0>] (kthread+0x160/0x17c)
> [  188.830000] [<8003c8c0>] (kthread) from [<8000a888>] (ret_from_fork+0x14/0x2c)
>
> Avoid the warning (or using GFP_ATOMIC memory) by switching to a mutex, as we
> only require mutual exclusion, not pre-emption be disabled (occ_worker() is
> executed on a workqueue). This should also reduce the latency impact of calling
> into the OCC.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/fsi/fsi-sbefifo.c | 35 ++++++++++++++++-------------------
>   1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 5b12eec2602b..cc9b9e36ac72 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -67,7 +67,7 @@ struct sbefifo {
>   	wait_queue_head_t wait;
>   	struct list_head xfrs;
>   	struct kref kref;
> -	spinlock_t list_lock;		/* lock access to the xfrs list */
> +	struct mutex list_lock;		/* lock access to the xfrs list */
>   	struct mutex sbefifo_lock;	/* lock access to the hardware */
>   	char name[32];
>   	int idx;
> @@ -399,12 +399,11 @@ static void sbefifo_worker(struct work_struct *work)
>   	int ret = 0;
>   	u32 sts;
>   	int i;
> -	unsigned long flags;
>
> -	spin_lock_irqsave(&sbefifo->list_lock, flags);
> +	mutex_lock(&sbefifo->list_lock);
>   	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
>   				       xfrs);
> -	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +	mutex_unlock(&sbefifo->list_lock);
>   	if (!xfr)
>   		return;
>
> @@ -516,9 +515,9 @@ static void sbefifo_worker(struct work_struct *work)
>
>   			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
>
> -			spin_lock_irqsave(&sbefifo->list_lock, flags);
> +			mutex_lock(&sbefifo->list_lock);
>   			list_del(&xfr->xfrs);
> -			spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +			mutex_unlock(&sbefifo->list_lock);
>
>   			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
>   					      &xfr->flags)))
> @@ -535,17 +534,17 @@ static void sbefifo_worker(struct work_struct *work)
>   		dev_err(&sbefifo->fsi_dev->dev,
>   			"Fatal bus access failure: %d\n", ret);
>
> -		spin_lock_irqsave(&sbefifo->list_lock, flags);
> +		mutex_lock(&sbefifo->list_lock);
>   		list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
>   			list_del(&xfr->xfrs);
>   			kfree(xfr);
>   		}
>   		INIT_LIST_HEAD(&sbefifo->xfrs);
> -		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +		mutex_unlock(&sbefifo->list_lock);
>   	} else if (eot) {
> -		spin_lock_irqsave(&sbefifo->list_lock, flags);
> +		mutex_lock(&sbefifo->list_lock);
>   		xfr = sbefifo_next_xfr(sbefifo);
> -		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +		mutex_unlock(&sbefifo->list_lock);
>
>   		if (xfr) {
>   			wake_up_interruptible(&sbefifo->wait);
> @@ -715,7 +714,6 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>   	struct sbefifo_xfr *xfr;
>   	ssize_t ret = 0;
>   	size_t n;
> -	unsigned long flags;
>
>   	if ((len >> 2) << 2 != len)
>   		return -EINVAL;
> @@ -726,26 +724,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>   	sbefifo_get_client(client);
>   	n = sbefifo_buf_nbwriteable(&client->wbuf);
>
> -	spin_lock_irqsave(&sbefifo->list_lock, flags);
> +	mutex_lock(&sbefifo->list_lock);
>    
>   	/* next xfr to be executed */
>   	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
>   				       xfrs);
>
>   	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
> -		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +		mutex_unlock(&sbefifo->list_lock);
>   		ret = -EAGAIN;
>   		goto out;
>   	}
>
>   	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
>   	if (IS_ERR(xfr)) {
> -		spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +		mutex_unlock(&sbefifo->list_lock);
>   		ret = PTR_ERR(xfr);
>   		goto out;
>   	}
>
> -	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +	mutex_unlock(&sbefifo->list_lock);
>
>   	/*
>   	 * Partial writes are not really allowed in that EOT is sent exactly
> @@ -954,7 +952,7 @@ static int sbefifo_probe(struct device *dev)
>   		}
>   	}
>
> -	spin_lock_init(&sbefifo->list_lock);
> +	mutex_init(&sbefifo->list_lock);
>   	mutex_init(&sbefifo->sbefifo_lock);
>   	kref_init(&sbefifo->kref);
>   	init_waitqueue_head(&sbefifo->wait);
> @@ -998,11 +996,10 @@ static int sbefifo_remove(struct device *dev)
>   {
>   	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>   	struct sbefifo_xfr *xfr, *tmp;
> -	unsigned long flags;
>
>   	/* lock the sbefifo so to prevent deleting an ongoing xfr */
>   	mutex_lock(&sbefifo->sbefifo_lock);
> -	spin_lock_irqsave(&sbefifo->list_lock, flags);
> +	mutex_lock(&sbefifo->list_lock);
>
>   	WRITE_ONCE(sbefifo->rc, -ENODEV);
>   	list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
> @@ -1012,7 +1009,7 @@ static int sbefifo_remove(struct device *dev)
>
>   	INIT_LIST_HEAD(&sbefifo->xfrs);
>
> -	spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> +	mutex_unlock(&sbefifo->list_lock);
>   	mutex_unlock(&sbefifo->sbefifo_lock);
>
>   	wake_up_all(&sbefifo->wait);

[PATCH linux dev-4.13 15/16] fsi: gpio: Remove unused 'id' variable

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-master-gpio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 2a49b167effe..20b334f1827d 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -270,7 +270,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 		uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
 {
 	struct fsi_gpio_msg msg;
-	uint8_t id, tag;
+	uint8_t tag;
 	uint32_t crc;
 	int i;
 
@@ -295,7 +295,6 @@ static int read_one_response(struct fsi_master_gpio *master,
 	/* Read slave ID & response tag */
 	serial_in(master, &msg, 4);
 
-	id = (msg.msg >> FSI_GPIO_MSG_RESPID_SIZE) & 0x3;
 	tag = msg.msg & 0x3;
 
 	/* If we have an ACK and we're expecting data, clock the data in too */
-- 
2.14.1

[PATCH linux dev-4.13 16/16] fsi: gpio: Use a mutex to protect transfers

[Andrew Jeffery]

From: Jeremy Kerr <jk@ozlabs.org>

Reduce time spent with interrupts disabled by limiting the critical sections to
bitbanging FSI symbols. We only need to ensure exclusive use of the bus for an
entire transfer, not that the transfer be performed in atomic context.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/fsi-master-gpio.c | 90 +++++++++++++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 24 deletions(-)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 20b334f1827d..4295a46780cb 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -54,7 +54,8 @@
 struct fsi_master_gpio {
 	struct fsi_master	master;
 	struct device		*dev;
-	spinlock_t		cmd_lock;	/* Lock for commands */
+	struct mutex		cmd_lock;	/* mutex for command ordering */
+	spinlock_t		bit_lock;	/* lock for clocking bits out */
 	struct gpio_desc	*gpio_clk;
 	struct gpio_desc	*gpio_data;
 	struct gpio_desc	*gpio_trans;	/* Voltage translator */
@@ -270,10 +271,13 @@ static int read_one_response(struct fsi_master_gpio *master,
 		uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
 {
 	struct fsi_gpio_msg msg;
-	uint8_t tag;
+	unsigned long flags;
 	uint32_t crc;
+	uint8_t tag;
 	int i;
 
+	spin_lock_irqsave(&master->bit_lock, flags);
+
 	/* wait for the start bit */
 	for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {
 		msg.bits = 0;
@@ -286,6 +290,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 		dev_dbg(master->dev,
 			"Master time out waiting for response\n");
 		fsi_master_gpio_error(master, FSI_GPIO_MTOE);
+		spin_unlock_irqrestore(&master->bit_lock, flags);
 		return -EIO;
 	}
 
@@ -304,6 +309,8 @@ static int read_one_response(struct fsi_master_gpio *master,
 	/* read CRC */
 	serial_in(master, &msg, FSI_GPIO_CRC_SIZE);
 
+	spin_unlock_irqrestore(&master->bit_lock, flags);
+
 	/* we have a whole message now; check CRC */
 	crc = crc4(0, 1, 1);
 	crc = crc4(crc, msg.msg, msg.bits);
@@ -324,12 +331,16 @@ static int read_one_response(struct fsi_master_gpio *master,
 static int issue_term(struct fsi_master_gpio *master, uint8_t slave)
 {
 	struct fsi_gpio_msg cmd;
+	unsigned long flags;
 	uint8_t tag;
 	int rc;
 
 	build_term_command(&cmd, slave);
+
+	spin_lock_irqsave(&master->bit_lock, flags);
 	serial_out(master, &cmd);
 	echo_delay(master);
+	spin_unlock_irqrestore(&master->bit_lock, flags);
 
 	rc = read_one_response(master, 0, NULL, &tag);
 	if (rc < 0) {
@@ -349,6 +360,7 @@ static int poll_for_response(struct fsi_master_gpio *master,
 {
 	struct fsi_gpio_msg response, cmd;
 	int busy_count = 0, rc, i;
+	unsigned long flags;
 	uint8_t tag;
 	uint8_t *data_byte = data;
 
@@ -377,15 +389,20 @@ static int poll_for_response(struct fsi_master_gpio *master,
 		 * d-poll, not indicated in the hardware protocol
 		 * spec. < 20 clocks causes slave to hang, 21 ok.
 		 */
-		clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
 		if (busy_count++ < FSI_GPIO_MAX_BUSY) {
 			build_dpoll_command(&cmd, slave);
+			spin_lock_irqsave(&master->bit_lock, flags);
+			clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
 			serial_out(master, &cmd);
 			echo_delay(master);
+			spin_unlock_irqrestore(&master->bit_lock, flags);
 			goto retry;
 		}
 		dev_warn(master->dev,
 			"ERR slave is stuck in busy state, issuing TERM\n");
+		spin_lock_irqsave(&master->bit_lock, flags);
+		clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
+		spin_unlock_irqrestore(&master->bit_lock, flags);
 		issue_term(master, slave);
 		rc = -EIO;
 		break;
@@ -404,27 +421,42 @@ static int poll_for_response(struct fsi_master_gpio *master,
 		trace_fsi_master_gpio_poll_response_busy(master, busy_count);
 
 	/* Clock the slave enough to be ready for next operation */
+	spin_lock_irqsave(&master->bit_lock, flags);
 	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
+	spin_unlock_irqrestore(&master->bit_lock, flags);
 	return rc;
 }
 
+static int send_request(struct fsi_master_gpio *master,
+		struct fsi_gpio_msg *cmd)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->bit_lock, flags);
+	if (master->external_mode) {
+		spin_unlock_irqrestore(&master->bit_lock, flags);
+		return -EBUSY;
+	}
+
+	serial_out(master, cmd);
+	echo_delay(master);
+	spin_unlock_irqrestore(&master->bit_lock, flags);
+
+	return 0;
+}
+
 static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
 		struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
 {
-	unsigned long flags;
 	int rc;
 
-	spin_lock_irqsave(&master->cmd_lock, flags);
+	mutex_lock(&master->cmd_lock);
 
-	if (master->external_mode) {
-		spin_unlock_irqrestore(&master->cmd_lock, flags);
-		return -EBUSY;
-	}
+	rc = send_request(master, cmd);
+	if (!rc)
+		rc = poll_for_response(master, slave, resp_len, resp);
 
-	serial_out(master, cmd);
-	echo_delay(master);
-	rc = poll_for_response(master, slave, resp_len, resp);
-	spin_unlock_irqrestore(&master->cmd_lock, flags);
+	mutex_unlock(&master->cmd_lock);
 
 	return rc;
 }
@@ -478,11 +510,14 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 
 	trace_fsi_master_gpio_break(master);
 
-	spin_lock_irqsave(&master->cmd_lock, flags);
+	mutex_lock(&master->cmd_lock);
 	if (master->external_mode) {
-		spin_unlock_irqrestore(&master->cmd_lock, flags);
+		mutex_unlock(&master->cmd_lock);
 		return -EBUSY;
 	}
+
+	spin_lock_irqsave(&master->bit_lock, flags);
+
 	set_sda_output(master, 1);
 	sda_out(master, 1);
 	clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
@@ -491,7 +526,9 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 	echo_delay(master);
 	sda_out(master, 1);
 	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
-	spin_unlock_irqrestore(&master->cmd_lock, flags);
+
+	spin_unlock_irqrestore(&master->bit_lock, flags);
+	mutex_unlock(&master->cmd_lock);
 
 	/* Wait for logic reset to take effect */
 	udelay(200);
@@ -501,6 +538,8 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 
 static void fsi_master_gpio_init(struct fsi_master_gpio *master)
 {
+	unsigned long flags;
+
 	gpiod_direction_output(master->gpio_mux, 1);
 	gpiod_direction_output(master->gpio_trans, 1);
 	gpiod_direction_output(master->gpio_enable, 1);
@@ -508,7 +547,9 @@ static void fsi_master_gpio_init(struct fsi_master_gpio *master)
 	gpiod_direction_output(master->gpio_data, 1);
 
 	/* todo: evaluate if clocks can be reduced */
+	spin_lock_irqsave(&master->bit_lock, flags);
 	clock_zeros(master, FSI_INIT_CLOCKS);
+	spin_unlock_irqrestore(&master->bit_lock, flags);
 }
 
 static void fsi_master_gpio_init_external(struct fsi_master_gpio *master)
@@ -523,18 +564,17 @@ static void fsi_master_gpio_init_external(struct fsi_master_gpio *master)
 static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link)
 {
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
-	unsigned long flags;
 	int rc = -EBUSY;
 
 	if (link != 0)
 		return -ENODEV;
 
-	spin_lock_irqsave(&master->cmd_lock, flags);
+	mutex_lock(&master->cmd_lock);
 	if (!master->external_mode) {
 		gpiod_set_value(master->gpio_enable, 1);
 		rc = 0;
 	}
-	spin_unlock_irqrestore(&master->cmd_lock, flags);
+	mutex_unlock(&master->cmd_lock);
 
 	return rc;
 }
@@ -552,7 +592,7 @@ static ssize_t external_mode_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct fsi_master_gpio *master = dev_get_drvdata(dev);
-	unsigned long flags, val;
+	unsigned long val;
 	bool external_mode;
 	int err;
 
@@ -562,10 +602,10 @@ static ssize_t external_mode_store(struct device *dev,
 
 	external_mode = !!val;
 
-	spin_lock_irqsave(&master->cmd_lock, flags);
+	mutex_lock(&master->cmd_lock);
 
 	if (external_mode == master->external_mode) {
-		spin_unlock_irqrestore(&master->cmd_lock, flags);
+		mutex_unlock(&master->cmd_lock);
 		return count;
 	}
 
@@ -574,7 +614,8 @@ static ssize_t external_mode_store(struct device *dev,
 		fsi_master_gpio_init_external(master);
 	else
 		fsi_master_gpio_init(master);
-	spin_unlock_irqrestore(&master->cmd_lock, flags);
+
+	mutex_unlock(&master->cmd_lock);
 
 	fsi_master_rescan(&master->master);
 
@@ -642,7 +683,8 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	master->master.send_break = fsi_master_gpio_break;
 	master->master.link_enable = fsi_master_gpio_link_enable;
 	platform_set_drvdata(pdev, master);
-	spin_lock_init(&master->cmd_lock);
+	spin_lock_init(&master->bit_lock);
+	mutex_init(&master->cmd_lock);
 
 	fsi_master_gpio_init(master);
 
-- 
2.14.1