[2/4] i2c: pasemi: Improve error recovery

From: Hector Martin <marcan@marcan.st>

The hardware (supposedly) has a 25ms timeout for clock stretching
and the driver uses 100ms which should be plenty. The error
reocvery itself is however lacking.

Add handling for all the missing error condition, and better recovery in
pasemi_smb_clear(). Also move the timeout to a #define since it's used
in multiple places now.

Signed-off-by: Hector Martin <marcan@marcan.st>
[sven: adjusted commit message after splitting the commit into two]
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/i2c/busses/i2c-pasemi-core.c | 70 +++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 12 deletions(-)

Hi Sven,

On Sat, Feb 22, 2025 at 01:38:34PM +0000, Sven Peter via B4 Relay wrote:
> The hardware (supposedly) has a 25ms timeout for clock stretching
> and the driver uses 100ms which should be plenty.

Can we add this lines as a comment to the define you are adding?

> The error
> reocvery itself is however lacking.

...

> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
>  {
>  	unsigned int status;
> +	int timeout = TRANSFER_TIMEOUT_MS;
>  
>  	status = reg_read(smbus, REG_SMSTA);
> +
> +	/* First wait for the bus to go idle */
> +	while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
> +		msleep(1);

Please, use usleep_range for 1 millisecond timeout.

> +		status = reg_read(smbus, REG_SMSTA);
> +	}

You could use here readx_poll_timeout() here.

> +
> +	if (timeout < 0) {
> +		dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);

if it's an error, please use an error.

> +		return -EIO;
> +	}
> +
> +	/* If any badness happened or there is data in the FIFOs, reset the FIFOs */
> +	if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
> +		!(status & SMSTA_MTE))

Please, fixe the alignment here.

> +		pasemi_reset(smbus);
> +
> +	/* Clear the flags */
>  	reg_write(smbus, REG_SMSTA, status);
> +
> +	return 0;
>  }
>  
>  static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
>  {
> -	int timeout = 100;
> +	int timeout = TRANSFER_TIMEOUT_MS;
>  	unsigned int status;
>  
>  	if (smbus->use_irq) {
>  		reinit_completion(&smbus->irq_completion);
> -		reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> -		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(100));
> +		/* XEN should be set when a transaction terminates, whether due to error or not */
> +		reg_write(smbus, REG_IMASK, SMSTA_XEN);
> +		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(timeout));

what happens if the timeout expires?

>  		reg_write(smbus, REG_IMASK, 0);
>  		status = reg_read(smbus, REG_SMSTA);
>  	} else {

...

>  	struct pasemi_smbus *smbus = adapter->algo_data;
>  	int ret, i;
>  
> -	pasemi_smb_clear(smbus);
> +	if (pasemi_smb_clear(smbus))
> +		return -EIO;

Can we use

	ret = ...
	if (ret)
		return ret;

This way we return whatever comes from pasemi_smb_clear().

>  
>  	ret = 0;

This way we can remove this line, as well.

Andi

Hi Andi,

Thanks for the review! Will send a v2 after -rc1 is out.

On Thu, Mar 20, 2025, at 01:17, Andi Shyti wrote:
> Hi Sven,
>
> On Sat, Feb 22, 2025 at 01:38:34PM +0000, Sven Peter via B4 Relay wrote:
>> The hardware (supposedly) has a 25ms timeout for clock stretching
>> and the driver uses 100ms which should be plenty.
>
> Can we add this lines as a comment to the define you are adding?

Sure.

>
>> The error
>> reocvery itself is however lacking.
>
> ...
>
>> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
>> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
>>  {
>>  	unsigned int status;
>> +	int timeout = TRANSFER_TIMEOUT_MS;
>>  
>>  	status = reg_read(smbus, REG_SMSTA);
>> +
>> +	/* First wait for the bus to go idle */
>> +	while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
>> +		msleep(1);
>
> Please, use usleep_range for 1 millisecond timeout.

Ack.

>
>> +		status = reg_read(smbus, REG_SMSTA);
>> +	}
>
> You could use here readx_poll_timeout() here.

Yup, that should work.

>
>> +
>> +	if (timeout < 0) {
>> +		dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);
>
> if it's an error, please use an error.

Ack.

>
>> +		return -EIO;
>> +	}
>> +
>> +	/* If any badness happened or there is data in the FIFOs, reset the FIFOs */
>> +	if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
>> +		!(status & SMSTA_MTE))
>
> Please, fixe the alignment here.

Ok.

>
>> +		pasemi_reset(smbus);
>> +
>> +	/* Clear the flags */
>>  	reg_write(smbus, REG_SMSTA, status);
>> +
>> +	return 0;
>>  }
>>  
>>  static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
>>  {
>> -	int timeout = 100;
>> +	int timeout = TRANSFER_TIMEOUT_MS;
>>  	unsigned int status;
>>  
>>  	if (smbus->use_irq) {
>>  		reinit_completion(&smbus->irq_completion);
>> -		reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
>> -		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(100));
>> +		/* XEN should be set when a transaction terminates, whether due to error or not */
>> +		reg_write(smbus, REG_IMASK, SMSTA_XEN);
>> +		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(timeout));
>
> what happens if the timeout expires?

I think that can only happen if the hardware is seriously broken because
it's always supposed to set XEN. I'll make sure to catch that case in v2
though and print a separate error message similar to how the polling case
below is taken care of right now.

>
>>  		reg_write(smbus, REG_IMASK, 0);
>>  		status = reg_read(smbus, REG_SMSTA);
>>  	} else {
>
> ...
>
>>  	struct pasemi_smbus *smbus = adapter->algo_data;
>>  	int ret, i;
>>  
>> -	pasemi_smb_clear(smbus);
>> +	if (pasemi_smb_clear(smbus))
>> +		return -EIO;
>
> Can we use
>
> 	ret = ...
> 	if (ret)
> 		return ret;
>
> This way we return whatever comes from pasemi_smb_clear().
>
>>  
>>  	ret = 0;
>
> This way we can remove this line, as well.

Sure, will do both for v2.



Thanks,


Sven