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

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2015/3/19 22:08, Bjorn Helgaas wrote:
> 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.
I will investigate about reverting.

> 
> 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.
Actually I prefer solution 2 above. The key idea is to decouple
IRQ resource allocation from pci_enabe/disable_device(), so irq
resource will be allocated just before driver binding and will
be released after driver unbinding.

One issue left is the way to hook driver binding/unbinding events.
Currently pcibios_enable/disable_irq() are x86 specific, so I use
PCI notification to hook driver binding/unbinding evetns.
If you are OK with introducing two new weak functions
pcibios_enable/disable_irq() into PCI core, that's obviously
a clear solution, easier to maintain and may benefit other platforms
too in future.

So should I introduce pcibios_enable/disable_irq() into PCI core?
Thanks!
Gerry

> 
> Does either of those make sense?
> 
> Bjorn
>