This v3 of a series of patches for the ST95HF driver.
Patch #1 reverts a change that I have submitted earlier and which is
sitting in nfc-next already. Given that the tree hasn't been sent out
for being merged yet, it could also still be removed with rebasing, in
which case #1 is not necessary of course.
The changes are all rather simple and are explained in their individual
commit logs.
Note that this series builds upon the patch titled "nfc: st95hf: remove
redundant pointers 'dev' and 'nfcddev'" that Colin posted the other day.
Thanks,
Daniel
Changelog:
v1 → v2:
* Improved commit logs, identical patch content.
v2 → v3:
* Added another patch titled "NFC: st95hf: ignore spurious interrupts"
Daniel Mack (11):
Revert "NFC: st95hf: drop illegal kfree_skb()"
NFC: st95hf: drop nfcdev_free
NFC: st95hf: drop illegal kfree_skb() in IRQ handler
NFC: st95hf: remove logging from spi functions
NFC: st95hf: remove exchange_lock
NFC: st95hf: move skb allocation to ISR
NFC: st95hf: ignore spurious interrupts
NFC: st95hf: re-order command defines
NFC: st95hf: unify sync/async flags
NFC: st95hf: two small style nits
NFC: st95hf: add of match table
drivers/nfc/st95hf/core.c | 154 ++++++++++++++------------------------
drivers/nfc/st95hf/spi.c | 31 +++-----
drivers/nfc/st95hf/spi.h | 8 +-
3 files changed, 66 insertions(+), 127 deletions(-)
--
2.17.1
In the error path of the IRQ handler, don't free the skb in flight. The
callback in the digital core will do that for us. Doing it from both
places causes a memory corruption.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index d58424ab5c48..d857197ec7b2 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -863,7 +863,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
return IRQ_HANDLED;
end:
- kfree_skb(skb_resp);
wtx = false;
cb_arg->rats = false;
skb_resp = ERR_PTR(result);
--
2.17.1
Add a match table for device tree compatible strings. Interestingly, a
document describing the bindings already exists since a while, but users
currently rely on the implicit matching in the drivers core. Let's be
explicit and add a real match table.
Signed-off-by: Daniel Mack <[email protected]>
Cc: [email protected]
---
drivers/nfc/st95hf/core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index ccb1f1456871..6161838fcf08 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -1027,6 +1027,12 @@ static const struct spi_device_id st95hf_id[] = {
};
MODULE_DEVICE_TABLE(spi, st95hf_id);
+static const struct of_device_id st95hf_of_match[] = {
+ { .compatible = "st,st95hf", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, st95hf_of_match);
+
static int st95hf_probe(struct spi_device *nfc_spi_dev)
{
int ret;
@@ -1204,6 +1210,7 @@ static struct spi_driver st95hf_driver = {
.driver = {
.name = "st95hf",
.owner = THIS_MODULE,
+ .of_match_table = st95hf_of_match,
},
.id_table = st95hf_id,
.probe = st95hf_probe,
--
2.17.1
This patch removes the exchange_lock sempahore. Its intended function
was two-fold:
a) Lock the remove() callback of the driver against the ISR, so that
the resources only go away after the ISR has finished. This is
unnecessary though, because `rm_lock' does that already, in
combination with the nullification of `scontext->ddev'.
b) Indicate whether a command was sent previously. If the semaphore
is found unused in the threaded ISR, an error is reported.
This case can be handled much nicer by checking whether `skb_resp'
is present in the context. For this, nullify the `skb_resp' pointer
in the callback context after it was sent back to the NFC core.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 52 +++++++--------------------------------
1 file changed, 9 insertions(+), 43 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index d857197ec7b2..6761ab90f68d 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -214,8 +214,6 @@ struct st95_digital_cmd_complete_arg {
* @st95hf_supply: regulator "consumer" for NFC device.
* @sendrcv_trflag: last byte of frame send by sendrecv command
* of st95hf. This byte contains transmission flag info.
- * @exchange_lock: semaphore used for signaling the st95hf_remove
- * function that the last outstanding async nfc request is finished.
* @rm_lock: mutex for ensuring safe access of nfc digital object
* from threaded ISR. Usage of this mutex avoids any race between
* deletion of the object from st95hf_remove() and its access from
@@ -233,7 +231,6 @@ struct st95hf_context {
struct st95_digital_cmd_complete_arg complete_cb_arg;
struct regulator *st95hf_supply;
unsigned char sendrcv_trflag;
- struct semaphore exchange_lock;
struct mutex rm_lock;
u8 current_protocol;
u8 current_rf_tech;
@@ -785,29 +782,14 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
struct st95_digital_cmd_complete_arg *cb_arg;
spidevice = &stcontext->spicontext.spidev->dev;
+ cb_arg = &stcontext->complete_cb_arg;
+ skb_resp = cb_arg->skb_resp;
- /*
- * check semaphore, if not down() already, then we don't
- * know in which context the ISR is called and surely it
- * will be a bug. Note that down() of the semaphore is done
- * in the corresponding st95hf_in_send_cmd() and then
- * only this ISR should be called. ISR will up() the
- * semaphore before leaving. Hence when the ISR is called
- * the correct behaviour is down_trylock() should always
- * return 1 (indicating semaphore cant be taken and hence no
- * change in semaphore count).
- * If not, then we up() the semaphore and crash on
- * a BUG() !
- */
- if (!down_trylock(&stcontext->exchange_lock)) {
- up(&stcontext->exchange_lock);
+ if (unlikely(!skb_resp)) {
WARN(1, "unknown context in ST95HF ISR");
return IRQ_NONE;
}
- cb_arg = &stcontext->complete_cb_arg;
- skb_resp = cb_arg->skb_resp;
-
mutex_lock(&stcontext->rm_lock);
res_len = st95hf_spi_recv_response(&stcontext->spicontext,
skb_resp->data);
@@ -856,8 +838,13 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
/* call digital layer callback */
cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
+ /*
+ * This skb is now owned by the core layer.
+ * Make sure not to use it again.
+ */
+ cb_arg->skb_resp = NULL;
+
/* up the semaphore before returning */
- up(&stcontext->exchange_lock);
mutex_unlock(&stcontext->rm_lock);
return IRQ_HANDLED;
@@ -868,8 +855,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
skb_resp = ERR_PTR(result);
/* call of callback with error */
cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
- /* up the semaphore before returning */
- up(&stcontext->exchange_lock);
mutex_unlock(&stcontext->rm_lock);
return IRQ_HANDLED;
}
@@ -965,25 +950,12 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
ddev->curr_protocol == NFC_PROTO_ISO14443)
stcontext->complete_cb_arg.rats = true;
- /*
- * down the semaphore to indicate to remove func that an
- * ISR is pending, note that it will not block here in any case.
- * If found blocked, it is a BUG!
- */
- rc = down_killable(&stcontext->exchange_lock);
- if (rc) {
- WARN(1, "Semaphore is not found up in st95hf_in_send_cmd\n");
- return rc;
- }
-
rc = st95hf_spi_send(&stcontext->spicontext, skb->data,
skb->len,
ASYNC);
if (rc) {
dev_err(&stcontext->nfcdev->dev,
"Error %d trying to perform data_exchange", rc);
- /* up the semaphore since ISR will never come in this case */
- up(&stcontext->exchange_lock);
goto free_skb_resp;
}
@@ -1104,7 +1076,6 @@ static int st95hf_probe(struct spi_device *nfc_spi_dev)
}
}
- sema_init(&st95context->exchange_lock, 1);
init_completion(&spicontext->done);
mutex_init(&spicontext->spi_lock);
mutex_init(&st95context->rm_lock);
@@ -1220,11 +1191,6 @@ static int st95hf_remove(struct spi_device *nfc_spi_dev)
mutex_unlock(&stcontext->rm_lock);
- /* if last in_send_cmd's ISR is pending, wait for it to finish */
- result = down_killable(&stcontext->exchange_lock);
- if (result == -EINTR)
- dev_err(&spictx->spidev->dev, "sleep for semaphore interrupted by signal\n");
-
/* next reset the ST95HF controller */
result = st95hf_spi_send(&stcontext->spicontext,
&reset_cmd,
--
2.17.1
When an interrupt occurs before st95hf_in_send_cmd() was called, the ISR
will currently dereference a NULL pointer. Fix this by checking whether
`cb_arg->complete_cb' is set, and bail out early if that's not the case.
Again spurious interrupts are likely to occur with EMI noise through the
antenna, and need to be handled gracefully.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 99f84ddfdfef..7fdad67b1a4d 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -796,6 +796,13 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
goto end;
}
+ /*
+ * If the completion callback is not set, no command is currently
+ * active. Ignore the spurious interrupt.
+ */
+ if (unlikely(!cb_arg->complete_cb))
+ goto end;
+
/* if stcontext->ddev is %NULL, it means remove already ran */
if (!stcontext->ddev) {
result = -ENODEV;
@@ -844,8 +851,16 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
wtx = false;
cb_arg->rats = false;
skb_resp = ERR_PTR(result);
- /* call of callback with error */
- cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
+
+ /*
+ * Report an error to the core. If cb_arg->complete_cb is unset,
+ * we're handling a spurious interrupt that can be ignored.
+ */
+ if (cb_arg->complete_cb)
+ cb_arg->complete_cb(stcontext->ddev,
+ cb_arg->cb_usrarg,
+ skb_resp);
+
mutex_unlock(&stcontext->rm_lock);
return IRQ_HANDLED;
}
--
2.17.1
This flag is unneccesary. We can just nullify `ddev' instead after we freed
it and check for that in the ISR.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index bc1a2070f9bb..d58424ab5c48 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -220,8 +220,6 @@ struct st95_digital_cmd_complete_arg {
* from threaded ISR. Usage of this mutex avoids any race between
* deletion of the object from st95hf_remove() and its access from
* the threaded ISR.
- * @nfcdev_free: flag to have the state of nfc device object.
- * [alive | died]
* @current_protocol: current nfc protocol.
* @current_rf_tech: current rf technology.
* @fwi: frame waiting index, received in reply of RATS according to
@@ -237,7 +235,6 @@ struct st95hf_context {
unsigned char sendrcv_trflag;
struct semaphore exchange_lock;
struct mutex rm_lock;
- bool nfcdev_free;
u8 current_protocol;
u8 current_rf_tech;
int fwi;
@@ -820,8 +817,8 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
goto end;
}
- /* if stcontext->nfcdev_free is true, it means remove already ran */
- if (stcontext->nfcdev_free) {
+ /* if stcontext->ddev is %NULL, it means remove already ran */
+ if (!stcontext->ddev) {
result = -ENODEV;
goto end;
}
@@ -1220,7 +1217,7 @@ static int st95hf_remove(struct spi_device *nfc_spi_dev)
nfc_digital_unregister_device(stcontext->ddev);
nfc_digital_free_device(stcontext->ddev);
- stcontext->nfcdev_free = true;
+ stcontext->ddev = NULL;
mutex_unlock(&stcontext->rm_lock);
--
2.17.1
Keep the information whether a command is asynchronous in a boolean flag
like it is done elsewhere in the code. This way, the enum can go away.
No function change intended.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 38 ++++++++++++++++----------------------
drivers/nfc/st95hf/spi.c | 12 +++++-------
drivers/nfc/st95hf/spi.h | 8 +-------
3 files changed, 22 insertions(+), 36 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 53447394666b..10321dba15dc 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -101,7 +101,7 @@ struct cmd {
unsigned char cmd_id;
unsigned char no_cmd_params;
unsigned char cmd_params[MAX_CMD_PARAMS];
- enum req_type req;
+ bool is_sync;
};
struct param_list {
@@ -134,63 +134,62 @@ static const struct cmd cmd_array[] = {
.cmd_len = 0x2,
.cmd_id = ECHO_CMD,
.no_cmd_params = 0,
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443A_CONFIG] = {
.cmd_len = 0x7,
.cmd_id = WRITE_REGISTER_CMD,
.no_cmd_params = 0x4,
.cmd_params = {0x3A, 0x00, 0x5A, 0x04},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443A_DEMOGAIN] = {
.cmd_len = 0x7,
.cmd_id = WRITE_REGISTER_CMD,
.no_cmd_params = 0x4,
.cmd_params = {0x68, 0x01, 0x01, 0xDF},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443B_DEMOGAIN] = {
.cmd_len = 0x7,
.cmd_id = WRITE_REGISTER_CMD,
.no_cmd_params = 0x4,
.cmd_params = {0x68, 0x01, 0x01, 0x51},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443A_PROTOCOL_SELECT] = {
.cmd_len = 0x7,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x4,
.cmd_params = {ISO14443A_PROTOCOL_CODE, 0x00, 0x01, 0xA0},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443B_PROTOCOL_SELECT] = {
.cmd_len = 0x7,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x4,
.cmd_params = {ISO14443B_PROTOCOL_CODE, 0x01, 0x03, 0xFF},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_WTX_RESPONSE] = {
.cmd_len = 0x6,
.cmd_id = SEND_RECEIVE_CMD,
.no_cmd_params = 0x3,
.cmd_params = {0xF2, 0x00, TRFLAG_NFCA_STD_FRAME_CRC},
- .req = ASYNC,
},
[CMD_FIELD_OFF] = {
.cmd_len = 0x5,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x2,
.cmd_params = {0x0, 0x0},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO15693_PROTOCOL_SELECT] = {
.cmd_len = 0x5,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x2,
.cmd_params = {ISO15693_PROTOCOL_CODE, 0x0D},
- .req = SYNC,
+ .is_sync = true,
},
};
@@ -279,13 +278,13 @@ static int st95hf_send_recv_cmd(struct st95hf_context *st95context,
ret = st95hf_spi_send(&st95context->spicontext,
spi_cmd_buffer,
cmd_array[cmd].cmd_len,
- cmd_array[cmd].req);
+ cmd_array[cmd].is_sync);
if (ret) {
dev_err(dev, "st95hf_spi_send failed with error %d\n", ret);
return ret;
}
- if (cmd_array[cmd].req == SYNC && recv_res) {
+ if (cmd_array[cmd].is_sync && recv_res) {
unsigned char st95hf_response_arr[2];
ret = st95hf_spi_recv_response(&st95context->spicontext,
@@ -480,10 +479,8 @@ static int st95hf_send_spi_reset_sequence(struct st95hf_context *st95context)
int result = 0;
unsigned char reset_cmd = ST95HF_COMMAND_RESET;
- result = st95hf_spi_send(&st95context->spicontext,
- &reset_cmd,
- ST95HF_RESET_CMD_LEN,
- ASYNC);
+ result = st95hf_spi_send(&st95context->spicontext, &reset_cmd,
+ ST95HF_RESET_CMD_LEN, false);
if (result) {
dev_err(&st95context->spicontext.spidev->dev,
"spi reset sequence cmd error = %d", result);
@@ -947,8 +944,7 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
stcontext->complete_cb_arg.rats = true;
rc = st95hf_spi_send(&stcontext->spicontext, skb->data,
- skb->len,
- ASYNC);
+ skb->len, false);
if (rc) {
dev_err(&stcontext->nfcdev->dev,
"Error %d trying to perform data_exchange", rc);
@@ -1183,10 +1179,8 @@ static int st95hf_remove(struct spi_device *nfc_spi_dev)
mutex_unlock(&stcontext->rm_lock);
/* next reset the ST95HF controller */
- result = st95hf_spi_send(&stcontext->spicontext,
- &reset_cmd,
- ST95HF_RESET_CMD_LEN,
- ASYNC);
+ result = st95hf_spi_send(&stcontext->spicontext, &reset_cmd,
+ ST95HF_RESET_CMD_LEN, false);
if (result) {
dev_err(&spictx->spidev->dev,
"ST95HF reset failed in remove() err = %d\n", result);
diff --git a/drivers/nfc/st95hf/spi.c b/drivers/nfc/st95hf/spi.c
index d5894d4546b1..ef12ed67343f 100644
--- a/drivers/nfc/st95hf/spi.c
+++ b/drivers/nfc/st95hf/spi.c
@@ -23,7 +23,7 @@
int st95hf_spi_send(struct st95hf_spi_context *spicontext,
unsigned char *buffertx,
int datalen,
- enum req_type reqtype)
+ bool is_sync)
{
struct spi_message m;
int result = 0;
@@ -35,12 +35,10 @@ int st95hf_spi_send(struct st95hf_spi_context *spicontext,
mutex_lock(&spicontext->spi_lock);
- if (reqtype == SYNC) {
- spicontext->req_issync = true;
+ spicontext->req_issync = is_sync;
+
+ if (is_sync)
reinit_completion(&spicontext->done);
- } else {
- spicontext->req_issync = false;
- }
spi_message_init(&m);
spi_message_add_tail(&tx_transfer, &m);
@@ -52,7 +50,7 @@ int st95hf_spi_send(struct st95hf_spi_context *spicontext,
}
/* return for asynchronous or no-wait case */
- if (reqtype == ASYNC) {
+ if (!is_sync) {
mutex_unlock(&spicontext->spi_lock);
return 0;
}
diff --git a/drivers/nfc/st95hf/spi.h b/drivers/nfc/st95hf/spi.h
index 552d220747cd..ede17eef6ab1 100644
--- a/drivers/nfc/st95hf/spi.h
+++ b/drivers/nfc/st95hf/spi.h
@@ -44,16 +44,10 @@ struct st95hf_spi_context {
struct mutex spi_lock;
};
-/* flag to differentiate synchronous & asynchronous spi request */
-enum req_type {
- SYNC,
- ASYNC,
-};
-
int st95hf_spi_send(struct st95hf_spi_context *spicontext,
unsigned char *buffertx,
int datalen,
- enum req_type reqtype);
+ bool is_sync);
int st95hf_spi_recv_response(struct st95hf_spi_context *spicontext,
unsigned char *receivebuff);
--
2.17.1
Address two minor style issues that I came across while working on the
driver.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 10321dba15dc..ccb1f1456871 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -662,7 +662,8 @@ static int st95hf_error_handling(struct st95hf_context *stcontext,
result = -ETIMEDOUT;
else
result = -EIO;
- return result;
+
+ return result;
}
/* Check for CRC err only if CRC is present in the tag response */
@@ -859,6 +860,7 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
skb_resp);
mutex_unlock(&stcontext->rm_lock);
+
return IRQ_HANDLED;
}
--
2.17.1
Just a small cleanup to bring the command defines in a numerical order.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 7fdad67b1a4d..53447394666b 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -45,10 +45,10 @@
/* Command Send Interface */
/* ST95HF_COMMAND_SEND CMD Ids */
-#define ECHO_CMD 0x55
-#define WRITE_REGISTER_CMD 0x9
#define PROTOCOL_SELECT_CMD 0x2
#define SEND_RECEIVE_CMD 0x4
+#define WRITE_REGISTER_CMD 0x9
+#define ECHO_CMD 0x55
/* Select protocol codes */
#define ISO15693_PROTOCOL_CODE 0x1
--
2.17.1
The driver currently assumes that interrupts can only occur between the
time when when a command has been sent to the device and the response
to it.
This assumption, however, is wrong. The antenna of the chip is likely to
catch a lot of environmental noise, and once in a while, the device will
latch the interrupt to signal a protocol error, and keep it latched until
the response bytes are read from the chip.
As the code currently stands, skbs for responses are only prepared when
a command is sent, and the ISR bails out early if that wasn't the case.
Hence, the interrupt remains latched, and no further communication with
device is possible.
To fix this, move the call to nfc_alloc_recv_skb() from
st95hf_in_send_cmd() to st95hf_irq_thread_handler() and always read back
the interrupt reason.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/core.c | 36 ++++++------------------------------
1 file changed, 6 insertions(+), 30 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 6761ab90f68d..99f84ddfdfef 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -196,7 +196,6 @@ static const struct cmd cmd_array[] = {
/* st95_digital_cmd_complete_arg stores client context */
struct st95_digital_cmd_complete_arg {
- struct sk_buff *skb_resp;
nfc_digital_cmd_complete_t complete_cb;
void *cb_usrarg;
bool rats;
@@ -783,12 +782,10 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
spidevice = &stcontext->spicontext.spidev->dev;
cb_arg = &stcontext->complete_cb_arg;
- skb_resp = cb_arg->skb_resp;
- if (unlikely(!skb_resp)) {
- WARN(1, "unknown context in ST95HF ISR");
+ skb_resp = nfc_alloc_recv_skb(MAX_RESPONSE_BUFFER_SIZE, GFP_KERNEL);
+ if (WARN_ON(!skb_resp))
return IRQ_NONE;
- }
mutex_lock(&stcontext->rm_lock);
res_len = st95hf_spi_recv_response(&stcontext->spicontext,
@@ -838,12 +835,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
/* call digital layer callback */
cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
- /*
- * This skb is now owned by the core layer.
- * Make sure not to use it again.
- */
- cb_arg->skb_resp = NULL;
-
/* up the semaphore before returning */
mutex_unlock(&stcontext->rm_lock);
@@ -913,15 +904,7 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
void *arg)
{
struct st95hf_context *stcontext = nfc_digital_get_drvdata(ddev);
- int rc;
- struct sk_buff *skb_resp;
- int len_data_to_tag = 0;
-
- skb_resp = nfc_alloc_recv_skb(MAX_RESPONSE_BUFFER_SIZE, GFP_KERNEL);
- if (!skb_resp) {
- rc = -ENOMEM;
- goto error;
- }
+ int rc, len_data_to_tag = 0;
switch (stcontext->current_rf_tech) {
case NFC_DIGITAL_RF_TECH_106A:
@@ -933,8 +916,7 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
len_data_to_tag = skb->len;
break;
default:
- rc = -EINVAL;
- goto free_skb_resp;
+ return -EINVAL;
}
skb_push(skb, 3);
@@ -942,7 +924,6 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
skb->data[1] = SEND_RECEIVE_CMD;
skb->data[2] = len_data_to_tag;
- stcontext->complete_cb_arg.skb_resp = skb_resp;
stcontext->complete_cb_arg.cb_usrarg = arg;
stcontext->complete_cb_arg.complete_cb = cb;
@@ -956,17 +937,12 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
if (rc) {
dev_err(&stcontext->nfcdev->dev,
"Error %d trying to perform data_exchange", rc);
- goto free_skb_resp;
+ return rc;
}
kfree_skb(skb);
- return rc;
-
-free_skb_resp:
- kfree_skb(skb_resp);
-error:
- return rc;
+ return 0;
}
/* p2p will be supported in a later release ! */
--
2.17.1
This reverts commit c99f996b2ba49 ("NFC: st95hf: drop illegal
kfree_skb()").
It turns out that the st95hf_in_send_cmd() is in fact the sole owner of
this skb, and by not freeing it here, we not only causing a memory leak
but also mess up the refcount of the socket that holds it. This will in
turn lead to activated targets not being cleaned up, even after
stopping userspace processes.
The memory corruption that I was hunting was caused by another
kfree_skb(). This will be fixed in a later commit.
Signed-off-by: Daniel Mack <[email protected]>
Fixes: c99f996b2ba49 ("NFC: st95hf: drop illegal kfree_skb()")
---
drivers/nfc/st95hf/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 36ef0e905ba3..bc1a2070f9bb 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -991,6 +991,8 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
goto free_skb_resp;
}
+ kfree_skb(skb);
+
return rc;
free_skb_resp:
--
2.17.1
The callers of these functions already log on errors, and there's no
need to do it from two places.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/nfc/st95hf/spi.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/nfc/st95hf/spi.c b/drivers/nfc/st95hf/spi.c
index e2d3bbcc8c34..d5894d4546b1 100644
--- a/drivers/nfc/st95hf/spi.c
+++ b/drivers/nfc/st95hf/spi.c
@@ -47,8 +47,6 @@ int st95hf_spi_send(struct st95hf_spi_context *spicontext,
result = spi_sync(spidev, &m);
if (result) {
- dev_err(&spidev->dev, "error: sending cmd to st95hf using SPI = %d\n",
- result);
mutex_unlock(&spicontext->spi_lock);
return result;
}
@@ -62,12 +60,10 @@ int st95hf_spi_send(struct st95hf_spi_context *spicontext,
result = wait_for_completion_timeout(&spicontext->done,
msecs_to_jiffies(1000));
/* check for timeout or success */
- if (!result) {
- dev_err(&spidev->dev, "error: response not ready timeout\n");
+ if (!result)
result = -ETIMEDOUT;
- } else {
+ else
result = 0;
- }
mutex_unlock(&spicontext->spi_lock);
@@ -79,7 +75,7 @@ EXPORT_SYMBOL_GPL(st95hf_spi_send);
int st95hf_spi_recv_response(struct st95hf_spi_context *spicontext,
unsigned char *receivebuff)
{
- int len = 0;
+ int ret, len;
struct spi_transfer tx_takedata;
struct spi_message m;
struct spi_device *spidev = spicontext->spidev;
@@ -89,8 +85,6 @@ int st95hf_spi_recv_response(struct st95hf_spi_context *spicontext,
{.rx_buf = receivebuff, .len = 2, .cs_change = 1,},
};
- int ret = 0;
-
memset(&tx_takedata, 0x0, sizeof(struct spi_transfer));
mutex_lock(&spicontext->spi_lock);
@@ -102,8 +96,6 @@ int st95hf_spi_recv_response(struct st95hf_spi_context *spicontext,
ret = spi_sync(spidev, &m);
if (ret) {
- dev_err(&spidev->dev, "spi_recv_resp, data length error = %d\n",
- ret);
mutex_unlock(&spicontext->spi_lock);
return ret;
}
@@ -127,11 +119,8 @@ int st95hf_spi_recv_response(struct st95hf_spi_context *spicontext,
ret = spi_sync(spidev, &m);
mutex_unlock(&spicontext->spi_lock);
- if (ret) {
- dev_err(&spidev->dev, "spi_recv_resp, data read error = %d\n",
- ret);
+ if (ret)
return ret;
- }
return len;
}
--
2.17.1
Hi Samuel,
On Tuesday, July 24, 2018 11:59 AM, Daniel Mack wrote:
> This v3 of a series of patches for the ST95HF driver.
>
> Patch #1 reverts a change that I have submitted earlier and which is
> sitting in nfc-next already. Given that the tree hasn't been sent out
> for being merged yet, it could also still be removed with rebasing, in
> which case #1 is not necessary of course.
>
> The changes are all rather simple and are explained in their individual
> commit logs.
>
> Note that this series builds upon the patch titled "nfc: st95hf: remove
> redundant pointers 'dev' and 'nfcddev'" that Colin posted the other day.
Could you still apply this set for 4.19?
At least #1 should go in, or commit c99f996b2ba49 ("NFC: st95hf: drop
illegal kfree_skb()") which is already in your tree should be removed
with a rebase.
For the rest, I'm in no hurry. I just want to prevent a regression in 4.19.
Thanks,
Daniel
>
>
> Thanks,
> Daniel
>
> Changelog:
>
> v1 → v2:
>
> * Improved commit logs, identical patch content.
>
>
> v2 → v3:
>
> * Added another patch titled "NFC: st95hf: ignore spurious interrupts"
>
>
> Daniel Mack (11):
> Revert "NFC: st95hf: drop illegal kfree_skb()"
> NFC: st95hf: drop nfcdev_free
> NFC: st95hf: drop illegal kfree_skb() in IRQ handler
> NFC: st95hf: remove logging from spi functions
> NFC: st95hf: remove exchange_lock
> NFC: st95hf: move skb allocation to ISR
> NFC: st95hf: ignore spurious interrupts
> NFC: st95hf: re-order command defines
> NFC: st95hf: unify sync/async flags
> NFC: st95hf: two small style nits
> NFC: st95hf: add of match table
>
> drivers/nfc/st95hf/core.c | 154 ++++++++++++++------------------------
> drivers/nfc/st95hf/spi.c | 31 +++-----
> drivers/nfc/st95hf/spi.h | 8 +-
> 3 files changed, 66 insertions(+), 127 deletions(-)
>
Hi Samuel,
On 6/8/2018 12:38 PM, Daniel Mack wrote:
> On Tuesday, July 24, 2018 11:59 AM, Daniel Mack wrote:
>> This v3 of a series of patches for the ST95HF driver.
>>
>> Patch #1 reverts a change that I have submitted earlier and which is
>> sitting in nfc-next already. Given that the tree hasn't been sent out
>> for being merged yet, it could also still be removed with rebasing, in
>> which case #1 is not necessary of course.
>>
>> The changes are all rather simple and are explained in their individual
>> commit logs.
>>
>> Note that this series builds upon the patch titled "nfc: st95hf: remove
>> redundant pointers 'dev' and 'nfcddev'" that Colin posted the other day.
>
> Could you still apply this set for 4.19?
Hmm it seems it has missed the merge window again.
May I ask whether you still consider applying these to your tree and get
it merged?
Thanks,
Daniel