2001-02-07 19:18:23

by Dave Jones

[permalink] [raw]
Subject: [PATCH] Hamachi not doing pci_enable before reading resources


Hi Alan,

Another driver not doing pci_enable_device() early enough.

Dave.

--
| Dave Jones. http://www.suse.de/~davej
| SuSE Labs

diff -urN --exclude-from=/home/davej/.exclude linux/drivers/net/hamachi.c linux-dj/drivers/net/hamachi.c
--- linux/drivers/net/hamachi.c Wed Feb 7 12:42:39 2001
+++ linux-dj/drivers/net/hamachi.c Wed Feb 7 19:09:31 2001
@@ -562,13 +562,14 @@
if (hamachi_debug > 0 && did_version++ == 0)
printk(version);

+ if (pci_enable_device(pdev))
+ return -EIO;
+
ioaddr = pci_resource_start(pdev, 0);
#ifdef __alpha__ /* Really "64 bit addrs" */
ioaddr |= (pci_resource_start(pdev, 1) << 32);
#endif

- if (pci_enable_device(pdev))
- return -EIO;
pci_set_master(pdev);

ioaddr = (long) ioremap(ioaddr, 0x400);



2001-02-07 19:25:03

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources

Applied locally: pci_device_enable() cleanups for hamachi, eepro100,
starfire

I'll have to look at ne2k-pci, I think a patch in -ac may be spurious.

Jeff


--
Jeff Garzik | "You see, in this world there's two kinds of
Building 1024 | people, my friend: Those with loaded guns
MandrakeSoft | and those who dig. You dig." --Blondie

2001-02-07 19:44:43

by Dave Jones

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources


Jeff Garzik wrote..

> rejected -- 'irq' assigned a value before pci_enable_device called.
> better patch installed locally.

Ugh, yep missed that one.
Will look more carefully for those assignments.

Dave.

--
| Dave Jones. http://www.suse.de/~davej
| SuSE Labs

2001-02-07 19:52:13

by Richard B. Johnson

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources

On Wed, 7 Feb 2001 [email protected] wrote:

>
> Hi Alan,
>
> Another driver not doing pci_enable_device() early enough.
>
> Dave.
>

A PCI device does not and should not be enabled to probe for resources!
It is only devices that have BIOS that require the device to be enabled
for memory I/O prior to downloading the BIOS into RAM. The BARs are
read/writable (and are required to be), even when the Mem/I/O bits
in the cmd/status register are clear.

This is a required condition! You certainly don't want to write all
ones to a decode (to find the resource length) of a live, on-line chip!
If the chip hickups (think network chips connected to networks, on a
warm-boot), you will trash lots of stuff in memory.

It looks as though you are "fixing" drivers that are not broken and,
in fact, are trying to do the right thing. Maybe the PCI code in the
kernel is preventing access to resources unless the device has been
enabled??? If so, it's broken and should be fixed, instead of all
the drivers.

Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

"Memory is like gasoline. You use it up when you are running. Of
course you get it all back when you reboot..."; Actual explanation
obtained from the Micro$oft help desk.


2001-02-07 20:06:43

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources

"Richard B. Johnson" wrote:
> A PCI device does not and should not be enabled to probe for resources!

Some PCI devices do not -have- resources until pci_enable_device() is
called, hence the rule.


> It is only devices that have BIOS that require the device to be enabled
> for memory I/O prior to downloading the BIOS into RAM. The BARs are
> read/writable (and are required to be), even when the Mem/I/O bits
> in the cmd/status register are clear.
>
> This is a required condition! You certainly don't want to write all
> ones to a decode (to find the resource length) of a live, on-line chip!
> If the chip hickups (think network chips connected to networks, on a
> warm-boot), you will trash lots of stuff in memory.

When writing 0xFFFFFFFF to a BAR to find its length, you must disable IO
and MEM decoding. This is a ideally what we should be doing anyway..
But you can re-enabling decoding once region size detection is
complete. AND. Region sizing only occurs once, and the value is cached
in dev->resource[], so it should not be occurring all over again, even
if pci_enable_device() is called.


