linux-kernel - Re: [PATCH 2/6] PCI: acpiphp: enable_device(): rescan even if no new devices on slot

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]

Message-ID: <CAErSpo4hvH08bNrObYODFVcwhhiGOj4cKntyJD6Aw6CjecgCow@mail.gmail.com>
Date:	Wed, 26 Jun 2013 17:37:58 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Yinghai Lu <yinghai@...nel.org>,
	"Ronciak, John" <john.ronciak@...el.com>,
	"Penner, Miles J" <miles.j.penner@...el.com>,
	Bruce Allan <bruce.w.allan@...el.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH 2/6] PCI: acpiphp: enable_device(): rescan even if no new
 devices on slot

On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
<mika.westerberg@...ux.intel.com> wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
>
> pci_scan_slot() returns number of new devices connected *directly*
> connected to the slot. Current enable_device() checks the return value
> and stops if it doesn't see a new device.
>
> In Thunderbolt chaining case the new device can be deeper in hierarchy, so
> do the rescan anyway.
>
> Because of that we must make sure that pcibios_resource_survey_bus() and
> check_hotplug_bridge() get called only for a just found bus and not the
> ones already added to the system. Failure to do so will lead to resource
> conflicts.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index b983e29..80a6ea1 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>         struct pci_dev *dev;
>         struct pci_bus *bus = slot->bridge->pci_bus;
>         struct acpiphp_func *func;
> -       int num, max, pass;
> +       int max, pass;
>         LIST_HEAD(add_list);
>
>         list_for_each_entry(func, &slot->funcs, sibling)
>                 acpiphp_bus_add(func);
>
> -       num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
> -       if (num == 0) {
> -               /* Maybe only part of funcs are added. */
> -               dbg("No new device found\n");
> -               goto err_exit;
> -       }
> +       pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
>
>         max = acpiphp_max_busnr(bus);
>         for (pass = 0; pass < 2; pass++) {

I think this is two logical changes: the change below looks like it
could by done by itself first, followed by the change above.

> @@ -707,8 +702,11 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
>                                 max = pci_scan_bridge(bus, dev, max, pass);
>                                 if (pass && dev->subordinate) {
> -                                       check_hotplug_bridge(slot, dev);
> -                                       pcibios_resource_survey_bus(dev->subordinate);
> +                                       if (!dev->subordinate->is_added) {
> +                                               check_hotplug_bridge(slot, dev);
> +                                               pcibios_resource_survey_bus(
> +                                                       dev->subordinate);

It's a shame that pcibios_resource_survey_bus() can't be called twice.
 It would be nice if it were smart enough to notice on the second call
that "oh, resources have already been assigned, so there's nothing
more to be done."  Did you look to see whether that's feasible?

> +                                       }
>                                         __pci_bus_size_bridges(dev->subordinate,
>                                                                &add_list);
>                                 }
> @@ -742,8 +740,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>                 }
>         }
>
> -
> - err_exit:
>         return 0;
>  }
>
> --
> 1.8.3.1
>
--
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/