[PATCH] add pci revision id to struct pci_dev

[PATCH] add pci revision id to struct pci_dev

[Conke Hu]

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

    Signed-off-by: Conke Hu <conke.hu@gmail.com>

-----
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 */

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

[Arjan van de Ven]

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 <conke.hu@gmail.com>

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

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

[Alan Cox]

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

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

[Conke Hu]

On 11/6/06, Alan Cox <alan@lxorguk.ukuu.org.uk> 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.

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

[Arjan van de Ven]

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

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

[Alan Cox]

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

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

[Alan Cox]

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.

> 

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

[Luca Tettamanti]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF8, Size: 1700 bytes --]

Conke Hu <conke.hu@gmail.com> 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