> It looks as though you are "fixing" drivers that are not broken and,
> in fact, are trying to do the right thing. Maybe the PCI code in the
> kernel is preventing access to resources unless the device has been
> enabled??? If so, it's broken and should be fixed, instead of all
> the drivers.

wrong, you just missed this new "rule"...

Jeff



--
Jeff Garzik | "You see, in this world there's two kinds of
Building 1024 | people, my friend: Those with loaded guns
MandrakeSoft | and those who dig. You dig." --Blondie

2001-02-07 20:31:24

by Richard B. Johnson

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources

On Wed, 7 Feb 2001, Jeff Garzik wrote:

> "Richard B. Johnson" wrote:
> > A PCI device does not and should not be enabled to probe for resources!
>
> Some PCI devices do not -have- resources until pci_enable_device() is
> called, hence the rule.
>

I stand by my statement. PCI devices that require resources are
required to provide read/write registers indicating these resources
whether or not the enable bits are set. This is mandatory.


Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

"Memory is like gasoline. You use it up when you are running. Of
course you get it all back when you reboot..."; Actual explanation
obtained from the Micro$oft help desk.


2001-02-07 20:44:06

by Alan Cox

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources

> I stand by my statement. PCI devices that require resources are
> required to provide read/write registers indicating these resources
> whether or not the enable bits are set. This is mandatory.

And the assignment of those resources is done by pci_enable_device. So
looking at the irq data or the BAR assignment until pci_enable_device
has done its work doesnt tell you anything meaningful

It doesnt just flip the IO and MEM bits

2001-02-07 21:55:44

by Gérard Roudier

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources


You missed the newer statements about every piece of hardware being
assumed to be hot-pluggable and all the hardware being under full control
by CPU.

You also missed the well known point that only device drivers are broken
under Linux and that all the generic O/S code is just perfect. :-)

G?rard.

On Wed, 7 Feb 2001, Richard B. Johnson wrote:

> On Wed, 7 Feb 2001 [email protected] wrote:
>
> >
> > Hi Alan,
> >
> > Another driver not doing pci_enable_device() early enough.
> >
> > Dave.
> >
>
> A PCI device does not and should not be enabled to probe for resources!
> It is only devices that have BIOS that require the device to be enabled
> for memory I/O prior to downloading the BIOS into RAM. The BARs are
> read/writable (and are required to be), even when the Mem/I/O bits
> in the cmd/status register are clear.
>
> This is a required condition! You certainly don't want to write all
> ones to a decode (to find the resource length) of a live, on-line chip!
> If the chip hickups (think network chips connected to networks, on a
> warm-boot), you will trash lots of stuff in memory.
>
> It looks as though you are "fixing" drivers that are not broken and,
> in fact, are trying to do the right thing. Maybe the PCI code in the
> kernel is preventing access to resources unless the device has been
> enabled??? If so, it's broken and should be fixed, instead of all
> the drivers.
>
> Cheers,
> Dick Johnson
>
> Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
>
> "Memory is like gasoline. You use it up when you are running. Of
> course you get it all back when you reboot..."; Actual explanation
> obtained from the Micro$oft help desk.
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> Please read the FAQ at http://www.tux.org/lkml/
>

2001-02-07 22:19:29

by Richard B. Johnson

[permalink] [raw]
Subject: Re: [PATCH] Hamachi not doing pci_enable before reading resources

On Wed, 7 Feb 2001, [ISO-8859-1] G?rard Roudier wrote:

>
> You missed the newer statements about every piece of hardware being
> assumed to be hot-pluggable and all the hardware being under full control
> by CPU.
>
> You also missed the well known point that only device drivers are broken
> under Linux and that all the generic O/S code is just perfect. :-)
>
> G?rard.
>

Yep. I missed a lot. When the next 'release' comes out, I'll have
to rewrite all my drivers again --sigh...

Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

"Memory is like gasoline. You use it up when you are running. Of
course you get it all back when you reboot..."; Actual explanation
obtained from the Micro$oft help desk.