PCI device header type doesn't include bit7

Hi chihpinwu, thanks for reporting this issue. I don't think there's been a lot of testing with multi-function cards so this may have gone untested. I'm a bit uncertain how this should be fixed - would you be willing to suggest a patch?

Bryce,

Here is how I fixed it. Please search for "~cpw" to find my comments
(three of them). I stumbled across the bug with a chipset whose PCI
bridge is a multifunction device.

Best regards,
Chih-Pin Wu

*************************************************************
Extracted from common_bridge.c, libpciaccess 0.12.0

int
pci_device_get_bridge_buses(struct pci_device * dev, int *primary_bus,
       int *secondary_bus, int *subordinate_bus)
{
    struct pci_device_private * priv = (struct pci_device_private *)
dev;

    /* If the device isn't a bridge, return an error.
     */

    if (((dev->device_class >> 16) & 0x0ff) != 0x06) {
 return ENODEV;
    }
//~cpw Removed the following three statements
// priv->bridge.pci is invalid since it is initialized to null;
// As a result, the function always returns ENODEV.
//
// if (!priv->bridge.pci) {
// return ENODEV;
// }

    switch ((dev->device_class >> 8) & 0x0ff) {
    case 0x00:
 /* What to do for host bridges? I'm pretty sure this isn't
right.
  */
 *primary_bus = dev->bus;
 *secondary_bus = -1;
 *subordinate_bus = -1;
 break;

    case 0x01:
    case 0x02:
    case 0x03:
 *primary_bus = dev->bus;
 *secondary_bus = -1;
 *subordinate_bus = -1;
 break;

    case 0x04:
    if (priv->bridge.pci == NULL)
        read_bridge_info(priv);

//~cpw Mask bit7 of header type since it is a multi-function device
// indicator; Bridge itself could be a multi-function device and
// the comparison should be against 0x01 and 0x81
// if (priv->header_type == 0x01) {
    if ((priv->header_type & 0x7f) == 0x01) {
 *primary_bus = priv->bridge.pci->primary_bus;
 *secondary_bus = priv->bridge.pci->secondary_bus;
 *subordinate_bus = priv->bridge.pci->subordinate_bus;
    } else {
 *primary_bus = dev->bus;
 *secondary_bus = -1;
 *subordinate_bus = -1;
    }
 break;

    case 0x07:
    if (priv->bridge.pcmcia == NULL)
        read_bridge_info(priv);
//~cpw Mask bit7 of header type since it is a multi-function device
// indicator; Bridge itself could be a multi-function device and
// the comparison should be against 0x02 and 0x82
// if (priv->header_type == 0x02) {
    if ((priv->header_type & 0x7f) == 0x02) {
 *primary_bus = priv->bridge.pcmcia->primary_bus;
 *secondary_bus = priv->bridge.pcmcia->card_bus;
 *subordinate_bus = priv->bridge.pcmcia->subordinate_bus;
    } else {
 *primary_bus = dev->bus;
 *secondary_bus = -1;
 *subordinate_bus = -1;
    }
 break;
    }

    return 0;
}

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of
Bryce Harrington
Sent: Friday, November 26, 2010 4:52 PM
To: Chihpin Wu
Subject: [Bug 681207] Re: PCI device header type doesn't include bit7

Hi chihpinwu, thanks for reporting this issue. I don't think there's
been a lot of testing with multi-function cards so this may have gone
untested. I'm a bit uncertain how this should be fixed - would you be
willing to suggest a patch?

** Changed in: libpciaccess (Ubuntu)
       Status: New => Triaged

** Changed in: libpciaccess (Ubuntu)
   Importance: Undecided => Medium

** Changed in: libpciaccess (Ubuntu)
     Assignee: (unassigned) => Bryce Harringt...

This bug was fixed in the package libpciaccess - 0.12.0-1ubuntu1

---------------
libpciaccess (0.12.0-1ubuntu1) natty; urgency=low

  * Add 100_multifunction.patch: For multi-function cards, need to
    mask bit 7 in the bridge pci header to determine what type of bus
    it is. Also revert upstream 2bda5b73 as it causes the function
    to bail out early unnecessarily for these cards.
    (LP: #681207)
  * Restore patch system
 -- Bryce Harrington <email address hidden> Wed, 01 Dec 2010 18:13:35 -0800

Chih-Pin Wu thank you, I've incorporated your change into Ubuntu.

I've also forwarded the patch upstream. I left out the removal of the bridge.pci since it appears this was deliberately added; instead I've emailed Jesse Barnes for clarification as to why that was added. I may follow up on this bug report later depending on his advice.

Thanks, Bryce.

About the bridge.pci pointer, as it stands the function will not go pass
that pointer checking and always return with ENODEV. You see priv is a
local variable (structure) and only its first member "base", which is a
pci_device structure, gets filled in with valid data from caller by type
casting (from pci_device* to pci_device_priv). The union member
"bridge", which is a pointer, is not part of the pci_device structure.
As such, the value will be null every time the function is called. So
without fixing this pointer checking thing, the code below it will never
get executed.

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of
Bryce Harrington
Sent: Wednesday, December 01, 2010 6:57 PM
To: Chihpin Wu
Subject: [Bug 681207] Re: PCI device header type doesn't include bit7

Chih-Pin Wu thank you, I've incorporated your change into Ubuntu.

I've also forwarded the patch upstream. I left out the removal of the
bridge.pci since it appears this was deliberately added; instead I've
emailed Jesse Barnes for clarification as to why that was added. I may
follow up on this bug report later depending on his advice.

--
You received this bug notification because you are a direct subscriber
of the bug.
https://bugs.launchpad.net/bugs/681207

Title:
  PCI device header type doesn't include bit7

Status in "libpciaccess" package in Ubuntu:
  Fix Released

Bug description:
  Within the function pci_device_get_bridge_buses() in file
common_bridge.c, "priiv->header_type" is used to detect the bridge type
without bit 7 (indicating multi-function or not) stripped. As a result
the detection would always fail for multi-function card. I see this
error in both versions 0.10.6 and 0.12.0.

Beside this problem, version 0.12.0 has a pointer check
"priv->bridge.pci" upon fuction entrance and just return "ENODEV" if it
is null. The problem is that the pointer never got initialized in the
first place and will always be null. This checking should be removed.

To unsubscribe from this bug, go to:
https://bugs.launchpad.net/ubuntu/+source/libpciaccess/+bug/681207/+subs
cribe

On Thu, Dec 02, 2010 at 03:34:40AM -0000, Chih-Pin Wu wrote:
> Thanks, Bryce.
>
> About the bridge.pci pointer, as it stands the function will not go pass
> that pointer checking and always return with ENODEV. You see priv is a
> local variable (structure) and only its first member "base", which is a
> pci_device structure, gets filled in with valid data from caller by type
> casting (from pci_device* to pci_device_priv). The union member
> "bridge", which is a pointer, is not part of the pci_device structure.
> As such, the value will be null every time the function is called. So
> without fixing this pointer checking thing, the code below it will never
> get executed.

Ah, thanks for the added explanation, yes that makes sense.

The reason I left that bit out of the patch I sent upstream is because
there was a specific commit that added it:

http://cgit.freedesktop.org/xorg/lib/libpciaccess/commit/?id=2bda5b733bb12854760750c08138db95e77aea0c

So it's a conceptually distinct change and I figure upstream will prefer
to consider that separate from the bit mask change. Also, because it
looks like that change would best be implemented upstream via a revert.