[PATCH 0/6] Coverity and trivial fixes

[PATCH 0/6] Coverity and trivial fixes

[Bjorn Helgaas]

These fix a few issues reported by the Coverity checker and a trivial
style issue (__weak).

Only two should have interesting effects:

  - The OBFF/LTR enable functions should panic without this fix, so
    I don't think they've ever been tested.

  - The P2P bridge I/O window fix doesn't affect x86, which never uses
    32-bit I/O decoding.  But other architectures might notice because
    we incorrectly sign-extended I/O port addresses of 0x80000000 and
    above.

I'll wait for comments before putting them in "next" (targeted for 3.6).
---

Bjorn Helgaas (6):
      PCI: use __weak consistently
      PCI: fix upstream P2P bridge checks when enabling OBFF and LTR
      PCI: fix P2P bridge I/O port window sign extension
      PCI: shpchp: remove dead code
      PCI: acpiphp: check whether _ADR evaluation succeeded
      PCI: remove useless pcix_set_mmrbc() dev->bus check


 drivers/pci/hotplug/acpiphp_glue.c |   13 +++++++++----
 drivers/pci/hotplug/shpchp_ctrl.c  |    3 ---
 drivers/pci/pci-sysfs.c            |    2 +-
 drivers/pci/pci.c                  |   15 +++++++--------
 drivers/pci/probe.c                |   18 ++++++++++--------
 5 files changed, 27 insertions(+), 24 deletions(-)

-- 
Bjorn

[PATCH 1/6] PCI: use __weak consistently

[Bjorn Helgaas]

Use "__weak" instead of the gcc-specific "__attribute__ ((weak))"

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-sysfs.c |    2 +-
 drivers/pci/pci.c       |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 86c63fe..a0b435f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1112,7 +1112,7 @@ static struct bin_attribute pcie_config_attr = {
 	.write = pci_write_config,
 };
 
-int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
+int __weak pcibios_add_platform_entries(struct pci_dev *dev)
 {
 	return 0;
 }
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 447e834..8f4a5ea 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1349,7 +1349,7 @@ void pcim_pin_device(struct pci_dev *pdev)
  * is the default implementation. Architecture implementations can
  * override this.
  */
-void __attribute__ ((weak)) pcibios_disable_device (struct pci_dev *dev) {}
+void __weak pcibios_disable_device (struct pci_dev *dev) {}
 
 static void do_pci_disable_device(struct pci_dev *dev)
 {
@@ -1413,8 +1413,8 @@ pci_disable_device(struct pci_dev *dev)
  * Sets the PCIe reset state for the device. This is the default
  * implementation. Architecture implementations can override this.
  */
-int __attribute__ ((weak)) pcibios_set_pcie_reset_state(struct pci_dev *dev,
-							enum pcie_reset_state state)
+int __weak pcibios_set_pcie_reset_state(struct pci_dev *dev,
+					enum pcie_reset_state state)
 {
 	return -EINVAL;
 }
@@ -3851,7 +3851,7 @@ static void __devinit pci_no_domains(void)
  * greater than 0xff). This is the default implementation. Architecture
  * implementations can override this.
  */
-int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
+int __weak pci_ext_cfg_avail(struct pci_dev *dev)
 {
 	return 1;
 }

[PATCH 2/6] PCI: fix upstream P2P bridge checks when enabling OBFF and LTR

[Bjorn Helgaas]

pci_enable_obff() and pci_enable_ltr() incorrectly check "dev->bus" instead
of "dev->bus->self" to determine whether the upstream device is a P2P
bridge or a host bridge.  For devices on the root bus, the upstream device
is a host bridge, "dev->bus != NULL" and "dev->bus->self == NULL", and we
panic with a null pointer dereference.

These functions should previously have panicked when called on devices
supporting OBFF or LTR, so they should be regarded as untested.

Found by Coverity (CID 143038 and CID 143039).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8f4a5ea..b9e93cf 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2108,7 +2108,7 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
 		return -ENOTSUPP; /* no OBFF support at all */
 
 	/* Make sure the topology supports OBFF as well */
