| Message ID | 49ED22EC.2040204@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, 20 Apr 2009 18:35:40 -0700 Yinghai Lu <yinghai@kernel.org> wrote: > > it wil be overwriten later if _CRS is used, so don't bother to set it. > > [ Impact: cleanup ] Applied, thanks. A general comment on your patches though: please spent a few more minutes coming up with readable & useful summaries & changelogs. Most of the time when applying your patches (which are generally fine technically) I have to delete the whole changelog and come up with a new one based on reading the sources and then your patch. It would be nice if I didn't have to. Grammar and spelling mistakes are fine (I usually catch those) but when the logic of the changelog is all wrong, or it doesn't describe what it's doing and why, it makes things much more difficult. Thanks,
On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote: > it wil be overwriten later if _CRS is used, so don't bother to set it. > > [ Impact: cleanup ] > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > arch/x86/pci/amd_bus.c | 4 ++++ > 1 file changed, 4 insertions(+) > > Index: linux-2.6/arch/x86/pci/amd_bus.c > =================================================================== > --- linux-2.6.orig/arch/x86/pci/amd_bus.c > +++ linux-2.6/arch/x86/pci/amd_bus.c > @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct > int j; > struct pci_root_info *info; > > + /* don't go for it if _CRS is used */ > + if (pci_probe & PCI_USE__CRS) > + return; > + > /* if only one root bus, don't need to anything */ > if (pci_root_num < 2) > return; This isn't a comment on this patch per se. I am concerned about the fact that "pci=use_crs" is not the default. From the changelog of 62f420f8282, it sounds like you have to boot an IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI tells us everything we need to know. That's backwards. We shouldn't need an option to tell Linux that the firmware is trustworthy. We should have an option to *ignore* it for the times when we trip over something broken and haven't figured out a way to work around it yet. 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
On Mon, 27 Apr 2009 13:44:01 -0600 Bjorn Helgaas <bjorn.helgaas@hp.com> wrote: > On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote: > > it wil be overwriten later if _CRS is used, so don't bother to set > > it. > > > > [ Impact: cleanup ] > > > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > > > --- > > arch/x86/pci/amd_bus.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > Index: linux-2.6/arch/x86/pci/amd_bus.c > > =================================================================== > > --- linux-2.6.orig/arch/x86/pci/amd_bus.c > > +++ linux-2.6/arch/x86/pci/amd_bus.c > > @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct > > int j; > > struct pci_root_info *info; > > > > + /* don't go for it if _CRS is used */ > > + if (pci_probe & PCI_USE__CRS) > > + return; > > + > > /* if only one root bus, don't need to anything */ > > if (pci_root_num < 2) > > return; > > This isn't a comment on this patch per se. > > I am concerned about the fact that "pci=use_crs" is not the default. > From the changelog of 62f420f8282, it sounds like you have to boot an > IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI > tells us everything we need to know. That's backwards. > > We shouldn't need an option to tell Linux that the firmware is > trustworthy. We should have an option to *ignore* it for the times > when we trip over something broken and haven't figured out a way to > work around it yet. Well, we could try using _CRS by default, but like many things ACPI we can probably only trust firmwares after a certain date (i.e. the date when Windows started relying on the data being correct in order to boot). Do we have a good cutoff for that? Or should we try generally enabling it early in 2.6.31 to see what happens?
Bjorn Helgaas wrote: > On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote: >> it wil be overwriten later if _CRS is used, so don't bother to set it. >> >> [ Impact: cleanup ] >> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >> >> --- >> arch/x86/pci/amd_bus.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> Index: linux-2.6/arch/x86/pci/amd_bus.c >> =================================================================== >> --- linux-2.6.orig/arch/x86/pci/amd_bus.c >> +++ linux-2.6/arch/x86/pci/amd_bus.c >> @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct >> int j; >> struct pci_root_info *info; >> >> + /* don't go for it if _CRS is used */ >> + if (pci_probe & PCI_USE__CRS) >> + return; >> + >> /* if only one root bus, don't need to anything */ >> if (pci_root_num < 2) >> return; > > This isn't a comment on this patch per se. > > I am concerned about the fact that "pci=use_crs" is not the default. > From the changelog of 62f420f8282, it sounds like you have to boot an > IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI > tells us everything we need to know. That's backwards. > > We shouldn't need an option to tell Linux that the firmware is > trustworthy. We should have an option to *ignore* it for the times > when we trip over something broken and haven't figured out a way to > work around it yet. other system may have broken _CRS. maybe we could try to use DMI whitelist them? YH -- 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 Monday 27 April 2009 02:15:33 pm Yinghai Lu wrote: > Bjorn Helgaas wrote: > > On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote: > >> it wil be overwriten later if _CRS is used, so don't bother to set it. > >> > >> [ Impact: cleanup ] > >> > >> Signed-off-by: Yinghai Lu <yinghai@kernel.org> > >> > >> --- > >> arch/x86/pci/amd_bus.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> Index: linux-2.6/arch/x86/pci/amd_bus.c > >> =================================================================== > >> --- linux-2.6.orig/arch/x86/pci/amd_bus.c > >> +++ linux-2.6/arch/x86/pci/amd_bus.c > >> @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct > >> int j; > >> struct pci_root_info *info; > >> > >> + /* don't go for it if _CRS is used */ > >> + if (pci_probe & PCI_USE__CRS) > >> + return; > >> + > >> /* if only one root bus, don't need to anything */ > >> if (pci_root_num < 2) > >> return; > > > > This isn't a comment on this patch per se. > > > > I am concerned about the fact that "pci=use_crs" is not the default. > > From the changelog of 62f420f8282, it sounds like you have to boot an > > IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI > > tells us everything we need to know. That's backwards. > > > > We shouldn't need an option to tell Linux that the firmware is > > trustworthy. We should have an option to *ignore* it for the times > > when we trip over something broken and haven't figured out a way to > > work around it yet. > > other system may have broken _CRS. Do you have examples of problems here, or are you just worried that there *may* be problems? > maybe we could try to use DMI whitelist them? I don't like a whitelist because it requires ongoing maintenance for correctly-working machines. A blacklist is nicer because it only requires maintenance for *broken* machines. A date-based solution would be better from that point of view. 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
On Mon, Apr 27, 2009 at 1:39 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote: >> >> other system may have broken _CRS. > > Do you have examples of problems here, or are you just worried that > there *may* be problems? one system with three chains... with pci=use_crs [ 9.365669] pci_bus 0000:00: resource 0 io: [0x00-0x3af] [ 9.371065] pci_bus 0000:00: resource 1 io: [0x3e0-0xcf7] [ 9.376551] pci_bus 0000:00: resource 2 io: [0x3b0-0x3bb] [ 9.382028] pci_bus 0000:00: resource 3 io: [0x3c0-0x3df] [ 9.387513] pci_bus 0000:00: resource 4 io: [0xd00-0xefff] [ 9.393077] pci_bus 0000:00: resource 5 mem: [0x0a0000-0x0bffff] [ 9.399084] pci_bus 0000:00: resource 6 mem: [0x0d0000-0x0dffff] [ 9.405089] pci_bus 0000:00: resource 7 mem: [0xdd000000-0xdfffffff] [ 9.505332] pci_bus 0000:40: resource 0 io: [0x5000-0x8fff] [ 9.510991] pci_bus 0000:40: resource 1 mem: [0xdb000000-0xdcffffff] [ 9.553378] pci_bus 0000:80: resource 0 io: [0x1000-0x4fff] [ 9.559036] pci_bus 0000:80: resource 1 mem: [0xda000000-0xdaffffff] without that: amd_bus.c will read that from pci conf space [ 9.310965] pci_bus 0000:00: resource 0 io: [0x9000-0xefff] [ 9.316621] pci_bus 0000:00: resource 1 io: [0x00-0xfff] [ 9.322020] pci_bus 0000:00: resource 2 mem: [0xdd000000-0xdfffffff] [ 9.328373] pci_bus 0000:00: resource 3 mem: [0x0a0000-0x0bffff] [ 9.334378] pci_bus 0000:00: resource 4 mem: [0xc0000000-0xd9ffffff] [ 9.340731] pci_bus 0000:00: resource 5 mem: [0xf0000000-0xffffffff] [ 9.347084] pci_bus 0000:00: resource 6 mem: [0x840000000-0xfcffffffff] [ 9.444440] pci_bus 0000:40: resource 0 io: [0x5000-0x8fff] [ 9.450099] pci_bus 0000:40: resource 1 io: [0xf000-0xffff] [ 9.455757] pci_bus 0000:40: resource 2 mem: [0xdb000000-0xdcffffff] [ 9.498118] pci_bus 0000:80: resource 0 io: [0x1000-0x4fff] [ 9.503777] pci_bus 0000:80: resource 1 mem: [0xda000000-0xdaffffff] > >> maybe we could try to use DMI whitelist them? > > I don't like a whitelist because it requires ongoing maintenance > for correctly-working machines. A blacklist is nicer because it > only requires maintenance for *broken* machines. A date-based > solution would be better from that point of view. could try apply that in development cycle like -rcX, and disable that formal release. so could find more broken system. YH -- 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, Apr 27, 2009 at 01:44:01PM -0600, Bjorn Helgaas wrote: > On Monday 20 April 2009 07:35:40 pm Yinghai Lu wrote: > > it wil be overwriten later if _CRS is used, so don't bother to set it. > > > > [ Impact: cleanup ] > > > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > > > --- > > arch/x86/pci/amd_bus.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > Index: linux-2.6/arch/x86/pci/amd_bus.c > > =================================================================== > > --- linux-2.6.orig/arch/x86/pci/amd_bus.c > > +++ linux-2.6/arch/x86/pci/amd_bus.c > > @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct > > int j; > > struct pci_root_info *info; > > > > + /* don't go for it if _CRS is used */ > > + if (pci_probe & PCI_USE__CRS) > > + return; > > + > > /* if only one root bus, don't need to anything */ > > if (pci_root_num < 2) > > return; > > This isn't a comment on this patch per se. > > I am concerned about the fact that "pci=use_crs" is not the default. > >From the changelog of 62f420f8282, it sounds like you have to boot an > IBM x3850 with "pci=use_crs" to make hot-plug work, even though ACPI > tells us everything we need to know. That's backwards. > > We shouldn't need an option to tell Linux that the firmware is > trustworthy. We should have an option to *ignore* it for the times > when we trip over something broken and haven't figured out a way to > work around it yet. Sorry, I am behind on my email and just noticed this. When I posted the patches to add "pci=use_crs" it was only needed to enable PCI hotplug on a subset of our systems. At that time it was not apparent that others were interested. I was also concerned that real or anticipated breakage on on other systems might delay or prevent acceptance. As long as there is an option to disable it I am also in favor of trying to make it the default. Thanks! Gary
Index: linux-2.6/arch/x86/pci/amd_bus.c =================================================================== --- linux-2.6.orig/arch/x86/pci/amd_bus.c +++ linux-2.6/arch/x86/pci/amd_bus.c @@ -100,6 +100,10 @@ void x86_pci_root_bus_res_quirks(struct int j; struct pci_root_info *info; + /* don't go for it if _CRS is used */ + if (pci_probe & PCI_USE__CRS) + return; + /* if only one root bus, don't need to anything */ if (pci_root_num < 2) return;
it wil be overwriten later if _CRS is used, so don't bother to set it. [ Impact: cleanup ] Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- arch/x86/pci/amd_bus.c | 4 ++++ 1 file changed, 4 insertions(+) -- 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