2006-11-06 12:40:18

by Conke Hu

[permalink] [raw]
Subject: [PATCH] add pci revision id to struct pci_dev

Hi all,
PCI revision id had better be added to struct pci_dev and
initialized in pci_scan_device.

Signed-off-by: Conke Hu <[email protected]>

-----
diff -Nur linux-2.6.19-rc4-git10.orig/drivers/pci/probe.c
linux-2.6.19-rc4-git10/drivers/pci/probe.c
--- linux-2.6.19-rc4-git10.orig/drivers/pci/probe.c 2006-11-06
19:38:43.000000000 +0800
+++ linux-2.6.19-rc4-git10/drivers/pci/probe.c 2006-11-06
19:41:17.000000000 +0800
@@ -785,6 +785,7 @@
u32 l;
u8 hdr_type;
int delay = 1;
+ u8 rev;

if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
return NULL;
@@ -813,6 +814,9 @@
if (pci_bus_read_config_byte(bus, devfn, PCI_HEADER_TYPE, &hdr_type))
return NULL;

+ if (pci_bus_read_config_byte(bus, devfn, PCI_REVISION_ID, &rev))
+ return NULL;
+
dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
if (!dev)
return NULL;
@@ -828,6 +832,7 @@
dev->device = (l >> 16) & 0xffff;
dev->cfg_size = pci_cfg_space_size(dev);
dev->error_state = pci_channel_io_normal;
+ dev->revision = rev;

/* Assume 32-bit PCI; let 64-bit PCI cards (which are far rarer)
set this higher, assuming the system even supports it. */
diff -Nur linux-2.6.19-rc4-git10.orig/include/linux/pci.h
linux-2.6.19-rc4-git10/include/linux/pci.h
--- linux-2.6.19-rc4-git10.orig/include/linux/pci.h 2006-11-06
19:39:07.000000000 +0800
+++ linux-2.6.19-rc4-git10/include/linux/pci.h 2006-11-06
19:41:57.000000000 +0800
@@ -123,6 +123,7 @@
unsigned short device;
unsigned short subsystem_vendor;
unsigned short subsystem_device;
+ u8 revision; /* PCI revision ID */
unsigned int class; /* 3 bytes: (base,sub,prog-if) */
u8 hdr_type; /* PCI header type (`multi'
flag masked out) */
u8 rom_base_reg; /* which config register
controls the ROM */


2006-11-06 12:46:37

by Arjan van de Ven

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

On Mon, 2006-11-06 at 20:40 +0800, Conke Hu wrote:
> Hi all,
> PCI revision id had better be added to struct pci_dev and
> initialized in pci_scan_device.
>
> Signed-off-by: Conke Hu <[email protected]>

Hi,

it's customary to use the email address from the copyright holder (eg
your employer, AMD) in the Signed-off-by line.

>
> -----
> diff -Nur linux-2.6.19-rc4-git10.orig/drivers/pci/probe.c
> linux-2.6.19-rc4-git10/drivers/pci/probe.c
> --- linux-2.6.19-rc4-git10.orig/drivers/pci/probe.c 2006-11-06
> 19:38:43.000000000 +0800

and your patch is word wrapped...

> +++ linux-2.6.19-rc4-git10/drivers/pci/probe.c 2006-11-06
> 19:41:17.000000000 +0800
> @@ -785,6 +785,7 @@
> u32 l;
> u8 hdr_type;
> int delay = 1;
> + u8 rev;
>
> if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
> return NULL;
> @@ -813,6 +814,9 @@
> if (pci_bus_read_config_byte(bus, devfn, PCI_HEADER_TYPE, &hdr_type))
> return NULL;
>
> + if (pci_bus_read_config_byte(bus, devfn, PCI_REVISION_ID, &rev))
> + return NULL;
> +
> dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
> if (!dev)
> return NULL;
> @@ -828,6 +832,7 @@
> dev->device = (l >> 16) & 0xffff;
> dev->cfg_size = pci_cfg_space_size(dev);
> dev->error_state = pci_channel_io_normal;
> + dev->revision = rev;
>
> /* Assume 32-bit PCI; let 64-bit PCI cards (which are far rarer)
> set this higher, assuming the system even supports it. */
> diff -Nur linux-2.6.19-rc4-git10.orig/include/linux/pci.h
> linux-2.6.19-rc4-git10/include/linux/pci.h
> --- linux-2.6.19-rc4-git10.orig/include/linux/pci.h 2006-11-06
> 19:39:07.000000000 +0800
> +++ linux-2.6.19-rc4-git10/include/linux/pci.h 2006-11-06
> 19:41:57.000000000 +0800
> @@ -123,6 +123,7 @@
> unsigned short device;
> unsigned short subsystem_vendor;
> unsigned short subsystem_device;
> + u8 revision; /* PCI revision ID */
> unsigned int class; /* 3 bytes: (base,sub,prog-if) */
> u8 hdr_type; /* PCI header type (`multi'
> flag masked out) */

