[PATCH linux dev-4.10 0/3] drivers: fsi: sbefifo: Fix race conditon, memory leak, and probe

[PATCH linux dev-4.10 0/3] drivers: fsi: sbefifo: Fix race conditon, memory leak, and probe

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This series fixes some outstanding issues with the SBE FIFO FSI client driver.
The most important fix is preventing a race condition in the remove() path.
The second patch is to fix a memory leak that was happening on most transfers.
The third patch is unrelated, though it will add some robustness to probing
the SBE FIFO, instead of failing immediately if we see some data already in
the fifo.

Edward A. James (3):
  drivers: fsi: sbefifo: Fix queued xfrs race condition
  drivers: fsi: sbefifo: Fix memory leak
  drivers: fsi: sbefifo: Attempt reset request during probe

 drivers/fsi/fsi-sbefifo.c | 97 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 71 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[PATCH linux dev-4.10 1/3] drivers: fsi: sbefifo: Fix queued xfrs race condition

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

There was a race condition where: sbefifo remove() is called, a transfer
is queued, a client is waiting, and a client is released at the same
time. In this situation, remove() wakes up the client waiter, which
destroys the client's list of transfers. This must be done since in an
error case, we don't know if the transfers are deleted or not. Then,
the client is released. Since it has no transfers, it fails to cancel
the queued transfer. This transfer then wakes up on the timer, where it
attempts to access it's client buffers, which have been freed.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 5d73437..c914eaf 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -312,22 +312,27 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
 
 static void sbefifo_client_release(struct kref *kref)
 {
+	struct sbefifo *sbefifo;
 	struct sbefifo_client *client;
 	struct sbefifo_xfr *xfr;
 
 	client = container_of(kref, struct sbefifo_client, kref);
-	list_for_each_entry(xfr, &client->xfrs, client) {
-		/*
-		 * The client left with pending or running xfrs.
-		 * Cancel them.
-		 */
-		set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
-		sbefifo_get(client->dev);
-		if (mod_timer(&client->dev->poll_timer, jiffies))
-			sbefifo_put(client->dev);
+	sbefifo = client->dev;
+
+	if (!READ_ONCE(sbefifo->rc)) {
+		list_for_each_entry(xfr, &client->xfrs, client) {
+			/*
+			 * The client left with pending or running xfrs.
+			 * Cancel them.
+			 */
+			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
+			sbefifo_get(sbefifo);
+			if (mod_timer(&client->dev->poll_timer, jiffies))
+				sbefifo_put(sbefifo);
+		}
 	}
 
-	sbefifo_put(client->dev);
+	sbefifo_put(sbefifo);
 	kfree(client);
 }
 
@@ -901,7 +906,16 @@ static int sbefifo_remove(struct device *dev)
 	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 	struct sbefifo_xfr *xfr;
 
+	spin_lock(&sbefifo->lock);
+
 	WRITE_ONCE(sbefifo->rc, -ENODEV);
+	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
+		kfree(xfr);
+
+	INIT_LIST_HEAD(&sbefifo->xfrs);
+
+	spin_unlock(&sbefifo->lock);
+
 	wake_up_all(&sbefifo->wait);
 
 	misc_deregister(&sbefifo->mdev);
@@ -912,11 +926,6 @@ static int sbefifo_remove(struct device *dev)
 	if (del_timer_sync(&sbefifo->poll_timer))
 		sbefifo_put(sbefifo);
 
-	spin_lock(&sbefifo->lock);
-	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
-		kfree(xfr);
-	spin_unlock(&sbefifo->lock);
-
 	sbefifo_put(sbefifo);
 
 	return 0;
-- 
1.8.3.1

[PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Fix memory leak

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Transfers weren't being cleaned up if they were complete but the user
never finished reading. This is very common.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index c914eaf..764d8f3 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -321,6 +321,11 @@ static void sbefifo_client_release(struct kref *kref)
 
 	if (!READ_ONCE(sbefifo->rc)) {
 		list_for_each_entry(xfr, &client->xfrs, client) {
+			if (test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
+				kfree(xfr);
+				continue;
+			}
+
 			/*
 			 * The client left with pending or running xfrs.
 			 * Cancel them.
-- 
1.8.3.1

[PATCH linux dev-4.10 3/3] drivers: fsi: sbefifo: Attempt reset request during probe

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

If data is found in the FIFO, the driver currently immediately fails
the probe. Instead, try a reset request and poll for it's completion for
a while (this protocol is defined in the SBE FIFO spec, though the
timeout lengths are my own invention).

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 53 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 764d8f3..f756822 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -47,8 +47,10 @@
 
 #define SBEFIFO_STS		0x04
 #define   SBEFIFO_EMPTY			BIT(20)
+#define   SBEFIFO_STS_RESET_REQ		BIT(25)
 #define SBEFIFO_EOT_RAISE	0x08
 #define   SBEFIFO_EOT_MAGIC		0xffffffff
+#define SBEFIFO_REQ_RESET	0x0C
 #define SBEFIFO_EOT_ACK		0x14
 
 #define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
@@ -831,6 +833,36 @@ static int sbefifo_unregister_child(struct device *dev, void *data)
 	return 0;
 }
 
+static int sbefifo_request_reset(struct sbefifo *sbefifo)
+{
+	int ret;
+	u32 status;
+	unsigned long start;
+	const unsigned int wait_time = 5;	/* jiffies */
+	const unsigned long timeout = msecs_to_jiffies(250);
+
+	ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_REQ_RESET, 1);
+	if (ret)
+		return ret;
+
+	start = jiffies;
+
+	do {
+		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &status);
+		if (ret)
+			return ret;
+
+		if (!(status & SBEFIFO_STS_RESET_REQ))
+			return 0;
+
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (schedule_timeout(wait_time) > 0)
+			return -EINTR;
+	} while (time_after(start + timeout, jiffies));
+
+	return -ETIME;
+}
+
 static int sbefifo_probe(struct device *dev)
 {
 	struct fsi_device *fsi_dev = to_fsi_dev(dev);
@@ -838,7 +870,7 @@ static int sbefifo_probe(struct device *dev)
 	struct device_node *np;
 	struct platform_device *child;
 	char child_name[32];
-	u32 sts;
+	u32 up, down;
 	int ret, child_idx = 0;
 
 	dev_dbg(dev, "Found sbefifo device\n");
@@ -848,22 +880,21 @@ static int sbefifo_probe(struct device *dev)
 
 	sbefifo->fsi_dev = fsi_dev;
 
-	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
+	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &up);
 	if (ret)
 		return ret;
 
-	if (!(sts & SBEFIFO_EMPTY)) {
-		dev_err(dev, "Found data in upstream fifo\n");
-		return -EIO;
-	}
-
-	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
+	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &down);
 	if (ret)
 		return ret;
 
