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

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

On Wed, Feb 21, 2018 at 10:44:05AM +0000, Ard Biesheuvel wrote:
> 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.

So stopping an ongoing transfer even if none have been started (as is
possible here) is a non-issue? If so, I have no objection (but would
appreciate that mentioned where the "// Stop the transfer" comment
currently is).

(("// Force bus state to idle, terminating any ongoing transfer"))?

And could commit message mention this change in behaviour please?

/
    Leif