Commit fab4c256a58b ("PCI/AER: Add a TLP header print helper")
introduced the helper function __print_tlp_header, but contrary to the
intention, the behaviour did change: Since we're taking the address of
the parameter t, the first 4 or 8 bytes printed will be the value of
the pointer t itself, and the remaining 12 or 8 bytes will be
who-knows-what (something from the stack).
We want to treat the four members of the struct aer_header_log_regs as
little-endian 32 bit numbers and print those. That can be done without
ugly and confusing casts.
Fixes: fab4c256a58b ("PCI/AER: Add a TLP header print helper")
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/pci/pcie/aer/aerdrv_errprint.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
index c6849d9e86ce..e328978038c1 100644
--- a/drivers/pci/pcie/aer/aerdrv_errprint.c
+++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
@@ -132,16 +132,9 @@ static const char *aer_agent_string[] = {
static void __print_tlp_header(struct pci_dev *dev,
struct aer_header_log_regs *t)
{
- unsigned char *tlp = (unsigned char *)&t;
-
- dev_err(&dev->dev, " TLP Header:"
- " %02x%02x%02x%02x %02x%02x%02x%02x"
- " %02x%02x%02x%02x %02x%02x%02x%02x\n",
- *(tlp + 3), *(tlp + 2), *(tlp + 1), *tlp,
- *(tlp + 7), *(tlp + 6), *(tlp + 5), *(tlp + 4),
- *(tlp + 11), *(tlp + 10), *(tlp + 9),
- *(tlp + 8), *(tlp + 15), *(tlp + 14),
- *(tlp + 13), *(tlp + 12));
+ dev_err(&dev->dev, " TLP Header: %08x %08x %08x %08x\n",
+ le32_to_cpu(t->dw0), le32_to_cpu(t->dw1),
+ le32_to_cpu(t->dw2), le32_to_cpu(t->dw3));
}
static void __aer_print_error(struct pci_dev *dev,
--
2.1.3
On Tue, Feb 24, 2015 at 11:50:46PM +0100, Rasmus Villemoes wrote:
> Commit fab4c256a58b ("PCI/AER: Add a TLP header print helper")
> introduced the helper function __print_tlp_header, but contrary to the
> intention, the behaviour did change: Since we're taking the address of
Whoops, good catch.
> the parameter t, the first 4 or 8 bytes printed will be the value of
> the pointer t itself, and the remaining 12 or 8 bytes will be
> who-knows-what (something from the stack).
>
> We want to treat the four members of the struct aer_header_log_regs as
> little-endian 32 bit numbers and print those. That can be done without
> ugly and confusing casts.
>
> Fixes: fab4c256a58b ("PCI/AER: Add a TLP header print helper")
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> drivers/pci/pcie/aer/aerdrv_errprint.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
> index c6849d9e86ce..e328978038c1 100644
> --- a/drivers/pci/pcie/aer/aerdrv_errprint.c
> +++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
> @@ -132,16 +132,9 @@ static const char *aer_agent_string[] = {
> static void __print_tlp_header(struct pci_dev *dev,
> struct aer_header_log_regs *t)
> {
> - unsigned char *tlp = (unsigned char *)&t;
> -
> - dev_err(&dev->dev, " TLP Header:"
> - " %02x%02x%02x%02x %02x%02x%02x%02x"
> - " %02x%02x%02x%02x %02x%02x%02x%02x\n",
> - *(tlp + 3), *(tlp + 2), *(tlp + 1), *tlp,
> - *(tlp + 7), *(tlp + 6), *(tlp + 5), *(tlp + 4),
> - *(tlp + 11), *(tlp + 10), *(tlp + 9),
> - *(tlp + 8), *(tlp + 15), *(tlp + 14),
> - *(tlp + 13), *(tlp + 12));
> + dev_err(&dev->dev, " TLP Header: %08x %08x %08x %08x\n",
> + le32_to_cpu(t->dw0), le32_to_cpu(t->dw1),
> + le32_to_cpu(t->dw2), le32_to_cpu(t->dw3));
I'm not sure about this: I think the original intention was to dump the
dwords MS-bit to LS-bit like this here:
http://www.fpga4fun.com/PCI-Express4.html
Now, if this runs on a big endian machine, converting to CPU order would
be wrong IMHO. You'd rather want do do cpu_to_le32() for consistency.
But I don't know whether big endian machines are even sporting PCIE
AER...
Bjorn?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
[+cc Tony]
On Wed, Feb 25, 2015 at 10:54 AM, Borislav Petkov <[email protected]> wrote:
> On Tue, Feb 24, 2015 at 11:50:46PM +0100, Rasmus Villemoes wrote:
>> Commit fab4c256a58b ("PCI/AER: Add a TLP header print helper")
>> introduced the helper function __print_tlp_header, but contrary to the
>> intention, the behaviour did change: Since we're taking the address of
>
> Whoops, good catch.
>
>> the parameter t, the first 4 or 8 bytes printed will be the value of
>> the pointer t itself, and the remaining 12 or 8 bytes will be
>> who-knows-what (something from the stack).
>>
>> We want to treat the four members of the struct aer_header_log_regs as
>> little-endian 32 bit numbers and print those. That can be done without
>> ugly and confusing casts.
>>
>> Fixes: fab4c256a58b ("PCI/AER: Add a TLP header print helper")
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>> ---
>> drivers/pci/pcie/aer/aerdrv_errprint.c | 13 +++----------
>> 1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
>> index c6849d9e86ce..e328978038c1 100644
>> --- a/drivers/pci/pcie/aer/aerdrv_errprint.c
>> +++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
>> @@ -132,16 +132,9 @@ static const char *aer_agent_string[] = {
>> static void __print_tlp_header(struct pci_dev *dev,
>> struct aer_header_log_regs *t)
>> {
>> - unsigned char *tlp = (unsigned char *)&t;
>> -
>> - dev_err(&dev->dev, " TLP Header:"
>> - " %02x%02x%02x%02x %02x%02x%02x%02x"
>> - " %02x%02x%02x%02x %02x%02x%02x%02x\n",
>> - *(tlp + 3), *(tlp + 2), *(tlp + 1), *tlp,
>> - *(tlp + 7), *(tlp + 6), *(tlp + 5), *(tlp + 4),
>> - *(tlp + 11), *(tlp + 10), *(tlp + 9),
>> - *(tlp + 8), *(tlp + 15), *(tlp + 14),
>> - *(tlp + 13), *(tlp + 12));
>> + dev_err(&dev->dev, " TLP Header: %08x %08x %08x %08x\n",
>> + le32_to_cpu(t->dw0), le32_to_cpu(t->dw1),
>> + le32_to_cpu(t->dw2), le32_to_cpu(t->dw3));
>
> I'm not sure about this: I think the original intention was to dump the
> dwords MS-bit to LS-bit like this here:
>
> http://www.fpga4fun.com/PCI-Express4.html
>
> Now, if this runs on a big endian machine, converting to CPU order would
> be wrong IMHO. You'd rather want do do cpu_to_le32() for consistency.
> But I don't know whether big endian machines are even sporting PCIE
> AER...
I think we should expect AER to be used on big-endian machines. I'm
pretty sure it's used on Itanium in big-endian mode.
Why are we worrying about byte order here at all? I'd think we could
just print t->dw0 directly with %08x.
Any byte order issues should be handled when we fill in the struct
aer_header_log_regs. For normal AER (non-APEI), we use
pci_read_config_dword() to directly fill in info->tlp.dw0, etc. in
get_device_error_info(), so I don't think there's a problem there.
For APEI, it looks like it would happen somewhere in
ghes_read_estatus(). I didn't follow the whole path here, but I would
argue that by the time we put data in t->dw0, it should be in CPU
order so a mask like 0x80000000 would work the same on LE and BE
boxes.
Bjorn
> I think we should expect AER to be used on big-endian machines. I'm
> pretty sure it's used on Itanium in big-endian mode.
Itanium can run in either big or little endian mode - but Linux uses little endian:
arch/ia64/include/uapi/asm/byteorder.h:#include <linux/byteorder/little_endian.h>
-Tony
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Feb 25, 2015 at 12:18:14PM -0800, Bjorn Helgaas wrote:
> Why are we worrying about byte order here at all? I'd think we could
> just print t->dw0 directly with %08x.
Right, my only concern is since this is user-facing, someone/something
might depend on its format/byte order. But since no one has complained
about the bug so far, we might just as well do the simple thing and just
print t->dwX.
Btw, Bjorn, when applying the final version, please add the stable tag
too.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
Commit fab4c256a58b ("PCI/AER: Add a TLP header print helper")
introduced the helper function __print_tlp_header, but contrary to the
intention, the behaviour did change: Since we're taking the address of
the parameter t, the first 4 or 8 bytes printed will be the value of
the pointer t itself, and the remaining 12 or 8 bytes will be
who-knows-what (something from the stack).
We want to show the values of the four members of the struct
aer_header_log_regs; that can be done without ugly and error-prone
casts. On little-endian this should produce the same output as
originally intended, and since no-one has complained about getting
garbage output so far, I think big-endian should be ok too.
Fixes: fab4c256a58b ("PCI/AER: Add a TLP header print helper")
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: Just print ->dwX as-is.
drivers/pci/pcie/aer/aerdrv_errprint.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
index c6849d9e86ce..167fe411ce2e 100644
--- a/drivers/pci/pcie/aer/aerdrv_errprint.c
+++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
@@ -132,16 +132,8 @@ static const char *aer_agent_string[] = {
static void __print_tlp_header(struct pci_dev *dev,
struct aer_header_log_regs *t)
{
- unsigned char *tlp = (unsigned char *)&t;
-
- dev_err(&dev->dev, " TLP Header:"
- " %02x%02x%02x%02x %02x%02x%02x%02x"
- " %02x%02x%02x%02x %02x%02x%02x%02x\n",
- *(tlp + 3), *(tlp + 2), *(tlp + 1), *tlp,
- *(tlp + 7), *(tlp + 6), *(tlp + 5), *(tlp + 4),
- *(tlp + 11), *(tlp + 10), *(tlp + 9),
- *(tlp + 8), *(tlp + 15), *(tlp + 14),
- *(tlp + 13), *(tlp + 12));
+ dev_err(&dev->dev, " TLP Header: %08x %08x %08x %08x\n",
+ t->dw0, t->dw1, t->dw2, t->dw3);
}
static void __aer_print_error(struct pci_dev *dev,
--
2.1.3
On Thu, Feb 26, 2015 at 09:55:03AM +0100, Rasmus Villemoes wrote:
> Commit fab4c256a58b ("PCI/AER: Add a TLP header print helper")
> introduced the helper function __print_tlp_header, but contrary to the
> intention, the behaviour did change: Since we're taking the address of
> the parameter t, the first 4 or 8 bytes printed will be the value of
> the pointer t itself, and the remaining 12 or 8 bytes will be
> who-knows-what (something from the stack).
>
> We want to show the values of the four members of the struct
> aer_header_log_regs; that can be done without ugly and error-prone
> casts. On little-endian this should produce the same output as
> originally intended, and since no-one has complained about getting
> garbage output so far, I think big-endian should be ok too.
>
> Fixes: fab4c256a58b ("PCI/AER: Add a TLP header print helper")
> Signed-off-by: Rasmus Villemoes <[email protected]>
Cc: <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--