login
Search
Here are a bunch of PCI patches against your 2.6.24 git tree. Some general cleanups, minor tweaks, and a bit of PCI hotplug updates, and some PCI Express updates for new features, if your hardware happens to support it. Please pull from: master.kernel.org:/pub/scm/linux/kernel/git/gregkh/pci-2.6.git/ All of these have been in the -mm releases for a while. The full patches will be sent to the linux-pci mailing list, if anyone wants to see them. thanks, greg k-h ------------- Documentation/pci.txt | 37 +-- arch/alpha/Kconfig | 5 - arch/arm/Kconfig | 5 - arch/frv/Kconfig | 5 - arch/m32r/Kconfig | 5 - arch/m68k/Kconfig | 5 - arch/mips/Kconfig | 5 - arch/sh/drivers/pci/Kconfig | 5 - arch/sparc64/Kconfig | 5 - arch/x86/Kconfig | 5 - arch/x86/kernel/quirks.c | 43 +- arch/x86/pci/fixup.c | 22 +- arch/xtensa/Kconfig | 5 - drivers/ata/pata_cs5520.c | 2 +- drivers/i2c/busses/scx200_acb.c | 2 +- drivers/ide/pci/cs5520.c | 10 +- drivers/ide/setup-pci.c | 6 +- drivers/pci/bus.c | 18 +- drivers/pci/dmar.c | 20 +- drivers/pci/hotplug/Kconfig | 4 +- drivers/pci/hotplug/Makefile | 4 +- drivers/pci/hotplug/acpiphp.h | 1 - drivers/pci/hotplug/acpiphp_glue.c | 5 +- drivers/pci/hotplug/fakephp.c | 39 ++- drivers/pci/hotplug/ibmphp_core.c | 11 +- drivers/pci/hotplug/pci_hotplug_core.c | 4 +- drivers/pci/hotplug/pciehp.h | 9 +- drivers/pci/hotplug/pciehp_core.c | 33 +- drivers/pci/hotplug/pciehp_ctrl.c | 27 +- drivers/pci/hotplug/pciehp_hpc.c | 314 ++++++++----- ...
On Fri, 1 Feb 2008 15:11:47 -0800 drivers/built-in.o: In function `pci_scan_slot': drivers/pci/probe.c:1016: undefined reference to `pcie_aspm_init_link_state' drivers/built-in.o: In function `pci_stop_dev': drivers/pci/remove.c:36: undefined reference to `pcie_aspm_exit_link_state' drivers/built-in.o: In function `pci_set_power_state': drivers/pci/pci.c:524: undefined reference to `pcie_aspm_pm_state_change' make: *** [.tmp_vmlinux1] Error 1 http://userweb.kernel.org/~akpm/config-vmm.txt Needs this, I guess: --- a/drivers/pci/pcie/Kconfig~fix-gregkh-pci-pci-pcie-aspm-support +++ a/drivers/pci/pcie/Kconfig @@ -32,7 +32,7 @@ source "drivers/pci/pcie/aer/Kconfig" # config PCIEASPM bool "PCI Express ASPM support(Experimental)" - depends on PCI && EXPERIMENTAL + depends on PCI && EXPERIMENTAL && PCIEPORTBUS default y help This enables PCI Express ASPM (Active State Power Management) and _ Also, that ASPM patch unnecessarily adds a pile of macros: +#ifdef CONFIG_PCIEASPM +extern void pcie_aspm_init_link_state(struct pci_dev *pdev); +extern void pcie_aspm_exit_link_state(struct pci_dev *pdev); +extern void pcie_aspm_pm_state_change(struct pci_dev *pdev); +extern void pci_disable_link_state(struct pci_dev *pdev, int state); +#else +#define pcie_aspm_init_link_state(pdev) do {} while (0) +#define pcie_aspm_exit_link_state(pdev) do {} while (0) +#define pcie_aspm_pm_state_change(pdev) do {} while (0) +#define pci_disable_link_state(pdev, state) do {} while (0) +#endif Please don't do this. A static inline function is cleaner and provides typechecking. It also provides an access to the caller's argument and can avoid unused-varaiable warnings. The only reason to use a macro in this situation is if the caller's argument is for some reason not defined if !CONFIG_PCIEASPM. Greg, please check for this in your reviewing - reject macros *by default*. They are inferior. --
Ick, missed that, again, my appologies. In cleaning up pci.h I saw a few places like this, I'll fix that next go-around also. thanks, greg k-h --
On Fri, 1 Feb 2008 16:49:16 -0800 Was "default y" appropriate? --
simple allyesconfig testing found a build failure due to last night's PCI merge, on 32-bit x86: drivers/scsi/lpfc/lpfc_init.c: In function 'lpfc_pci_probe_one': drivers/scsi/lpfc/lpfc_init.c:1897: error: implicit declaration of function 'pci_enable_device_bars' fix attached. ( This call has been introduced upstream 3 weeks ago by commit 8a4df120b07, but the PCI tree has apparently not been fully re-tested with Linus-latest since that point. ) Ingo --------------> Subject: pci: pci_enable_device_bars() fix From: Ingo Molnar <mingo@elte.hu> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- drivers/scsi/lpfc/lpfc_init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/drivers/scsi/lpfc/lpfc_init.c =================================================================== --- linux.orig/drivers/scsi/lpfc/lpfc_init.c +++ linux/drivers/scsi/lpfc/lpfc_init.c @@ -1894,7 +1894,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev, uint16_t iotag; int bars = pci_select_bars(pdev, IORESOURCE_MEM); - if (pci_enable_device_bars(pdev, bars)) + if (pci_enable_device_io(pdev)) goto out; if (pci_request_selected_regions(pdev, bars, LPFC_DRIVER_NAME)) goto out_disable_device;
Look at the line right above it... AFAICS you want pci_enable_device_mem(), if the mask is selecting IORESOURCE_MEM BARs. Also a CC to linux-scsi and the driver author would be nice, as they are the ones with hardware and can verify. This set of changes seemed like 50% guesswork to me, without consulting the authors :( And unlike many changes, you actually have to know the hardware [or get clues from surrounding code] to make the change. Jeff --
Jeff is right, this fix is wrong. The correct fix is here: http://marc.info/?l=linux-scsi&m=120196768605031 Ingo, could you please cc linux-scsi on your mails ... it would have been a complete disaster to have this incorrect patch go in. James --
it would have been totally appropriate for me to just send a mail to lkml with the proper subject line about the breakage. (I might even have decided to stay completely silent about the issue and fix it for my own build, letting you guys figure it out.) Instead i did a search of lkml (based on the function name in the build error) and figured out where the pull request was on lkml: Greg. I replied to that mail, he'll obviously know whom else to Cc from that point on (if anyone). I really didnt want to (nor did i need to) figure out whether this was some general driver level API change that happen kernel-wide, or some SCSI specific change. I simply replied to the pull request whose Cc: line seemed well-populated to me already. I also took a look at the commit itself and did a quick hack in a hurry to keep the tests rolling. It really did not occur to me that i should have added anyone else to the Cc: line, as linux-pci@atrey.karlin.mff.cuni.cz was Cc:-ed already so i assumed the interest was from that angle. ( And as this was spent from my family's weekend time and i had no time and no interest to dig any further than to figure out the "first hop" of the change that broke the build, and the parties who initiated that hop. I'm in fact surprised that your and James's answer to my bugreport is hostility. ) So i find your suggestion that i should have added more people to the you mean the whole set of changes? Or just mine? Mine was a 30 seconds guesswork of course, i dont have the hardware, i didnt do the change, nor did i do anything near that code - i just saw the build failure stop my testing and sent in this notification and a hack. That's why i sent it to Greg, as a reply to the mail where he pushed these changes upstream and who'll know what to do with it from that point on. My patch can be totally thrown away (and should be, apparently). but ... i guess next time i'll think twice before sending any bugreports about or related to the ...
Oh come on... You are smart enough to know to at least CC the driver maintainer, the key POC who should be aware of breakage of their driver. As I noted, it is an obvious courtesy to CC the driver maintainer, at the very least. _Especially_ when it is a change that requires some knowledge of the The fact is, each larger subsystem (net, scsi, ata I know) has several vendor contacts and driver maintainers who for various reasons prefer a more focused -- and often less hostile -- mailing list to LKML. I have a hard enough time as it is, trying to convince hardware vendors to work with us, that we are not all assholes. How about respecting the preferences of certain segments of a very large community, even when they differ from your own? We have a community-accepted method of expressing these preferences, the MAINTAINERS file. IMO, standard practice should be: * To or CC: driver maintainer mentioned in MAINTAINERS (if any) * CC: LKML, any list mentioned in MAINTAINERS So, how about CC'ing the targets that have nicely requested to be CC'd? Jeff --
is there any particular reason why you cut out the most relevant part of had you read this portion you'd have realized that i did not search for any "owner" of the file, i simply searched for the person who introduced the change, and the on-lkml mail where the change was introduced. and that's all that should be needed, really. Believe me, i hit tons of bugs all across the kernel, often several bugs a day, and it's hard even for me to figure out who "maintains" a file and when. (and in Linux there's no "ownership" of files anyway) So as a general rule i go after changes instead, and that's exactly what i did here too. I do delta/regression QA - i.e. i watch for _changes_ that break the kernel and hence the general 'owner' of a file is often irrelevant - it's the maintainer who introduces a change who matters, and we do lots of cross-maintain merges. Only if i do not manage to identify a change do i try to figure out who maintains a file at that given moment. (But those mails often go into black holes, they get bounced, subscriber-required email lists, etc. etc.) It's also nontrivial to map the files to the MAINTAINERS file, and it's also quite outdated in some portions. So the MAINTAINERS file is the last resort i use. so i'm still totally befuddled why you think that there was anything particularly wrong or unhelpful about me replying to the specific pull request that introduced a particular breakage into the kernel. Had i mailed to lkml with a terse "kernel build broke" message with just an URL to a config and the build breakage, you could rightfully have complained that i should have done more to properly direct my bugreport. But this breakage was about a PCI API change, the pull request had a PCI mailing list Cc:-ed, why should i have thought that this needs the attention of any other parties? Ingo --
And therein lies the problem. The original submittor omitted relevant maintainers, you followed that [incorrect] example, and the end result was clear: an obviously wrong change. Thus, the problem is precisely what you stated: you did not bother to Hardly. What part of "this change requires knowledge of the hardware" is difficult to understand? And if you do not have that knowledge, why is it so trying to CC people who actively maintain a driver, and have that knowledge? It's simple common sense to -ask- or at least -notify- in such cases. That the original submittor made the same mistake is no reason to repeat That's a long list of excuses in an attempt to ignore the facts: Fact 1: The driver you modified is actively maintained Fact 2: The driver maintainer has respectfully indicated, through the standard community mechanism, the useful points of contact. Fact 3: The MAINTAINERS entry is correct and up-to-date. Fact 4: Even if you wanted to ignore MAINTAINERS, 'git log' on the relevant file could have told you who are useful parties to CC. It's just common courtesy to CC a driver maintainer, ESPECIALLY when a Because the change required knowledge not only of PCI, but of the hardware in question. As your patch demonstrated. And yes -- the original changes should have been CC'd to interested parties as well. I'm still waiting to hear back from Alan or Bart whether the ATA/IDE changes in that PCI pile actually work... the original changeset even noted that relevant parties had not yet been queried. Jeff --
so please tell me Jeff. If Greg, who is the super-maintainer of your code area, and who deals with your code every day and changes it every minute and hour, simply did not Cc: the SCSI list - how am i, a largely outside party in this matter, supposed to notice that 3 maintainers and 3 mailing lists in the Cc: were somehow not enough and that i was supposed to grow the already sizable Cc: list even more? Ingo --
Because, regardless of the situation, it's both common courtesy and wise practice to CC relevant driver maintainers, when you touch a driver. And it's just common sense: Greg simply does not know the intimate details of every PCI driver. Nor do I. Nor you. In the case of lpfc here, we have an active driver maintainer, and an up-to-date MAINTAINERS entry. Even if you are too slack to read MAINTAINERS, 'git log' would have given you the same info. Don't pretend there is some benefit here to ignoring the people that best know the driver. I don't buy that; it simply makes no engineering sense whatsoever. Jeff --
what you _STILL_ do not realize is the following: you still attribute the lack of Cc:s to some intention of mine. No, it was not my intention. At first glance the Cc: looked large and complete enough in an _existing_ discussion and that's was the end of my (brief) attention regarding the Cc: line. Yes, it would have been a bit better had i noticed the lack of Cc:s in an existing discussion, but i didnt. [ And it might not surprise you if i observe here that i think this little mishap is further (incidental) proof that having this many mailing list aliases to get the 'guaranteed attention' of maintainers is just super fragile and does not serve users at all. Even Greg and i got it wrong accidentally. If _we_ get it wrong, who will get it right? I see several mis-Cc:ed emails every day and there's over a 1000 unfixed bugs in bugzilla for 2.6 alone. It does not take a genius to observe that something is fundamentally wrong ;-) That 2.6 works on your or my box is a _very_ poor metric - we both fix bugs on our systems super fast so we've got a very biased first-hand experience about the stability and reliability of the kernel. We never really feel the kind of frustration that testers feel when their mail to lkml gets ignored. We never really feel the helplessness that comes from unfixed bugs. ] Ingo --
Actually I (and probably others) generally avoid cc'ing mailing lists on whoa. You must know better mailing lists than I do ;) --
You pretty much always ensure the driver author gets CC'd, which is exemplary :) Jeff --
I was never speaking to intent. I was noting that, having been in the kernel community for years, both of you guys should know that you should always CC a driver author, when touching their driver. Even after this thread, I have not even heard a "yes, I agree, I should have CC'd the driver author since they know the most about the driver" from either of you, which is quite disappointing. Sure. But... do you agree the CC list should have included the driver author? Do you agree that a mistake was made in this case? Jeff --
We agreed to differ a while ago on your head in the sand, post it to LKML because everybody follows that list attitude. Some nice soul will always (eventually) forward to the correct list, so this practice more or less works (just not in as timely fashion as actually posting to the What's hostile about telling you your patch is wrong and pointing you at Oh good grief, don't come the raw prawn with us! You're a kernel maintainer, you are expected to know the rules and follow them. The very fact that a well known maintainer posts a patch with a signed-off-by to Andrew and Linus means that it likely gets applied. Because of this, maintainers and other people with similarly trusted positions are expected to go via the lists and subsystems in part as general courtesy, but also to verify that their patch is actually valid before it gets applied. Are you seriously telling us that it required too much investigation on your part to figure out that something with a compile failure in drivers/scsi might belong on the scsi list? Let me tell you exactly what would have happened if that patch had been applied: Because the wrong bars were enabled, the device would have attached but been non-functional. Likely Emulex would have picked this up in their -rc1 testing and spent several days trying to trace the cause in their labs. Chances are, that, because this type of breakage is extremely subtle, they wouldn't have noticed the fact that the enable should have been _mem not _io. Then they would have raised the issue to linux-scsi. It would probably take at least a person week of effort before anyone finally noticed what the issue was. If you can't see that your behaviour needs modifying, I suggest you seriously consider your position. For all our talk of a nice utopian democracy of meritocracy in Open Source, we're critically dependent on the tree gatekeepers to maintain this. None of us is immune from the subtle lure of the arrogance of power but it's a corrosive poison ...
This is getting silly. Let me repeat it, because IMO it's really straightforward. My (quick) investigation based on the function name that was in the error message: drivers/scsi/lpfc/lpfc_init.c:1897: error: implicit declaration of function 'pci_enable_device_bars' a straightforward search on "pci_enable_device_bars" led to a recent PCI API related change pushed by Greg, with the following straightforward subject line: [GIT PATCH] PCI patches for 2.6.24 http://lkml.org/lkml/2008/2/1/483 the email had this description: | Some general cleanups, minor tweaks, and a bit of PCI hotplug | updates, and some PCI Express updates for new features, if your | hardware happens to support it. furthermore, the mail already had two PCI mailing lists in its Cc: line. The PCI subsystem regularly does cross-treee changes, by its nature. i had all reasons to believe that this was a (innocious looking) PCI subsystem change and a harmless (but a tad under-tested) API cleanup that went haywire: it smelled like PCI, it walked like PCI and it quacked like PCI. So to me it was clearly a PCI merge not an SCSI merge, and i was really only interested in the first hop, i.e. i was primarily interested in the pull request that clearly changed multiple subsystems, and a seemingly API change that broke the build. Three mailing lists and three maintainers were already on the Cc: line for that pull request. So tell me, exactly what should have let me to believe that i should have added anyone else to the _already_ sizable Cc: line?? I could have done it, had i have more time and had i realized the full scope of the change and the somewhat misleading Cc:s that were on the original pull request, but i clearly was not _required_ to - and your suggestions to the contrary are ridiculous. Furthermore i reject the sometimes derogatory undertone of your mails that implies that i should somehow have done more or different work than i already did. I really ...
Wait, my testing caught this. I made the change to the patch myself, adding the needed conversion to my tree, it's here, on my disk! Oh crap, I never checked it in. /me goes off to sulk in shame. Very sorry about this, totally my fault, I knew this needed to be fixed, fixed it, but it didn't propogate to the tree to send to Linus. I suck. I need a vacation and an empty inbox. I'll be sending fixup patches for this and other merge messes in a few hours, once the coffee has kicked in... greg k-h --
you might want to join the (sizable) club of us deeply ashamed people who recently introduced upstream build failures :-/ Ingo --
Personal CC differences aside <grin> I do think you guys both do a good job of staying on top of your subsystems. We all wear the brown paper bag on occasion, and with the "merge maelstrom" during each merge window, I'm quite frankly amazed at how _little_ stuff get broken overall. Jeff --
Yeah, I re-ran some statistics the other day on our kernel development rate, and changed my formula after Andrew accused me of severely undercounting the rate of change. Turns out that as of 2.6.24-rc8 for the 2.6.24 kernel release we did: lines added per day: 4945 lines removed per day: 2006 lines modified per day: 1702 And note, that is real stuff, not renames or file moves at all, git handles not reporting that. That's for the 99 days that it took to do 2.6.24-rc8 (I need to re-run the scripts now that 2.6.24 is out.) It's fricken scarily amazing that things are still working at all... Just something to make you all sleep well at night :) thanks, greg k-h --
