lkml.org 
[lkml]   [2015]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding
On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> 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


\
 
 \ /
  Last update: 2015-03-19 15:21    [W:0.176 / U:0.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site