Re: [PATCH v2 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once

From: "NG, TZE YEE" <tze.yee.ng@altera.com>
To: "Frank.Li@kernel.org" <Frank.Li@kernel.org>,
	"linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Frank Li <Frank.li@nxp.com>,
	"NG, ADRIAN HO YIN" <adrian.ho.yin.ng@altera.com>,
	Felix Gu <ustc.gu@gmail.com>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Manikanta Guntupalli <manikanta.guntupalli@amd.com>,
	Jorge Marques <jorge.marques@analog.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once
Date: Wed, 10 Jun 2026 02:08:05 +0000	[thread overview]
Message-ID: <101697c2-64fd-499b-98e0-9e65eb219810@altera.com> (raw)
In-Reply-To: <20260609103029.994891F00893@smtp.kernel.org>

On 9/6/2026 6:30 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Address NACKs are incorrectly mapped to -EINVAL instead of a hardware error code (-EIO or -ENXIO), breaking standard subsystem error reporting conventions for standard transfers.
> --
> 
> commit f0f1354839be36f55ac51ec9d8d9c182653a6497
> Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
> 
> i3c: master: Validate GET CCC payload length and retry M0/M2 once
> 
> Validate GET CCC payload length after a successful transfer. Treat a
> short read as I3C_ERROR_M0 and return -EIO. Retry GET CCCs once on
> retriable errors.
> 
> [ ... ]
>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>> index c4004f87c6857..a8b5ebabd3508 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -493,7 +493,6 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
>>   			break;
>>   		case RESPONSE_ERROR_PARITY:
>>   		case RESPONSE_ERROR_IBA_NACK:
>> -		case RESPONSE_ERROR_ADDRESS_NACK:
>>   		case RESPONSE_ERROR_TRANSF_ABORT:
>>   		case RESPONSE_ERROR_CRC:
>>   		case RESPONSE_ERROR_FRAME:
>> @@ -503,6 +502,7 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
>>   			ret = -ENOSPC;
>>   			break;
>>   		case RESPONSE_ERROR_I2C_W_NACK_ERR:
>> +		case RESPONSE_ERROR_ADDRESS_NACK:
>>   		default:
>>   			ret = -EINVAL;
>>   			break;
> 
> [Severity: High]
> Does moving RESPONSE_ERROR_ADDRESS_NACK to the default case incorrectly map
> address NACKs to -EINVAL instead of a hardware error code like -EIO or
> -ENXIO?
> 
> This function serves as the generic completion handler for all transfers,
> including standard I3C and I2C private reads and writes. An address NACK
> usually indicates an absent, disconnected, or busy device.
> 
> Returning -EINVAL for a hardware NACK during a data transfer diverges from
> subsystem conventions. Can this cause userspace tools like i2cdetect or
> core probing logic to misinterpret an absent device as a fatal invalid
> parameter error, rather than allowing graceful fallbacks?
> 
I will submit v3 to move RESPONSE_ERROR_ADDRESS_NACK to return -EIO 
(alongside IBA_NACK) in patch 2/3 and drop the accidental revert in 
patch 3/3.

Regards,
Tze Yee
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c