| Message ID | 1424818246-11841-1-git-send-email-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 <linux@rasmusvillemoes.dk> > --- > 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?
[+cc Tony] On Wed, Feb 25, 2015 at 10:54 AM, Borislav Petkov <bp@suse.de> 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 <linux@rasmusvillemoes.dk> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiBJIHRoaW5rIHdlIHNob3VsZCBleHBlY3QgQUVSIHRvIGJlIHVzZWQgb24gYmlnLWVuZGlhbiBt YWNoaW5lcy4gIEknbQ0KPiBwcmV0dHkgc3VyZSBpdCdzIHVzZWQgb24gSXRhbml1bSBpbiBiaWct ZW5kaWFuIG1vZGUuDQoNCkl0YW5pdW0gY2FuIHJ1biBpbiBlaXRoZXIgYmlnIG9yIGxpdHRsZSBl bmRpYW4gbW9kZSAtIGJ1dCBMaW51eCB1c2VzIGxpdHRsZSBlbmRpYW46DQoNCmFyY2gvaWE2NC9p bmNsdWRlL3VhcGkvYXNtL2J5dGVvcmRlci5oOiNpbmNsdWRlIDxsaW51eC9ieXRlb3JkZXIvbGl0 dGxlX2VuZGlhbi5oPg0KDQotVG9ueQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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,
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 <linux@rasmusvillemoes.dk> --- drivers/pci/pcie/aer/aerdrv_errprint.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)