Re: [PATCH] PCI / PCIe: Ask BIOS for control of all native services at once (v4)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux