Hi Leif,
2017-10-26 14:51 GMT+02:00 Leif Lindholm <[email protected]>:
> On Thu, Oct 26, 2017 at 03:19:28AM +0200, Marcin Wojtas wrote:
>> From: David Greeson <[email protected]>
>>
>> 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 <[email protected]>
>> [Style adjustment and cleanup]
>> Signed-off-by: Marcin Wojtas <[email protected]>
>> ---
>> 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
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
