Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 21 February 2018 at 10:24, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Feb 21, 2018 at 10:19:18AM +0000, Ard Biesheuvel wrote:
>> Currently, SynQuacerI2cStartRequest() increases the TPL to TPL_HIGH_LEVEL
>> while accessing the I2C controller hardware, but fails to restore the TPL
>> to the original level if the call to SynQuacerI2cMasterStart() fails, and
>> returns right away. Given the TPL_HIGH_LEVEL implies that interrupts are
>> disabled, this results in a complete system hang. So instead, break out
>> of the loop, so that the TPL restore will occur before leaving the function.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
>> index 46c512a20151..b2318a6f5a8c 100644
>> --- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
>> +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
>> @@ -324,7 +324,7 @@ SynQuacerI2cStartRequest (
>>
>>      Status = SynQuacerI2cMasterStart (I2c, SlaveAddress, Op);
>>      if (EFI_ERROR (Status)) {
>> -      return Status;
>> +      break;
>>      }
>>
>>      Status = WaitForInterrupt (I2c);
>
> This change also causes
>
>   // Stop the transfer
>   MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, 0);
>
> to be executed in the faulting case.
>
> Which at the very least looks quirky.
>

I don't think it is unreasonable in general to stop any ongoing
transfer no matter how we leave this function. And in this particular
case, one of the failure modes is F_I2C_BCR_MSS being set without the
bus being busy as a result, and so clearing BCR is actually rather
appropriate here.