On Friday, July 30, 2010, Kenji Kaneshige wrote: > (2010/07/30 0:45), Rafael J. Wysocki wrote: > > On Thursday, July 29, 2010, Kenji Kaneshige wrote: > >> (2010/07/29 6:43), Jesse Barnes wrote: > >>> On Wed, 28 Jul 2010 23:23:56 +0200 > >>> "Rafael J. Wysocki"<rjw@xxxxxxx> wrote: > >>> > >>>> From: Rafael J. Wysocki<rjw@xxxxxxx> > >>>> > >>>> PCIe port service drivers ask the BIOS, through _OSC, for control of > >>>> the services they handle. Unfortunately, each of them individually > >>>> asks for control of the PCIe capability structure and if that is > >>>> granted, some BIOSes expect that the other PCIe port services will be > >>>> configured and handled by the kernel as well. If that is not the > >>>> case (eg. one of the PCIe port service drivers is not loaded), the > >>>> BIOS may be confused and may cause the system as a whole to misbehave > >>>> (eg. on one of such systems enabling the native PCIe PME service > >>>> without loading the native PCIe hot-plug service driver causes a > >>>> storm of ACPI notify requests to appear). > >>>> > >>>> For this reason rework the PCIe port driver so that (1) it checks > >>>> which native PCIe port services can be enabled, according to the > >>>> BIOS, and (2) it requests control of all these services > >>>> simultaneously. In particular, this causes pcie_portdrv_probe() to > >>>> fail if the BIOS refuses to grant control of the PCIe capability > >>>> structure, which means that no native PCIe port services can be > >>>> enabled for the PCIe root complex the given port belongs to. > >>>> > >>>> Make it possible to override this behavior using a new command line > >>>> switch pcie_ports= that can be set to 'auto' (ask the BIOS, the > >>>> default), 'native' (use the PCIe native services regardless of the > >>>> BIOS response to the control request), or 'compat' (do not use the > >>>> PCIe native services at all). > >>>> > >>>> Accordingly, rework the existing PCIe port service drivers so that > >>>> they don't request control of the services directly. > >>>> > >>>> Signed-off-by: Rafael J. Wysocki<rjw@xxxxxxx> > >>>> --- > >>> > >>> Kenji-san, are you ok with this version? I would like to get your ack > >>> (and ideally your tested-by) for this one since it affects > >>> functionality you need. > >>> > >> > >> Hi Jesse, Rafael, > >> > >> I've just started reviewing the latest version of the patch. I have good > >> impression about this version so far. Please give me a few days for deeper > >> review. And I've booked the test machine for this patch in next week. I'll > >> try to test the patch and send you the result as soon as I can. > >> > >> Here are two comments I have so far. > >> > >> Though it is not directly related to Rafael's patch, I found one problem > >> in _OSC query handling while reviewing this patch. Currently, all the > >> _OSC controls are queried at the same time in acpi_pci_osc_support() and > >> the result is preserved for later acpi_pci_osc_control_set() call. But > >> query result can vary depending on the combination of requested controls. > > > > In principle it can, but acpi_pci_query_osc() asks for all aplicable control > > bits, in which case the formware can only clear those bits in the response if > > the corresponding features are unsupported. Doing otherwise would be in > > violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular: > > > > "If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method, > > the respective feature is designated unsupported by the platform and must not be enabled by the OS." > > > > My concern is as follows. > > For easy, assume as follows: > - all the _OSC controls are A, B, C > -B depends on C. > - When requesting A, B and C, all A, B and C are granted to OS. > - When requesting A, B, only A is granted to OS. That's why we should always ask for A, B, C at the same time. > Current linux queries all A, B and C at the boot time and preserve the > result (all A, B and C can be granted to OS). Here, if a service driver > requests A and B using acpi_pci_osc_control_set(), Service drivers should not use acpi_pci_osc_control_set(). That's why the current code is wrong. > acpi_pci_osc_control_set() checks if both of those controls can be granted > to OS. The expected result here is only A can be granted to OS, but current > linux judges both of them can be granted because linux does this check by > seeing preserved result. > As a result, acpi_pci_osc_control_set() evaluates _OSC with query flag > cleared to request A and B. Since the control B returned cleared, > acpi_pci_osc_control() returns error to the service driver. As a result, > control A is granted to OS even though there is no driver to handle the > control A. That indeed is a bug in the current code. With the $subject patch, though (without your modifications), that will never happen. > >> So I think query result must not be preserved. Since Rafael's patch uses > >> query result, this problem should be fixed at the same time. I'll try to > >> make a patch for this and update Rafael's patch if needed. > > > > It would be sufficient to change acpi_pci_osc_control_get(), introduced > > by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above, > > I don't think it's necessary. > > I made a patch for this and updated your patch slightly. I'll send them soon. > > > > >> I think the following changes should be done in the separated patch. I > >> guess this change is for the BIOS which enables hotplug interrupt at when > >> _OSC is evaluated with native hot-plug control. Right? > > > > Yes, it is. > > > >> Anyway, it should be separated patch with appropriate description. > > > > But the $subject patch does analogous things for AER and PCIe PME. Do you > > think they also should be introduced by separate patches? > > I think the following changes in your patch should be introduced by > separate patches. > > - optimizing the checks in acpi_pci_osc_control_set() I don't think that is necessary. > - Disabling hot-plug interrupt OK, but what exactly is the difference between that and disabling the PME interrupt? It is done for the same reason. > - Removing module_exit() OK Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
