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

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

2017-10-26 14:51 GMT+02:00 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?
>

No problem. I of course saw style violations, but I gave up on them
for "no mix of functional improvements and style cleanups" contraint
:) I will correct the modified function calls.

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

Right, I saw it after sending - I was cleaning dirty patch and
splitting into 3, this line got here by mistake.

> No objection to functional aspects of this patch.

Ok, thanks!

Marcin

>
> /
>     Leif
>
>>      }
>>    }
>>
>> --
>> 2.7.4
>>