| Message ID | 1374526866-6813-1-git-send-email-bp@alien8.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Jul 22, 2013 at 2:01 PM, Borislav Petkov <bp@alien8.de> wrote: > From: Borislav Petkov <bp@suse.de> > > [ 5.525861] ERST: Can not request iomem region <0x c7eff000-0x c7f00000> for ERST. > > This needs to have leading zeroes. Make it so. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Cc: Anton Vorontsov <anton@enomsg.org> > Cc: Colin Cross <ccross@android.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Len Brown <lenb@kernel.org> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl> > Cc: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org Yes, please. :) Nice catch. Acked-by: Kees Cook <keescook@chromium.org>
On 2013/07/22 11:01PM, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > [ 5.525861] ERST: Can not request iomem region <0x c7eff000-0x c7f00000> for ERST. > > This needs to have leading zeroes. Make it so. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Cc: Anton Vorontsov <anton@enomsg.org> > Cc: Colin Cross <ccross@android.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Len Brown <lenb@kernel.org> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl> > Cc: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/acpi/apei/erst.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c > index 88d0b0f9f92b..1126afeb7e22 100644 > --- a/drivers/acpi/apei/erst.c > +++ b/drivers/acpi/apei/erst.c > @@ -1168,7 +1168,7 @@ static int __init erst_init(void) > r = request_mem_region(erst_erange.base, erst_erange.size, "APEI ERST"); > if (!r) { > pr_err(ERST_PFX > - "Can not request iomem region <0x%16llx-0x%16llx> for ERST.\n", > + "Can not request iomem region <0x%016llx-0x%016llx> for ERST.\n", > (unsigned long long)erst_erange.base, > (unsigned long long)erst_erange.base + erst_erange.size); > rc = -EIO; Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> While looking at this, I noticed that we seem to be using varying field widths in our APEI code: - einj.c has two instances using %#010llx. - apei-base.c uses widths of 10 (4 bytes) and 6 (2 bytes). Not sure if these are intentional and those fields truly aren't 64-bit (as suggested by the usage of long long int). Regards, Naveen > -- > 1.8.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-07-24 at 22:43 +0530, Naveen N. Rao wrote: > On 2013/07/22 11:01PM, Borislav Petkov wrote: > > From: Borislav Petkov <bp@suse.de> > > > > [ 5.525861] ERST: Can not request iomem region <0x c7eff000-0x c7f00000> for ERST. > > > > This needs to have leading zeroes. Make it so. Why does it need leading zeros? > While looking at this, I noticed that we seem to be using varying field > widths in our APEI code: > - einj.c has two instances using %#010llx. > - apei-base.c uses widths of 10 (4 bytes) and 6 (2 bytes). > > Not sure if these are intentional and those fields truly aren't 64-bit > (as suggested by the usage of long long int). I suggest using "0x%llx" everywhere unless there's a compelling reason like columnar alignment for them. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 24, 2013 at 10:43:47PM +0530, Naveen N. Rao wrote: > While looking at this, I noticed that we seem to be using varying > field widths in our APEI code: - einj.c has two instances using > %#010llx. - apei-base.c uses widths of 10 (4 bytes) and 6 (2 bytes). > > Not sure if these are intentional and those fields truly aren't 64-bit > (as suggested by the usage of long long int). That's an interesting question but I think you're better fitted to answer it - I try to avoid the APEI spec as much as possible. :-)
On 07/24/2013 10:53 PM, Joe Perches wrote: > On Wed, 2013-07-24 at 22:43 +0530, Naveen N. Rao wrote: >> On 2013/07/22 11:01PM, Borislav Petkov wrote: >>> From: Borislav Petkov <bp@suse.de> >>> >>> [ 5.525861] ERST: Can not request iomem region <0x c7eff000-0x c7f00000> for ERST. >>> >>> This needs to have leading zeroes. Make it so. > > Why does it need leading zeros? > >> While looking at this, I noticed that we seem to be using varying field >> widths in our APEI code: >> - einj.c has two instances using %#010llx. >> - apei-base.c uses widths of 10 (4 bytes) and 6 (2 bytes). >> >> Not sure if these are intentional and those fields truly aren't 64-bit >> (as suggested by the usage of long long int). > > I suggest using "0x%llx" everywhere unless there's a > compelling reason like columnar alignment for them. I think that might be better. I see that these changes were done in commit 46b91e37. Copying Bjorn Helgaas. On 07/24/2013 11:40 PM, Borislav Petkov wrote: > That's an interesting question but I think you're better fitted to > answer it - I try to avoid the APEI spec as much as possible. > > :-) > Oh, I've hardly scratched the surface w.r.t APEI. I think I'll let the experts comment on this :) Thanks, Naveen -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 25, 2013 at 5:23 AM, Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote: > On 07/24/2013 10:53 PM, Joe Perches wrote: >> >> On Wed, 2013-07-24 at 22:43 +0530, Naveen N. Rao wrote: >>> >>> On 2013/07/22 11:01PM, Borislav Petkov wrote: >>>> >>>> From: Borislav Petkov <bp@suse.de> >>>> >>>> [ 5.525861] ERST: Can not request iomem region <0x c7eff000-0x >>>> c7f00000> for ERST. >>>> >>>> This needs to have leading zeroes. Make it so. >> >> >> Why does it need leading zeros? >> >>> While looking at this, I noticed that we seem to be using varying field >>> widths in our APEI code: >>> - einj.c has two instances using %#010llx. >>> - apei-base.c uses widths of 10 (4 bytes) and 6 (2 bytes). >>> >>> Not sure if these are intentional and those fields truly aren't 64-bit >>> (as suggested by the usage of long long int). >> >> >> I suggest using "0x%llx" everywhere unless there's a >> compelling reason like columnar alignment for them. > > > I think that might be better. I see that these changes were done in commit > 46b91e37. Copying Bjorn Helgaas. As the 46b91e37 changelog says, it was done to use "the normal %pR-like format". I think that's a valid goal. When we're printing the same sort of information, we should use the same sort of format. But I don't think the "Can not request iomem region <0x c7eff000-0x c7f00000> for ERST" output mentioned in the original post was affected by 46b91e37. I would suggest a change similar to 46b91e37 for ERST, and I would suggest using the leading zeros, with %#010llx for physical memory addresses and %#06llx for ioport addresses. That's what %pR uses, and it produces columnar alignment in many cases (though not this one). Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/25/2013 11:02 PM, Bjorn Helgaas wrote: > On Thu, Jul 25, 2013 at 5:23 AM, Naveen N. Rao > <naveen.n.rao@linux.vnet.ibm.com> wrote: >> On 07/24/2013 10:53 PM, Joe Perches wrote: >>> >>> On Wed, 2013-07-24 at 22:43 +0530, Naveen N. Rao wrote: >>>> >>>> On 2013/07/22 11:01PM, Borislav Petkov wrote: >>>>> >>>>> From: Borislav Petkov <bp@suse.de> >>>>> >>>>> [ 5.525861] ERST: Can not request iomem region <0x c7eff000-0x >>>>> c7f00000> for ERST. >>>>> >>>>> This needs to have leading zeroes. Make it so. >>> >>> >>> Why does it need leading zeros? >>> >>>> While looking at this, I noticed that we seem to be using varying field >>>> widths in our APEI code: >>>> - einj.c has two instances using %#010llx. >>>> - apei-base.c uses widths of 10 (4 bytes) and 6 (2 bytes). >>>> >>>> Not sure if these are intentional and those fields truly aren't 64-bit >>>> (as suggested by the usage of long long int). >>> >>> >>> I suggest using "0x%llx" everywhere unless there's a >>> compelling reason like columnar alignment for them. >> >> >> I think that might be better. I see that these changes were done in commit >> 46b91e37. Copying Bjorn Helgaas. > > As the 46b91e37 changelog says, it was done to use "the normal > %pR-like format". I think that's a valid goal. When we're printing > the same sort of information, we should use the same sort of format. Bjorn, My key question was about why we are using a field width of 10 implying a 32-bit value, rather than a field width of 18 as suggested by the data type? This shouldn't truncate the value, but if we are specifying the field width for alignment, seems to me it is better to match the data type. Thanks, Naveen > > But I don't think the "Can not request iomem region <0x > c7eff000-0x c7f00000> for ERST" output mentioned in the > original post was affected by 46b91e37. I would suggest a change > similar to 46b91e37 for ERST, and I would suggest using the leading > zeros, with %#010llx for physical memory addresses and %#06llx for > ioport addresses. That's what %pR uses, and it produces columnar > alignment in many cases (though not this one). > > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 31, 2013 at 3:46 AM, Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote: > My key question was about why we are using a field width of 10 implying a > 32-bit value, rather than a field width of 18 as suggested by the data type? > This shouldn't truncate the value, but if we are specifying the field width > for alignment, seems to me it is better to match the data type. %pR uses a field width of 10 (two for "0x", eight for the value) simply because the majority of resource values fit in 32 bits. Larger values extend the width, so it's not a question of truncating any data. But it's no fun to read memory addresses when most of them have eight extra leading zeros (the high 32-bits of a 64-bit value). I think the same applies here; most ACPI table addresses still fit in 32 bits. We *do* use a field width of 18 for the e820 table, even though many of those regions fit in 32 bits. But that's sort of an exception because it's a table where addresses above 4GB are pretty common. But at the end of the day, I guess I'm just stating my personal preferences and yours might be different. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2013 11:30 PM, Bjorn Helgaas wrote: > On Wed, Jul 31, 2013 at 3:46 AM, Naveen N. Rao > <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> My key question was about why we are using a field width of 10 implying a >> 32-bit value, rather than a field width of 18 as suggested by the data type? >> This shouldn't truncate the value, but if we are specifying the field width >> for alignment, seems to me it is better to match the data type. > > %pR uses a field width of 10 (two for "0x", eight for the value) > simply because the majority of resource values fit in 32 bits. Larger > values extend the width, so it's not a question of truncating any > data. But it's no fun to read memory addresses when most of them have > eight extra leading zeros (the high 32-bits of a 64-bit value). I > think the same applies here; most ACPI table addresses still fit in 32 > bits. > > We *do* use a field width of 18 for the e820 table, even though many > of those regions fit in 32 bits. But that's sort of an exception > because it's a table where addresses above 4GB are pretty common. Makes sense, thanks for the explanation. > > But at the end of the day, I guess I'm just stating my personal > preferences and yours might be different. Right - I'd probably prefer just %#llx. But yeah, the currently used field width of 10 looks fine too. Thanks, Naveen -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c index 88d0b0f9f92b..1126afeb7e22 100644 --- a/drivers/acpi/apei/erst.c +++ b/drivers/acpi/apei/erst.c @@ -1168,7 +1168,7 @@ static int __init erst_init(void) r = request_mem_region(erst_erange.base, erst_erange.size, "APEI ERST"); if (!r) { pr_err(ERST_PFX - "Can not request iomem region <0x%16llx-0x%16llx> for ERST.\n", + "Can not request iomem region <0x%016llx-0x%016llx> for ERST.\n", (unsigned long long)erst_erange.base, (unsigned long long)erst_erange.base + erst_erange.size); rc = -EIO;