Re: Ignore disabled ROM resources at setup

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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.

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

> 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.

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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

Re: Ignore disabled ROM resources at setup

[David S. Miller]

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.

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

> 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.

Re: Ignore disabled ROM resources at setup

[Jon Smirl]

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

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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.

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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, &region, res);
+	pci_write_config_dword(pdev, pdev->rom_base_reg, region.start | PCI_ROM_ADDRESS_ENABLE);
 }
 
 /**

Re: Ignore disabled ROM resources at setup

[Jon Smirl]

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

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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, &region, 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)

Re: Ignore disabled ROM resources at setup

[Jon Smirl]

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, &region, 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

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

> 
> 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.
 

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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

Re: Ignore disabled ROM resources at setup

[Linus Torvalds]

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, &region, 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);
 }
 

Re: Ignore disabled ROM resources at setup

[David S. Miller]

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.

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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.

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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.

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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.

Re: Ignore disabled ROM resources at setup

[Alan Cox]

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.

Re: Ignore disabled ROM resources at setup

[Petr Vandrovec]

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

Re: Ignore disabled ROM resources at setup

[Benjamin Herrenschmidt]

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, &region, 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;
 		}
 	}