[PATCH 0/3] NFC: pn533: fixes for i2c driver

[PATCH 0/3] NFC: pn533: fixes for i2c driver

[Andrey Rusalin]

Each of these patches fix some oops that I met during  
tests of the driver with itead pn532 nfc module.

First and third patches related to order of initialization driver,
where interrupt handler was registered before work queues
were ready to handle it.
Also iqr was freed already after work queues were deinitialized.

Second patch originally sent by Michael Thalmeier.
I reworked a little bit to make it more readable.

Andrey Rusalin (3):
  NFC: pn533: change order of free_irq and dev unregistration
  NFC: pn533: improve cmd queue handling
  NFC: pn533: change order operations in dev registation

 drivers/nfc/pn533/i2c.c   | 32 +++++++++++-------
 drivers/nfc/pn533/pn533.c | 82 ++++++++++++++++++++++++++++++-----------------
 drivers/nfc/pn533/pn533.h |  1 +
 drivers/nfc/pn533/usb.c   |  4 +++
 4 files changed, 77 insertions(+), 42 deletions(-)

-- 
2.7.4

[PATCH 1/3] NFC: pn533: change order of free_irq and dev unregistration

[Andrey Rusalin]

Change order of free_irq and dev unregistration.
It fixes situation when device already unregistered and
an interrupt happens and nobody can handle it.

Signed-off-by: Andrey Rusalin <arusalin@dev.rtsoft.ru>
---
 drivers/nfc/pn533/i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 5de5a49..e2848e4 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -242,10 +242,10 @@ static int pn533_i2c_remove(struct i2c_client *client)
 
 	dev_dbg(&client->dev, "%s\n", __func__);
 
-	pn533_unregister_device(phy->priv);
-
 	free_irq(client->irq, phy);
 
+	pn533_unregister_device(phy->priv);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH 2/3] NFC: pn533: improve cmd queue handling

[Andrey Rusalin]

Make sure cmd is set before a frame is passed to the transport layer for
sending. In addition pn533_send_async_complete checks if cmd is set before
accessing its members.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>

Rework a little bit changes in pn532_send_async_complete.

