[PATCH linux dev-4.13 0/2] Fix SBEFIFO chardev read behaviour

[PATCH linux dev-4.13 0/2] Fix SBEFIFO chardev read behaviour

[Andrew Jeffery]

Hello,

read() on the SBEFIFO chardevs is currently unusuable as it will always return
-EAGAIN on a read subsequent to the EOT flag being observed. This breaks reads
using short buffers as we now have no way to know when we have received the
entire message. This is made infinitely worse by the protocol, which as
defined, means we cannot know how much data must be read, and therefore all
buffers are effectively short*.

These two patches help by at least returning 0 for a read subsequent to one
which drains the last byte from the transfer buffer. A read subsequent to one
returning 0 will then return -EAGAIN if necessary, under the expectation that a
write() has been issued.

This is a work-around until BenH gets his alternative SBEFIFO implementation in
shape.

* This is true to a point - it recently came to light that we can expect any
  FFDC to be at a maximum 8K in size, and the maximum size of the transfer is
  defined as:

  * The requested response size, plus
  * The status "header", plus
  * Up to 8K of FFDC

Andrew Jeffery (2):
  fsi: sbefifo: Unpack !sbefifo_xfr_rsp_pending(client) test
  fsi: sbefifo: Return 0 on read() to indicate end of response

 drivers/fsi/fsi-sbefifo.c | 48 +++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 10 deletions(-)

-- 
2.17.0

[PATCH linux dev-4.13 1/2] fsi: sbefifo: Unpack !sbefifo_xfr_rsp_pending(client) test

[Andrew Jeffery]

Currently a read() on a non-blocking SBEFIFO file-descriptor will return
EAGAIN despite the entire response having been read out of the device on
a prior read. This is broken if userspace performs partial reads due to
undersizing its destination buffer relative to the (unknowable) response
size.

This change doesn't fix the bug, rather lays the ground-work for the fix
by reworking the EAGAIN handling in sbefifo_read_common().

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

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index cc9b9e36ac72..afb5ff48aa3b 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -89,6 +89,7 @@ struct sbefifo_xfr {
 	struct list_head client;
 	struct list_head xfrs;
 	unsigned long flags;
+#define SBEFIFO_XFR_INIT		0
 #define SBEFIFO_XFR_WRITE_DONE		1
 #define SBEFIFO_XFR_RESP_PENDING	2
 #define SBEFIFO_XFR_COMPLETE		3
@@ -292,16 +293,21 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
 	return xfr;
 }
 
-static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
+static bool sbefifo_xfr_test(struct sbefifo_client *client, uint8_t idx)
 {
 	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
 							   struct sbefifo_xfr,
 							   client);
 
-	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
-		return true;
+	if (!xfr)
+		return false;
 
-	return false;
+	/* No flags set indicates no action has been attempted on the xfr */
+	if (!idx)
+		return !xfr->flags;
+
+	/* Otherwise some bit will have been set, so test it */
+	return test_bit(idx, &xfr->flags);
 }
 
 static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
@@ -626,8 +632,20 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 	if ((len >> 2) << 2 != len)
 		return -EINVAL;
 
-	if ((client->f_flags & O_NONBLOCK) && !sbefifo_xfr_rsp_pending(client))
-		return -EAGAIN;
+	if (client->f_flags & O_NONBLOCK) {
+		/* Performing a read before completing a write */
+		if (sbefifo_xfr_test(client, SBEFIFO_XFR_INIT))
+			return -EAGAIN;
+
+		if (sbefifo_xfr_test(client, SBEFIFO_XFR_WRITE_DONE))
+			return -EAGAIN;
+
+		if (sbefifo_xfr_test(client, SBEFIFO_XFR_COMPLETE))
+			return -EAGAIN;
+
+		if (sbefifo_xfr_test(client, SBEFIFO_XFR_CANCEL))
+			return -EAGAIN;
+	}
 
 	sbefifo_get_client(client);
 	if (wait_event_interruptible(sbefifo->wait,
-- 
2.17.0

[PATCH linux dev-4.13 2/2] fsi: sbefifo: Return 0 on read() to indicate end of response

[Andrew Jeffery]

Previously the driver would return EAGAIN for a read() subsequent to
last byte being read. This makes it difficult to userspace to read
messages out of the pipe without knowing the size of the response ahead
of time, which is actually impossible given the design of the hardware.

The justification for moving the SBEFIFO_XFR_COMPLETE test is as
follows:

1. Assume that the transfer has completed from the hardware perspective,
   that is, we've acknowledged the EOT flag and set SBEFIFO_XFR_COMPLETE,
   but the client (caller) has not completely read out the response data

2. The client calls through sbefifo_read_common() and hits
   wait_event_interruptible(), which calls through sbefifo_read_ready()
   to judge the state of the transfer

3. sbefifo_read_ready() reports the number of words available and tests
   for SBEFIFO_XFR_COMPLETE. Given the assumed state above this will
   cause wait_event_interruptible() to return immediately

4. The appropriate copy is performed dependent on the client and the
   relative buffer sizes

5. If the destination buffer is smaller than the remaining transfer data
   then sbefifo_buf_readnb() will return false by its ring-buffer index
   calculations, the transfer will remain the current transfer, and we
   repeat from step 2.

6. If the destination buffer is equal to or larger than the remaining
   transfer data then sbefifo_buf_readnb() will return true indicating
   the transfer has been completely drained.

7. Assuming we take 6, we go around again, repeating steps 2 and 3,
   however sbefifo_read_read() will report 0 this time around, which
   triggers the removal of the transfer from the client and transfers
   list, and returns 0 to the caller.

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

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index afb5ff48aa3b..6f0e3f6c53d5 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -640,9 +640,6 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		if (sbefifo_xfr_test(client, SBEFIFO_XFR_WRITE_DONE))
 			return -EAGAIN;
 
-		if (sbefifo_xfr_test(client, SBEFIFO_XFR_COMPLETE))
-			return -EAGAIN;
-
 		if (sbefifo_xfr_test(client, SBEFIFO_XFR_CANCEL))
 			return -EAGAIN;
 	}
@@ -666,6 +663,23 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 
 	n = min_t(size_t, n, len);
 
+	/* Check if we've performed the final read */
+	if (sbefifo_xfr_test(client, SBEFIFO_XFR_COMPLETE) && !n) {
+		xfr = list_first_entry_or_null(&client->xfrs,
+					       struct sbefifo_xfr, client);
+		if (!xfr) {
+			/* should be impossible to not have an xfr here */
+			WARN_ONCE(1, "no xfr in queue");
+			ret = -EPROTO;
+			goto out;
+		}
+
+		list_del(&xfr->client);
+		kfree(xfr);
+		wake_up_interruptible(&sbefifo->wait);
+		return 0;
+	}
+
 	if (ubuf) {
 		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
 			ret = -EFAULT;
@@ -690,10 +704,6 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 			sbefifo_get(sbefifo);
 			if (!queue_work(sbefifo_wq, &sbefifo->work.work))
 				sbefifo_put(sbefifo);
-		} else {
-			list_del(&xfr->client);
-			kfree(xfr);
-			wake_up_interruptible(&sbefifo->wait);
 		}
 	}
 
-- 
2.17.0

Re: [PATCH linux dev-4.13 0/2] Fix SBEFIFO chardev read behaviour

[Eddie James]

On 05/14/2018 01:18 AM, Andrew Jeffery wrote:
> Hello,
>
> read() on the SBEFIFO chardevs is currently unusuable as it will always return
> -EAGAIN on a read subsequent to the EOT flag being observed. This breaks reads
> using short buffers as we now have no way to know when we have received the
> entire message. This is made infinitely worse by the protocol, which as
> defined, means we cannot know how much data must be read, and therefore all
> buffers are effectively short*.
>
> These two patches help by at least returning 0 for a read subsequent to one
> which drains the last byte from the transfer buffer. A read subsequent to one
> returning 0 will then return -EAGAIN if necessary, under the expectation that a
> write() has been issued.

