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-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
