Re: [platforms: PATCH 01/10] Marvell/Drivers: MvI2cDxe: Abort transaction immediately upon fail

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Oct 26, 2017 at 03:19:28AM +0200, Marcin Wojtas wrote:
> From: David Greeson <dgreeson@cisco.com>
> 
> Although the I2C transaction routines were prepared to
> return their status, they were never used. This could
> cause bus lock-up e.g. in case of failing to send a
> slave address, the data transfer was attempted to be
> continued anyway.
> 
> This patch fixes faulty behavior by checking transaction
> status and stopping it immediately, once the fail
> is detected.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: David Greeson <dgreeson@cisco.com>
> [Style adjustment and cleanup]
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 34 +++++++++++++++++---
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index d85ee0b..7faf1f7 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -565,6 +565,7 @@ MvI2cStartRequest (
>    UINTN Transmitted;
>    I2C_MASTER_CONTEXT *I2cMasterContext = I2C_SC_FROM_MASTER(This);
>    EFI_I2C_OPERATION *Operation;
> +  EFI_STATUS Status = EFI_SUCCESS;
>  
>    ASSERT (RequestPacket != NULL);
>    ASSERT (I2cMasterContext != NULL);
> @@ -574,35 +575,58 @@ MvI2cStartRequest (
>      ReadMode = Operation->Flags & I2C_FLAG_READ;
>  
>      if (Count == 0) {
> -      MvI2cStart ( I2cMasterContext,
> +      Status = MvI2cStart (I2cMasterContext,
>                     (SlaveAddress << 1) | ReadMode,
>                     I2C_TRANSFER_TIMEOUT

Much as I appreciate seeing this form of the code, since it simplifies
seeing the functional changes, this does cause those lines left
unchanges to no longer conform to coding style.
Can you please adjust throughout for a v2?

>                   );
>      } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) {
> -      MvI2cRepeatedStart ( I2cMasterContext,
> +      Status = MvI2cRepeatedStart (I2cMasterContext,
>                             (SlaveAddress << 1) | ReadMode,
>                             I2C_TRANSFER_TIMEOUT
>                           );
>      }
>  
> +    /* I2C transaction was aborted, so stop further transactions */
> +    if (EFI_ERROR (Status)) {
> +      MvI2cStop (I2cMasterContext);
> +      break;
> +    }
> +
> +    /*
> +     * If sending the slave address was successful,
> +     * proceed to read or write section.
> +     */
>      if (ReadMode) {
> -      MvI2cRead ( I2cMasterContext,
> +      Status = MvI2cRead (I2cMasterContext,
>                    Operation->Buffer,
>                    Operation->LengthInBytes,
>                    &Transmitted,
>                    Count == 1,
>                    I2C_TRANSFER_TIMEOUT
>                   );
> +      Operation->LengthInBytes = Transmitted;
>      } else {
> -      MvI2cWrite ( I2cMasterContext,
> +      Status = MvI2cWrite (I2cMasterContext,
>                     Operation->Buffer,
>                     Operation->LengthInBytes,
>                     &Transmitted,
>                     I2C_TRANSFER_TIMEOUT
>                    );
> +      Operation->LengthInBytes = Transmitted;
>      }
> +
> +    /*
> +     * The I2C read or write transaction failed.
> +     * Stop the I2C transaction.
> +     */
> +    if (EFI_ERROR (Status)) {
> +      MvI2cStop (I2cMasterContext);
> +      break;
> +    }
> +
> +    /* Check if there is any more data to be sent */
>      if (Count == RequestPacket->OperationCount - 1) {
> -      MvI2cStop ( I2cMasterContext );
> +      MvI2cStop (I2cMasterContext);

Can you simply drop this non-functional change?
I'd prefer the non-adherence to coding-style over a misleading
history.

No objection to functional aspects of this patch.

/
    Leif

>      }
>    }
>  
> -- 
> 2.7.4
>