| Message ID | 1246905824-12695-2-git-send-email-turkal@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hello! > * Hopefully fixed ABI issue with u64. Please scrutinize. > - I am wondering if we shouldn't just take the ABI hit and just change > pciaddr_t to be u64 for simplicity. I can't imagine that there are that many > apps that rely on the library that can't be recompiled. I would really like to keep the ABI of the shared library compatible as long as possible -- both having to recompile everything and keeping multiple versions of the same library on a single system are very annoying. > diff --git a/lib/types.h b/lib/types.h > index 3e0e5c3..818872d 100644 > --- a/lib/types.h > +++ b/lib/types.h > @@ -15,39 +15,50 @@ > typedef BYTE u8; > typedef WORD u16; > typedef DWORD u32; > +typedef unsigned __int64 u64; > + > +#ifdef PCI_HAVE_64BIT_ADDRESS > +#include <limits.h> > +#if ULONG_MAX > 0xffffffff > +#define PRIu64 "l" > +#else > +#define PRIu64 "ll" > +#endif > +#endif Does this make sense? Does anyone promise what format sequence should be used with __int64? > #elif defined(PCI_HAVE_STDINT_H) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) > -#include <stdint.h> > +#include <inttypes.h> Is there any guarantee that <inttypes.h> is available? Isn't it C99 only? > #ifdef PCI_HAVE_64BIT_ADDRESS > #include <limits.h> > #if ULONG_MAX > 0xffffffff > -typedef unsigned long u64; > -#define PCI_U64_FMT "l" > +typedef unsigned long pciaddr_t; > #else > -typedef unsigned long long u64; > -#define PCI_U64_FMT "ll" > -#endif > +typedef unsigned long long pciaddr_t; > #endif > > -#endif /* PCI_HAVE_Uxx_TYPES */ > +#define PCIADDR_T_FMT "%08" PRIu64 "x" > +#define PCIADDR_PORT_FMT "%04" PRIu64 "x" You cannot do this. `unsigned long long' and PRIu64 do not mix. > --- a/setpci.c > +++ b/setpci.c > @@ -12,6 +12,7 @@ > #include <stdarg.h> > #include <unistd.h> > #include <errno.h> > +#include <inttypes.h> Again, you cannot rely on C99. > -Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, or \fB.L\fP to choose > -how many bytes (1, 2, or 4) should be transferred. The width can be omitted if you are > -accessing a named register whose width is well known. > +Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, \fB.L\fP, or \fB.Q\fP > +to choose how many bytes (1, 2, 4, or 8) should be transferred. The width can be > +omitted if you are accessing a named register whose width is well known. Please emphasize that the 8-byte accesses are not atomic, unlike their shorter counterparts. Have a nice fortnight
On Mon, Jul 06, 2009 at 09:08:21PM +0200, Martin Mares wrote: > > +Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, \fB.L\fP, or \fB.Q\fP > > +to choose how many bytes (1, 2, 4, or 8) should be transferred. The width can be > > +omitted if you are accessing a named register whose width is well known. > > Please emphasize that the 8-byte accesses are not atomic, unlike their shorter > counterparts. Do you want us to start working on an 8-byte atomic access method inside the kernel? As I said before, it won't be available on all systems. Or should we stand firm against this bad habit of putting a 64-bit register in config space?
Martin Mares wrote: > > diff --git a/lib/types.h b/lib/types.h > > index 3e0e5c3..818872d 100644 > > --- a/lib/types.h > > +++ b/lib/types.h > > @@ -15,39 +15,50 @@ > > typedef BYTE u8; > > typedef WORD u16; > > typedef DWORD u32; > > +typedef unsigned __int64 u64; > > + > > +#ifdef PCI_HAVE_64BIT_ADDRESS > > +#include <limits.h> > > +#if ULONG_MAX > 0xffffffff > > +#define PRIu64 "l" > > +#else > > +#define PRIu64 "ll" > > +#endif > > +#endif > > Does this make sense? Does anyone promise what format sequence should be > used with __int64? No, it doesn't. "I64u" is the correct one. http://code.google.com/p/msinttypes/ is a good point to start with that MSVC mess. > > #elif defined(PCI_HAVE_STDINT_H) || (defined(__STDC_VERSION__) && > > __STDC_VERSION__ >= 199901L) -#include <stdint.h> > > +#include <inttypes.h> > > Is there any guarantee that <inttypes.h> is available? Isn't it C99 only? It is. And MSVC is my favorite example of a compiler that still doesn't support it 10 years later. Greetings, Eike
On Mon, Jul 6, 2009 at 12:22, Matthew Wilcox<matthew@wil.cx> wrote: > Do you want us to start working on an 8-byte atomic access method inside > the kernel? Â As I said before, it won't be available on all systems. > > Or should we stand firm against this bad habit of putting a 64-bit > register in config space? I'm not sure there is much we can do about it. At least Intel already has hardware with 64-bit registers in config space on the market. I imagine there will be others. Having said that, my current case doesn't require it, but I am sure it would be nice. wt -- 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 Mon, Jul 6, 2009 at 12:33, Rolf Eike Beer<eike-kernel@sf-tec.de> wrote: > No, it doesn't. "I64u" is the correct one. > http://code.google.com/p/msinttypes/ is a good point to start with that MSVC > mess. Should we just include the headers from that project? They are under the BSD license, so I don't think it'd be a problem. wt -- 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 Mon, Jul 6, 2009 at 12:08, Martin Mares<mj@ucw.cz> wrote: ... > Does this make sense? Does anyone promise what format sequence should be used > with __int64? Fixed with Rolf's suggested I64u for take 6. It looks like: typedef I64u u64; >>  #elif defined(PCI_HAVE_STDINT_H) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) >> -#include <stdint.h> >> +#include <inttypes.h> > > Is there any guarantee that <inttypes.h> is available? Isn't it C99 only? Doesn't __STDC_VERSION__ >= 199901L indicate C99+? That's why I put it within this block. In fact stdint.h, is also a c99 header, so I think this instance of inttypes.h is ok. ...but there is another just below. Please forgive me for assuming if I am wrong, but were you thinking of the spot a few lines down under #else of that same if block, where I also included inttypes.h? If that's the case, I have a question. Are we actually supporting platforms, other than MS Windows, that don't include the stdint.h and inttypes.h headers from C99? For instance, this works out of the box on GCC. It also works with GCC if I add the -c99 flag. I am basically wondering inttypes.h just exists on all the platforms we support, other than MS Windows. For the MS Windows case, could we include the headers from the project that Rolf mentioned at [1]? >>  #ifdef PCI_HAVE_64BIT_ADDRESS >>  #include <limits.h> >>  #if ULONG_MAX > 0xffffffff >> -typedef unsigned long u64; >> -#define PCI_U64_FMT "l" >> +typedef unsigned long pciaddr_t; >>  #else >> -typedef unsigned long long u64; >> -#define PCI_U64_FMT "ll" >> -#endif >> +typedef unsigned long long pciaddr_t; >>  #endif >> >> -#endif    /* PCI_HAVE_Uxx_TYPES */ >> +#define PCIADDR_T_FMT "%08" PRIu64 "x" >> +#define PCIADDR_PORT_FMT "%04" PRIu64 "x" > > You cannot do this. `unsigned long long' and PRIu64 do not mix. Ok. I'll take another shot at this. I'm really trying, thanks for being patient. :) >> --- a/setpci.c >> +++ b/setpci.c >> @@ -12,6 +12,7 @@ >>  #include <stdarg.h> >>  #include <unistd.h> >>  #include <errno.h> >> +#include <inttypes.h> > > Again, you cannot rely on C99. This file ultimately includes lib/types.h, so it wasn't needed anyway. Done for take 6. >> -Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, or \fB.L\fP to choose >> -how many bytes (1, 2, or 4) should be transferred. The width can be omitted if you are >> -accessing a named register whose width is well known. >> +Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, \fB.L\fP, or \fB.Q\fP >> +to choose how many bytes (1, 2, 4, or 8) should be transferred. The width can be >> +omitted if you are accessing a named register whose width is well known. > > Please emphasize that the 8-byte accesses are not atomic, unlike their shorter > counterparts. Done for take 6. BTW, I am about to post take 6, so please post comments in that part of the thread. [1]http://code.google.com/p/msinttypes/ wt -- 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 Mon, Jul 06, 2009 at 12:53:54PM -0700, Warren Turkal wrote: > On Mon, Jul 6, 2009 at 12:22, Matthew Wilcox<matthew@wil.cx> wrote: > > Do you want us to start working on an 8-byte atomic access method inside > > the kernel? ??As I said before, it won't be available on all systems. > > > > Or should we stand firm against this bad habit of putting a 64-bit > > register in config space? > > I'm not sure there is much we can do about it. At least Intel already > has hardware with 64-bit registers in config space on the market. I > imagine there will be others. Interesting! Which device, out of interest? > Having said that, my current case doesn't require it, but I am sure it > would be nice. Actually, we're safe, it can't be done. Refer to PCI Express 2.1, section 2.2.7 where it mandates that the data part of a config packet must contain exactly one DWORD. Maybe they'll change that for PCIe 3, but there's no need for a major firedrill yet.
Hello! > ...but there is another just below. Please forgive me for assuming if > I am wrong, but were you thinking of the spot a few lines down under > #else of that same if block, where I also included inttypes.h? If > that's the case, I have a question. Are we actually supporting > platforms, other than MS Windows, that don't include the stdint.h and > inttypes.h headers from C99? That's what I would like to know, too... :-) I unfortunately don't have access to some of the more obscure systems on which libpci runs, including for example AIX. I do not think that we can rely on any C99 headers there. Changing <stdint.h> to <inttypes.h> on C99 systems should be OK. > For the MS Windows case, could we include the headers from the project > that Rolf mentioned at [1]? Isn't it easier to copy the few definitions than importing whole files? Have a nice fortnight
diff --git a/lib/access.c b/lib/access.c index 691df39..b8addea 100644 --- a/lib/access.c +++ b/lib/access.c @@ -98,6 +98,14 @@ pci_read_long(struct pci_dev *d, int pos) return le32_to_cpu(buf); } +u64 +pci_read_quad(struct pci_dev *d, int pos) +{ + u64 buf; + pci_read_data(d, &buf, pos, 8); + return le64_to_cpu(buf); +} + int pci_read_block(struct pci_dev *d, int pos, byte *buf, int len) { @@ -141,6 +149,13 @@ pci_write_long(struct pci_dev *d, int pos, u32 data) } int +pci_write_quad(struct pci_dev *d, int pos, u64 data) +{ + u64 buf = cpu_to_le64(data); + return pci_write_data(d, &buf, pos, 8); +} + +int pci_write_block(struct pci_dev *d, int pos, byte *buf, int len) { if (pos < d->cache_len) diff --git a/lib/pci.h b/lib/pci.h index 7a5a6b8..71d673f 100644 --- a/lib/pci.h +++ b/lib/pci.h @@ -147,11 +147,13 @@ struct pci_dev { u8 pci_read_byte(struct pci_dev *, int pos) PCI_ABI; /* Access to configuration space */ u16 pci_read_word(struct pci_dev *, int pos) PCI_ABI; u32 pci_read_long(struct pci_dev *, int pos) PCI_ABI; +u64 pci_read_quad(struct pci_dev *, int pos) PCI_ABI; int pci_read_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI; int pci_read_vpd(struct pci_dev *d, int pos, u8 *buf, int len) PCI_ABI; int pci_write_byte(struct pci_dev *, int pos, u8 data) PCI_ABI; int pci_write_word(struct pci_dev *, int pos, u16 data) PCI_ABI; int pci_write_long(struct pci_dev *, int pos, u32 data) PCI_ABI; +int pci_write_quad(struct pci_dev *, int pos, u64 data) PCI_ABI; int pci_write_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI; int pci_fill_info(struct pci_dev *, int flags) PCI_ABI; /* Fill in device information */ diff --git a/lib/sysdep.h b/lib/sysdep.h index 2a25c93..4701884 100644 --- a/lib/sysdep.h +++ b/lib/sysdep.h @@ -27,8 +27,10 @@ typedef u16 word; #include <asm/byteorder.h> #define cpu_to_le16 __cpu_to_le16 #define cpu_to_le32 __cpu_to_le32 +#define cpu_to_le64 __cpu_to_le64 #define le16_to_cpu __le16_to_cpu #define le32_to_cpu __le32_to_cpu +#define le64_to_cpu __le64_to_cpu #else @@ -63,8 +65,10 @@ typedef u16 word; #if BYTE_ORDER == BIG_ENDIAN #define cpu_to_le16 swab16 #define cpu_to_le32 swab32 +#define cpu_to_le64 swab64 #define le16_to_cpu swab16 #define le32_to_cpu swab32 +#define le64_to_cpu swab64 static inline word swab16(word w) { @@ -78,11 +82,25 @@ static inline u32 swab32(u32 w) ((w & 0x0000ff00) << 8) | ((w & 0x000000ff) << 24); } + +static inline u64 swab64(u64 w) +{ + return ((w & 0xff00000000000000) >> 56) | + ((w & 0x00ff000000000000) >> 40) | + ((w & 0x0000ff0000000000) >> 24) | + ((w & 0x000000ff00000000) >> 8) | + ((w & 0x00000000ff000000) << 8) | + ((w & 0x0000000000ff0000) << 24) | + ((w & 0x000000000000ff00) << 40) | + ((w & 0x00000000000000ff) << 56); +} #else #define cpu_to_le16(x) (x) #define cpu_to_le32(x) (x) +#define cpu_to_le64(x) (x) #define le16_to_cpu(x) (x) #define le32_to_cpu(x) (x) +#define le64_to_cpu(x) (x) #endif #endif diff --git a/lib/types.h b/lib/types.h index 3e0e5c3..818872d 100644 --- a/lib/types.h +++ b/lib/types.h @@ -15,39 +15,50 @@ typedef BYTE u8; typedef WORD u16; typedef DWORD u32; +typedef unsigned __int64 u64; + +#ifdef PCI_HAVE_64BIT_ADDRESS +#include <limits.h> +#if ULONG_MAX > 0xffffffff +#define PRIu64 "l" +#else +#define PRIu64 "ll" +#endif +#endif + #elif defined(PCI_HAVE_STDINT_H) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) -#include <stdint.h> +#include <inttypes.h> typedef uint8_t u8; typedef uint16_t u16; typedef uint32_t u32; +typedef uint64_t u64; #else +#include <inttypes.h> typedef u_int8_t u8; typedef u_int16_t u16; typedef u_int32_t u32; +typedef u_int64_t u64; #endif +#endif /* PCI_HAVE_Uxx_TYPES */ + #ifdef PCI_HAVE_64BIT_ADDRESS #include <limits.h> #if ULONG_MAX > 0xffffffff -typedef unsigned long u64; -#define PCI_U64_FMT "l" +typedef unsigned long pciaddr_t; #else -typedef unsigned long long u64; -#define PCI_U64_FMT "ll" -#endif +typedef unsigned long long pciaddr_t; #endif -#endif /* PCI_HAVE_Uxx_TYPES */ +#define PCIADDR_T_FMT "%08" PRIu64 "x" +#define PCIADDR_PORT_FMT "%04" PRIu64 "x" -#ifdef PCI_HAVE_64BIT_ADDRESS -typedef u64 pciaddr_t; -#define PCIADDR_T_FMT "%08" PCI_U64_FMT "x" -#define PCIADDR_PORT_FMT "%04" PCI_U64_FMT "x" #else typedef u32 pciaddr_t; #define PCIADDR_T_FMT "%08x" #define PCIADDR_PORT_FMT "%04x" -#endif + +#endif /* PCI_HAVE_64BIT_ADDRESS */ #ifdef PCI_ARCH_SPARC64 /* On sparc64 Linux the kernel reports remapped port addresses and IRQ numbers */ diff --git a/setpci.c b/setpci.c index 97740e8..30195b0 100644 --- a/setpci.c +++ b/setpci.c @@ -12,6 +12,7 @@ #include <stdarg.h> #include <unistd.h> #include <errno.h> +#include <inttypes.h> #define PCIUTILS_SETPCI #include "pciutils.h" @@ -73,9 +74,10 @@ trace(const char *fmt, ...) static void exec_op(struct op *op, struct pci_dev *dev) { - const char * const formats[] = { NULL, " %02x", " %04x", NULL, " %08x" }; - const char * const mask_formats[] = { NULL, " %02x->(%02x:%02x)->%02x", " %04x->(%04x:%04x)->%04x", NULL, " %08x->(%08x:%08x)->%08x" }; - unsigned int i, x, y; + const char * const formats[] = { NULL, " %02x", " %04x", NULL, " %08x", NULL, NULL, NULL, " %016" PRIu64 "x"}; + const char * const mask_formats[] = { NULL, " %02x->(%02x:%02x)->%02x", " %04x->(%04x:%04x)->%04x", NULL, " %08x->(%08x:%08x)->%08x", NULL, NULL, NULL, " %016" PRIu64 "x->(%016" PRIu64 "x:%016" PRIu64 "x)->%016" PRIu64 "x"}; + unsigned int i; + u64 x, y; int addr = 0; int width = op->width; char slot[16]; @@ -120,6 +122,9 @@ exec_op(struct op *op, struct pci_dev *dev) case 2: y = pci_read_word(dev, addr); break; + case 8: + y = pci_read_quad(dev, addr); + break; default: y = pci_read_long(dev, addr); break; @@ -137,6 +142,9 @@ exec_op(struct op *op, struct pci_dev *dev) case 2: pci_write_word(dev, addr, x); break; + case 8: + pci_write_quad(dev, addr, x); + break; default: pci_write_long(dev, addr, x); break; @@ -157,6 +165,9 @@ exec_op(struct op *op, struct pci_dev *dev) case 2: x = pci_read_word(dev, addr); break; + case 8: + x = pci_read_quad(dev, addr); + break; default: x = pci_read_long(dev, addr); break; @@ -600,6 +611,8 @@ static void parse_op(char *c, struct pci_dev **selected_devices) op->width = 2; break; case 'L': op->width = 4; break; + case 'Q': + op->width = 8; break; default: parse_err("Invalid width \"%c\"", *width); } diff --git a/setpci.man b/setpci.man index 6c1ad8c..21ecf35 100644 --- a/setpci.man +++ b/setpci.man @@ -149,9 +149,9 @@ Each of the previous formats can be followed by \fB+offset\fP to add an offset (a hex number) to the address. This feature can be useful for addressing of registers living within a capability, or to modify parts of standard registers. \IP \(bu -Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, or \fB.L\fP to choose -how many bytes (1, 2, or 4) should be transferred. The width can be omitted if you are -accessing a named register whose width is well known. +Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, \fB.L\fP, or \fB.Q\fP +to choose how many bytes (1, 2, 4, or 8) should be transferred. The width can be +omitted if you are accessing a named register whose width is well known. .PP All names of registers and width specifiers are case-insensitive.
Signed-off-by: Warren Turkal <turkal@google.com> --- lib/access.c | 15 +++++++++++++++ lib/pci.h | 2 ++ lib/sysdep.h | 18 ++++++++++++++++++ lib/types.h | 35 +++++++++++++++++++++++------------ setpci.c | 19 ++++++++++++++++--- setpci.man | 6 +++--- 6 files changed, 77 insertions(+), 18 deletions(-)