linux-kernel - Re: [PATCH 2/3] PCI: correctly detect ACPI PCI host bridge objects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 26 Oct 2012 03:10:21 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...e.hu>,
	"H. Peter Anvin" <hpa@...or.com>, Jacob Shin <jacob.shin@....com>,
	Tejun Heo <tj@...nel.org>,
	Stefano Stabellini <stefano.stabellini@...citrix.com>,
	linux-kernel@...r.kernel.org, Jiang Liu <jiang.liu@...wei.com>,
	Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH 2/3] PCI: correctly detect ACPI PCI host bridge objects

On Thu, Oct 18, 2012 at 2:50 PM, Yinghai Lu <yinghai@...nel.org> wrote:
> From: Jiang Liu <jiang.liu@...wei.com>
>
> The code in pci_root_hp.c depends on function acpi_is_root_bridge()
> to check whether an ACPI object is a PCI host bridge or not.
> If an ACPI device hasn't been created for the ACPI object yet,
> function acpi_is_root_bridge() will return false even if the object
> is a PCI host bridge object. That behavior will cause two issues:
> 1) No ACPI notification handler installed for PCI host bridges absent
>    at startup, so hotplug events for those bridges won't be handled.
> 2) rescan_root_bridge() can't reenumerate offlined PCI host bridges
>    because the ACPI devices have been already destroyed.
>
> So use acpi_match_object_info_ids() to correctly detect PCI host bridges.
>
> -v2: update to use acpi_match_object_info_ids() from Tang Chen  - Yinghai
>
> Signed-off-by: Jiang Liu <jiang.liu@...wei.com>
> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
> Cc: Len Brown <lenb@...nel.org>
> Cc: linux-acpi@...r.kernel.org
> ---
>  drivers/acpi/pci_root_hp.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
> index 2aebf78..3edec7f 100644
> --- a/drivers/acpi/pci_root_hp.c
> +++ b/drivers/acpi/pci_root_hp.c
> @@ -19,6 +19,12 @@ struct acpi_root_bridge {
>         u32 flags;
>  };
>
> +static const struct acpi_device_id root_device_ids[] = {
> +       {"PNP0A03", 0},
> +       {"PNP0A08", 0},

Why do we need PNP0A08 here?

Per the PCI Firmware Spec, sec 4.1.5, PNP0A08 identifies a PCI Express
or a PCI-X Mode 2 host bridge, and such a device is required to
include PNP0A03 in _CID.  Therefore, it should be sufficient to look
only for PNP0A03.

That raises the question of why the spec defined PNP0A08 in the first
place.  It looks like PNP0A08 is intended specifically to indicate
support for extended config space (offsets 256-4095), per sec 4.1.5
(Note 1) and sec 4.3.1.

I don't think Linux currently does anything differently with PNP0A08,
so if the spec authors thought it was important to add PNP0A08, maybe
Linux is missing something.  For example, maybe we should be limiting
config space size to 256 unless we're below a PNP0A08 device.  I don't
know what that could *fix*, unless it would allow us to get rid of
some quirks or something, but usually spec authors don't add things
unless they think there's a reason it's needed.

> +       {"", 0},
> +};
> +
>  /* bridge flags */
>  #define ROOT_BRIDGE_HAS_EJ0    (0x00000002)
>  #define ROOT_BRIDGE_HAS_PS3    (0x00000080)
> @@ -256,6 +262,23 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
>                                 _handle_hotplug_event_root);
>  }
>
> +static bool acpi_is_root_bridge_object(acpi_handle handle)
> +{
> +       struct acpi_device_info *info = NULL;
> +       acpi_status status;
> +       bool ret;
> +
> +       status = acpi_get_object_info(handle, &info);
> +       if (ACPI_FAILURE(status))
> +               return false;
> +
> +       ret = !acpi_match_object_info_ids(info, root_device_ids);
> +
> +       kfree(info);
> +
> +       return ret;
> +}
> +
>  static acpi_status __init
>  find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>  {
> @@ -264,7 +287,7 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>                                       .pointer = objname };
>         int *count = (int *)context;
>
> -       if (!acpi_is_root_bridge(handle))
> +       if (!acpi_is_root_bridge_object(handle))
>                 return AE_OK;
>
>         (*count)++;
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/