| 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
| ||
|
Message-ID: <4AE93761.6000503@jp.fujitsu.com>
Date: Thu, 29 Oct 2009 15:34:09 +0900
From: Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>
To: Yinghai Lu <yinghai@...nel.org>
CC: "Eric W. Biederman" <ebiederm@...ssion.com>,
Jesse Barnes <jbarnes@...tuousgeek.org>,
Alex Chiang <achiang@...com>,
Bjorn Helgaas <bjorn.helgaas@...com>,
Ingo Molnar <mingo@...e.hu>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
Ivan Kokshaysky <ink@...assic.park.msu.ru>
Subject: Re: [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@...nel.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
--
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/
Powered by blists - more mailing lists