Re: [PATCH v2 2/7] PCI: don't touch enable_cnt in pci_device_shutdown()

[Khalid Aziz <khalid@gonehiking.org>]

On Tue, 2013-02-05 at 12:22 -0700, Bjorn Helgaas wrote:
> On Tue, Feb 5, 2013 at 8:28 AM, Khalid Aziz <khalid@gonehiking.org> wrote:
> > On Mon, 2013-02-04 at 16:13 -0700, Bjorn Helgaas wrote:
> >> Did you figure out specifically why pcibios_disable_device() helps?
> >> Using pcibios_disable_device() doesn't seem like the ideal solution
> >> because on most architectures, it is an empty function with no obvious
> >> connection to IRQs.  On x86 with ACPI, it cleans up some ACPI PCI IRQ
> >> stuff, but as far as I can tell, it doesn't actually touch the PCI
> >> device itself or even the IOAPIC to which it's connected, so I'm not
> >> sure how this would help kexec.
> >
> > My reading of the code was that pcibios_disable_device() does clear the
> > interrupt on x86 and ia64. I am not deeply familiar with the ACPI code
> > and I might be interpreting it incorrectly, so please do correct me if I
> > am reading it incorrectly. Here is the code sequence I see:
> >
> > pcibios_disable_device() ->
> >    pcibios_disable_irq() ->
> >        acpi_pci_irq_disable() ->
> >            acpi_pci_link_free_irq() ->
> >               acpi_evaluate_object(link->device->handle, "_DIS", NULL,
> > NULL);
> >
> > My understanding is the evaluation of ACPI _DIS method will disable the
> > interrupt from the device. Does that sound reasonable?
> 
> I see the code you're looking at in acpi_pci_link_free_irq(), but we
> only evaluate _DIS if link->refcnt == 0, and I don't think refcnt is
> ever zero at that point.
> 
> refcnt starts out at zero in acpi_pci_link_add() (called when we find
> PNP0C0F devices), and it's incremented in acpi_pci_link_allocate_irq()
> (called in the pci_enable_device() path), but as far as I can tell,
> it's never decremented, so I doubt that _DIS is ever evaluated.

Ah, that is interesting. I was assuming as we disable PCI devices, the
refcnt would have been decremented and if no one was using the IRQ, we
would evaluate _DIS method and disable the interrupt link.

> 
> If we did evaluate _DIS, it would act on an "interrupt link" device,
> not on the PCI device itself.  

Right, it should be the shutdown() routine for the device driver that
disables interrupt on the device itself. We want to turn interrupt off
one level higher in pci_device_shutdown().

> I guess that could help, but only for
> devices connected to such a link device.  For others, I guess we might
> be able to accomplish something similar by updating local APIC and/or
> IOAPIC config.  I don't think we do that today, at least not in the
> pci_disable_device() path, but it might be something interesting to
> explore.  There is also the INTx Disable bit, though it's obviously
> only on new PCI devices.

Turning interrupt of at local APIC or IOAPIC level sounds like more
reliable thing to do.

> 
> > ... The two ways I can think of are to stop
> > DMA by clearing Bus Master bit and turn off the interrupt, which have
> > been shown to get kexec (and thus kdump) working on machines it didn't
> > work on before.
> 
> I was just curious if you had actually verified that _DIS was being
> evaluated and making a difference here, or if the Bus Master bit was
> really the important part.
> 
> Bjorn

I have been able to reproduce the kexec problem caused by active PCI
devices only in limited cases and in those cases it was really the Bus
Master bit that was important. Keeping interrupts from errant devices
from reaching the kernel until newly kexc'd kernel is initialized is
additional safety measure.

--
Khalid