lkml.org 
[lkml]   [2012]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH v1 06/22] PCI: use a global lock to serialize PCI root bridge hotplug operations
    On Tue, Aug 7, 2012 at 10:10 AM, Jiang Liu <liuj97@gmail.com> wrote:
    > Currently there's no mechanism to protect the global pci_root_buses list
    > from dynamic change at runtime. That means, PCI root bridge hotplug
    > operations, which dynamically change the pci_root_buses list, may cause
    > invalid memory accesses.
    >
    > So introduce a global lock to serialize accesses to the pci_root_buses
    > list and serialize PCI host bridge hotplug operations.
    >
    > Be careful, never try to acquire this global lock from PCI device drivers,
    > that may cause deadlocks.
    >
    > Signed-off-by: Jiang Liu <liuj97@gmail.com>
    > ---
    > drivers/acpi/pci_root.c | 8 +++++++-
    > drivers/edac/i7core_edac.c | 16 +++++++---------
    > drivers/gpu/drm/drm_fops.c | 6 +++++-
    > drivers/pci/host-bridge.c | 19 +++++++++++++++++++
    > drivers/pci/hotplug/sgi_hotplug.c | 2 ++
    > drivers/pci/pci-sysfs.c | 2 ++
    > drivers/pci/probe.c | 5 ++++-
    > drivers/pci/search.c | 9 ++++++++-
    > include/linux/pci.h | 8 ++++++++
    > 9 files changed, 62 insertions(+), 13 deletions(-)
    >
    > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
    > index 7aff631..6bd0e32 100644
    > --- a/drivers/acpi/pci_root.c
    > +++ b/drivers/acpi/pci_root.c
    > @@ -463,6 +463,8 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
    > if (!root)
    > return -ENOMEM;
    >
    > + pci_host_bridge_hotplug_lock();

    Here's where I get lost. This is an ACPI driver's .add() routine,
    which is analogous to a PCI driver's .probe() routine. PCI driver
    .probe() routines don't need to be concerned with PCI device hotplug.
    All the hotplug-related locking is handled by the PCI core, not by
    individual drivers. So why do we need it here?

    I'm not suggesting that the existing locking is correct. I'm just not
    convinced this is the right way to fix it.

    The commit log says we need protection for the global pci_root_buses
    list. But even with this whole series, we still traverse the list
    without protection in places like pcibios_resource_survey() and
    pci_assign_unassigned_resources().

    Maybe we can make progress on this by identifying specific failures
    that can happen in a couple of these paths, e.g., acpi_pci_root_add()
    and i7core_xeon_pci_fixup(). If we look at those paths, we might a
    way to fix this in a more general fashion than throwing in lock/unlock
    pairs.

    It might also help to know what the rule is for when we need to use
    pci_host_bridge_hotplug_lock() and pci_host_bridge_hotplug_unlock().
    Apparently it is not as simple as protecting every reference to the
    pci_root_buses list.

    > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
    > index 123de28..f559b5b 100644
    > --- a/drivers/gpu/drm/drm_fops.c
    > +++ b/drivers/gpu/drm/drm_fops.c
    > @@ -344,9 +344,13 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
    > pci_dev_put(pci_dev);
    > }
    > if (!dev->hose) {
    > - struct pci_bus *b = pci_bus_b(pci_root_buses.next);
    > + struct pci_bus *b;
    > +
    > + pci_host_bridge_hotplug_lock();
    > + b = pci_find_next_bus(NULL);

    Here's another case I don't understand. We know already that
    pci_find_next_bus() is unsafe with respect to hotplug because it
    doesn't hold a reference on the struct pci_bus it returns. Can't we
    replace this with some variety of pci_get_next_bus() that *does*
    acquire a reference?

    Actually, I looked at the callers of pci_find_next_bus(), and most of
    them are unsafe in an even deeper way: they're doing device setup in
    initcalls, so that setup won't be done for hot-added devices. For
    example, I can pick on sba_init() because I think I wrote it back in
    the dark ages. sba_init() is a subsys_initcall that calls
    sba_connect_bus() for every bus we know about at boot-time, and it
    sets the host bridge's iommu pointer. If we were to hot-add a host
    bridge, we would never set the iommu pointer.

    I'm not sure why you didn't add a pci_host_bridge_hotplug_lock() in
    the sba_init() path, since it looks similar to the drm_open_helper()
    path above. But in any case, I think that would be the wrong thing to
    do because it would fix the superficial problem while leaving the
    deeper problem of host bridge hot-add not setting the iommu pointer.

    > if (b)
    > dev->hose = b->sysdata;
    > + pci_host_bridge_hotplug_unlock();
    > }
    > }
    > #endif
    ...
    > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
    > index 993d4a0..f1147a7 100644
    > --- a/drivers/pci/search.c
    > +++ b/drivers/pci/search.c
    > @@ -100,6 +100,13 @@ struct pci_bus * pci_find_bus(int domain, int busnr)
    > * initiated by passing %NULL as the @from argument. Otherwise if
    > * @from is not %NULL, searches continue from next device on the
    > * global list.
    > + *
    > + * Please don't call this function at rumtime if possible.
    > + * It's designed to be called at boot time only because it's unsafe
    > + * to PCI root bridge hotplug operations. But some drivers do invoke
    > + * it at runtime and it's hard to fix those drivers. In such cases,
    > + * use pci_host_bridge_hotplug()_{lock|unlock} to protect the PCI root
    > + * bus list, but you need to be really careful to avoid deadlock.

    I'm not convinced that it's too hard to fix these drivers :) There
    are only six callers, and the only ones that could possibly be at
    runtime are drm_open_helper(), sn_pci_hotplug_init(), and
    bus_rescan_store().

    Bjorn


    \
     
     \ /
      Last update: 2012-09-12 01:41    [W:4.171 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site