Re: [PATCH v5 5/6] nfc: s3fwrn5: support the S3NRN4V variant

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing error check on nci_prop_cmd() for GET_VER causes command-response desynchronization on timeouts.
- [High] Hardware error statuses are ignored because the driver checks for negative return values instead of non-zero.
--

--- Patch [5]: [PATCH v5 5/6] nfc: s3fwrn5: support the S3NRN4V variant ---
commit fe1c1a464a09b355d0a14c76e391f1834e4afedf
Author: Jorijn van der Graaf <[email protected]>

nfc: s3fwrn5: support the S3NRN4V variant

Adds support for the Samsung S3NRN4V NFC controller, which requires a new
calibration data bring-up sequence and different clock speed configuration.

> diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
> --- a/drivers/nfc/s3fwrn5/nci.c
> +++ b/drivers/nfc/s3fwrn5/nci.c

[ ... ]

> +int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
> +				  const char *hw_name, const char *sw_name)
> +{

[ ... ]

> +	/*
> +	 * Ask the chip for its current calibration versions and skip the
> +	 * upload when both already match the blobs; a mismatch or an
> +	 * unparseable answer means the upload proceeds. GET_VER answers with
> +	 * versions, not a status byte, so nci_prop_cmd()'s return carries no
> +	 * meaning here.
> +	 */
> +	sub_oid = NCI_PROP_DUAL_SUB_GET_VER;
> +	info->dual_rsp_len = 0;
> +	nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1, &sub_oid);
> +	if (s3fwrn5_nci_dual_cal_current(info, hw_fw, sw_fw)) {

[Severity: High]
Is it safe to completely ignore the return value of nci_prop_cmd() here?

If the hardware is slow to respond to the initial GET_VER command,
nci_prop_cmd() will return -ETIMEDOUT. Continuing directly to START_UPDATE
without checking for this timeout might cause command-response
desynchronization.

A late response to the timed-out GET_VER command could arrive while waiting
for the START_UPDATE response, which might silently corrupt the sequence
since all DUAL_OPTION sub-commands share the same NCI opcode.

> +		dev_dbg(dev, "calibration data already current\n");
> +		ret = 0;
> +		goto out_sw;
> +	}
> +
> +	merged = kvmalloc(merged_size, GFP_KERNEL);
> +	if (!merged) {

[ ... ]

> +	/* SET_OPTION per section */
> +	set_option.sub_oid = NCI_PROP_DUAL_SUB_SET_OPTION;
> +	set_option.index = 0;
> +	for (i = 0; i < merged_size; i += NCI_PROP_DUAL_SECTION_SIZE) {
> +		len = min_t(size_t, merged_size - i, NCI_PROP_DUAL_SECTION_SIZE);
> +		memcpy(set_option.data, merged + i, len);
> +		ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION,
> +				   len + 2, (__u8 *)&set_option);
> +		if (ret < 0) {

[Severity: High]
Does this check miss positive hardware error statuses?

When the chip rejects a command, it returns a positive NCI status byte
(like NCI_STATUS_REJECTED, 0x09). The driver's RX handler passes this status
to nci_req_complete(), which sets req_result. nci_prop_cmd() then returns
this positive integer.

Checking only for ret < 0 might cause the driver to ignore hardware
rejections and erroneously consider the upload chunks successful. Should this
check for ret != 0 instead?

> +			dev_err(dev, "calibration data update error: %d\n",
> +				ret);
> +			/* Abort form: STOP_UPDATE with the sub-OID alone. */
> +			sub_oid = NCI_PROP_DUAL_SUB_STOP_UPDATE;
> +			nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1,
> +				     &sub_oid);
> +			goto out;
> +		}
> +		set_option.index++;
> +	}
> +
> +	/* STOP_UPDATE with checksum */
> +	stop_cmd[0] = NCI_PROP_DUAL_SUB_STOP_UPDATE;
> +	put_unaligned_le16(checksum, &stop_cmd[1]);
> +	ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 3, stop_cmd);
> +	if (ret < 0) {

[Severity: High]
Similarly, would a positive error status from the hardware on STOP_UPDATE be
missed here due to the ret < 0 check?

> +		dev_err(dev, "Unable to stop calibration data update\n");
> +		goto out;
> +	}
> +
> +	dev_dbg(dev, "calibration data update: success\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5