-	if (dev->bus) {
+	if (dev->bus->self) {
 		ret = pci_enable_obff(dev->bus->self, type);
 		if (ret)
 			return ret;
@@ -2215,7 +2215,7 @@ int pci_enable_ltr(struct pci_dev *dev)
 		return -EINVAL;
 
 	/* Enable upstream ports first */
-	if (dev->bus) {
+	if (dev->bus->self) {
 		ret = pci_enable_ltr(dev->bus->self);
 		if (ret)
 			return ret;

[PATCH 3/6] PCI: fix P2P bridge I/O port window sign extension

[Bjorn Helgaas]

On P2P bridges with 32-bit I/O decoding, we incorrectly sign-extended
windows starting at 0x80000000 or above.  In "base |= (io_base_hi << 16)",
"io_base_hi" is promoted to a signed int before being extended to an
unsigned long.

This would cause a window starting at I/O address 0x80000000 to be
treated as though it started at 0xffffffff80008000 instead, which
should cause "no compatible bridge window" errors when we enumerate
devices using that I/O space.

The mmio and mmio_pref casts are not strictly necessary, but without
them, correctness depends on the types of the PCI_MEMORY_RANGE_MASK and
PCI_PREF_RANGE_MASK constants, which are not obvious from reading the
local code.

Found by Coverity (CID 138747 and CID 138748).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 658ac97..a7a504f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -281,10 +281,11 @@ static void __devinit pci_read_bridge_io(struct pci_bus *child)
 
 	if ((io_base_lo & PCI_IO_RANGE_TYPE_MASK) == PCI_IO_RANGE_TYPE_32) {
 		u16 io_base_hi, io_limit_hi;
+
 		pci_read_config_word(dev, PCI_IO_BASE_UPPER16, &io_base_hi);
 		pci_read_config_word(dev, PCI_IO_LIMIT_UPPER16, &io_limit_hi);
-		base |= (io_base_hi << 16);
-		limit |= (io_limit_hi << 16);
+		base |= ((unsigned long) io_base_hi << 16);
+		limit |= ((unsigned long) io_limit_hi << 16);
 	}
 
 	if (base && base <= limit) {
@@ -312,8 +313,8 @@ static void __devinit pci_read_bridge_mmio(struct pci_bus *child)
 	res = child->resource[1];
 	pci_read_config_word(dev, PCI_MEMORY_BASE, &mem_base_lo);
 	pci_read_config_word(dev, PCI_MEMORY_LIMIT, &mem_limit_lo);
-	base = (mem_base_lo & PCI_MEMORY_RANGE_MASK) << 16;
-	limit = (mem_limit_lo & PCI_MEMORY_RANGE_MASK) << 16;
+	base = ((unsigned long) mem_base_lo & PCI_MEMORY_RANGE_MASK) << 16;
+	limit = ((unsigned long) mem_limit_lo & PCI_MEMORY_RANGE_MASK) << 16;
 	if (base && base <= limit) {
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		region.start = base;
@@ -334,11 +335,12 @@ static void __devinit pci_read_bridge_mmio_pref(struct pci_bus *child)
 	res = child->resource[2];
 	pci_read_config_word(dev, PCI_PREF_MEMORY_BASE, &mem_base_lo);
 	pci_read_config_word(dev, PCI_PREF_MEMORY_LIMIT, &mem_limit_lo);
-	base = (mem_base_lo & PCI_PREF_RANGE_MASK) << 16;
-	limit = (mem_limit_lo & PCI_PREF_RANGE_MASK) << 16;
+	base = ((unsigned long) mem_base_lo & PCI_PREF_RANGE_MASK) << 16;
+	limit = ((unsigned long) mem_limit_lo & PCI_PREF_RANGE_MASK) << 16;
 
 	if ((mem_base_lo & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
 		u32 mem_base_hi, mem_limit_hi;
+
 		pci_read_config_dword(dev, PCI_PREF_BASE_UPPER32, &mem_base_hi);
 		pci_read_config_dword(dev, PCI_PREF_LIMIT_UPPER32, &mem_limit_hi);
 
@@ -349,8 +351,8 @@ static void __devinit pci_read_bridge_mmio_pref(struct pci_bus *child)
 		 */
 		if (mem_base_hi <= mem_limit_hi) {
 #if BITS_PER_LONG == 64
-			base |= ((long) mem_base_hi) << 32;
-			limit |= ((long) mem_limit_hi) << 32;
+			base |= ((unsigned long) mem_base_hi) << 32;
+			limit |= ((unsigned long) mem_limit_hi) << 32;
 #else
 			if (mem_base_hi || mem_limit_hi) {
 				dev_err(&dev->dev, "can't handle 64-bit "

[PATCH 4/6] PCI: shpchp: remove dead code

[Bjorn Helgaas]

"slots_not_empty" is initialized to zero and can't be set again before
reaching this point, so this return statement is dead.  Remove it.

Found by Coverity (CID 114324).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/shpchp_ctrl.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp_ctrl.c b/drivers/pci/hotplug/shpchp_ctrl.c
index b00b09b..f9b5a52 100644
--- a/drivers/pci/hotplug/shpchp_ctrl.c
+++ b/drivers/pci/hotplug/shpchp_ctrl.c
@@ -262,9 +262,6 @@ static int board_added(struct slot *p_slot)
 	}
 
 	if ((ctrl->pci_dev->vendor == 0x8086) && (ctrl->pci_dev->device == 0x0332)) {
-		if (slots_not_empty)
-			return WRONG_BUS_FREQUENCY;
-
 		if ((rc = p_slot->hpc_ops->set_bus_speed_mode(p_slot, PCI_SPEED_33MHz))) {
 			ctrl_err(ctrl, "%s: Issue of set bus speed mode command"
 				 " failed\n", __func__);

[PATCH 5/6] PCI: acpiphp: check whether _ADR evaluation succeeded

[Bjorn Helgaas]

Check whether we evaluated _ADR successfully.  Previously we ignored
failure, so we would have used garbage data from the stack as the device
and function number.

We return AE_OK so that we ignore only this slot and continue looking
for other slots.

Found by Coverity (CID 113981).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..09bf377 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -132,6 +132,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	if (!acpi_pci_check_ejectable(pbus, handle) && !is_dock_device(handle))
 		return AE_OK;
 
+	status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+	if (ACPI_FAILURE(status)) {
+		warn("can't evaluate _ADR (%#x)\n", status);
+		return AE_OK;
+	}
+
+	device = (adr >> 16) & 0xffff;
+	function = adr & 0xffff;
+
 	pdev = pbus->self;
 	if (pdev && pci_is_pcie(pdev)) {
 		tmp = acpi_find_root_bridge_handle(pdev);
@@ -144,10 +153,6 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 		}
 	}
 
-	acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
-	device = (adr >> 16) & 0xffff;
-	function = adr & 0xffff;
-
 	newfunc = kzalloc(sizeof(struct acpiphp_func), GFP_KERNEL);
 	if (!newfunc)
 		return AE_NO_MEMORY;

[PATCH 6/6] PCI: remove useless pcix_set_mmrbc() dev->bus check

[Bjorn Helgaas]

For a valid pci_dev, dev->bus != NULL always, so remove this
unnecessary test.

Found by Coverity (CID 101680).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9e93cf..7f1310e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3395,8 +3395,7 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 
 	o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
 	if (o != v) {
-		if (v > o && dev->bus &&
-		   (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
+		if (v > o && (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
 			return -EIO;
 
 		cmd &= ~PCI_X_CMD_MAX_READ;

Re: [PATCH 0/6] Coverity and trivial fixes

[Bjorn Helgaas]

On Wed, Jun 20, 2012 at 6:16 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> These fix a few issues reported by the Coverity checker and a trivial
> style issue (__weak).
>
> Only two should have interesting effects:
>
>  - The OBFF/LTR enable functions should panic without this fix, so
>    I don't think they've ever been tested.
>
>  - The P2P bridge I/O window fix doesn't affect x86, which never uses
>    32-bit I/O decoding.  But other architectures might notice because
>    we incorrectly sign-extended I/O port addresses of 0x80000000 and
>    above.
>
> I'll wait for comments before putting them in "next" (targeted for 3.6).
> ---
>
> Bjorn Helgaas (6):
>      PCI: use __weak consistently
>      PCI: fix upstream P2P bridge checks when enabling OBFF and LTR
>      PCI: fix P2P bridge I/O port window sign extension
>      PCI: shpchp: remove dead code
>      PCI: acpiphp: check whether _ADR evaluation succeeded
>      PCI: remove useless pcix_set_mmrbc() dev->bus check
>
>
>  drivers/pci/hotplug/acpiphp_glue.c |   13 +++++++++----
>  drivers/pci/hotplug/shpchp_ctrl.c  |    3 ---
>  drivers/pci/pci-sysfs.c            |    2 +-
>  drivers/pci/pci.c                  |   15 +++++++--------
>  drivers/pci/probe.c                |   18 ++++++++++--------
>  5 files changed, 27 insertions(+), 24 deletions(-)

I applied these to my "next" branch.  Let me know if you see any issues.