drivers: fsi: sbefifo: Fix race conditon, memory leak, and probe
cover drivers: fsi: sbefifo: Fix race conditon, memory leak, and probe
From: "Edward A. James" <[email protected]>
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(-)
Signature · 1 line
1.8.3.1
PATCH 1/3 drivers: fsi: sbefifo: Fix queued xfrs race condition
From: "Edward A. James" <[email protected]>
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.
--- drivers/fsi/fsi-sbefifo.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)
drivers/fsi/fsi-sbefifo.c +24 -15
@@ -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 ;
Signature · 1 line
1.8.3.1
PATCH 2/3 drivers: fsi: sbefifo: Fix memory leak
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Transfers weren't being cleaned up if they were complete but the user never finished reading. This is very common.
--- drivers/fsi/fsi-sbefifo.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/fsi/fsi-sbefifo.c +5 -0
@@ -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.
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Fix memory leak
Quoted 4 lines · Eddie James
From: "Edward A. James" <[email protected]>
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
Quoted 22 lines
Signed-off-by Edward A. James <[email protected]>---
drivers/fsi/fsi-sbefifo.c | 5 +++++1 file changed, 5 insertions(+)drivers/fsi/fsi-sbefifo.c +0
-0@@ -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){drivers/fsi/fsi-sbefifo.c +5
-0+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 2/3] drivers: fsi: sbefifo: Fix memory leak
Quoted 7 lines · Andrew Jeffery
Quoted 4 lines · Eddie James
From: "Edward A. James" <[email protected]>
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
Quoted 25 lines
Andrew
Quoted 22 lines
Signed-off-by Edward A. James <[email protected]>--- drivers/fsi/fsi-sbefifo.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/fsi/fsi-sbefifo.c +5
-0@@ -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.
PATCH 3/3 drivers: fsi: sbefifo: Attempt reset request during probe
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
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).
--- drivers/fsi/fsi-sbefifo.c | 53 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 11 deletions(-)
drivers/fsi/fsi-sbefifo.c +42 -11
@@ -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);
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 3/3] drivers: fsi: sbefifo: Attempt reset request during probe
Quoted 17 lines · Eddie James
From: "Edward A. James" <[email protected]>
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 <[email protected]>---
drivers/fsi/fsi-sbefifo.c | 53 +++++++++++++++++++++++++++++++++++++----------1 file changed, 42 insertions(+), 11 deletions(-)drivers/fsi/fsi-sbefifo.c +0
-0@@ -47,8 +47,10 @@
Quoted 7 lines
#defineSBEFIFO_STS0x04
#define SBEFIFO_EMPTY BIT(20)
+#define SBEFIFO_STS_RESET_REQ BIT(25)
#defineSBEFIFO_EOT_RAISE0x08
#define SBEFIFO_EOT_MAGIC0xffffffff
+#defineSBEFIFO_REQ_RESET0x0C
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
Quoted 81 lines
#define
SBEFIFO_EOT_ACK0x14
#defineSBEFIFO_RESCHEDULEmsecs_to_jiffies(500)drivers/fsi/fsi-sbefifo.c +0
-0@@ -831,6 +833,36 @@ static int sbefifo_unregister_child(struct device *dev, void *data) return 0; }drivers/fsi/fsi-sbefifo.c +30
-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){
structfsi_device*fsi_dev=to_fsi_dev(dev);drivers/fsi/fsi-sbefifo.c +1
-1@@ -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");drivers/fsi/fsi-sbefifo.c +0
-0@@ -848,22 +880,21 @@ static int sbefifo_probe(struct device *dev)
sbefifo->fsi_dev=fsi_dev;drivers/fsi/fsi-sbefifo.c +1
-1- ret =sbefifo_inw (sbefifo,SBEFIFO_UP |SBEFIFO_STS , &sts); + ret =sbefifo_inw (sbefifo,SBEFIFO_UP |SBEFIFO_STS , &up);if (ret)
return ret;drivers/fsi/fsi-sbefifo.c +1
-6-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;drivers/fsi/fsi-sbefifo.c +7
-3-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);
Andrew Jeffery
AppliedQuoted 9 lines · Eddie James
Quoted 1 line
From: "Edward A. James" <[email protected]>
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.
Quoted 4 lines
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
Quoted 4 lines
drivers/fsi/fsi-sbefifo.c | 97 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 71 insertions(+), 26 deletions(-)