Re: [linux-pm] [PATCH 7/8] ACPI / PCI: Do not preserve _OSC control bits returned by a query (v2)

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

 



On Tuesday, August 03, 2010, Rafael J. Wysocki wrote:
> On Tuesday, August 03, 2010, Hidetoshi Seto wrote:
> > (2010/08/03 13:52), Kenji Kaneshige wrote:
> > > (2010/08/03 6:59), Rafael J. Wysocki wrote:
> > >> @@ -434,19 +432,6 @@ acpi_status acpi_pci_osc_control_set(acp
> > >>       if ((root->osc_control_set&  control_req) == control_req)
> > >>           goto out;
> > >>
> > >> -    /* Need to query controls first before requesting them */
> > >> -    if (!root->osc_queried) {
> > >> -        status = acpi_pci_query_osc(root, root->osc_support_set, NULL);
> > >> -        if (ACPI_FAILURE(status))
> > >> -            goto out;
> > >> -    }
> > >> -    if ((root->osc_control_qry&  control_req) != control_req) {
> > >> -        printk(KERN_DEBUG
> > >> -               "Firmware did not grant requested _OSC control\n");
> > >> -        status = AE_SUPPORT;
> > >> -        goto out;
> > >> -    }
> > > 
> > > I think acpi_pci_osc_control_set() still need to query before commit
> > > to ensure all the requested controls are granted to OS.
> > > 
> > > So the code needs to be
> > > 
> > >     status = acpi_pci_query_osc(root, root->osc_support_set, &control_req);
> > >     if (ACPI_FAILURE(status))
> > >         goto out;
> > 
> > Hum, since acpi_status acpi_pci_osc_control_set() is an exported
> > function, we cannot be too careful here. 
> > 
> > OTOH, I think this second query should be done in caller too,
> > i.e. pcie_port_acpi_setup() in patch [4/8].
> > 
> > Now:
> >   pcie_port_acpi_setup()
> >   {
> >     flags = A|B|C|D;
> >     acpi_pci_osc_control_query(handle, &flags);
> >     /* note: flags might be changed after query */
> >     acpi_pci_osc_control_set(handle, flags);
> >   }
> > 
> > Strictly, it could be like:
> > 
> > New:
> >   pcie_port_acpi_setup()
> >   {
> >     flags = A|B|C|D;
> >     do {
> >       pre_flags = flags;
> >       acpi_pci_osc_control_query(handle, &flags);
> >     } while (flags && pre_flags != flags);
> >     if (flags)
> >       acpi_pci_osc_control_set(handle, flags);
> >   }
> > 
> > IMHO these checks are kind of preventive guard for corner cases,
> > and I suppose it can be implemented by an incremental patch later.
> 
> OK
> 
> The assumption here is that the BIOS would only mask the bits it's not
> going to grant control of and will do it in a consistent way.  So, I guess the
> purpose of the change above would be to protect us from buggy BIOSes.

What about the appended patch (on top of 4/8)?

Rafael


---
 drivers/pci/pcie/portdrv_acpi.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/pci/pcie/portdrv_acpi.c
===================================================================
--- linux-2.6.orig/drivers/pci/pcie/portdrv_acpi.c
+++ linux-2.6/drivers/pci/pcie/portdrv_acpi.c
@@ -35,7 +35,7 @@ int pcie_port_acpi_setup(struct pci_dev 
 {
 	acpi_status status;
 	acpi_handle handle;
-	u32 flags;
+	u32 flags, prev_flags = 0;
 
 	if (acpi_pci_disabled)
 		return 0;
@@ -55,11 +55,14 @@ int pcie_port_acpi_setup(struct pci_dev 
 			flags |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
-	status = acpi_pci_osc_control_query(handle, &flags);
-	if (ACPI_FAILURE(status)) {
-		dev_dbg(&port->dev, "ACPI _OSC query failed (code %d)\n",
-			status);
-		return -ENODEV;
+	while (flags != prev_flags) {
+		prev_flags = flags;
+		status = acpi_pci_osc_control_query(handle, &flags);
+		if (ACPI_FAILURE(status)) {
+			dev_dbg(&port->dev,
+				"ACPI _OSC query failed (code %d)\n", status);
+			return -ENODEV;
+		}
 	}
 
 	if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
--
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