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

[Leif Lindholm <leif.lindholm@linaro.org>]

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.

/
    Leif