diff mbox series

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

Message ID 20180221101918.16088-1-ard.biesheuvel@linaro.org
State New
Headers show
Series [edk2,edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug | expand

Commit Message

Ard Biesheuvel Feb. 21, 2018, 10:19 a.m. UTC 
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(-)

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Feb. 21, 2018, 10:24 a.m. UTC | #1 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 21, 2018, 10:44 a.m. UTC | #2 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Feb. 21, 2018, 10:54 a.m. UTC | #3 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 21, 2018, 10:59 a.m. UTC | #4 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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);