Thanks Andrew, I haven't dug into the diffs too much but I just tested 
the series and it appears to have made the in-kernel API less reliable. 
I'm getting some errors when attempting to use the OCC driver.

Thanks,
Eddie

>
> This is a work-around until BenH gets his alternative SBEFIFO implementation in
> shape.
>
> * This is true to a point - it recently came to light that we can expect any
>    FFDC to be at a maximum 8K in size, and the maximum size of the transfer is
>    defined as:
>
>    * The requested response size, plus
>    * The status "header", plus
>    * Up to 8K of FFDC
>
> Andrew Jeffery (2):
>    fsi: sbefifo: Unpack !sbefifo_xfr_rsp_pending(client) test
>    fsi: sbefifo: Return 0 on read() to indicate end of response
>
>   drivers/fsi/fsi-sbefifo.c | 48 +++++++++++++++++++++++++++++++--------
>   1 file changed, 38 insertions(+), 10 deletions(-)
>

Re: [PATCH linux dev-4.13 0/2] Fix SBEFIFO chardev read behaviour

[Andrew Jeffery]

Hi Eddie,

On Tue, 15 May 2018, at 07:57, Eddie James wrote:
> 
> 
> On 05/14/2018 01:18 AM, Andrew Jeffery wrote:
> > Hello,
> >
> > read() on the SBEFIFO chardevs is currently unusuable as it will always return
> > -EAGAIN on a read subsequent to the EOT flag being observed. This breaks reads
> > using short buffers as we now have no way to know when we have received the
> > entire message. This is made infinitely worse by the protocol, which as
> > defined, means we cannot know how much data must be read, and therefore all
> > buffers are effectively short*.
> >
> > These two patches help by at least returning 0 for a read subsequent to one
> > which drains the last byte from the transfer buffer. A read subsequent to one
> > returning 0 will then return -EAGAIN if necessary, under the expectation that a
> > write() has been issued.
> 
> Thanks Andrew, I haven't dug into the diffs too much but I just tested 
> the series and it appears to have made the in-kernel API less reliable. 
> I'm getting some errors when attempting to use the OCC driver.

Thanks for testing. What errors are you sometimes seeing? I need some more info to reproduce and understand what might be going wrong.

Cheers,

Andrew

Re: [PATCH linux dev-4.13 0/2] Fix SBEFIFO chardev read behaviour

[Eddie James]

On 05/14/2018 06:09 PM, Andrew Jeffery wrote:
> Hi Eddie,
>
> On Tue, 15 May 2018, at 07:57, Eddie James wrote:
>>
>> On 05/14/2018 01:18 AM, Andrew Jeffery wrote:
>>> Hello,
>>>
>>> read() on the SBEFIFO chardevs is currently unusuable as it will always return
>>> -EAGAIN on a read subsequent to the EOT flag being observed. This breaks reads
>>> using short buffers as we now have no way to know when we have received the
>>> entire message. This is made infinitely worse by the protocol, which as
>>> defined, means we cannot know how much data must be read, and therefore all
>>> buffers are effectively short*.
>>>
>>> These two patches help by at least returning 0 for a read subsequent to one
>>> which drains the last byte from the transfer buffer. A read subsequent to one
>>> returning 0 will then return -EAGAIN if necessary, under the expectation that a
>>> write() has been issued.
>> Thanks Andrew, I haven't dug into the diffs too much but I just tested
>> the series and it appears to have made the in-kernel API less reliable.
>> I'm getting some errors when attempting to use the OCC driver.
> Thanks for testing. What errors are you sometimes seeing? I need some more info to reproduce and understand what might be going wrong.

Any of EIO, ENODATA, and EBADMSG from the OCC driver. All I did was 
power the system on, and would see the errors most of the time when the 
host attempted to set the OCCs active.

Thanks,
Eddie

>
> Cheers,
>
> Andrew
>