lkml.org 
[lkml]   [2009]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 2/2] pci: only release that resource index is less than 3 -v5
    Yinghai Lu wrote:
    > after
    >
    > | commit 308cf8e13f42f476dfd6552aeff58fdc0788e566
    > |
    > | PCI: get larger bridge ranges when space is available
    >
    > found one of resource of peer root bus (0x00) get released from root
    > resource. later one hotplug device can not get big range anymore.
    > other peer root buses is ok.
    >
    > it turns out it is from transparent path.
    >
    > those resources will be used for pci bridge BAR updated.
    > so need to limit it to 3.
    >
    > v2: Jesse doesn't like it is in find_free_bus_resource...
    > try to move out of pci_bus_size_bridges loop.
    > need to apply after:
    > [PATCH] pci: pciehp update the slot bridge res to get big range for pcie devices - v4
    > v3: add pci_setup_bridge calling after pci_bridge_release_not_used_res.
    > only clear release those res for x86.
    > v4: Bjorn want to release use dev instead of bus.
    > v5: Kenji pointed out it will have problem with several level bridge.
    > so let only handle leaf bridge.
    >
    > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
    >
    > ---
    > arch/x86/pci/i386.c | 6 ++
    > drivers/pci/hotplug/pciehp_pci.c | 2
    > drivers/pci/setup-bus.c | 81 ++++++++++++++++++++++++++++++++++++++-
    > include/linux/pci.h | 2
    > 4 files changed, 90 insertions(+), 1 deletion(-)
    >
    > Index: linux-2.6/drivers/pci/setup-bus.c
    > ===================================================================
    > --- linux-2.6.orig/drivers/pci/setup-bus.c
    > +++ linux-2.6/drivers/pci/setup-bus.c
    > @@ -319,6 +319,42 @@ static void pci_bridge_check_ranges(stru
    > }
    > }
    >
    > +void pci_bridge_release_not_used_res(struct pci_bus *bus)
    > +{
    > + int idx;
    > + bool changed = false;
    > + struct pci_dev *dev;
    > + struct resource *r;
    > + unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
    > + IORESOURCE_PREFETCH;
    > +
    > + /* for pci bridges res only */
    > + dev = bus->self;
    > + for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_BRIDGE_RESOURCES + 3;
    > + idx++) {

    If this function is only for normal PCI bridge, we need additional
    check at the head of this function.

    if ((dev->class >> 8) == PCI_CLASS_BRIDGE_CARDBUS)
    return;

    if not, 3 should be 4 instead. And we can do as follows

    for (i = PCI_BRIDGE_RESOURCE; i <= PCI_BRIDGE_RESOURCE_END; i++) {

    > + r = &dev->resource[idx];
    > + if (r->flags & type_mask) {

    How about
    if (!(r->flags & type_mask))
    continue;

    > + if (!r->parent)
    > + continue;
    > + /*
    > + * if there is no child under that, we should release
    > + * and use it.
    > + */

    I think this comment should be updated because whether "we should
    release and use it" is decided by the caller of this function.

    > + if (!r->child && !release_resource(r)) {
    > + dev_info(&dev->dev,
    > + "resource %d %pRt released\n",
    > + idx, r);
    > + r->flags = 0;
    > + changed = true;
    > + }

    How about
    if (r->child)
    continue;

    if (!release_resource(r)) {
    ...
    }

    > + }
    > + }
    > +
    > + if (changed)
    > + pci_setup_bridge(bus);

    The pci_setup_bridge() is called even if some resources are used
    by children. For example, mem resource is used by children, but
    prefetchable mem resource is not used by children. In this case,
    I think mem resource is released and pci_setup_bridge() is called
    while prefetcable mem resource is being used. I'm worrying that it
    might cause something wrong.

    > +}
    > +EXPORT_SYMBOL(pci_bridge_release_not_used_res);
    > +
    > /* Helper function for sizing routines: find first available
    > bus resource of a given type. Note: we intentionally skip
    > the bus resources which have already been assigned (that is,
    > @@ -576,6 +612,48 @@ void __ref pci_bus_size_bridges(struct p
    > }
    > EXPORT_SYMBOL(pci_bus_size_bridges);
    >
    > +
    > +/* only release those resources that is on leaf bridge */
    > +void __ref pci_bus_release_bridges_not_used_res(struct pci_bus *bus)
    > +{
    > + struct pci_dev *dev;
    > + bool is_leaf_bridge = true;
    > +
    > + list_for_each_entry(dev, &bus->devices, bus_list) {
    > + struct pci_bus *b = dev->subordinate;
    > + if (!b)
    > + continue;
    > +
    > + switch (dev->class >> 8) {
    > + case PCI_CLASS_BRIDGE_CARDBUS:
    > + is_leaf_bridge = false;
    > + break;
    > +
    > + case PCI_CLASS_BRIDGE_PCI:
    > + default:
    > + is_leaf_bridge = false;
    > + pci_bus_release_bridges_not_used_res(b);
    > + break;
    > + }
    > + }
    > +
    > + /* The root bus? */
    > + if (!bus->self)
    > + return;
    > +
    > + switch (bus->self->class >> 8) {
    > + case PCI_CLASS_BRIDGE_CARDBUS:
    > + break;
    > +
    > + case PCI_CLASS_BRIDGE_PCI:
    > + default:
    > + if (is_leaf_bridge)
    > + pci_bridge_release_not_used_res(bus);
    > + break;
    > + }
    > +}

    How about like below. This might need to be called with "pci_bus_sem" held.

    void __ref pci_bus_release_bridges_not_used_res(struct pci_bus *bus)
    {
    struct pci_bus *b;

    /* If the bus is cardbus, do nothing */
    if (bus->self && (bus->self->class >> 8) == PCI_CLASS_BRIDGE_CARDBUS)
    return;
    /* We only release the resources under the leaf bridge */
    if (list_empty(&bus->children)) {
    if (!pci_is_root_bus(bus))
    pci_bridge_release_not_used_res(bus);
    return;
    }
    /* Scan child buses if the bus is not leaf */
    list_for_each_entry(b, &bus->children, node)
    pci_bus_release_bridges_not_used_res(b);
    }
    > +EXPORT_SYMBOL(pci_bus_release_bridges_not_used_res);
    > +
    > void __ref pci_bridge_assign_resources(const struct pci_dev *bridge)
    > {
    > struct pci_bus *b;
    > @@ -644,7 +722,8 @@ static void pci_bus_dump_res(struct pci_
    >
    > for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
    > struct resource *res = bus->resource[i];
    > - if (!res || !res->end)
    > +
    > + if (!res || !res->end || !res->flags)
    > continue;
    >
    > dev_printk(KERN_DEBUG, &bus->dev, "resource %d %pRt\n", i, res);
    > Index: linux-2.6/drivers/pci/hotplug/pciehp_pci.c
    > ===================================================================
    > --- linux-2.6.orig/drivers/pci/hotplug/pciehp_pci.c
    > +++ linux-2.6/drivers/pci/hotplug/pciehp_pci.c
    > @@ -98,6 +98,7 @@ int pciehp_configure_device(struct slot
    > pci_dev_put(dev);
    > }
    >
    > + pci_bridge_release_not_used_res(parent);

    I guess this is for the slots that are empty at the boot time on non x86
    systems. And it only works at the first hot-add. Correct? How about doing
    this at the pciehp's slot initialization.


    > pci_bus_size_bridges(parent);
    > pci_clear_master(bridge);
    > pci_bridge_assign_resources(bridge);
    > @@ -171,6 +172,7 @@ int pciehp_unconfigure_device(struct slo
    > }
    > pci_dev_put(temp);
    > }
    > + pci_bridge_release_not_used_res(parent);

    How about call pci_bus_release_bridges_not_used_res() instead and
    make pci_bridge_release_not_used_res() static function.

    >
    > return rc;
    > }
    > Index: linux-2.6/include/linux/pci.h
    > ===================================================================
    > --- linux-2.6.orig/include/linux/pci.h
    > +++ linux-2.6/include/linux/pci.h
    > @@ -759,6 +759,8 @@ int pci_vpd_truncate(struct pci_dev *dev
    > void pci_bridge_assign_resources(const struct pci_dev *bridge);
    > void pci_bus_assign_resources(const struct pci_bus *bus);
    > void pci_bus_size_bridges(struct pci_bus *bus);
    > +void pci_bus_release_bridges_not_used_res(struct pci_bus *bus);
    > +void pci_bridge_release_not_used_res(struct pci_bus *bus);
    > int pci_claim_resource(struct pci_dev *, int);
    > void pci_assign_unassigned_resources(void);
    > void pdev_enable_device(struct pci_dev *);
    > Index: linux-2.6/arch/x86/pci/i386.c
    > ===================================================================
    > --- linux-2.6.orig/arch/x86/pci/i386.c
    > +++ linux-2.6/arch/x86/pci/i386.c
    > @@ -194,6 +194,7 @@ static void __init pcibios_allocate_reso
    > static int __init pcibios_assign_resources(void)
    > {
    > struct pci_dev *dev = NULL;
    > + struct pci_bus *bus;
    > struct resource *r;
    >
    > if (!(pci_probe & PCI_ASSIGN_ROMS)) {
    > @@ -213,6 +214,11 @@ static int __init pcibios_assign_resourc
    > }
    > }
    >
    > + /* Try to release bridge resources, that there is not child under it */
    > + list_for_each_entry(bus, &pci_root_buses, node) {
    > + pci_bus_release_bridges_not_used_res(bus);
    > + }
    > +

    If this is only for PCIe hotplug, I don't think we need it here (as
    you're doing for non x86 platforms). If not, I think you should do
    it in the another patch.

    Thanks,
    Kenji Kaneshige




    \
     
     \ /
      Last update: 2009-10-29 07:37    [W:3.838 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site