Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Bjorn Helgaas]

On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
>> On 2015/3/19 6:11, Bjorn Helgaas wrote:
>> > On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
>> >> To support IOAPIC hot-removal, we need to release PCI interrupt resource
>> >> when unbinding PCI device driver. But due to historical reason,
>> >> /*
>> >>  * We would love to complain here if pci_dev->is_enabled is set, that
>> >>  * the driver should have called pci_disable_device(), but the
>> >>  * unfortunate fact is there are too many odd BIOS and bridge setups
>> >>  * that don't like drivers doing that all of the time.
>> >>  * Oh well, we can dream of sane hardware when we sleep, no matter how
>> >>  * horrible the crap we have to deal with is when we are awake...
>> >>  */
>> >
>> > Quoting the comment here (especially the last two lines) is overkill and
>> > obscures the real point.  The important thing is that some drivers have
>> > legitimate reasons for not calling pci_disable_device().
>> Hi Bjorn,
>>       Thanks for review. I will rewrite the commit message.
>> >> some drivers don't call pci_disable_device() when unloading, which
>> >> prevents us from reallocating PCI interrupt resource on reloading
>> >> PCI driver and causes regressions.
>> >
>> > This isn't very clear.  I can believe that "drivers not calling
>> > pci_disable_device()" means we don't release IRQ resources, which might
>> > prevent you from hot-removing an IOAPIC.
>> >
>> > But "drivers not calling pci_disable_device()" doesn't cause regressions.
>> >
>> >> So release PCI interrupt resource only if PCI device is disabled when
>> >> unbinding. By this way, we could support IOAPIC hot-removal on latest
>> >> platforms and avoid regressions on old platforms.
>> >
>> > Does this mean you can only hot-remove IOAPICs if all drivers for devices
>> > using the IOAPIC call pci_disable_device()?  If so, it seems sort of
>> > dubious that we have to rely on drivers for that.
>> This is a quickfix for v4.0 merging window. We will try to solve this
>> issue for next merging window.
>
> If that is the plan, then I'd rather revert the offending commit and try
> again in the next cycle.
>
> Bjorn, what do you think?

I don't know how hard it is to just revert that one commit at this
point, but I would be in favor of doing that if it's feasible.

We're headed toward a real morass of changelogs for a design that
seems destined for overhaul.  That makes it really hard to backport
and rework things later.

>From the revised changelog:

>> When suspending, PCI
>> device driver may call pci_disable_device() and eventually release
>> IOAPIC pin. When resuming, PCI device driver call pci_enable_device()
>> and reallocating IOAPIC pin. Since v3.19, IOAPIC driver dynamically
>> allocates IRQ number for IOAPIC pin. So when resuming, a different
>> IRQ number may assigned, which breaks some PCI drivers' suspend/resume
>> implementation.

It seems like you're really standing on your head to make this
situation work, and I think the result is too complicated and
error-prone.  One test is to see whether you can write a short, simple
description of how driver writers need to manage IRQs with respect to
probe/remove/suspend/remove.

There are two other possibilities I can see:

1) Decide that a driver that captures the IRQ and then calls
pci_enable_device() is just broken, and fix those drivers to
re-capture the IRQ every time they call pci_enable_device().  I assume
you've looked at this already and concluded it's not practical?

2) Configure the IRQ in pci_device_probe().  Then it would be
configured before the driver sees the device, and you could dispose of
it in pci_device_remove() when the driver is unbound.

Does either of those make sense?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/