Hi all, Thanks for comments. Jeff, I specially appreciate time you spent walking through code. I am not subscribed to lkml, thus please CC me (Vladimir Kondratiev ) in replies. To keep posting short, I skipped most of original mail. I attached fixed version for the patch. Sure, I tested it prior to posting. > > The patch could use some cleaning up, for coding style. The general > style is documented in CodingStyle file in the kernel source tree. > But also, you should consider the "golden rule of style" for > programmers: When modifying a file, follow the same formatting as is > found in the rest of the file. > Agree. It was my fault to not do it from the beginning. I habited to trust Emacs. Fixed. >> +/** >> + * Virtual address for RRBAR >> + */ >> +static void* rrbar_virt=NULL; > > > Do not bother initializing static variables to zero. This just wastes > bss space, since these variables are automatically zeroed for you, > anyway. I did not found this feature in standard. More, future versions of gcc will give at least warning, if not error, like "use of uninitialized variable". Many good sources also say it is good practice to initialize all variables. I rely on its value later. I' ll keep it as is unless really strong arguments provided. >> +/** >> + * It used to be #define, but I am going to change it. >> + */ >> +extern int PCI_CFG_SPACE_SIZE; > > > Hopefully this is temporary... variables should not be ALL CAPS. If > you change it to a variable, also change its case. Agree. Fixed. >> +/** >> + * Initializes PCI Express method for config space access. >> + * + * There is no standard method to recognize presence of PCI >> Express, >> + * thus we will assume it is PCI-E, and rely on sanity check to >> + * deassert PCI-E presense. If PCI-E not present, >> + * there is no physical RAM on RRBAR address, and we should read >> + * something like 0xff. >> + * + * Creates mapping for whole 256M area. >> + * + * @return 1 if OK, 0 if error >> + */ >> +static int pci_express_init(void) >> +{ >> + /* TODO: check PCI-Ex presense */ > > > Um, this sounds critical. We should not merge this patch until this > 'TODO' is complete, in my opinion. As I explained in comment to function, it is not really critical. The problem is, there is no generic way (or I don't know it) to recognize PCI-E. One suggest to go over all devices and see whether PCI-E capability present for at least one of them. I don't think it is good way to do. Sanity check do pretty good job here. If it is not PCI-E platform, this address in physical memory will not be connected to anything real. You will get 0xff's. >> +static int pci_exp_read (int seg, int bus, int dev, int fn, int reg, >> int len, u32 *value) > > > I think these arguments should be 'unsigned int', not 'int'. > > Let's be proactive, and protect against signed/unsigned problems now, > before they appear. Sound reasonable. But this is how this function prototyped: int (*pci_config_read)(int seg, int bus, int dev, int fn, int reg, int len, u32 *value) = NULL; int (*pci_config_write)(int seg, int bus, int dev, int fn, int reg, int len, u32 value) = NULL; I think it is better to ask author (Martin) of this code, may be he had some reason for this prototype? I added him to CC list. >> + void* addr=rrbar_virt+(bus << 20)+(dev << 15)+(fn << 12)+reg; > > > Why do you prefer '+' to '|' here? > > '|' normally has less side effects. > Agree. Fixed, except 1-st "+", since virtual addres may be not aligned on 256M boundary > Further, PCI posting: a writeb() / writew() / writel() will not be > flushed immediately to the processor. The CPU and/or PCI bridge may > post (delay/combine) such writes. I do not think this is a desireable > effect, for PCI config register accesses. > Good point. Fixed. > As with other code, define a macro that creates these functions. Then > add a series of lines such as > > DEF_PCIEX_READ(word, 2) > DEF_PCIEX_READ(dword, 4) > ... > 2 'read' functions (one do not follow pattern) and 3 'write' functions do not worth 2 macros. Also, coding style for the rest of file is different. Other methods defined w/o macros. >+ if (rrbar_virt) { <====== Not trusting own code >> + iounmap(rrbar_virt); >> + } > > > >All in all, raw. Also, a healthy dose of Itanium is prescribed. > > > Extra check is here for the case code will be modified. In not frequently executed code, it is better to check twice rather then forget to check. >It should go into 2.6 first. though. And while you're at it add your >copyright info to the top of the file instead of the middle. > Copyright - done, thanks for input. Regarding 2.6 - you are right, but now I need it for 2.4, and I have no 2.6 handy. I will do it in a week or 2. Vladimir.