On Wednesday, August 04, 2010, Hidetoshi Seto wrote:
> (2010/08/04 14:46), Kenji Kaneshige wrote:
> > By the way, I think it is getting confusing regarding who query the
> > controls. IMO, querying controls to ensure all the requested controls
> > are granted to OS should be done in acpi_pci_osc_control_set(), as
> > I said above. On the other hand, PCIe port driver need to query
> > controls for other reason... Now I think it might be better to change
> > acpi_pci_osc_control_set() like below instead of introducing
> > acpi_pci_osc_control_query().
> >
> > acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *flags)
> > {
> > ...
> > query = control_req;
> > status = acpi_pci_query_osc(root, root->osc_support_set, &query);
> > if (ACPI_FAILURE(status))
> > goto out;
> > if ((query & control_req) != control_req) {
> > printk_(KERN_DEBUG
> > "Firmware did not grant requested _OSC control\n");
> > status = AE_SUPPORT;
> > *flags = (query & control_req);
> > goto out;
> > }
> > ...
> > }
> >
> > And do as follows in pcie_port_acpi_setup()
> >
> > status = acpi_pci_osc_control_set(handle, &flags);
> > if (status == AE_SUPPORT) {
> > /* 2nd try */
> > status = acpi_pci_osc_control_set(handle, &flags);
> > }
> > if (ACPI_FAILURE(status)) {
> > ...
> >
> > What do you think about this?
>
> I think it makes sense, though some minor cares are required.
>
> Does this incremental patch (apply after [8/8]) looks good?
> If it is OK, I'll test these 8+1 patches within the next 2days.
>
> Thanks,
> H.Seto
>
> =====
> Subject: ACPI/PCI: Unify acpi_pci_osc_control_*()
>
> Now AE_SUPPORT of acpi_pci_osc_control_set() tells not only
> that query fails with the requested control bits but also that
> the result of query is stored into the pointed place.
>
> This allow user to retry acpi_pci_osc_control_set() with the
> result of query.
>
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@xxxxxxxxxxxxxx>
It looks good to me.
Thanks,
Rafael
> ---
> drivers/acpi/pci_root.c | 54 +++++++++++--------------------------
> drivers/pci/hotplug/acpi_pcihp.c | 2 +-
> drivers/pci/pcie/portdrv_acpi.c | 23 +++++++---------
> include/linux/acpi.h | 3 +-
> 4 files changed, 28 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 061e6f4..9a288df 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -372,38 +372,6 @@ out:
> }
> EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
>
> - /**
> - * acpi_pci_osc_control_query - Get the _OSC bits the kernel can control.
> - * @handle: ACPI handle of a PCI root bridge (or PCIe Root Complex).
> - * @mask: Mask of _OSC bits to query and the place to put the result into.
> - **/
> -acpi_status acpi_pci_osc_control_query(acpi_handle handle, u32 *mask)
> -{
> - struct acpi_pci_root *root;
> - acpi_handle tmp;
> - acpi_status status = AE_OK;
> -
> - if (!mask || !(*mask & OSC_PCI_CONTROL_MASKS))
> - return AE_BAD_PARAMETER;
> -
> - root = acpi_pci_find_root(handle);
> - if (!root)
> - return AE_NOT_EXIST;
> -
> - status = acpi_get_handle(handle, "_OSC", &tmp);
> - if (ACPI_FAILURE(status))
> - return status;
> -
> - mutex_lock(&osc_lock);
> - if ((*mask & root->osc_control_set) == *mask)
> - *mask = root->osc_control_set;
> - else
> - status = acpi_pci_query_osc(root, root->osc_support_set, mask);
> - mutex_unlock(&osc_lock);
> -
> - return status;
> -}
> -
> /**
> * acpi_pci_osc_control_set - commit requested control to Firmware
> * @handle: acpi_handle for the target ACPI object
> @@ -411,14 +379,17 @@ acpi_status acpi_pci_osc_control_query(acpi_handle handle, u32 *mask)
> *
> * Attempt to take control from Firmware on requested control bits.
> **/
> -acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
> +acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *flags)
> {
> acpi_status status;
> u32 control_req, result, capbuf[3];
> acpi_handle tmp;
> struct acpi_pci_root *root;
>
> - control_req = (flags & OSC_PCI_CONTROL_MASKS);
> + if (!flags)
> + return AE_BAD_PARAMETER;
> +
> + control_req = (*flags & OSC_PCI_CONTROL_MASKS);
> if (!control_req)
> return AE_TYPE;
>
> @@ -432,8 +403,10 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
>
> mutex_lock(&osc_lock);
> /* No need to evaluate _OSC if the control was already granted. */
> - if ((root->osc_control_set & control_req) == control_req)
> + if ((root->osc_control_set & control_req) == control_req) {
> + *flags = root->osc_control_set;
> goto out;
> + }
>
> /* Need to query controls first before requesting them */
> result = control_req;
> @@ -441,8 +414,10 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
> if (ACPI_FAILURE(status))
> goto out;
> if ((result & control_req) != control_req) {
> - printk(KERN_DEBUG
> - "Firmware did not grant requested _OSC control\n");
> + printk(KERN_DEBUG PREFIX
> + "Firmware did not grant requested _OSC control: %x/%x\n",
> + control_req, result);
> + *flags = result;
> status = AE_SUPPORT;
> goto out;
> }
> @@ -452,7 +427,10 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
> capbuf[OSC_CONTROL_TYPE] = root->osc_control_set | control_req;
> status = acpi_pci_run_osc(handle, capbuf, &result);
> if (ACPI_SUCCESS(status))
> - root->osc_control_set = result;
> + root->osc_control_set = *flags = result;
> + else
> + printk(KERN_WARNING FW_BUG PREFIX
> + "_OSC did not grant controls that passed query\n");
> out:
> mutex_unlock(&osc_lock);
> return status;
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 12ea415..675181d 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -358,7 +358,7 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
> acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> dbg("Trying to get hotplug control for %s\n",
> (char *)string.pointer);
> - status = acpi_pci_osc_control_set(handle, flags);
> + status = acpi_pci_osc_control_set(handle, &flags);
> if (ACPI_SUCCESS(status))
> goto got_one;
> if (status == AE_SUPPORT)
> diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
> index a07a70e..764d0b3 100644
> --- a/drivers/pci/pcie/portdrv_acpi.c
> +++ b/drivers/pci/pcie/portdrv_acpi.c
> @@ -55,20 +55,17 @@ int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
> 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;
> +retry:
> + status = acpi_pci_osc_control_set(handle, &flags);
> + if (status == AE_SUPPORT) {
> + if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
> + dev_dbg(&port->dev, "BIOS refuses to grant control of "
> + "PCIe Capability Structure\n");
> + return -EACCES;
> + }
> + if (flags & ~OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
> + goto retry;
> }
> -
> - if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
> - dev_dbg(&port->dev, "BIOS refuses to grant control of PCIe "
> - "Capability Structure\n");
> - return -EACCES;
> - }
> -
> - status = acpi_pci_osc_control_set(handle, flags);
> if (ACPI_FAILURE(status)) {
> dev_dbg(&port->dev, "ACPI _OSC request failed (code %d)\n",
> status);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a9afe9c..1b5aac6 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -305,8 +305,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
> OSC_PCI_EXPRESS_AER_CONTROL | \
> OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
>
> -extern acpi_status acpi_pci_osc_control_query(acpi_handle handle, u32 *mask);
> -extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags);
> +extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *flags);
> extern void acpi_early_init(void);
>
> #else /* !CONFIG_ACPI */
>
--
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
