Re: [PATCH 1/8] i2c: i801: improve interrupt handler

[Heiner Kallweit <hkallweit1@gmail.com>]

On 07.06.2022 14:34, Jean Delvare wrote:
> Hi Heiner, 
> 
> On Fri, 15 Apr 2022 18:54:32 +0200, Heiner Kallweit wrote:
>> Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
>> and an error flag is set, then don't trust the result and skip calling
>> i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
>> in the main interrupt handler, this allows to simplify the code a
>> little.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
>>  1 file changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index ff706349b..c481f121d 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -556,9 +556,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>>  		/* Write next byte, except for IRQ after last byte */
>>  		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
>>  	}
>> -
>> -	/* Clear BYTE_DONE to continue with next byte */
>> -	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>>  }
>>  
>>  static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
>> @@ -588,7 +585,6 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
>>   *      BUS_ERR - SMI# transaction collision
>>   *      FAILED - transaction was canceled due to a KILL request
>>   *    When any of these occur, update ->status and signal completion.
>> - *    ->status must be cleared before kicking off the next transaction.
>>   *
>>   * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
>>   *    occurs for each byte of a byte-by-byte to prepare the next byte.
>> @@ -613,23 +609,18 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>>  	}
>>  
>>  	status = inb_p(SMBHSTSTS(priv));
>> -	if (status & SMBHSTSTS_BYTE_DONE)
>> +	if ((status & SMBHSTSTS_BYTE_DONE) && !(status & STATUS_ERROR_FLAGS))
> 
> Isn't this better written
> 
> 	if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)
> 
> ? At least my compiler generates smaller binary code.
> 
OK. Meanwhile I prefer readability over saving few bytes, but here IMO
the readability doesn't suffer.

>>  		i801_isr_byte_done(priv);
>>  
>>  	/*
>> -	 * Clear remaining IRQ sources: Completion of last command, errors
>> -	 * and the SMB_ALERT signal. SMB_ALERT status is set after signal
>> -	 * assertion independently of the interrupt generation being blocked
>> -	 * or not so clear it always when the status is set.
>> -	 */
>> -	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
>> -	if (status)
>> -		outb_p(status, SMBHSTSTS(priv));
>> -	status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
>> -	/*
>> -	 * Report transaction result.
>> -	 * ->status must be cleared before the next transaction is started.
>> +	 * Clear IRQ sources: SMB_ALERT status is set after signal assertion
>> +	 * independently of the interrupt generation being blocked or not
>> +	 * so clear it always when the status is set.
>>  	 */
>> +	status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
>> +	outb_p(status, SMBHSTSTS(priv));
> 
> You are making the call to outb_p() unconditional. Is this under the
> assumption that at least one of the bits must be set, so the condition
> was always met?
> 
So far we exclude here the case where SMBHSTSTS is written in
i801_isr_byte_done(). The patch allows to write SMBHSTSTS here only,
so we don't need the condition any longer.

>> +
>> +	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>>  	if (status) {
>>  		priv->status = status;
>>  		complete(&priv->done);
> 
> Tested OK on my system, looks good overall, nice simplification.
>