pretty badly in fact.


can you resend it without the word wrappings ?

It looks good to me otherwise....


2006-11-06 13:23:32

by Alan

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

Ar Llu, 2006-11-06 am 20:40 +0800, ysgrifennodd Conke Hu:
> Hi all,
> PCI revision id had better be added to struct pci_dev and
> initialized in pci_scan_device.

You can read the revision any time you like, we don't need to cache a
copy as we don't reference it very often

2006-11-06 14:10:34

by Conke Hu

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

On 11/6/06, Alan Cox <[email protected]> wrote:
> Ar Llu, 2006-11-06 am 20:40 +0800, ysgrifennodd Conke Hu:
> > Hi all,
> > PCI revision id had better be added to struct pci_dev and
> > initialized in pci_scan_device.
>
> You can read the revision any time you like, we don't need to cache a
> copy as we don't reference it very often
>
>

I've searched the kernel soruce code and it seems that the revision id
is widely used in pci drivers.

2006-11-06 14:19:22

by Arjan van de Ven

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

On Mon, 2006-11-06 at 13:28 +0000, Alan Cox wrote:
> Ar Llu, 2006-11-06 am 20:40 +0800, ysgrifennodd Conke Hu:
> > Hi all,
> > PCI revision id had better be added to struct pci_dev and
> > initialized in pci_scan_device.
>
> You can read the revision any time you like, we don't need to cache a
> copy as we don't reference it very often

one consideration is that if you read it from the hw you can't actually
fix it up in quirks at all, so it might make a lot of sense to just also
store it in the pci device struct, it's a very logical thing esp since
the pci device/vendor ids are stored there too (and those you can also
read from the hw if you want ;)

--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

2006-11-06 14:27:26

by Alan

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

Ar Llu, 2006-11-06 am 07:10 -0700, ysgrifennodd Conke Hu:
> > You can read the revision any time you like, we don't need to cache a
> > copy as we don't reference it very often
> I've searched the kernel soruce code and it seems that the revision id
> is widely used in pci drivers.

It is not however regularly and continually accessed - it doesn't need
caching.

Alan

2006-11-06 14:30:34

by Alan

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

Ar Llu, 2006-11-06 am 15:19 +0100, ysgrifennodd Arjan van de Ven:
> store it in the pci device struct, it's a very logical thing esp since
> the pci device/vendor ids are stored there too (and those you can also
> read from the hw if you want ;)

We need those cached because we iterate them on every PCI device match
and that is both regularly accessed and performance relevant. We also
need to cache them for the removed device case, unlike revision id.

>

2006-11-06 21:01:35

by Luca Tettamanti

[permalink] [raw]
Subject: Re: [PATCH] add pci revision id to struct pci_dev

Conke Hu <[email protected]> ha scritto:
> Hi all,
> PCI revision id had better be added to struct pci_dev and
> initialized in pci_scan_device.
[...]
> diff -Nur linux-2.6.19-rc4-git10.orig/include/linux/pci.h
> linux-2.6.19-rc4-git10/include/linux/pci.h
> --- linux-2.6.19-rc4-git10.orig/include/linux/pci.h 2006-11-06
> 19:39:07.000000000 +0800
> +++ linux-2.6.19-rc4-git10/include/linux/pci.h 2006-11-06
> 19:41:57.000000000 +0800
> @@ -123,6 +123,7 @@
> unsigned short device;
> unsigned short subsystem_vendor;
> unsigned short subsystem_device;
> + u8 revision; /* PCI revision ID */
> unsigned int class; /* 3 bytes: (base,sub,prog-if) */
> u8 hdr_type; /* PCI header type (`multi'
> flag masked out) */
> u8 rom_base_reg; /* which config register
> controls the ROM */

Hi,
I've noticed that after the 'class' field there are 3 u8 fields. If you
add the revision there then sizeof(struct pci_dev) stays constant:

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 09be0f8..c8586b7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -124,6 +124,7 @@ struct pci_dev {
unsigned short subsystem_vendor;
unsigned short subsystem_device;
unsigned int class; /* 3 bytes: (base,sub,prog-if) */
+ u8 revision; /* PCI revision id */
u8 hdr_type; /* PCI header type (`multi' flag masked out) */
u8 rom_base_reg; /* which config register controls the ROM */
u8 pin; /* which interrupt pin this device uses */


Luca
--
E' bene ricordare che l'intero Universo ? formato,
con un'unica trascurabile eccezione, dagli "altri".
John Andrew Holmes