| Message ID | 1246912380-28786-2-git-send-email-turkal@google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hello! > #elif defined(PCI_HAVE_STDINT_H) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) > -#include <stdint.h> > +#include <inttypes.h> Even though it is probably OK to assume that <inttypes.h> exists whenever <stdint.h> does, keeping using PCI_HAVE_STDINT_H for that is confusing :) > #else > +#include <inttypes.h> > typedef u_int8_t u8; > typedef u_int16_t u16; > typedef u_int32_t u32; > +typedef u_int64_t u64; As I already said, we cannot rely on <inttypes.h> on non-C99 systems. Also, we have to define the printf sequences ourselves. > #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" No, this is wrong. Since pciaddr_t is no longer u64, we must not use u64 printf sequences for it. > - 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 +121,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; This is also wrong. You have changed x,y to u64, but when you read a smaller value to them, you try to print them with a non-u64 format string. Have a nice fortnight
On Mon, Jul 6, 2009 at 14:42, Martin Mares<mj@ucw.cz> wrote: >>  #elif defined(PCI_HAVE_STDINT_H) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) >> -#include <stdint.h> >> +#include <inttypes.h> > > Even though it is probably OK to assume that <inttypes.h> exists whenever > <stdint.h> does, keeping using PCI_HAVE_STDINT_H for that is confusing :) Are you suggesting just removing the defined(PCI_HAVE_STDINT_H) part? Also, I looked at the configure script and it looks like PCI_HAVE_STDINT_H is only defined on sunos. That doesn't make much since to me. There are only two references to that symbol in all the source code. One in the lib/configure script and one in types.h. Shall I just remove them both? >>  #else >> +#include <inttypes.h> >>  typedef u_int8_t u8; >>  typedef u_int16_t u16; >>  typedef u_int32_t u32; >> +typedef u_int64_t u64; > > As I already said, we cannot rely on <inttypes.h> on non-C99 systems. For non-MS Windows systems, what systems do we support that don't have those c99 headers? I found that AIX 4.x, which has been end of lifed for a while, doesn't have the headers, but AIX 5.3 does. Do we really need to maintain support of systems as old as AIX 4.x? Could we take a more pragmatic approach and introduce a change that may break the platform if I promise to work with a user to get it building on AIX 4 if they really need it. I doubt that and AIX 4 user would need new pciutils anyhow. Also, do we have a list of supported platforms somewhere? > Also, we have to define the printf sequences ourselves. True. I hadn't solved that one yet. :( >>  #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" > > No, this is wrong. Since pciaddr_t is no longer u64, we must not use u64 > printf sequences for it. This is reasonable. >> -  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 +121,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; > > This is also wrong. You have changed x,y to u64, but when you read a smaller > value to them, you try to print them with a non-u64 format string. I don't understand this comment fully. Previously, these values were u32. Didn't they get printed the same way, that is using the non-u32 format strings? Thanks, 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
Hello! > > Even though it is probably OK to assume that <inttypes.h> exists whenever > > <stdint.h> does, keeping using PCI_HAVE_STDINT_H for that is confusing :) > > Are you suggesting just removing the defined(PCI_HAVE_STDINT_H) part? I suggest either removing the test altogether (if we come to a conclusion that we can rely on C99 includes everywhere), or renaming the symbol to PCI_HAVE_INTTYPES_H. > Also, I looked at the configure script and it looks like > PCI_HAVE_STDINT_H is only defined on sunos. That doesn't make much > since to me. AFAIK old versions of Solaris had some C99 includes, but not a C99 compiler, so the STDC_VERSION test did not trigger. > For non-MS Windows systems, what systems do we support that don't have > those c99 headers? As I have already said, I am not sure about that as I have never seen some of the supported systems myself. Current versions of all systems except Windows should be safe, but older versions frequently lack them. > I found that AIX 4.x, which has been end of lifed for a while, doesn't > have the headers, but AIX 5.3 does. Do we really need to maintain > support of systems as old as AIX 4.x? Judging from the occasional bug reports I receive, people still use such systems, so as long as supporting the old systems is a matter of a couple of lines, I would really like not to break them. I would therefore keep the fallback to u_intXX_t types, but instead of u_int64_t (for which we do not know the format sequence) use either long or long long, depending on ULONG_MAX. > Also, do we have a list of supported platforms somewhere? They are listed in README, but only names of the platforms, not specific versions. > I don't understand this comment fully. Previously, these values were > u32. Didn't they get printed the same way, that is using the non-u32 > format strings? In exec_op(), we call: printf(formats[width]+1, x); (and similarly trace()). I.e., we are printing the same variable `x', but with different format strings. As long as the format strings were "%<something>x", it worked, because all those required `unsigned int' as an argument. If you change `x' to u64, you have to change _all_ format strings used on it. Have a nice fortnight
Warren Turkal wrote: > diff --git a/lib/types.h b/lib/types.h > index 3e0e5c3..9bdd82a 100644 > --- a/lib/types.h > +++ b/lib/types.h > @@ -15,39 +15,50 @@ > typedef BYTE u8; > typedef WORD u16; > typedef DWORD u32; > +typedef I64u u64; > + > +#ifdef PCI_HAVE_64BIT_ADDRESS > +#include <limits.h> > +#if ULONG_MAX > 0xffffffff > +#define PRIu64 "l" > +#else > +#define PRIu64 "ll" > +#endif > +#endif > + Ehm, no, the other way round: typedef unsigned __int64 u64; #define PRIu64 I64u Greetings, Eike
On Tue, Jul 7, 2009 at 04:02, Martin Mares<mj@ucw.cz> wrote: >> > Even though it is probably OK to assume that <inttypes.h> exists whenever >> > <stdint.h> does, keeping using PCI_HAVE_STDINT_H for that is confusing :) >> >> Are you suggesting just removing the defined(PCI_HAVE_STDINT_H) part? > > I suggest either removing the test altogether (if we come to a conclusion > that we can rely on C99 includes everywhere), or renaming the symbol > to PCI_HAVE_INTTYPES_H. From my research, inttypes.h and stdint.h has existed since the listed timeframe for each of the following systems. I looked for evidence in public source repositories and on mailing lists of folks trying to compile software. Linux GNU/kFreeBSD GNU Hurd -These 3 are dependant on GNU libc which has had the files since the late '90s. FreeBSD - since at least 5.x, which is long out of even legacy releases and was released in 2003. NetBSD - since 2001 or so at least OpenBSD - since at least 2003 AIX - at least since version 5.1, which was released in 2001 and end of lifed in 2006. 4.3.3, which didn't have the files, was EOLed in 2003. Windows - The best one can do is a third party implementation. CYGWIN - don't know Solaris (since Solaris 10 at least) - There is evidence that Solaris 8 or 9 may not have had inttypes.h, but I think it's just that you had to get the sun's compilers (or another c library) to get the header file. Having said all this, I suggest we remove the test altogether, which is what I have sitting in my take 7 version of the patch. I also suggest that we drop the non-C99 types (like u_int32_t), as I suspect that the uint32_t style types will probably work pretty far back considering the above observations. I think that we can safely say that we are covering at least 6 years worth of development environments (back to 2003), if not more. Here is my proposal, for that section in the types.h, I don't think that we need the test for C99. It should probably look more like the following: #ifndef PCI_HAVE_Uxx_TYPES #ifdef PCI_OS_WINDOWS #include <windef.h> typedef BYTE u8; typedef WORD u16; typedef DWORD u32; typedef unsigned __int64 u64; //define printf format strings for pci address for windows here #else #include <inttypes.h> typedef uint8_t u8; typedef uint16_t u16; typedef uint32_t u32; typedef uint64_t u64; //define printf format strings for everyone else here #endif #endif>·/* PCI_HAVE_Uxx_TYPES */ I think this will work on all the above systems since 2003 at least. >> Also, I looked at the configure script and it looks like >> PCI_HAVE_STDINT_H is only defined on sunos. That doesn't make much >> since to me. > > AFAIK old versions of Solaris had some C99 includes, but not a C99 compiler, > so the STDC_VERSION test did not trigger. What would you think about trying to remove that STDC_VERSION test given the above info? >> For non-MS Windows systems, what systems do we support that don't have >> those c99 headers? > > As I have already said, I am not sure about that as I have never seen > some of the supported systems myself. Current versions of all systems > except Windows should be safe, but older versions frequently lack them. Windows can be worked around. As for the other system, does the info above make you feel any more confident? >> I found that AIX 4.x, which has been end of lifed for a while, doesn't >> have the headers, but AIX 5.3 does. Do we really need to maintain >> support of systems as old as AIX 4.x? > > Judging from the occasional bug reports I receive, people still use such > systems, so as long as supporting the old systems is a matter of a couple of > lines, I would really like not to break them. > > I would therefore keep the fallback to u_intXX_t types, but instead of u_int64_t > (for which we do not know the format sequence) use either long or long long, > depending on ULONG_MAX. Would you be willing to remove the u_intXX_t style types given the above info? >> Also, do we have a list of supported platforms somewhere? > > They are listed in README, but only names of the platforms, not specific versions. Thanks for the pointer. That's where I got the list of platforms to check for which versions had inttypes.h and stdint.h. >> I don't understand this comment fully. Previously, these values were >> u32. Didn't they get printed the same way, that is using the non-u32 >> format strings? > > In exec_op(), we call: > >     printf(formats[width]+1, x); > > (and similarly trace()). I.e., we are printing the same variable `x', but > with different format strings. As long as the format strings were "%<something>x", > it worked, because all those required `unsigned int' as an argument. If you > change `x' to u64, you have to change _all_ format strings used on it. I see. Thanks for the explanation. 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
Thanks, Updated for take 7. wt On Tue, Jul 7, 2009 at 04:06, Rolf Eike Beer<eike-kernel@sf-tec.de> wrote: > Warren Turkal wrote: > >> diff --git a/lib/types.h b/lib/types.h >> index 3e0e5c3..9bdd82a 100644 >> --- a/lib/types.h >> +++ b/lib/types.h >> @@ -15,39 +15,50 @@ >> Â typedef BYTE u8; >> Â typedef WORD u16; >> Â typedef DWORD u32; >> +typedef I64u u64; >> + >> +#ifdef PCI_HAVE_64BIT_ADDRESS >> +#include <limits.h> >> +#if ULONG_MAX > 0xffffffff >> +#define PRIu64 "l" >> +#else >> +#define PRIu64 "ll" >> +#endif >> +#endif >> + > > Ehm, no, the other way round: > > typedef unsigned __int64 u64; > #define PRIu64 I64u > > Greetings, > > Eike > -- 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
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..9bdd82a 100644 --- a/lib/types.h +++ b/lib/types.h @@ -15,39 +15,50 @@ typedef BYTE u8; typedef WORD u16; typedef DWORD u32; +typedef I64u 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..ebc89bc 100644 --- a/setpci.c +++ b/setpci.c @@ -73,9 +73,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 +121,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 +141,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 +164,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 +610,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..f4ba7d4 100644 --- a/setpci.man +++ b/setpci.man @@ -149,9 +149,11 @@ 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. At the time of writing, +8 byte wide read/writes are not atomic, and are executed as two 4 byte reads/writes. +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 | 18 +++++++++++++++--- setpci.man | 8 +++++--- 6 files changed, 78 insertions(+), 18 deletions(-)