Signed-off-by: Andrey Rusalin <arusalin@dev.rtsoft.ru>
---
 drivers/nfc/pn533/pn533.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 2f817b3..c67150f 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -383,14 +383,18 @@ static void pn533_build_cmd_frame(struct pn533 *dev, u8 cmd_code,
 static int pn533_send_async_complete(struct pn533 *dev)
 {
 	struct pn533_cmd *cmd = dev->cmd;
-	int status = cmd->status;
+	struct sk_buff *resp;
+	int status, rc = 0;
 
-	struct sk_buff *req = cmd->req;
-	struct sk_buff *resp = cmd->resp;
+	if (!cmd) {
+		dev_dbg(dev->dev, "%s: cmd not set\n", __func__);
+		goto done;
+	}
 
-	int rc;
+	dev_kfree_skb(cmd->req);
 
-	dev_kfree_skb(req);
+	status = cmd->status;
+	resp = cmd->resp;
 
 	if (status < 0) {
 		rc = cmd->complete_cb(dev, cmd->complete_cb_context,
@@ -399,8 +403,14 @@ static int pn533_send_async_complete(struct pn533 *dev)
 		goto done;
 	}
 
-	skb_pull(resp, dev->ops->rx_header_len);
-	skb_trim(resp, resp->len - dev->ops->rx_tail_len);
+	/* when no response is set we got interrupted */
+	if (!resp)
+		resp = ERR_PTR(-EINTR);
+
+	if (!IS_ERR(resp)) {
+		skb_pull(resp, dev->ops->rx_header_len);
+		skb_trim(resp, resp->len - dev->ops->rx_tail_len);
+	}
 
 	rc = cmd->complete_cb(dev, cmd->complete_cb_context, resp);
 
@@ -434,12 +444,14 @@ static int __pn533_send_async(struct pn533 *dev, u8 cmd_code,
 	mutex_lock(&dev->cmd_lock);
 
 	if (!dev->cmd_pending) {
+		dev->cmd = cmd;
 		rc = dev->phy_ops->send_frame(dev, req);
-		if (rc)
+		if (rc) {
+			dev->cmd = NULL;
 			goto error;
+		}
 
 		dev->cmd_pending = 1;
-		dev->cmd = cmd;
 		goto unlock;
 	}
 
@@ -511,11 +523,12 @@ static int pn533_send_cmd_direct_async(struct pn533 *dev, u8 cmd_code,
 
 	pn533_build_cmd_frame(dev, cmd_code, req);
 
+	dev->cmd = cmd;
 	rc = dev->phy_ops->send_frame(dev, req);
-	if (rc < 0)
+	if (rc < 0) {
+		dev->cmd = NULL;
 		kfree(cmd);
-	else
-		dev->cmd = cmd;
+	}
 
 	return rc;
 }
@@ -550,14 +563,15 @@ static void pn533_wq_cmd(struct work_struct *work)
 
 	mutex_unlock(&dev->cmd_lock);
 
+	dev->cmd = cmd;
 	rc = dev->phy_ops->send_frame(dev, cmd->req);
 	if (rc < 0) {
+		dev->cmd = NULL;
 		dev_kfree_skb(cmd->req);
 		kfree(cmd);
 		return;
 	}
 
-	dev->cmd = cmd;
 }
 
 struct pn533_sync_cmd_response {
-- 
2.7.4

[PATCH 3/3] NFC: pn533: change order operations in dev registation

[Andrey Rusalin]

Sometimes during probing and registration of pn533_i2c
NULL pointer dereference happens.
Reproduced in cycle of inserting and removing pn533_i2c
and pn533 modules.

Backtrace:
[<8004205c>] (__queue_work) from [<80042324>] (queue_work_on+0x50/0x5c)
r10:acdc7c80 r9:8006b330 r8:ac0dfb40 r7:ac50c600 r6:00000004 r5:acbbee40 r4:600f0113
[<800422d4>] (queue_work_on) from [<7f7d5b6c>] (pn533_recv_frame+0x158/0x1fc [pn533])
r7:ffffff87 r6:00000000 r5:acbbee40 r4:acbbee00
[<7f7d5a14>] (pn533_recv_frame [pn533]) from [<7f7df4b8>] (pn533_i2c_irq_thread_fn+0x184/0x)
r6:acb2a000 r5:00000000 r4:acdc7b90
[<7f7df334>] (pn533_i2c_irq_thread_fn [pn533_i2c]) from [<8006b354>] (irq_thread_fn+0x24/0x)
r7:00000000 r6:accde000 r5:ac0dfb40 r4:acdc7c80
...

Seems there is some race condition due registration of
irq handler until all data stuctures that could be needed
are ready. So I re-ordered some ops. After this, problem has gone.

Changes in USB part was not tested, but it should not break
anything.

Signed-off-by: Andrey Rusalin <arusalin@dev.rtsoft.ru>
---
 drivers/nfc/pn533/i2c.c   | 28 ++++++++++++++++++----------
 drivers/nfc/pn533/pn533.c | 42 +++++++++++++++++++++++++-----------------
 drivers/nfc/pn533/pn533.h |  1 +
 drivers/nfc/pn533/usb.c   |  4 ++++
 4 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index e2848e4..c1302de 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -206,14 +206,6 @@ static int pn533_i2c_probe(struct i2c_client *client,
 	phy->i2c_dev = client;
 	i2c_set_clientdata(client, phy);
 
-	r = request_threaded_irq(client->irq, NULL, pn533_i2c_irq_thread_fn,
-				 IRQF_TRIGGER_FALLING |
-				 IRQF_SHARED | IRQF_ONESHOT,
-				 PN533_I2C_DRIVER_NAME, phy);
-
-	if (r < 0)
-		nfc_err(&client->dev, "Unable to register IRQ handler\n");
-
 	priv = pn533_register_device(PN533_DEVICE_PN532,
 				     PN533_NO_TYPE_B_PROTOCOLS,
 				     PN533_PROTO_REQ_ACK_RESP,
@@ -223,16 +215,32 @@ static int pn533_i2c_probe(struct i2c_client *client,
 
 	if (IS_ERR(priv)) {
 		r = PTR_ERR(priv);
-		goto err_register;
+		return r;
 	}
 
 	phy->priv = priv;
 
+	r = request_threaded_irq(client->irq, NULL, pn533_i2c_irq_thread_fn,
+				IRQF_TRIGGER_FALLING |
+				IRQF_SHARED | IRQF_ONESHOT,
+				PN533_I2C_DRIVER_NAME, phy);
+	if (r < 0) {
+		nfc_err(&client->dev, "Unable to register IRQ handler\n");
+		goto irq_rqst_err;
+	}
+
+	r = pn533_finalize_setup(priv);
+	if (r)
+		goto fn_setup_err;
+
 	return 0;
 
-err_register:
+fn_setup_err:
 	free_irq(client->irq, phy);
 
+irq_rqst_err:
+	pn533_unregister_device(phy->priv);
+
 	return r;
 }
 
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index c67150f..bbae44b 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2570,6 +2570,31 @@ static int pn533_setup(struct pn533 *dev)
 	return 0;
 }
 
+int pn533_finalize_setup(struct pn533 *dev)
+{
+
+	struct pn533_fw_version fw_ver;
+	int rc;
+
+	memset(&fw_ver, 0, sizeof(fw_ver));
+
+	rc = pn533_get_firmware_version(dev, &fw_ver);
+	if (rc) {
+		nfc_err(dev->dev, "Unable to get FW version\n");
+		return rc;
+	}
+
+	nfc_info(dev->dev, "NXP PN5%02X firmware ver %d.%d now attached\n",
+		fw_ver.ic, fw_ver.ver, fw_ver.rev);
+
+	rc = pn533_setup(dev);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pn533_finalize_setup);
+
 struct pn533 *pn533_register_device(u32 device_type,
 				u32 protocols,
 				enum pn533_protocol_type protocol_type,
@@ -2579,7 +2604,6 @@ struct pn533 *pn533_register_device(u32 device_type,
 				struct device *dev,
 				struct device *parent)
 {
-	struct pn533_fw_version fw_ver;
 	struct pn533 *priv;
 	int rc = -ENOMEM;
 
@@ -2622,15 +2646,6 @@ struct pn533 *pn533_register_device(u32 device_type,
 
 	INIT_LIST_HEAD(&priv->cmd_queue);
 
-	memset(&fw_ver, 0, sizeof(fw_ver));
-	rc = pn533_get_firmware_version(priv, &fw_ver);
-	if (rc < 0)
-		goto destroy_wq;
-
-	nfc_info(dev, "NXP PN5%02X firmware ver %d.%d now attached\n",
-		 fw_ver.ic, fw_ver.ver, fw_ver.rev);
-
-
 	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
 					   priv->ops->tx_header_len +
 					   PN533_CMD_DATAEXCH_HEAD_LEN,
@@ -2647,15 +2662,8 @@ struct pn533 *pn533_register_device(u32 device_type,
 	if (rc)
 		goto free_nfc_dev;
 
-	rc = pn533_setup(priv);
-	if (rc)
-		goto unregister_nfc_dev;
-
 	return priv;
 
-unregister_nfc_dev:
-	nfc_unregister_device(priv->nfc_dev);
-
 free_nfc_dev:
 	nfc_free_device(priv->nfc_dev);
 
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index e26d634..12ea04e 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -231,6 +231,7 @@ struct pn533 *pn533_register_device(u32 device_type,
 				struct device *dev,
 				struct device *parent);
 
+int pn533_finalize_setup(struct pn533 *dev);
 void pn533_unregister_device(struct pn533 *priv);
 void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
 
diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index bcf3f54..909806f 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -543,6 +543,10 @@ static int pn533_usb_probe(struct usb_interface *interface,
 
 	phy->priv = priv;
 
+	rc = pn533_finalize_setup(priv);
+	if (rc)
+		goto error;
+
 	usb_set_intfdata(interface, phy);
 
 	return 0;
-- 
2.7.4

Re: [PATCH 0/3] NFC: pn533: fixes for i2c driver

[Samuel Ortiz]

Hi Andrey,

On Wed, Dec 28, 2016 at 08:10:56PM +0300, Andrey Rusalin wrote:
> Each of these patches fix some oops that I met during  
> tests of the driver with itead pn532 nfc module.
> 
> First and third patches related to order of initialization driver,
> where interrupt handler was registered before work queues
> were ready to handle it.
> Also iqr was freed already after work queues were deinitialized.
> 
> Second patch originally sent by Michael Thalmeier.
> I reworked a little bit to make it more readable.
> 
> Andrey Rusalin (3):
>   NFC: pn533: change order of free_irq and dev unregistration
>   NFC: pn533: improve cmd queue handling
>   NFC: pn533: change order operations in dev registation
All 3 patches applied, thanks.

Cheers,
Samuel.