According to the PI spec, Volume 3,
EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks():
> The variable argument list is a list of tuples. Each tuple describes a
> range of LBAs to erase and consists of the following:
> * An EFI_LBA that indicates the starting LBA
> * A UINTN that indicates the number of blocks to erase
(NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to
EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.)
In this driver, the NumOfLba local variable is defined with type UINTN,
but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch.
In addition, update the DEBUG macro invocation where NumOfLba is formatted
with the %d conversion specifier: UINTN values should be converted to
UINT64 and printed with %Lu or %Lx for portability between 32-bit and
64-bit.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Reported-by: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
index 12a861267a86..3ea858f46ffb 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
@@ -628,10 +628,16 @@ FvbEraseBlocks (
}
// How many Lba blocks are we requested to erase?
- NumOfLba = VA_ARG (Args, UINT32);
+ NumOfLba = VA_ARG (Args, UINTN);
// All blocks must be within range
- DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
+ DEBUG ((
+ DEBUG_BLKIO,
+ "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
+ Instance->StartLba + StartingLba,
+ (UINT64)NumOfLba,
+ Instance->Media.LastBlock
+ ));
if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) {
VA_END (Args);
DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n"));
@@ -656,7 +662,7 @@ FvbEraseBlocks (
}
// How many Lba blocks are we requested to erase?
- NumOfLba = VA_ARG (Args, UINT32);
+ NumOfLba = VA_ARG (Args, UINTN);
// Go through each one and erase it
while (NumOfLba > 0) {
--
2.9.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 18 May 2017 at 16:04, Laszlo Ersek <lersek@redhat.com> wrote:
> According to the PI spec, Volume 3,
> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks():
>
>> The variable argument list is a list of tuples. Each tuple describes a
>> range of LBAs to erase and consists of the following:
>> * An EFI_LBA that indicates the starting LBA
>> * A UINTN that indicates the number of blocks to erase
>
> (NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to
> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.)
>
> In this driver, the NumOfLba local variable is defined with type UINTN,
> but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch.
>
> In addition, update the DEBUG macro invocation where NumOfLba is formatted
> with the %d conversion specifier: UINTN values should be converted to
> UINT64 and printed with %Lu or %Lx for portability between 32-bit and
> 64-bit.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Reported-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> index 12a861267a86..3ea858f46ffb 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> @@ -628,10 +628,16 @@ FvbEraseBlocks (
> }
>
> // How many Lba blocks are we requested to erase?
> - NumOfLba = VA_ARG (Args, UINT32);
> + NumOfLba = VA_ARG (Args, UINTN);
>
> // All blocks must be within range
> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> + DEBUG ((
> + DEBUG_BLKIO,
> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> + Instance->StartLba + StartingLba,
> + (UINT64)NumOfLba,
> + Instance->Media.LastBlock
> + ));
> if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) {
> VA_END (Args);
> DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n"));
> @@ -656,7 +662,7 @@ FvbEraseBlocks (
> }
>
> // How many Lba blocks are we requested to erase?
> - NumOfLba = VA_ARG (Args, UINT32);
> + NumOfLba = VA_ARG (Args, UINTN);
>
> // Go through each one and erase it
> while (NumOfLba > 0) {
> --
> 2.9.3
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-05-18 08:04:20, Laszlo Ersek wrote:
> According to the PI spec, Volume 3,
> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks():
>
> > The variable argument list is a list of tuples. Each tuple describes a
> > range of LBAs to erase and consists of the following:
> > * An EFI_LBA that indicates the starting LBA
> > * A UINTN that indicates the number of blocks to erase
>
> (NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to
> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.)
>
> In this driver, the NumOfLba local variable is defined with type UINTN,
> but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch.
>
> In addition, update the DEBUG macro invocation where NumOfLba is formatted
> with the %d conversion specifier: UINTN values should be converted to
> UINT64 and printed with %Lu or %Lx for portability between 32-bit and
> 64-bit.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Reported-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> index 12a861267a86..3ea858f46ffb 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> @@ -628,10 +628,16 @@ FvbEraseBlocks (
> }
>
> // How many Lba blocks are we requested to erase?
> - NumOfLba = VA_ARG (Args, UINT32);
> + NumOfLba = VA_ARG (Args, UINTN);
>
> // All blocks must be within range
> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> + DEBUG ((
> + DEBUG_BLKIO,
> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
Notably this is still > 80 columns. Maybe?
"FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
"> LastBlock=%ld.\n",
I noticed the capital 'L' in "%Lu". Doesn't make a difference since
it's not hex...
The need for this debug message seems dubious to me, but I guess I
might not be familiar with situation.
Thanks for noticing and fixing this bug in all the EDK II drivers.
Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> + Instance->StartLba + StartingLba,
> + (UINT64)NumOfLba,
> + Instance->Media.LastBlock
> + ));
> if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) {
> VA_END (Args);
> DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n"));
> @@ -656,7 +662,7 @@ FvbEraseBlocks (
> }
>
> // How many Lba blocks are we requested to erase?
> - NumOfLba = VA_ARG (Args, UINT32);
> + NumOfLba = VA_ARG (Args, UINTN);
>
> // Go through each one and erase it
> while (NumOfLba > 0) {
> --
> 2.9.3
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 05/18/17 19:21, Jordan Justen wrote:
> On 2017-05-18 08:04:20, Laszlo Ersek wrote:
>> According to the PI spec, Volume 3,
>> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks():
>>
>>> The variable argument list is a list of tuples. Each tuple describes a
>>> range of LBAs to erase and consists of the following:
>>> * An EFI_LBA that indicates the starting LBA
>>> * A UINTN that indicates the number of blocks to erase
>>
>> (NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to
>> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.)
>>
>> In this driver, the NumOfLba local variable is defined with type UINTN,
>> but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch.
>>
>> In addition, update the DEBUG macro invocation where NumOfLba is formatted
>> with the %d conversion specifier: UINTN values should be converted to
>> UINT64 and printed with %Lu or %Lx for portability between 32-bit and
>> 64-bit.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Reported-by: Jordan Justen <jordan.l.justen@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
>> index 12a861267a86..3ea858f46ffb 100644
>> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
>> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
>> @@ -628,10 +628,16 @@ FvbEraseBlocks (
>> }
>>
>> // How many Lba blocks are we requested to erase?
>> - NumOfLba = VA_ARG (Args, UINT32);
>> + NumOfLba = VA_ARG (Args, UINTN);
>>
>> // All blocks must be within range
>> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
>> + DEBUG ((
>> + DEBUG_BLKIO,
>> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
>
> Notably this is still > 80 columns. Maybe?
>
> "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
> "> LastBlock=%ld.\n",
This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has
extremely long lines, the longest one (line 774) has 172 columns. I
broke up the above DEBUG so that it would at least fit in 120 chars per
line (which is the "second level" recommendation in the coding spec).
Also, the format string looked messy enough for me not to want to break
it up.
Finally, Leif has expressed a preference earlier for keeping DEBUG
format strings on one line, if memory serves. (I certainly don't follow
that in OvmfPkg and ArmVirtPkg, but ArmPlatformPkg is not my co-turf :) )
I think I'd like to leave the format string like this.
>
> I noticed the capital 'L' in "%Lu". Doesn't make a difference since
> it's not hex...
PrintLib handes "l" and "L" interchangeably. In
BasePrintLibSPrintMarker(), file
"MdePkg/Library/BasePrintLib/PrintLibInternal.c", we have
case 'L':
case 'l':
Flags |= LONG_TYPE;
break;
and all hex characters are always printed upper-case (see "mHexStr").
Instead, I use the "L" length modifier rather than "l" because:
- in ISO C, "l" (in %lx, %ld, %lu) stands for "long", and "L" is not
valid for integers,
- while in edk2, both "l" and "L" stand for 64-bit (which is a different
question from "long").
In other words, "l" has conflicting meanings between ISO C and edk2
(long vs. 64-bit), while "L" is uniquely defined (it is undefined in ISO
C, for integers, and in edk2 it stands for 64-bit).
> The need for this debug message seems dubious to me, but I guess I
> might not be familiar with situation.
>
> Thanks for noticing and fixing this bug in all the EDK II drivers.
For noticing the bug (outside of OvmfPkg/EmuVariableFvbRuntimeDxe, where
I originally fixed it), the credit goes to you :) The patches have the
right Reported-by: tag.
>
> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Thanks!
Laszlo
>> + Instance->StartLba + StartingLba,
>> + (UINT64)NumOfLba,
>> + Instance->Media.LastBlock
>> + ));
>> if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) {
>> VA_END (Args);
>> DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n"));
>> @@ -656,7 +662,7 @@ FvbEraseBlocks (
>> }
>>
>> // How many Lba blocks are we requested to erase?
>> - NumOfLba = VA_ARG (Args, UINT32);
>> + NumOfLba = VA_ARG (Args, UINTN);
>>
>> // Go through each one and erase it
>> while (NumOfLba > 0) {
>> --
>> 2.9.3
>>
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-05-18 12:29:09, Laszlo Ersek wrote:
> On 05/18/17 19:21, Jordan Justen wrote:
> > On 2017-05-18 08:04:20, Laszlo Ersek wrote:
> >> // All blocks must be within range
> >> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> >> + DEBUG ((
> >> + DEBUG_BLKIO,
> >> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> >
> > Notably this is still > 80 columns. Maybe?
> >
> > "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
> > "> LastBlock=%ld.\n",
>
> This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has
> extremely long lines, the longest one (line 774) has 172 columns. I
> broke up the above DEBUG so that it would at least fit in 120 chars per
> line (which is the "second level" recommendation in the coding spec).
Personally, I don't agree with that secondary 120 char rule. If we
ever get the style guide into an 'open source' process, I'd like to
suggest removing it. (But, it'll probably get shot down. :\ )
> Also, the format string looked messy enough for me not to want to break
> it up.
>
> Finally, Leif has expressed a preference earlier for keeping DEBUG
> format strings on one line, if memory serves. (I certainly don't follow
> that in OvmfPkg and ArmVirtPkg, but ArmPlatformPkg is not my co-turf :) )
Ah. I guess it is fine for a package maintainer to occasionally decide
to bend the rules for their package. Hopefully it would not eventually
lead to the style migrating into other packages though.
-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, May 18, 2017 at 01:44:58PM -0700, Jordan Justen wrote:
> On 2017-05-18 12:29:09, Laszlo Ersek wrote:
> > On 05/18/17 19:21, Jordan Justen wrote:
> > > On 2017-05-18 08:04:20, Laszlo Ersek wrote:
> > >> // All blocks must be within range
> > >> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> > >> + DEBUG ((
> > >> + DEBUG_BLKIO,
> > >> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> > >
> > > Notably this is still > 80 columns. Maybe?
> > >
> > > "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
> > > "> LastBlock=%ld.\n",
> >
> > This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has
> > extremely long lines, the longest one (line 774) has 172 columns. I
> > broke up the above DEBUG so that it would at least fit in 120 chars per
> > line (which is the "second level" recommendation in the coding spec).
>
> Personally, I don't agree with that secondary 120 char rule. If we
> ever get the style guide into an 'open source' process, I'd like to
> suggest removing it. (But, it'll probably get shot down. :\ )
Oh, I'm all for that one. And the style guide is definitely in need of
a shake-up.
But I consider violating line length restrictions less bad than making
user-(or in this case developer)-visible strings harder to search for.
> Ah. I guess it is fine for a package maintainer to occasionally decide
> to bend the rules for their package.
For this to be bending, it would require the 120-character rule to not
exist.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-05-19 03:07:56, Leif Lindholm wrote:
> On Thu, May 18, 2017 at 01:44:58PM -0700, Jordan Justen wrote:
> > On 2017-05-18 12:29:09, Laszlo Ersek wrote:
> > > On 05/18/17 19:21, Jordan Justen wrote:
> > > > On 2017-05-18 08:04:20, Laszlo Ersek wrote:
> > > >> // All blocks must be within range
> > > >> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> > > >> + DEBUG ((
> > > >> + DEBUG_BLKIO,
> > > >> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> > > >
> > > > Notably this is still > 80 columns. Maybe?
> > > >
> > > > "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
> > > > "> LastBlock=%ld.\n",
> > >
> > > This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has
> > > extremely long lines, the longest one (line 774) has 172 columns. I
> > > broke up the above DEBUG so that it would at least fit in 120 chars per
> > > line (which is the "second level" recommendation in the coding spec).
> >
> > Personally, I don't agree with that secondary 120 char rule. If we
> > ever get the style guide into an 'open source' process, I'd like to
> > suggest removing it. (But, it'll probably get shot down. :\ )
>
> Oh, I'm all for that one. And the style guide is definitely in need of
> a shake-up.
>
> But I consider violating line length restrictions less bad than making
> user-(or in this case developer)-visible strings harder to search for.
While of less concern, I would say the style guide should recommend
that logged debug messages should attempt to be less than 80 chars.
That could help here.
> > Ah. I guess it is fine for a package maintainer to occasionally decide
> > to bend the rules for their package.
>
> For this to be bending, it would require the 120-character rule to not
> exist.
Yeah. It is very silly to say, we prefer 80 chars, but if it's too
hard then 120 can be used. :)
The latest coding standard says: "Preferably, limit line lengths to 80
columns or less. When this doesn’t leave sufficient space for a good
postfix style comment, extend the line to a total of 120 columns."
This seems to imply that the extension beyond 80 chars is reserved for
postfix comments. This seems like a poor reason to push past 80. It
would be better to comment above the line instead.
-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Fri, May 19, 2017 at 09:59:06AM -0700, Jordan Justen wrote:
> On 2017-05-19 03:07:56, Leif Lindholm wrote:
> > On Thu, May 18, 2017 at 01:44:58PM -0700, Jordan Justen wrote:
> > > On 2017-05-18 12:29:09, Laszlo Ersek wrote:
> > > > On 05/18/17 19:21, Jordan Justen wrote:
> > > > > On 2017-05-18 08:04:20, Laszlo Ersek wrote:
> > > > >> // All blocks must be within range
> > > > >> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> > > > >> + DEBUG ((
> > > > >> + DEBUG_BLKIO,
> > > > >> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> > > > >
> > > > > Notably this is still > 80 columns. Maybe?
> > > > >
> > > > > "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
> > > > > "> LastBlock=%ld.\n",
> > > >
> > > > This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has
> > > > extremely long lines, the longest one (line 774) has 172 columns. I
> > > > broke up the above DEBUG so that it would at least fit in 120 chars per
> > > > line (which is the "second level" recommendation in the coding spec).
> > >
> > > Personally, I don't agree with that secondary 120 char rule. If we
> > > ever get the style guide into an 'open source' process, I'd like to
> > > suggest removing it. (But, it'll probably get shot down. :\ )
> >
> > Oh, I'm all for that one. And the style guide is definitely in need of
> > a shake-up.
> >
> > But I consider violating line length restrictions less bad than making
> > user-(or in this case developer)-visible strings harder to search for.
>
> While of less concern, I would say the style guide should recommend
> that logged debug messages should attempt to be less than 80 chars.
> That could help here.
Yes - had that string appeared as a new entity (instead of a
modification of existing code, with Laszlo being the good citizen to
make sure the modified lines complied as well as possible), that would
most likely have been my feedback.
> > > Ah. I guess it is fine for a package maintainer to occasionally decide
> > > to bend the rules for their package.
> >
> > For this to be bending, it would require the 120-character rule to not
> > exist.
>
> Yeah. It is very silly to say, we prefer 80 chars, but if it's too
> hard then 120 can be used. :)
>
> The latest coding standard says: "Preferably, limit line lengths to 80
> columns or less. When this doesn’t leave sufficient space for a good
> postfix style comment, extend the line to a total of 120 columns."
>
> This seems to imply that the extension beyond 80 chars is reserved for
> postfix comments. This seems like a poor reason to push past 80. It
> would be better to comment above the line instead.
Most definitely.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 05/18/17 17:04, Laszlo Ersek wrote:
> According to the PI spec, Volume 3,
> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks():
>
>> The variable argument list is a list of tuples. Each tuple describes a
>> range of LBAs to erase and consists of the following:
>> * An EFI_LBA that indicates the starting LBA
>> * A UINTN that indicates the number of blocks to erase
>
> (NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to
> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.)
>
> In this driver, the NumOfLba local variable is defined with type UINTN,
> but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch.
>
> In addition, update the DEBUG macro invocation where NumOfLba is formatted
> with the %d conversion specifier: UINTN values should be converted to
> UINT64 and printed with %Lu or %Lx for portability between 32-bit and
> 64-bit.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Reported-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> index 12a861267a86..3ea858f46ffb 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> @@ -628,10 +628,16 @@ FvbEraseBlocks (
> }
>
> // How many Lba blocks are we requested to erase?
> - NumOfLba = VA_ARG (Args, UINT32);
> + NumOfLba = VA_ARG (Args, UINTN);
>
> // All blocks must be within range
> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
> + DEBUG ((
> + DEBUG_BLKIO,
> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> + Instance->StartLba + StartingLba,
> + (UINT64)NumOfLba,
> + Instance->Media.LastBlock
> + ));
> if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) {
> VA_END (Args);
> DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n"));
> @@ -656,7 +662,7 @@ FvbEraseBlocks (
> }
>
> // How many Lba blocks are we requested to erase?
> - NumOfLba = VA_ARG (Args, UINT32);
> + NumOfLba = VA_ARG (Args, UINTN);
>
> // Go through each one and erase it
> while (NumOfLba > 0) {
>
Thanks all for the feedback, this is commit ce69cc776cfc.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.