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

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

On 21 February 2018 at 10:54, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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?

Well, there are two ways SynQuacerI2cMasterStart() can fail:
- the bus is busy and no transfer was started by *this* master
- an attempt to start a transfer was made but the bus is not busy as a result

The former case can only occur in the presence of multiple masters.
The latter can probably only occur if some kind of corruption occurs
on the bus (I'm not sure, and this part originates in the Fujitsu
code)

In the latter case, F_I2C_BCR_MSS will be asserted, and de-asserting
it is the correct thing to do.

> 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"))?
>

I can fix that, sure.

> And could commit message mention this change in behaviour please?
>

OK