-	if (!(sts & SBEFIFO_EMPTY)) {
-		dev_err(dev, "Found data in downstream fifo\n");
-		return -EIO;
+	if (!(up & SBEFIFO_EMPTY) || !(down & SBEFIFO_EMPTY)) {
+		ret = sbefifo_request_reset(sbefifo);
+		if (ret) {
+			dev_err(dev,
+				"fifos weren't empty and failed the reset\n");
+			return ret;
+		}
 	}
 
 	spin_lock_init(&sbefifo->lock);
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Fix memory leak

[Andrew Jeffery]

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

On Tue, 2017-10-17 at 16:35 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Transfers weren't being cleaned up if they were complete but the user
> never finished reading. This is very common.

I'm curious as to why this is common, but regardless, the patch looks
fine given the premise.

Andrew

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index c914eaf..764d8f3 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -321,6 +321,11 @@ static void sbefifo_client_release(struct kref *kref)
>  
>  	if (!READ_ONCE(sbefifo->rc)) {
>  		list_for_each_entry(xfr, &client->xfrs, client) {
> +			if (test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
> +				kfree(xfr);
> +				continue;
> +			}
> +
>  			/*
>  			 * The client left with pending or running xfrs.
>  			 * Cancel them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Fix memory leak

[Eddie James]

On 10/18/2017 08:24 AM, Andrew Jeffery wrote:
> On Tue, 2017-10-17 at 16:35 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Transfers weren't being cleaned up if they were complete but the user
>> never finished reading. This is very common.
> I'm curious as to why this is common, but regardless, the patch looks
> fine given the premise.

Users just seemed to do this very frequently, from my tracing. They just 
don't finish reading all the available data and instead just close the 
client. That's because the actual data packet is some number of words 
less than the amount of data read and returned by the SBEFIFO. In 
practice, a user should read all the extra bytes to check them to make 
sure the transfer was successful, but it's actually difficult to know 
how much extra data is there.

This only happens if the driver has actually read all the data out of 
the FIFO and therefore removed the transfer from it's queue.

Thanks,
Eddie

>
> Andrew
>
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>   
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index c914eaf..764d8f3 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -321,6 +321,11 @@ static void sbefifo_client_release(struct kref *kref)
>>   
>>   	if (!READ_ONCE(sbefifo->rc)) {
>>   		list_for_each_entry(xfr, &client->xfrs, client) {
>> +			if (test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>> +				kfree(xfr);
>> +				continue;
>> +			}
>> +
>>   			/*
>>   			 * The client left with pending or running xfrs.
>>   			 * Cancel them.

Re: [PATCH linux dev-4.10 3/3] drivers: fsi: sbefifo: Attempt reset request during probe

[Andrew Jeffery]

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

On Tue, 2017-10-17 at 16:35 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> If data is found in the FIFO, the driver currently immediately fails
> the probe. Instead, try a reset request and poll for it's completion for
> a while (this protocol is defined in the SBE FIFO spec, though the
> timeout lengths are my own invention).
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 53 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 764d8f3..f756822 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -47,8 +47,10 @@
>  
>  #define SBEFIFO_STS		0x04
>  #define   SBEFIFO_EMPTY			BIT(20)
> +#define   SBEFIFO_STS_RESET_REQ		BIT(25)
>  #define SBEFIFO_EOT_RAISE	0x08
>  #define   SBEFIFO_EOT_MAGIC		0xffffffff
> +#define SBEFIFO_REQ_RESET	0x0C

Bit of a nit, but it would be nice to have consistency between RESET_REQ and
REQ_RESET. I don't see any reason for it to be inconsistent from reading the
documentation. However that can probably be left for some other time.

Andrew

>  #define SBEFIFO_EOT_ACK		0x14
>  
>  #define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
> @@ -831,6 +833,36 @@ static int sbefifo_unregister_child(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +static int sbefifo_request_reset(struct sbefifo *sbefifo)
> +{
> +	int ret;
> +	u32 status;
> +	unsigned long start;
> +	const unsigned int wait_time = 5;	/* jiffies */
> +	const unsigned long timeout = msecs_to_jiffies(250);
> +
> +	ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_REQ_RESET, 1);
> +	if (ret)
> +		return ret;
> +
> +	start = jiffies;
> +
> +	do {
> +		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &status);
> +		if (ret)
> +			return ret;
> +
> +		if (!(status & SBEFIFO_STS_RESET_REQ))
> +			return 0;
> +
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (schedule_timeout(wait_time) > 0)
> +			return -EINTR;
> +	} while (time_after(start + timeout, jiffies));
> +
> +	return -ETIME;
> +}
> +
>  static int sbefifo_probe(struct device *dev)
>  {
>  	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> @@ -838,7 +870,7 @@ static int sbefifo_probe(struct device *dev)
>  	struct device_node *np;
>  	struct platform_device *child;
>  	char child_name[32];
> -	u32 sts;
> +	u32 up, down;
>  	int ret, child_idx = 0;
>  
>  	dev_dbg(dev, "Found sbefifo device\n");
> @@ -848,22 +880,21 @@ static int sbefifo_probe(struct device *dev)
>  
>  	sbefifo->fsi_dev = fsi_dev;
>  
> -	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
> +	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &up);
>  	if (ret)
>  		return ret;
>  
> -	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(dev, "Found data in upstream fifo\n");
> -		return -EIO;
> -	}
> -
> -	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
> +	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &down);
>  	if (ret)
>  		return ret;
>  
> -	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(dev, "Found data in downstream fifo\n");
> -		return -EIO;
> +	if (!(up & SBEFIFO_EMPTY) || !(down & SBEFIFO_EMPTY)) {
> +		ret = sbefifo_request_reset(sbefifo);
> +		if (ret) {
> +			dev_err(dev,
> +				"fifos weren't empty and failed the reset\n");
> +			return ret;
> +		}
>  	}
>  
>  	spin_lock_init(&sbefifo->lock);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 0/3] drivers: fsi: sbefifo: Fix race conditon, memory leak, and probe

[Andrew Jeffery]

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

On Tue, 2017-10-17 at 16:35 -0500, Eddie James wrote:
> > From: "Edward A. James" <eajames@us.ibm.com>
> 
> This series fixes some outstanding issues with the SBE FIFO FSI client driver.
> The most important fix is preventing a race condition in the remove() path.
> The second patch is to fix a memory leak that was happening on most transfers.
> The third patch is unrelated, though it will add some robustness to probing
> the SBE FIFO, instead of failing immediately if we see some data already in
> the fifo.
> 
> Edward A. James (3):
>   drivers: fsi: sbefifo: Fix queued xfrs race condition
>   drivers: fsi: sbefifo: Fix memory leak
>   drivers: fsi: sbefifo: Attempt reset request during probe

Applied to dev-4.10.

Cheers,

Andrew

> 
>  drivers/fsi/fsi-sbefifo.c | 97 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 71 insertions(+), 26 deletions(-)
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]