[PATCH] nfc: pn533: add error handling for i2c_master_send() in pn533_i2c_send_ack()

[PATCH] nfc: pn533: add error handling for i2c_master_send() in pn533_i2c_send_ack()

[Wenyuan Li]

In pn533_i2c_send_ack(), the return value of i2c_master_send() is not
checked. If the I2C transfer fails, the driver continues execution
without detecting the error, which may lead to incorrect device state
or silent failures.

Specifically:
1. The ACK may not be sent to the device, leaving it in an undefined state
2. The caller (pn533_i2c_abort_cmd()) ignores the return value entirely
3. No error logging is provided for debugging

Fix this by:
- Adding proper return value check for i2c_master_send()
- Converting partial sends to -EIO and preserving kernel error codes
- Adding nfc_err() logging with %pe format for human-readable errors
- Propagating the error to the caller pn533_i2c_abort_cmd()

Even if ACK sending fails, the abort procedure continues by scheduling
cmd_complete_work to ensure userspace is notified, but the error is
properly logged for debugging.

Signed-off-by: Wenyuan Li <2063309626@qq.com>
---
 drivers/nfc/pn533/i2c.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 132c050a365d..4d771fe24f70 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -40,8 +40,15 @@ static int pn533_i2c_send_ack(struct pn533 *dev, gfp_t flags)
 	struct i2c_client *client = phy->i2c_dev;
 	static const u8 ack[6] = {0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
 	/* spec 6.2.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
+	int ret;
 
-	return i2c_master_send(client, ack, 6);
+	ret = i2c_master_send(client, ack, 6);
+	if (ret != 6) {
+		nfc_err(&client->dev, "failed to send ACK: %pe\n", ERR_PTR(ret));
+		return ret < 0 ? ret : -EIO;
+	}
+
+	return 0;
 }
 
 static int pn533_i2c_send_frame(struct pn533 *dev,
@@ -82,12 +89,14 @@ static int pn533_i2c_send_frame(struct pn533 *dev,
 static void pn533_i2c_abort_cmd(struct pn533 *dev, gfp_t flags)
 {
 	struct pn533_i2c_phy *phy = dev->phy;
+	int ret;
 
 	phy->aborted = true;
 
 	/* An ack will cancel the last issued command */
-	pn533_i2c_send_ack(dev, flags);
-
+	ret = pn533_i2c_send_ack(dev, flags);
+	if (ret)
+		nfc_err(&phy->i2c_dev->dev, "failed to abort command: %pe\n", ERR_PTR(ret));
 	/* schedule cmd_complete_work to finish current command execution */
 	pn533_recv_frame(phy->priv, NULL, -ENOENT);
 }
-- 
2.43.0

Re: [PATCH] nfc: pn533: add error handling for i2c_master_send() in pn533_i2c_send_ack()

[Simon Horman]

On Thu, Mar 19, 2026 at 02:50:05PM +0800, Wenyuan Li wrote:
> In pn533_i2c_send_ack(), the return value of i2c_master_send() is not
> checked. If the I2C transfer fails, the driver continues execution
> without detecting the error, which may lead to incorrect device state
> or silent failures.

I would like to clarify that this patch addresses the silent failures
aspect, but not the potentially incorrect device state aspect. Although,
with notification users may be better able to diagnose that problem.

If so, I agree this patch should be treated as an enhancement
and routed via the net-next tree (the default for Networking patches)
without a fixes tag.

But for reference, it would be better if the tree was targeted like this:

Subject: [PATCH net-next]: ...

> 
> Specifically:
> 1. The ACK may not be sent to the device, leaving it in an undefined state
> 2. The caller (pn533_i2c_abort_cmd()) ignores the return value entirely
> 3. No error logging is provided for debugging
> 
> Fix this by:
> - Adding proper return value check for i2c_master_send()
> - Converting partial sends to -EIO and preserving kernel error codes
> - Adding nfc_err() logging with %pe format for human-readable errors
> - Propagating the error to the caller pn533_i2c_abort_cmd()
> 
> Even if ACK sending fails, the abort procedure continues by scheduling
> cmd_complete_work to ensure userspace is notified, but the error is
> properly logged for debugging.
> 
> Signed-off-by: Wenyuan Li <2063309626@qq.com>

...

Re: [PATCH] nfc: pn533: add error handling for i2c_master_send() in pn533_i2c_send_ack()

[Paolo Abeni]

On 3/19/26 7:50 AM, Wenyuan Li wrote:
> In pn533_i2c_send_ack(), the return value of i2c_master_send() is not
> checked. If the I2C transfer fails, the driver continues execution
> without detecting the error, which may lead to incorrect device state
> or silent failures.
> 
> Specifically:
> 1. The ACK may not be sent to the device, leaving it in an undefined state
> 2. The caller (pn533_i2c_abort_cmd()) ignores the return value entirely
> 3. No error logging is provided for debugging
> 
> Fix this by:
> - Adding proper return value check for i2c_master_send()
> - Converting partial sends to -EIO and preserving kernel error codes
> - Adding nfc_err() logging with %pe format for human-readable errors
> - Propagating the error to the caller pn533_i2c_abort_cmd()
> 
> Even if ACK sending fails, the abort procedure continues by scheduling
> cmd_complete_work to ensure userspace is notified, but the error is
> properly logged for debugging.
> 
> Signed-off-by: Wenyuan Li <2063309626@qq.com>
> ---
>  drivers/nfc/pn533/i2c.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
> index 132c050a365d..4d771fe24f70 100644
> --- a/drivers/nfc/pn533/i2c.c
> +++ b/drivers/nfc/pn533/i2c.c
> @@ -40,8 +40,15 @@ static int pn533_i2c_send_ack(struct pn533 *dev, gfp_t flags)
>  	struct i2c_client *client = phy->i2c_dev;
>  	static const u8 ack[6] = {0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
>  	/* spec 6.2.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
> +	int ret;
>  
> -	return i2c_master_send(client, ack, 6);
> +	ret = i2c_master_send(client, ack, 6);
> +	if (ret != 6) {
> +		nfc_err(&client->dev, "failed to send ACK: %pe\n", ERR_PTR(ret));
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	return 0;
>  }
>  
>  static int pn533_i2c_send_frame(struct pn533 *dev,
> @@ -82,12 +89,14 @@ static int pn533_i2c_send_frame(struct pn533 *dev,
>  static void pn533_i2c_abort_cmd(struct pn533 *dev, gfp_t flags)
>  {
>  	struct pn533_i2c_phy *phy = dev->phy;
> +	int ret;
>  
>  	phy->aborted = true;
>  
>  	/* An ack will cancel the last issued command */
> -	pn533_i2c_send_ack(dev, flags);
> -
> +	ret = pn533_i2c_send_ack(dev, flags);
> +	if (ret)
> +		nfc_err(&phy->i2c_dev->dev, "failed to abort command: %pe\n", ERR_PTR(ret));

A single failure on abort_cmd() will emit 2 err messages, which is IMHO
way too verbose. Also send_ack errors are still ignored in most cases,
and errors from other phy (usb, uart) too.

I'm sorry but I see little value here.

/P