On Tue, Jul 05, 2005 at 01:46:20PM -0400, John W. Linville wrote: > On Sat, Jul 02, 2005 at 01:29:54AM -0600, Grant Grundler wrote: > > On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote: > > > > + /* Some devices lose PCI config header data during D3hot->D0 > > > > Can you name some of those devices here? > > I just want to know what sort of devices need to be tested > > if this code changes in the future. > > I don't really have a list. The devices that brought this issue to > my attention are a 3c905B and a 3c556B, both covered by the 3c59x > driver. John, apologies for the late reply - been offline the past two weeks on holiday. Just listing the two devices in a comment would be sufficient. > According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT INTERFACE > SPECIFICATION, REV. 1.2", a device transitioning from D3hot to D0 > _may_ perform an internal reset, thereby going to "D0 Uninitialized" > rather than "D0 Initialized". Including the above paragraph in a comment would be a good thing. I don't know if this spec is publicly available. But even if it is, typically only a handful of people will be familiar enough with it to know where to look in it. > Since this behaviour is ratified by > the spec, I think we need to accomodate it. Yes - sounds reasonable to me too. > A bit in the PMCSR register indicates how a device will behave in > this regard. We could have a test to only execute the BAR restoration > for those devices that seem to need it. I left that out because it > seemed to add needless complexity. In the meantime the patch has > gotten bigger and more complex, so maybe that code doesn't make it > any worse. Do you want me to add that? I think I'd keep it simpler until someone proves we need it. I've read the rest of the thread and don't recall any such proof. > > > > > > + transition. Since some firmware leaves devices in D3hot > > > + state at boot, this information needs to be restored. > > > > Again, which firmware? > > Examples are good since it makes it possible to track down > > the offending devices for testing. > > The Thinkpad T21 does this. I don't know of any others specifically, > but it seems like something laptop BIOSes would be likely to do. That's fine - just listing the Thinkpad T21 in a comment is helpful. If you happen to know the firmware version too, that would be even better. > > The following chunk looks like it will have issues with 64-bit BARs: > > As RMK pointed-out, this code is inspired by setup-res.c; specifically > that in pci_update_resource. I'd prefer not to blaze any new trails > regarding 64-bit BAR support ATM... :-) After thinking about this more, I'm convinced it's broken if a 64-bit BAR is present on the PCI device. It doesn't matter if the MMIO value is greater than 4GB or not. The problem is pci_dev->resource[i] does NOT map 1:1 with PCI_BASE_ADDRESS_0+(i*4). > So, is the current patch acceptable? I don't think so. 64-bit BARs are just too common today. One solution is to use a seperate variable to track the offset into PCI config space. ie use "i" to walk through pci_dev->resource[] and add "unsigned int pcibar_offset" to keep track of 32 vs 64-bit BARs. > Or shall I add the check for the "no soft reset" bit in the PMCSR register? I don't see why that's necessary. thanks, grant
