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: <5615FFD4.3090202@linux.intel.com> Date: Thu, 8 Oct 2015 13:32:04 +0800 From: Jiang Liu <jiang.liu@...ux.intel.com> To: Bjorn Helgaas <helgaas@...nel.org> Cc: Bjorn Helgaas <bhelgaas@...gle.com>, "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>, Lorenzo Pieralisi <lorenzo.pieralisi@....com>, Marc Zyngier <marc.zyngier@....com>, Hanjun Guo <hanjun.guo@...aro.org>, Liviu Dudau <Liviu.Dudau@....com>, "Rafael J. Wysocki" <rjw@...ysocki.net>, Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org, linux-pci@...r.kernel.org Subject: Re: [Patch v6 4/7] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core On 2015/10/7 1:47, Bjorn Helgaas wrote: > Hi Jiang, > > Strictly speaking, this patch by itself doesn't actually "consolidate" > anything because it only adds acpi_pci_root_create() (which isn't called by > anything yet), but doesn't remove the original x86 copy. > <snit> >> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> + struct acpi_pci_root_ops *ops, >> + struct acpi_pci_root_info *info, >> + void *sysdata) >> +{ >> + int ret, busnum = root->secondary.start; >> + struct acpi_device *device = root->device; >> + int node = acpi_get_node(device->handle); >> + struct pci_bus *bus; >> + >> + info->root = root; >> + info->bridge = device; >> + info->ops = ops; >> + INIT_LIST_HEAD(&info->resources); >> + snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x", >> + root->segment, busnum); >> + >> + if (ops->init_info && ops->init_info(info)) >> + goto out_release_info; >> + ret = acpi_pci_probe_root_resources(info); >> + if (ops->prepare_resources) >> + ret = ops->prepare_resources(info, ret); >> + if (ret < 0) >> + goto out_release_info; >> + else if (ret > 0) >> + pci_acpi_root_add_resources(info); > > This is unnecessarily complicated: you set "ret", then overwrite it if > ops->prepare_resources. By the time you test "ret", it's messy to > figure out what it means. > > Both ops->prepare_resources() and pci_acpi_root_add_resources() > should be able to deal with empty resource lists, so can you do the > following instead? > > ret = acpi_pci_probe_root_resources(info); > if (ret < 0) > goto out_release_info; Hi Bjorn, Thanks for review:) The original code is used to handle a special case for x86, where acpi_pci_probe_root_resources() fails but ops->prepare_resources() succeeds. For x86, PCI host bridge resources may probed by means other than ACPI when pci_use_crs is true (AMD and Broadcom hostbridges). So we can't return failure when acpi_pci_probe_root_resources() fails. + ret = acpi_pci_probe_root_resources(info); + if (ops->prepare_resources) + ret = ops->prepare_resources(info, ret); + if (ret < 0) + goto out_release_info; > if (ops->prepare_resources) { > ret = ops->prepare_resources(info, ret); > if (ret < 0) > goto out_release_info; > } > pci_acpi_root_add_resources(info); I will remove the redundant check of (ret > 0) in: + else if (ret > 0) + pci_acpi_root_add_resources(info); > >> + pci_add_resource(&info->resources, &root->secondary); >> + >> + bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, >> + sysdata, &info->resources); >> + if (bus) { > > if (!bus) > goto out_release_info; > > Then it looks like the other error paths above, and you can un-indent > the following code, which is the normal path: Will do this. Thanks! Gerry -- 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