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

[Paolo Abeni <pabeni@redhat.com>]

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