* [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug @ 2018-02-21 10:19 Ard Biesheuvel 2018-02-21 10:24 ` Leif Lindholm 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2018-02-21 10:19 UTC (permalink / raw) To: edk2-devel; +Cc: leif.lindholm, Ard Biesheuvel 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); -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug 2018-02-21 10:19 [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug Ard Biesheuvel @ 2018-02-21 10:24 ` Leif Lindholm 2018-02-21 10:44 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Leif Lindholm @ 2018-02-21 10:24 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug 2018-02-21 10:24 ` Leif Lindholm @ 2018-02-21 10:44 ` Ard Biesheuvel 2018-02-21 10:54 ` Leif Lindholm 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2018-02-21 10:44 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug 2018-02-21 10:44 ` Ard Biesheuvel @ 2018-02-21 10:54 ` Leif Lindholm 2018-02-21 10:59 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Leif Lindholm @ 2018-02-21 10:54 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug 2018-02-21 10:54 ` Leif Lindholm @ 2018-02-21 10:59 ` Ard Biesheuvel 0 siblings, 0 replies; 5+ messages in thread From: Ard Biesheuvel @ 2018-02-21 10:59 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-21 10:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-21 10:19 [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug Ard Biesheuvel 2018-02-21 10:24 ` Leif Lindholm 2018-02-21 10:44 ` Ard Biesheuvel 2018-02-21 10:54 ` Leif Lindholm 2018-02-21 10:59 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox