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
);
} 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);
}
}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Oct 26, 2017 at 03:19:36PM +0200, Marcin Wojtas wrote:
> 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.
Clarification: this is and has always been _unrelated_ style cleanups.
Any statement that is actually being modified should be conformant
afterwards.
Thanks.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
2017-10-26 15:54 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:19:36PM +0200, Marcin Wojtas wrote:
>> 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.
>
> Clarification: this is and has always been _unrelated_ style cleanups.
> Any statement that is actually being modified should be conformant
> afterwards.
>
Ok, thanks for clarification.
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2025 Red Hat, Inc.