On Wed, Feb 21, 2018 at 10:44:05AM +0000, Ard Biesheuvel wrote:
> On 21 February 2018 at 10:24, Leif Lindholm <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
