lkml.org 
[lkml]   [2009]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 07/11] ACPI: acpi_pci_unbind should clean up properly after acpi_pci_bind
* Bjorn Helgaas <bjorn.helgaas@hp.com>:
> On Thursday 04 June 2009 05:35:21 pm Alex Chiang wrote:
> > * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> > >
> > > I have a concern about this change.
> > >
> > > The acpi_pci_irq_del_prt() against dev->bus removes not only
> > > the _PRT entries for PCI function corresponding to specified
> > > acpi_device, but also other _PRT entries for working PCI
> > > devices/functions on the same bus. As a result, interrupt
> > > initialization for those PCI functions would no longer work
> > > properly after that.
> > >
> > > So I think we should not call acpi_pci_irq_del_prt() against
> > > dev->bus.
> >
> > Thanks for the review. I agree with you.
>
> I agree that this respun version makes things more the way they were,
> so in that sense, it should do no harm. But I still have the niggling
> concern that .bind() adds _PRT info for non-bridges, and there's no
> corresponding removal. There should be some path that makes this
> more symmetric.

The comment in acpi_pci_bind doesn't seem to know if you can even
have a _PRT for non-bridges. The spec (3.0b) says that _PRT is
required for all root bridges, but doesn't mention anything about
non-bridge devices.

My gut feeling is the way to cure the symmetry is to remove the
path in .bind() that adds _PRT for non-bridges, but I'm a little
hesitant to do that without knowing for sure.

I think for this patch, the Hippocratic approach is the right
one for now.

Thanks.

/ac

> > Here is a respun version of this patch.
> >
> > From: Alex Chiang <achiang@hp.com>
> >
> > ACPI: acpi_pci_unbind should clean up properly after acpi_pci_bind
> >
> > In acpi_pci_bind, we set device->ops.bind and device->ops.unbind, but
> > never clear them out.
> >
> > Cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > Signed-off-by: Alex Chiang <achiang@hp.com>
> > ---
> > diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
> > index 62cb383..c9cc650 100644
> > --- a/drivers/acpi/pci_bind.c
> > +++ b/drivers/acpi/pci_bind.c
> > @@ -109,11 +109,13 @@ static int acpi_pci_unbind(struct acpi_device *device)
> > struct pci_dev *dev;
> >
> > dev = acpi_get_pci_dev(device->handle);
> > - if (!dev)
> > + if (!dev || !dev->subordinate)
> > return 0;
> >
> > - if (dev->subordinate)
> > - acpi_pci_irq_del_prt(dev->subordinate);
> > + acpi_pci_irq_del_prt(dev->subordinate);
> > +
> > + device->ops.bind = NULL;
> > + device->ops.unbind = NULL;
> >
> > pci_dev_put(dev);
> > return 0;
> >
>
>


\
 
 \ /
  Last update: 2009-06-05 18:03    [W:0.045 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site