2001-02-01 15:06:55

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] minor ne2k-pci irq fix

On Thu, 1 Feb 2001, Martin Diehl wrote:
> On Wed, 31 Jan 2001 [email protected] wrote:
> > I think it would be better to move the pci_enable_device(pdev);
> > above all this, as we should enable the device before reading the
> > pdev->resource[] too iirc.

Agreed.


> Probably I've missed this because the last time I hit such a thing was
> when my ob800 bios mapped the cardbus memory BAR's into bogus legacy
> 0xe0000 area. Hence there was good reason to read and correct this before
> trying to enable the device.

This is a PCI fixup, the driver shouldn't have to worry about this..


> BTW, will it ever happen the kernel starts remapping BAR's when enabling
> devices?

huh? The two steps do not occur simultaneously. The enabling should
occur first, at which point the BARs should be useable. The remapping
occurs after that. If the BARs are not usable after remapping, that is
a PCI quirk that needs to be added to the list [probably].

Jeff




2001-02-02 16:48:14

by Martin Diehl

[permalink] [raw]
Subject: Re: [PATCH] minor ne2k-pci irq fix


(apologies in case anybody should get this twice - was catched by the DUL
blocker again. Seems time to change my mail routing anyway...)

On Thu, 1 Feb 2001, Jeff Garzik wrote:

> > Probably I've missed this because the last time I hit such a thing was
> > when my ob800 bios mapped the cardbus memory BAR's into bogus legacy
> > 0xe0000 area. Hence there was good reason to read and correct this before
> > trying to enable the device.
>
> This is a PCI fixup, the driver shouldn't have to worry about this..

Agreed. Point was when I discovered the broken BAR location the BIOS had
set, it was at late 2.4.0-test12. So I prefered a simple fix in the yenta
driver without touching other stuff like PCI, just in case Linus would
have liked it for 2.4.

> > BTW, will it ever happen the kernel starts remapping BAR's when enabling
> > devices?
>
> huh? The two steps do not occur simultaneously. The enabling should
> occur first, at which point the BARs should be useable. The remapping
> occurs after that. If the BARs are not usable after remapping, that is
> a PCI quirk that needs to be added to the list [probably].

Sorry, wasn't clear enough. I've meant, the kernel (PCI stuff) changing
the BAR bus address in the config space when enabling the device (i.e.
the bus address value which is used for later mapping). Doing so would
make the pci_resource_start() value bogus (when obtained before enabling
the device) - even without accessing/ioremap() it.
My guess is this might happen, but I'm not sure when. Probably if our PCI
stuff assigned another BAR without inital bus address to overlap with
what the BIOS suggested for some initially disabled BAR. Or for real PCI
hotplugging in general.

Just to understand it's more than a cosmetical bug if a driver saves this
before the device is up...

Martin

2001-02-02 17:06:06

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] minor ne2k-pci irq fix

On Fri, 2 Feb 2001, Martin Diehl wrote:
> Sorry, wasn't clear enough. I've meant, the kernel (PCI stuff) changing
> the BAR bus address in the config space when enabling the device (i.e.
> the bus address value which is used for later mapping). Doing so would
> make the pci_resource_start() value bogus (when obtained before enabling
> the device) - even without accessing/ioremap() it.

The pci_resource_start() value is only bogus if the driver is changing
the BAR value -- which it should never do. Enabling the device could
indeed change the BAR address... that's why pci_enable_device must
ALWAYS be called before reading pci_dev->irq and pci_resource_start()
values.

Jeff



2001-02-03 20:21:15

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] minor ne2k-pci irq fix



On Thu, 1 Feb 2001, Jeff Garzik wrote:
>
> > Probably I've missed this because the last time I hit such a thing was
> > when my ob800 bios mapped the cardbus memory BAR's into bogus legacy
> > 0xe0000 area. Hence there was good reason to read and correct this before
> > trying to enable the device.
>
> This is a PCI fixup, the driver shouldn't have to worry about this..

Actually, I'd rather see the _drivers_ do most of the fixups for their own
chips, and leave the global PCI fixups for things like

- PCI/ISA/whatever bridges that affect drivers for _other_ chips. I hate
having some random PCI driver having to know about the fact that it
might be behind a bridge that needs special initialization. That kind
of "non-local" knowledge is that the PCI fixups are there for.

- stuff that needs to be fixed up early in order to have a working system
and make sure that we don't have any resource clashes we can't fix up
later on.

But if there is a BIOS/chip bug that affects only one driver, and that bug
is local to that driver only and can't affect anything else, then I'd
rather see the driver keep track of it. It's easy enough for a driver to
do any required fixup before it actually calls "pci_enable_device()".

Linus