* Re: Ignore disabled ROM resources at setup [not found] <200508261859.j7QIxT0I016917@hera.kernel.org> @ 2005-08-30 2:38 ` Benjamin Herrenschmidt 2005-08-30 3:15 ` Linus Torvalds ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 2:38 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: Greg KH, helgehaf, Linus Torvalds On Fri, 2005-08-26 at 11:59 -0700, Linux Kernel Mailing List wrote: > tree d8b7aaaec93de93841b46e8e05a3b454d05bd357 > parent 26aad69e3dd854abe9028ca873fb40b410a39dd7 > author Linus Torvalds <torvalds@g5.osdl.org> Sat, 27 Aug 2005 00:49:22 -0700 > committer Linus Torvalds <torvalds@g5.osdl.org> Sat, 27 Aug 2005 00:49:22 -0700 > > Ignore disabled ROM resources at setup > > Writing even a disabled value seems to mess up some matrox graphics > cards. It may be a card-related issue, but we may also be writing > reserved low bits in the result. > > This was a fall-out of switching x86 over to the generic PCI resource > allocation code, and needs more debugging. In particular, the old x86 > code defaulted to not doing any resource allocations at all for ROM > resources. > > In the meantime, this has been reported to make X happier by Helge > Hafting <helgehaf@aitel.hist.no>. This "fix" also seems to break all powermac laptops around :( In fact, it might break any user of pci_map_rom() as it exposes a bug in that function. The problem is that their firmware doesn't assign a ROM resource as they have no ROM on the video chip (like most laptops). radeonfb and aty128fb among others will call pci_map_rom() to try to find an x86 BIOS ROM with some config tables in it. pci_map_rom "sees" that the resource is unassigned by testing the parent pointer, and calls pci_assign_resource() which, with this new patch, will do nothing. Unfortunately, pci_map_rom will not notice this failure, and will happily ioremap & access the bogus resource, thus causing the crash. I'll come up with a fix for pci_map_rom later today. While looking there, I also noticed pci_map_rom_copy() stuff and I'm surprised it was ever accepted in the tree. While I can understand that we might need to keep a cached copy of the ROM content (due to cards like matrox who can't enable both the ROM and the BARs among other issues), the whole idea of whacking a kernel virtual pointer in the struct resource->start of the ROM bar is just too disgusting for words and will probably cause "intersting" side effects in /proc, sysfs and others... Shouldn't we just have a pointer in pci_dev for the optional "ROM cache" instead ? Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 2:38 ` Ignore disabled ROM resources at setup Benjamin Herrenschmidt @ 2005-08-30 3:15 ` Linus Torvalds 2005-08-30 4:47 ` Jon Smirl 2005-08-30 3:19 ` Benjamin Herrenschmidt 2005-08-30 4:35 ` Jon Smirl 2 siblings, 1 reply; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 3:15 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Linux Kernel Mailing List, Greg KH, helgehaf On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > pci_map_rom "sees" that the resource is unassigned by testing the parent > pointer, and calls pci_assign_resource() which, with this new patch, > will do nothing. Ehh.. It didn't do anything with the old code either for that case, so there's apparently something else also going on. It would write the base address, but since it wouldn't _enable_ the ROM mapping (only the "non-enabled" case changed by this commit), the end result from a hw standpoint should be exactly the same: the ROM isn't actually mapped. So after pci_assign_resource(), the resource has literally been assigned, but that patch should not have mattered in any way whether it was actually _enabled_ or not. Now, there's clearly a difference. What has always worked is then to do pci_write_config_dword(dev, PCI_ROM_ADDRESS, PCI_ROM_ADDRESS_ENABLE | res->start) (well, these days you're supposed to use "pcibios_resource_to_bus()" to get the start value to write out). Much preferable is probably to just enable the resource manually _before_ calling pci_assign_resource, ie do something like. dev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_ENABLE; pci_assign_resource(dev, PCI_ROM_RESOURCE); But yes, if something used to just blindly set PCI_ROM_ADDRESS_ENABLE, then that got broken. I grepped for that and didn't see anything like it, but I guess people are doing it with the magic constant "1".. I don't even find any access to "PCI_ROM_ADDRESS" in radeonfb, so I wonder if it has ever worked? I get the feeling that if the ROM wasn't enabled, it didn't work before either, but perhaps the change makes the failure mode more spectacular? Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 3:15 ` Linus Torvalds @ 2005-08-30 4:47 ` Jon Smirl 0 siblings, 0 replies; 24+ messages in thread From: Jon Smirl @ 2005-08-30 4:47 UTC (permalink / raw) To: Linus Torvalds Cc: Benjamin Herrenschmidt, Linux Kernel Mailing List, Greg KH, helgehaf On 8/29/05, Linus Torvalds <torvalds@osdl.org> wrote: > > > On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > > > pci_map_rom "sees" that the resource is unassigned by testing the parent > > pointer, and calls pci_assign_resource() which, with this new patch, > > will do nothing. > > Ehh.. It didn't do anything with the old code either for that case, so > there's apparently something else also going on. > > It would write the base address, but since it wouldn't _enable_ the ROM > mapping (only the "non-enabled" case changed by this commit), the end > result from a hw standpoint should be exactly the same: the ROM isn't > actually mapped. > > So after pci_assign_resource(), the resource has literally been assigned, > but that patch should not have mattered in any way whether it was actually > _enabled_ or not. > > Now, there's clearly a difference. What has always worked is then to do > > pci_write_config_dword(dev, > PCI_ROM_ADDRESS, > PCI_ROM_ADDRESS_ENABLE | res->start) > > (well, these days you're supposed to use "pcibios_resource_to_bus()" to > get the start value to write out). > > Much preferable is probably to just enable the resource manually _before_ > calling pci_assign_resource, ie do something like. > > dev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_ENABLE; > pci_assign_resource(dev, PCI_ROM_RESOURCE); > > But yes, if something used to just blindly set PCI_ROM_ADDRESS_ENABLE, > then that got broken. I grepped for that and didn't see anything like it, > but I guess people are doing it with the magic constant "1".. Nothing in the driver tree should be using PCI_ROM_ADDRESS_ENABLE except drivers/pci/*. Drivers that are still manipulating ROMs directly should be converted to use the PCI ROM API. I started to fix these but some of the use is non-obvious. It is best if the maintainers do it. These files are still directly manipulating ROMs: ide/pci/aec62xx.c ide/pci/cmd64x.c ide/pci/hpt34x.c ide/pci/hpt366.c ide/pci/pdc202xx_new. ide/pci/pdc202xx_old.c mtd/maps/pci.c net/sungem.c net/sunhme.c scsi/qla2xxx/qla_init.c video/console/sticore.c video/matrox/matroxfb_misc.c video/sis/sis_main.c -- Jon Smirl jonsmirl@gmail.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 2:38 ` Ignore disabled ROM resources at setup Benjamin Herrenschmidt 2005-08-30 3:15 ` Linus Torvalds @ 2005-08-30 3:19 ` Benjamin Herrenschmidt 2005-08-30 3:52 ` Linus Torvalds 2005-08-30 4:35 ` Jon Smirl 2 siblings, 1 reply; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 3:19 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: Greg KH, helgehaf, Linus Torvalds > pci_map_rom "sees" that the resource is unassigned by testing the parent > pointer, and calls pci_assign_resource() which, with this new patch, > will do nothing. Ok, it won't do nothing in fact. It's worse. It will return 0 (success), will actually assign a completely new address to the resource, will update the resource structure ... and will _not_ update the PCI resource BAR for the ROM. That is very bad and definitely not what you want, wether it's ppc, x86 or anything else. Either fail (don't assign the resource at all) or if you assign it, keep the BAR in sync with the struct resource. Also, why do we re-allocate a new address for it ? It's been properly allocated a non-conflicting address by the firmware ... That's a big problem I have with our common code as well. pci_assign_unassigned_resource() doesn't do what it claims: it will re-assign all resources, not only the unassigned ones, at least as soon as it spots a brige, and pci_assign_resource() here called by pci_map_rom() will re-assign even if the address there was already correct. At this point, i'm not sure what the proper fix it. Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 3:19 ` Benjamin Herrenschmidt @ 2005-08-30 3:52 ` Linus Torvalds 2005-08-30 4:09 ` Linus Torvalds 2005-08-30 4:33 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 3:52 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Linux Kernel Mailing List, Greg KH, helgehaf On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > Ok, it won't do nothing in fact. It's worse. It will return 0 (success), > will actually assign a completely new address to the resource, will > update the resource structure ... and will _not_ update the PCI resource > BAR for the ROM. > > That is very bad and definitely not what you want, wether it's ppc, x86 > or anything else. Either fail (don't assign the resource at all) or if > you assign it, keep the BAR in sync with the struct resource. I argue that the BAR _is_ in sync with the resource. The resource is allocated, but not enabled. You want to enable it, you need to write the BAR. The fact is, that writing the address (but not the enable bit) to the BAR when it's not enabled leads to problems. It wasn't entirely clear whether the problems were in the X server (possible) or whether it was actual hardware (hey, nothing surprises me any more). So what allocate does is to _allocate_ it. It so happens that with normal PIO and IOMEM resources, there is no per-resource "enable" bit, so they are always enabled. However, PCI ROM's have a per-resource enable bit both in hardware and in the "struct resource", and if it isn't set, then the resource allocation is done, but the resource is not enabled and not written to hardware. All very consistent. The allocation was successful, but you didn't ask to enable it, so pci_alloc_resource() will return success without actually enablign the hardware. What you want is a "zombie state", where we write the partial information to hardware. It's what we used to do, but it's certainly no more logical than what it does now, and it led to problem reports. Btw, why does this happen on powerpc, but not x86? I'm running a radeon laptop right now myself. Hmm.. Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 3:52 ` Linus Torvalds @ 2005-08-30 4:09 ` Linus Torvalds 2005-08-30 4:20 ` David S. Miller 2005-08-30 4:33 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 4:09 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Linux Kernel Mailing List, Greg KH, helgehaf On Mon, 29 Aug 2005, Linus Torvalds wrote: > > What you want is a "zombie state", where we write the partial information > to hardware. It's what we used to do, but it's certainly no more logical > than what it does now, and it led to problem reports. Btw, the fundamental reason for the change is that x86 used to not ever touch any ROM resources _at_all_ by default, unless you used the "pci=rom" kernel command line switch. But that got changed, and in an effort to make x86 more like all the other architectures, it now uses the generic setup-bus stuff (which used to be "generic, but not x86", since the BIOS tends to set up all PCI buses on PC's) that probes all resources (including rom resources) to see how big they are etc. That meant that suddenly the ROM resources _did_ get sized up and written, even if they didn't actually get enabled. So on an x86, 2.6.12 would never touch the ROM resource at all, while for a while in 2.6.13-rc it would write it with a disabled value by default. And that's the thing that broke. So 2.6.13 is being "safe". It allocates the space for the ROM in the resource tables, but it neither enables it nor does it write the (disabled) address out to the device, since both of those actions have been shown to break on PC's. And sadly (or happily, depends on your viewpoint), PC's have a _much_ wider range of hardware, so they are the ones we have to work around. So yes, behaviour changed, but it changed exactly because not changing it leads to problems. So what you will find is that /sbin/lspci actually understands this situation: 01:00.0 VGA compatible controller: ATI Technologies Inc Radeon Mobility M7 LW [Radeon Mobility 7500] (prog-if 00 [VGA]) ... [virtual] Expansion ROM at 90220000 [disabled] [size=128K] ... 30: 00 00 00 00 58 00 00 00 00 00 00 00 0a 01 08 00 Notice how the hardware ROM base at 0x30 is set to "00 00 00 00", but because the resource allocation has been made and the resource shows up in /sys, lspci shows the disabled allocation correctly. That's really how any kernel user will need to understand it too: the allocation exists, but since it's not enabled, the hardware hasn't been told. Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:09 ` Linus Torvalds @ 2005-08-30 4:20 ` David S. Miller 2005-08-30 4:37 ` Benjamin Herrenschmidt 2005-08-30 4:40 ` Linus Torvalds 0 siblings, 2 replies; 24+ messages in thread From: David S. Miller @ 2005-08-30 4:20 UTC (permalink / raw) To: torvalds; +Cc: benh, linux-kernel, greg, helgehaf From: Linus Torvalds <torvalds@osdl.org> Date: Mon, 29 Aug 2005 21:09:24 -0700 (PDT) > So 2.6.13 is being "safe". It allocates the space for the ROM in the > resource tables, but it neither enables it nor does it write the > (disabled) address out to the device, since both of those actions have > been shown to break on PC's. And sadly (or happily, depends on your > viewpoint), PC's have a _much_ wider range of hardware, so they are the > ones we have to work around. Actually, I can tell you that it is a known fact that Qlogic ISP PCI cards will not respond to I/O and MEM space when you enable the ROM. And this behavior exists in quite a few other PCI parts as well. So I think the kernel, by not enabling the ROM, is doing the right thing here. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:20 ` David S. Miller @ 2005-08-30 4:37 ` Benjamin Herrenschmidt 2005-08-30 4:40 ` Linus Torvalds 1 sibling, 0 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 4:37 UTC (permalink / raw) To: David S. Miller; +Cc: torvalds, linux-kernel, greg, helgehaf On Mon, 2005-08-29 at 21:20 -0700, David S. Miller wrote: > From: Linus Torvalds <torvalds@osdl.org> > Date: Mon, 29 Aug 2005 21:09:24 -0700 (PDT) > > > So 2.6.13 is being "safe". It allocates the space for the ROM in the > > resource tables, but it neither enables it nor does it write the > > (disabled) address out to the device, since both of those actions have > > been shown to break on PC's. And sadly (or happily, depends on your > > viewpoint), PC's have a _much_ wider range of hardware, so they are the > > ones we have to work around. > > Actually, I can tell you that it is a known fact that Qlogic ISP > PCI cards will not respond to I/O and MEM space when you enable > the ROM. And this behavior exists in quite a few other PCI parts > as well. Yes, including Matrox cards. > So I think the kernel, by not enabling the ROM, is doing the > right thing here. It is, the problem is that not only it doesn't enable it, but it also doesn't write the resource to the BAR, which triggers a bug in pci_map_rom which then enables the decoding but doesn't update the BAR with the new address neither. Note also the, imho totally broken, code in pci_map_rom_copy() which is supposed to keep a cached copy of the ROM in memory specifically for these cards to have an easier access afterward (or for sysfs rom file access to work). I think that code should have a pointer in pci_dev for the cache instead of stuffing a kernel virtual address in the middle of the resouce tree. Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:20 ` David S. Miller 2005-08-30 4:37 ` Benjamin Herrenschmidt @ 2005-08-30 4:40 ` Linus Torvalds 2005-08-30 4:49 ` Benjamin Herrenschmidt 2005-08-30 4:51 ` Jon Smirl 1 sibling, 2 replies; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 4:40 UTC (permalink / raw) To: David S. Miller; +Cc: benh, linux-kernel, greg, helgehaf On Mon, 29 Aug 2005, David S. Miller wrote: > > So I think the kernel, by not enabling the ROM, is doing the > right thing here. Notice that on ppc even older versions didn't actually _enable_ the rom, but they would write the non-enabled address to the PCI_ROM_ADDRESS register, so that anybody who read that register would see _where_ the ROM would be enabled if it was enabled. That's the thing that changed in the commit Ben dislikes. Now, if the ROM is disabled, we won't even write the disabled address to the PCI register, because it led to trouble on some strange Matrox card. Probably a card that nobody has ever used on PPC, and certainly not on a Powerbook, so in that sense the apparent breakage on ppc is arguably "unnecessary" as far as Ben is concerned. But I notice the problem: pci_enable_rom() is indeed broken with the change. Ben, does this (totally untested) patch fix it for you? Linus ---- diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c --- a/drivers/pci/rom.c +++ b/drivers/pci/rom.c @@ -23,11 +23,14 @@ */ static void pci_enable_rom(struct pci_dev *pdev) { - u32 rom_addr; + struct resource *res = pdev->resource + PCI_ROM_RESOURCE; + struct pci_bus_region region; - pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr); - rom_addr |= PCI_ROM_ADDRESS_ENABLE; - pci_write_config_dword(pdev, pdev->rom_base_reg, rom_addr); + if (!res->flags) + return; + + pcibios_resource_to_bus(pdev, ®ion, res); + pci_write_config_dword(pdev, pdev->rom_base_reg, region.start | PCI_ROM_ADDRESS_ENABLE); } /** ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:40 ` Linus Torvalds @ 2005-08-30 4:49 ` Benjamin Herrenschmidt 2005-08-30 5:29 ` Linus Torvalds 2005-08-30 4:51 ` Jon Smirl 1 sibling, 1 reply; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 4:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: David S. Miller, linux-kernel, greg, helgehaf On Mon, 2005-08-29 at 21:40 -0700, Linus Torvalds wrote: > > On Mon, 29 Aug 2005, David S. Miller wrote: > > > > So I think the kernel, by not enabling the ROM, is doing the > > right thing here. > > Notice that on ppc even older versions didn't actually _enable_ the rom, > but they would write the non-enabled address to the PCI_ROM_ADDRESS > register, so that anybody who read that register would see _where_ the ROM > would be enabled if it was enabled. > > That's the thing that changed in the commit Ben dislikes. Now, if the ROM > is disabled, we won't even write the disabled address to the PCI register, > because it led to trouble on some strange Matrox card. Probably a card > that nobody has ever used on PPC, and certainly not on a Powerbook, so in > that sense the apparent breakage on ppc is arguably "unnecessary" as far > as Ben is concerned. > > But I notice the problem: pci_enable_rom() is indeed broken with the > change. > > Ben, does this (totally untested) patch fix it for you? I was just testing a slightly different one that appear to fix the problem : Index: linux-work/drivers/pci/rom.c =================================================================== --- linux-work.orig/drivers/pci/rom.c 2005-08-01 22:03:44.000000000 +1000 +++ linux-work/drivers/pci/rom.c 2005-08-30 14:46:26.000000000 +1000 @@ -23,9 +23,12 @@ */ static void pci_enable_rom(struct pci_dev *pdev) { + struct pci_bus_region region; + struct resource *res = &pdev->resource[PCI_ROM_RESOURCE]; u32 rom_addr; - pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr); + pcibios_resource_to_bus(pdev, ®ion, res); + rom_addr = region.start | (res->flags & PCI_REGION_FLAG_MASK); rom_addr |= PCI_ROM_ADDRESS_ENABLE; pci_write_config_dword(pdev, pdev->rom_base_reg, rom_addr); } @@ -71,12 +74,17 @@ } else { if (res->flags & IORESOURCE_ROM_COPY) { *size = pci_resource_len(pdev, PCI_ROM_RESOURCE); - return (void __iomem *)pci_resource_start(pdev, PCI_ROM_RESOURCE); + return (void __iomem *)pci_resource_start(pdev, + PCI_ROM_RESOURCE); } else { /* assign the ROM an address if it doesn't have one */ - if (res->parent == NULL) - pci_assign_resource(pdev, PCI_ROM_RESOURCE); - + if (res->parent == NULL) { + int err; + err = pci_assign_resource(pdev, + PCI_ROM_RESOURCE); + if (err) + return NULL; + } start = pci_resource_start(pdev, PCI_ROM_RESOURCE); *size = pci_resource_len(pdev, PCI_ROM_RESOURCE); if (*size == 0) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:49 ` Benjamin Herrenschmidt @ 2005-08-30 5:29 ` Linus Torvalds 2005-08-30 6:46 ` Benjamin Herrenschmidt 2005-08-31 4:16 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 5:29 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: David S. Miller, linux-kernel, greg, helgehaf On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > I was just testing a slightly different one that appear to fix the > problem : ... > + rom_addr = region.start | (res->flags & PCI_REGION_FLAG_MASK); I worry about this one. It's not really correct. The low en bits are "reserved", and while I don't know whether writing zero is always correct, I do know that just writing the low 4 bits with the old value is a bit strange. And the flags don't save the other bits. So I'd prefer either my previous diff, or one that just re-reads the bits entirely, something like the appended.. What does the PCI spec say about the reserved bits? Do we have to save them? Linus --- diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c --- a/drivers/pci/rom.c +++ b/drivers/pci/rom.c @@ -24,9 +24,16 @@ static void pci_enable_rom(struct pci_dev *pdev) { u32 rom_addr; + struct resource *res = pdev->resource + PCI_ROM_RESOURCE; + struct pci_bus_region region; + if (!res->flags) + return; + + pcibios_resource_to_bus(pdev, ®ion, res); pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr); - rom_addr |= PCI_ROM_ADDRESS_ENABLE; + rom_addr &= ~PCI_ROM_ADDRESS_MASK; + rom_addr |= region.start | PCI_ROM_ADDRESS_ENABLE; pci_write_config_dword(pdev, pdev->rom_base_reg, rom_addr); } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 5:29 ` Linus Torvalds @ 2005-08-30 6:46 ` Benjamin Herrenschmidt 2005-08-31 4:16 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 6:46 UTC (permalink / raw) To: Linus Torvalds; +Cc: David S. Miller, linux-kernel, greg, helgehaf On Mon, 2005-08-29 at 22:29 -0700, Linus Torvalds wrote: > > On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > > > I was just testing a slightly different one that appear to fix the > > problem : > ... > > + rom_addr = region.start | (res->flags & PCI_REGION_FLAG_MASK); > > I worry about this one. It's not really correct. The low en bits are > "reserved", and while I don't know whether writing zero is always correct, > I do know that just writing the low 4 bits with the old value is a bit > strange. And the flags don't save the other bits. > > So I'd prefer either my previous diff, or one that just re-reads the bits > entirely, something like the appended.. Can you keep the part of my patch that adds error checking on the result of pci_assign_resource() ? > What does the PCI spec say about the reserved bits? Do we have to save > them? I didn't see anything special about them, but I may have missed it. Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 5:29 ` Linus Torvalds 2005-08-30 6:46 ` Benjamin Herrenschmidt @ 2005-08-31 4:16 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-31 4:16 UTC (permalink / raw) To: Linus Torvalds; +Cc: David S. Miller, linux-kernel, greg, helgehaf Here's a new patch based on Linus latest one with better error checking. Please push if you are fine with it. This patch fixes a problem with pci_map_rom() which doesn't properly update the ROM BAR value with the address thas allocated for it by the PCI code. This problem, among other, breaks boot on Mac laptops. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Index: linux-work/drivers/pci/rom.c =================================================================== --- linux-work.orig/drivers/pci/rom.c 2005-08-01 22:03:44.000000000 +1000 +++ linux-work/drivers/pci/rom.c 2005-08-31 14:09:02.000000000 +1000 @@ -21,13 +21,21 @@ * between the ROM and other resources, so enabling it may disable access * to MMIO registers or other card memory. */ -static void pci_enable_rom(struct pci_dev *pdev) +static int pci_enable_rom(struct pci_dev *pdev) { + struct resource *res = pdev->resource + PCI_ROM_RESOURCE; + struct pci_bus_region region; u32 rom_addr; + if (!res->flags) + return -1; + + pcibios_resource_to_bus(pdev, ®ion, res); pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr); - rom_addr |= PCI_ROM_ADDRESS_ENABLE; + rom_addr &= ~PCI_ROM_ADDRESS_MASK; + rom_addr |= region.start | PCI_ROM_ADDRESS_ENABLE; pci_write_config_dword(pdev, pdev->rom_base_reg, rom_addr); + return 0; } /** @@ -71,19 +79,21 @@ } else { if (res->flags & IORESOURCE_ROM_COPY) { *size = pci_resource_len(pdev, PCI_ROM_RESOURCE); - return (void __iomem *)pci_resource_start(pdev, PCI_ROM_RESOURCE); + return (void __iomem *)pci_resource_start(pdev, + PCI_ROM_RESOURCE); } else { /* assign the ROM an address if it doesn't have one */ - if (res->parent == NULL) - pci_assign_resource(pdev, PCI_ROM_RESOURCE); - + if (res->parent == NULL && + pci_assign_resource(pdev,PCI_ROM_RESOURCE)) + return NULL; start = pci_resource_start(pdev, PCI_ROM_RESOURCE); *size = pci_resource_len(pdev, PCI_ROM_RESOURCE); if (*size == 0) return NULL; /* Enable ROM space decodes */ - pci_enable_rom(pdev); + if (pci_enable_rom(pdev)) + return NULL; } } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:40 ` Linus Torvalds 2005-08-30 4:49 ` Benjamin Herrenschmidt @ 2005-08-30 4:51 ` Jon Smirl 2005-08-30 4:54 ` Benjamin Herrenschmidt 2005-08-30 5:15 ` Linus Torvalds 1 sibling, 2 replies; 24+ messages in thread From: Jon Smirl @ 2005-08-30 4:51 UTC (permalink / raw) To: Linus Torvalds; +Cc: David S. Miller, benh, linux-kernel, greg, helgehaf On 8/30/05, Linus Torvalds <torvalds@osdl.org> wrote: > > > On Mon, 29 Aug 2005, David S. Miller wrote: > > > > So I think the kernel, by not enabling the ROM, is doing the > > right thing here. > > Notice that on ppc even older versions didn't actually _enable_ the rom, > but they would write the non-enabled address to the PCI_ROM_ADDRESS > register, so that anybody who read that register would see _where_ the ROM > would be enabled if it was enabled. > > That's the thing that changed in the commit Ben dislikes. Now, if the ROM > is disabled, we won't even write the disabled address to the PCI register, > because it led to trouble on some strange Matrox card. Probably a card > that nobody has ever used on PPC, and certainly not on a Powerbook, so in > that sense the apparent breakage on ppc is arguably "unnecessary" as far > as Ben is concerned. > > But I notice the problem: pci_enable_rom() is indeed broken with the > change. > > Ben, does this (totally untested) patch fix it for you? > > Linus > > ---- > diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c > --- a/drivers/pci/rom.c > +++ b/drivers/pci/rom.c > @@ -23,11 +23,14 @@ > */ > static void pci_enable_rom(struct pci_dev *pdev) > { > - u32 rom_addr; > + struct resource *res = pdev->resource + PCI_ROM_RESOURCE; > + struct pci_bus_region region; > > - pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr); > - rom_addr |= PCI_ROM_ADDRESS_ENABLE; > - pci_write_config_dword(pdev, pdev->rom_base_reg, rom_addr); > + if (!res->flags) > + return; > + > + pcibios_resource_to_bus(pdev, ®ion, res); > + pci_write_config_dword(pdev, pdev->rom_base_reg, region.start | PCI_ROM_ADDRESS_ENABLE); > } I was reading the status out of the PCI config space to account for our friend X which enables ROMs without informing the OS. With X around PCI config space can get out of sync with the kernel structures. > > /** > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- Jon Smirl jonsmirl@gmail.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:51 ` Jon Smirl @ 2005-08-30 4:54 ` Benjamin Herrenschmidt 2005-08-30 5:15 ` Linus Torvalds 1 sibling, 0 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 4:54 UTC (permalink / raw) To: Jon Smirl; +Cc: Linus Torvalds, David S. Miller, linux-kernel, greg, helgehaf > > I was reading the status out of the PCI config space to account for > our friend X which enables ROMs without informing the OS. With X > around PCI config space can get out of sync with the kernel > structures. Well, X isn't supposed to keep the ROM enabled is it ? besides, most of the time, the kernel code will be run at boot. I think we shouldn't care here. If X does the wrong thing, it will eventually break but it shouldn't break in the "normal" case and it will ultimately be fixed (finger crossed) by R7.1 Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:51 ` Jon Smirl 2005-08-30 4:54 ` Benjamin Herrenschmidt @ 2005-08-30 5:15 ` Linus Torvalds 2005-08-30 14:39 ` Alan Cox 2005-08-30 15:29 ` Petr Vandrovec 1 sibling, 2 replies; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 5:15 UTC (permalink / raw) To: Jon Smirl; +Cc: David S. Miller, benh, linux-kernel, greg, helgehaf On Tue, 30 Aug 2005, Jon Smirl wrote: > > I was reading the status out of the PCI config space to account for > our friend X which enables ROMs without informing the OS. With X > around PCI config space can get out of sync with the kernel > structures. Well, yes, except that we use the in-kernel resource address for the actual ioremap() _anyway_ in the routine that calls this, so if X has remapped the ROM somewhere else, that wouldn't work in the first place. I'm sure X plays games with this register (I suspect that's why the Matrox thing broke in the first place), but I don't think it should do so while the kernel uses it. I don't think we have much choice anyway. See above. Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 5:15 ` Linus Torvalds @ 2005-08-30 14:39 ` Alan Cox 2005-08-30 15:29 ` Petr Vandrovec 1 sibling, 0 replies; 24+ messages in thread From: Alan Cox @ 2005-08-30 14:39 UTC (permalink / raw) To: Linus Torvalds Cc: Jon Smirl, David S. Miller, benh, linux-kernel, greg, helgehaf On Llu, 2005-08-29 at 22:15 -0700, Linus Torvalds wrote: > I'm sure X plays games with this register (I suspect that's why the Matrox > thing broke in the first place), but I don't think it should do so while > the kernel uses it. X maps it over ram during matrox set up but that is all it uses it for. X can also be persuaded to do it differently given a suitable kernel API. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 5:15 ` Linus Torvalds 2005-08-30 14:39 ` Alan Cox @ 2005-08-30 15:29 ` Petr Vandrovec 1 sibling, 0 replies; 24+ messages in thread From: Petr Vandrovec @ 2005-08-30 15:29 UTC (permalink / raw) To: Linus Torvalds Cc: Jon Smirl, David S. Miller, benh, linux-kernel, greg, helgehaf Linus Torvalds wrote: > > On Tue, 30 Aug 2005, Jon Smirl wrote: > >>I was reading the status out of the PCI config space to account for >>our friend X which enables ROMs without informing the OS. With X >>around PCI config space can get out of sync with the kernel >>structures. > > > Well, yes, except that we use the in-kernel resource address for the > actual ioremap() _anyway_ in the routine that calls this, so if X has > remapped the ROM somewhere else, that wouldn't work in the first place. > > I'm sure X plays games with this register (I suspect that's why the Matrox > thing broke in the first place), but I don't think it should do so while > the kernel uses it. Matrox broke because some of their chips have ROM base bits 31-1 reserved (read as 0) when bit 0 is cleared. So if you read valid ROM resource, clear enable bit, write it to the device, and later it read from device, enable it, and write back, you'll end up with ROM enabled at bus address zero. Probably not what you want. And FYI, on my Tyan S2885 box 2.6.13 relocates all (as far as I can tell) ROMs because BIOS assigns them into 'MEM window' (non-prefetchable) while kernel reassigns then to the 'PREFETCH window'. In the past code was even allocating ROM resources above 4GB (which is nonsense for ROM region, unfortunately pci_bus_alloc_resource does not seem to know about difference between 64bit and 32bit BARs, and it always uses -1 as max address, which is wrong on 64bit kernel), but it does not happen since I went from 4GB of memory back to 2GB... Petr Vandrovec ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 3:52 ` Linus Torvalds 2005-08-30 4:09 ` Linus Torvalds @ 2005-08-30 4:33 ` Benjamin Herrenschmidt 2005-08-30 5:03 ` Linus Torvalds 1 sibling, 1 reply; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 4:33 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Greg KH, helgehaf > What you want is a "zombie state", where we write the partial information > to hardware. It's what we used to do, but it's certainly no more logical > than what it does now, and it led to problem reports. > > Btw, why does this happen on powerpc, but not x86? I'm running a radeon > laptop right now myself. Hmm.. It's using the RAM shadow at c0000 on these ... I'm still not convinced that having the struct resource allocated and mismatched with the BAR value is a good thing... But anyway, so the bug would then be pci_map_rom who is writing the enable bit without fixing the rest of the BAR... So what about fixing pci_map_rom() to call pcibios_resource_to_bus() and then write the resource back to the BAR ? I'm still a bit annoyed that we re-allocate the address while the original one was perfectly good (though not enabled) but the above would work. Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:33 ` Benjamin Herrenschmidt @ 2005-08-30 5:03 ` Linus Torvalds 2005-08-30 6:40 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 24+ messages in thread From: Linus Torvalds @ 2005-08-30 5:03 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Linux Kernel Mailing List, Greg KH, helgehaf On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > So what about fixing pci_map_rom() to call pcibios_resource_to_bus() and > then write the resource back to the BAR ? I'm still a bit annoyed that > we re-allocate the address while the original one was perfectly good > (though not enabled) but the above would work. I just sent you a patch to try. Btw, as to the re-allocation of an existing address: most of the PCI layer really does try to avoid re-allocating known good addresses. In fact, I thought we did so for ROM resources too: at least pci_read_bases() does read the ROM base, and saves it off into the resource structure. We'll end up re-assigning that saved-off-address if there were resource clashes, though. And bugs always happen, especially since that code doesn't get much testing on x86 (there are almost never any interesting rom resources for _any_ device, and apparently the video device which is one of the few interesting ones always ends up using the shadow rom thing on x86 for the primary card). If you find the thing that causes us to re-assign the address, holler. (See drivers/pci/probe.c: pci_read_bases() for the code that probes the old address and saves it into the resource struct. It's called by pci_setup_device() from the device scanning routines). Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 5:03 ` Linus Torvalds @ 2005-08-30 6:40 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 6:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Greg KH, helgehaf On Mon, 2005-08-29 at 22:03 -0700, Linus Torvalds wrote: > > On Tue, 30 Aug 2005, Benjamin Herrenschmidt wrote: > > > > So what about fixing pci_map_rom() to call pcibios_resource_to_bus() and > > then write the resource back to the BAR ? I'm still a bit annoyed that > > we re-allocate the address while the original one was perfectly good > > (though not enabled) but the above would work. > > I just sent you a patch to try. > > Btw, as to the re-allocation of an existing address: most of the PCI layer > really does try to avoid re-allocating known good addresses. In fact, I > thought we did so for ROM resources too: at least pci_read_bases() does > read the ROM base, and saves it off into the resource structure. > > We'll end up re-assigning that saved-off-address if there were resource > clashes, though. And bugs always happen, especially since that code > doesn't get much testing on x86 (there are almost never any interesting > rom resources for _any_ device, and apparently the video device which is > one of the few interesting ones always ends up using the shadow rom thing > on x86 for the primary card). >From my experience, we tend to re-allocate a lot more than necessary. afaik, as soon as we hit a p2p bridge, we just blindly re-allocate everything in my experience. I have a lot of cases on ppc64 where calling pci_assign_unassigned_resources() will simply screw everything up and lead with a bus in a completely unuseable state, with things half relocated, conflicting bits, etc... Ok, that was about 2.6.12 timeframe, I never had time to fully debug that. Part of the problem was some assumptions about the existence of a prefetchable range in a given position in the resource array, or the kernel having a different idea than the firmware on where such things should go. I'll try to go back to it sooner or later. The problem on macs for example is that we can't afford to have some devices moved at all (like the Apple ASIC that contains the interrupt controller etc...). Those bits are probed & the drivers are setup before we touch the PCI bus, based on the open firmware device-tree. If we move them around, we are screwed. There are other issues with the fact that the PCI probe will temporarily cut access to thsoe devices during BAR sizing or bus numbering, thus if you take an interrupt at the wrong time, you are toast. Finally, on ppc64, we also have the case of partitioned machines where we aren't allowed to touch some busses at all (and the kernel really tries to reconfigure p2p bridges all the time). > If you find the thing that causes us to re-assign the address, holler. I'll try to find out. > (See drivers/pci/probe.c: pci_read_bases() for the code that probes the > old address and saves it into the resource struct. It's called by > pci_setup_device() from the device scanning routines). Yes I know that part, I'm not sure yet why it gets reallocated. I suppose paulus and I need to spend again some serious time on the PCI code. We have to anyway with the pending merge of ppc32 and ppc64 so it might be a good opportunity to try again getting the common code in drivers/pci/setup-* working for us. Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 2:38 ` Ignore disabled ROM resources at setup Benjamin Herrenschmidt 2005-08-30 3:15 ` Linus Torvalds 2005-08-30 3:19 ` Benjamin Herrenschmidt @ 2005-08-30 4:35 ` Jon Smirl 2005-08-30 5:32 ` David S. Miller 2 siblings, 1 reply; 24+ messages in thread From: Jon Smirl @ 2005-08-30 4:35 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Linux Kernel Mailing List, Greg KH, helgehaf, Linus Torvalds On 8/29/05, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > While looking there, I also noticed pci_map_rom_copy() stuff and I'm > surprised it was ever accepted in the tree. While I can understand that > we might need to keep a cached copy of the ROM content (due to cards > like matrox who can't enable both the ROM and the BARs among other > issues), the whole idea of whacking a kernel virtual pointer in the > struct resource->start of the ROM bar is just too disgusting for words > and will probably cause "intersting" side effects in /proc, sysfs and > others... Shouldn't we just have a pointer in pci_dev for the optional > "ROM cache" instead ? We should just delete the ROM copy stuff. It is there because the PCI spec allows for the ROM address decoder to be reused and the PCI people wanted it for completeness. It is legal to build a card that uses the address decoder to get at the ROM, then when the ROM was run it would set the same address decoder to decode other hardware on the card. You need to copy the ROM since once the decoder is changed you can't get to the ROM any more. As far as I can tell no one has built recent hardware this way. But I believe there are some old SCSI controllers that do this. I provided a ROM API for disabling sysfs access, if we identify one of these cards we should just add a call to it's driver to disable ROM access instead of bothering with the copy. Currently the copy is not being used anywhere in the kernel. -- Jon Smirl jonsmirl@gmail.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 4:35 ` Jon Smirl @ 2005-08-30 5:32 ` David S. Miller 2005-08-30 6:43 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 24+ messages in thread From: David S. Miller @ 2005-08-30 5:32 UTC (permalink / raw) To: jonsmirl; +Cc: benh, linux-kernel, greg, helgehaf, torvalds From: Jon Smirl <jonsmirl@gmail.com> Date: Tue, 30 Aug 2005 00:35:11 -0400 > As far as I can tell no one has built recent hardware this way. But I > believe there are some old SCSI controllers that do this. I provided a > ROM API for disabling sysfs access, if we identify one of these cards > we should just add a call to it's driver to disable ROM access instead > of bothering with the copy. Currently the copy is not being used > anywhere in the kernel. Qlogic ISP is one such card, but there are several others. I think enabling the ROM is a very bad idea, since we in fact know it disables the I/O and MEM space decoders on a non-empty set of PCI cards. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Ignore disabled ROM resources at setup 2005-08-30 5:32 ` David S. Miller @ 2005-08-30 6:43 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 24+ messages in thread From: Benjamin Herrenschmidt @ 2005-08-30 6:43 UTC (permalink / raw) To: David S. Miller; +Cc: jonsmirl, linux-kernel, greg, helgehaf, torvalds On Mon, 2005-08-29 at 22:32 -0700, David S. Miller wrote: > From: Jon Smirl <jonsmirl@gmail.com> > Date: Tue, 30 Aug 2005 00:35:11 -0400 > > > As far as I can tell no one has built recent hardware this way. But I > > believe there are some old SCSI controllers that do this. I provided a > > ROM API for disabling sysfs access, if we identify one of these cards > > we should just add a call to it's driver to disable ROM access instead > > of bothering with the copy. Currently the copy is not being used > > anywhere in the kernel. > > Qlogic ISP is one such card, but there are several others. > > I think enabling the ROM is a very bad idea, since we in fact > know it disables the I/O and MEM space decoders on a non-empty > set of PCI cards. This is why pci_map_rom is under driver control. There is still a potential issue with userland using the "rom" file in sysfs, which is why we probably should cleanup the pci_map_rom_copy to have the kernel read the ROM once & backstore it upon request from the driver so that further userland accesses will not toggle the real ROM enable. Ben. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2005-08-31 4:24 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <200508261859.j7QIxT0I016917@hera.kernel.org> 2005-08-30 2:38 ` Ignore disabled ROM resources at setup Benjamin Herrenschmidt 2005-08-30 3:15 ` Linus Torvalds 2005-08-30 4:47 ` Jon Smirl 2005-08-30 3:19 ` Benjamin Herrenschmidt 2005-08-30 3:52 ` Linus Torvalds 2005-08-30 4:09 ` Linus Torvalds 2005-08-30 4:20 ` David S. Miller 2005-08-30 4:37 ` Benjamin Herrenschmidt 2005-08-30 4:40 ` Linus Torvalds 2005-08-30 4:49 ` Benjamin Herrenschmidt 2005-08-30 5:29 ` Linus Torvalds 2005-08-30 6:46 ` Benjamin Herrenschmidt 2005-08-31 4:16 ` Benjamin Herrenschmidt 2005-08-30 4:51 ` Jon Smirl 2005-08-30 4:54 ` Benjamin Herrenschmidt 2005-08-30 5:15 ` Linus Torvalds 2005-08-30 14:39 ` Alan Cox 2005-08-30 15:29 ` Petr Vandrovec 2005-08-30 4:33 ` Benjamin Herrenschmidt 2005-08-30 5:03 ` Linus Torvalds 2005-08-30 6:40 ` Benjamin Herrenschmidt 2005-08-30 4:35 ` Jon Smirl 2005-08-30 5:32 ` David S. Miller 2005-08-30 6:43 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).