libpciaccess bogus bridge data - commit 2bda5b73
jbarnes at virtuousgeek.org
Mon Dec 6 12:18:09 PST 2010
On Mon, 6 Dec 2010 11:38:24 -0800
Bryce Harrington <bryce at canonical.com> wrote:
> On Mon, Dec 06, 2010 at 09:35:48AM -0800, Jesse Barnes wrote:
> > On Wed, 1 Dec 2010 18:54:22 -0800
> > Bryce Harrington <bryce at canonical.com> wrote:
> > > Jesse,
> > >
> > > I have a question about change 2bda5b73 that you did to libpciaccess
> > > last year, to stop pci_device_get_bridge_buses() from looking at bus
> > > data if the bridge data was not read.
> > >
> > > A ubuntu bug reporter says that this check is preventing his device from
> > > getting detected properly - see
> > >
> > > https://bugs.launchpad.net/ubuntu/+source/libpciaccess/+bug/681207
> > >
> > > Looking at the code I see that for case 0x04 there's already a check for
> > > NULL, where it attempts to re-read the bridge info, so presumably there
> > > are at least some situations where NULL bridge.pci is expected?
> > >
> > > What I'm wondering is if the check should be less strict in cases of
> > > multi-function cards, or if somewhere else in the code needs updated to
> > > read these devices properly.
> > The multifunction check is probably ok, but there should still be a
> > bridge above the device regardless, even if it's just a host bridge I
> > think, so we should figure out why that's not getting set...
> In the discussion on 681207, it seems the portion of the data structure
> being tested by this patch for null is always going to be null when it's
> cast. So the original reporter finds this check always results in the
> function exiting early, and suggests the patch should be reverted. What
> are your thoughts?
I remember adding that check for a reason; I ran into a case where it
was necessary. Unfortunately either there wasn't a bug open for it or
I didn't reference it in the commit, and I don't remember the details.
Looking at the code later on, the check at the top certainly seems
wrong; we should be filling in the bridge info later if needed. It
looks like Darren's commit came before mine and actually conflicts, but
we didn't catch it (I probably just did a blind rebase when I pushed my
change), so it's probably safe to just revert mine.
I'll do that now.
Jesse Barnes, Intel Open Source Technology Center
More information about the xorg-devel