Re: [PATCH v2 07/11] ACPI: acpi_pci_unbind should clean up properly after acpi_pci_bind

[Alex Chiang <achiang@hp.com>]

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