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

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

Commit Message

Ard Biesheuvel Feb. 21, 2018, 11:04 a.m.
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.

Note that this will result in the bus control bits to be de-asserted in
case of a failure to send the START condition, which is an appropriate
cleanup action to take after SynQuacerI2cMasterStart() fails.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
v2: clarify comment and commit log to explain that releasing the bus controls
    is an appropriate action for this failure mode

 Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
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, 11:11 a.m. | #1
On Wed, Feb 21, 2018 at 11:04:21AM +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.

> 

> Note that this will result in the bus control bits to be de-asserted in

> case of a failure to send the START condition, which is an appropriate

> cleanup action to take after SynQuacerI2cMasterStart() fails.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> ---

> v2: clarify comment and commit log to explain that releasing the bus controls

>     is an appropriate action for this failure mode

> 

>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c

> index 46c512a20151..5e70f9d921c3 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);

> @@ -397,7 +397,7 @@ SynQuacerI2cStartRequest (

>      } while (BufIdx < Op->LengthInBytes);

>    }

>  

> -  // Stop the transfer

> +  // Force bus state to idle, terminating any ongoing transfer

>    MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, 0);

>  

>    if (!AtRuntime) {

> -- 

> 2.11.0

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 21, 2018, 12:09 p.m. | #2
On 21 February 2018 at 11:11, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Feb 21, 2018 at 11:04:21AM +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.

>>

>> Note that this will result in the bus control bits to be de-asserted in

>> case of a failure to send the START condition, which is an appropriate

>> cleanup action to take after SynQuacerI2cMasterStart() fails.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>


Thanks

Pushed as 33d1b85bae27


>> ---

>> v2: clarify comment and commit log to explain that releasing the bus controls

>>     is an appropriate action for this failure mode

>>

>>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 4 ++--

>>  1 file changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c

>> index 46c512a20151..5e70f9d921c3 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);

>> @@ -397,7 +397,7 @@ SynQuacerI2cStartRequest (

>>      } while (BufIdx < Op->LengthInBytes);

>>    }

>>

>> -  // Stop the transfer

>> +  // Force bus state to idle, terminating any ongoing transfer

>>    MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, 0);

>>

>>    if (!AtRuntime) {

>> --

>> 2.11.0

>>

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

Patch

diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
index 46c512a20151..5e70f9d921c3 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);
@@ -397,7 +397,7 @@  SynQuacerI2cStartRequest (
     } while (BufIdx < Op->LengthInBytes);
   }
 
-  // Stop the transfer
+  // Force bus state to idle, terminating any ongoing transfer
   MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, 0);
 
   if (!AtRuntime) {