[PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Geert Uytterhoeven]

drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (slot == PCI_SLOT(devfn) && type == rtype) {
^

If there's ever gonna be a configuration space or 64-bit memory space
entry in DT, rtype will be uninitialized, and the wrong entry may be
returned.

Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.

Introduced in commit 11be65472a427dcf7a11ab6e3e3628f1c6768b5b ("PCI:
mvebu: Adapt to the new device tree layout").

Signed-off-by: Geert Uytterhoeven <geert+***@glider.be>
---
Alternatively, should the "else if (DT_FLAGS_TO_TYPE(flags) ==
DT_TYPE_MEM32)" just be changed to "else", assuming there can never be
other entries than for I/O or 32-bit memory space?
---
drivers/pci/host/pci-mvebu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ce23e0f076b6..9515f0d13fd4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
u32 flags = of_read_number(range, 1);
u32 slot = of_read_number(range + 1, 1);
u64 cpuaddr = of_read_number(range + na, pna);
- unsigned long rtype;
+ unsigned long rtype = 0;

if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;

[Bjorn Helgaas]

Post by Geert Uytterhoeven
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (slot == PCI_SLOT(devfn) && type == rtype) {
^
If there's ever gonna be a configuration space or 64-bit memory space
entry in DT, rtype will be uninitialized, and the wrong entry may be
returned.
Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.
mvebu: Adapt to the new device tree layout").
---
Alternatively, should the "else if (DT_FLAGS_TO_TYPE(flags) ==
DT_TYPE_MEM32)" just be changed to "else", assuming there can never be
other entries than for I/O or 32-bit memory space?
---
drivers/pci/host/pci-mvebu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ce23e0f076b6..9515f0d13fd4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
u32 flags = of_read_number(range, 1);
u32 slot = of_read_number(range + 1, 1);
u64 cpuaddr = of_read_number(range + na, pna);
- unsigned long rtype;
+ unsigned long rtype = 0;
if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;
--
1.9.1

This fix looks right to me. I added a stable tag as follows. Thomas
and/or Jason, and you ack this?


commit f96f4040d0d01b6eeacda212cf7db105d06a55ba
Author: Geert Uytterhoeven <geert+***@glider.be>
Date: Fri Aug 8 17:34:05 2014 +0200

PCI: mvebu: Fix uninitialized "rtype" in mvebu_get_tgt_attr()

drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (slot == PCI_SLOT(devfn) && type == rtype) {
^

If there's ever a DT entry other than DT_TYPE_IO or DT_TYPE_MEM32,
e.g., a configuration space or 64-bit memory space entry, rtype will
be uninitialized, and the wrong entry may be returned.

Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.

Fixes: 11be65472a42 ("PCI: mvebu: Adapt to the new device tree layout")
Signed-off-by: Geert Uytterhoeven <geert+***@glider.be>
Signed-off-by: Bjorn Helgaas <***@google.com>
CC: ***@vger.kernel.org # v3.12+

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a92e0f..081579c0971e 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
u32 flags = of_read_number(range, 1);
u32 slot = of_read_number(range + 1, 1);
u64 cpuaddr = of_read_number(range + na, pna);
- unsigned long rtype;
+ unsigned long rtype = 0;

if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;

[Arnd Bergmann]

Post by Bjorn Helgaas

Post by Geert Uytterhoeven
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (slot == PCI_SLOT(devfn) && type == rtype) {
^
If there's ever gonna be a configuration space or 64-bit memory space
entry in DT, rtype will be uninitialized, and the wrong entry may be
returned.
Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.
mvebu: Adapt to the new device tree layout").
---
Alternatively, should the "else if (DT_FLAGS_TO_TYPE(flags) ==
DT_TYPE_MEM32)" just be changed to "else", assuming there can never be
other entries than for I/O or 32-bit memory space?
---
drivers/pci/host/pci-mvebu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ce23e0f076b6..9515f0d13fd4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
u32 flags = of_read_number(range, 1);
u32 slot = of_read_number(range + 1, 1);
u64 cpuaddr = of_read_number(range + na, pna);
- unsigned long rtype;
+ unsigned long rtype = 0;
if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;

This fix looks right to me. I added a stable tag as follows. Thomas
and/or Jason, and you ack this?

I had a local fix for this, which I haven't gotten around to
send a proper changelog for, but it seems like a more appropriate
fix, avoiding the spurious initialization.

The other fix looks technically correct as well though.

Arnd

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a92e0f..678849836649 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -883,6 +883,8 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ rtype = -1;

if (slot == PCI_SLOT(devfn) && type == rtype) {
*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);

[Thomas Petazzoni]

Dear Bjorn Helgaas,

Post by Bjorn Helgaas
This fix looks right to me. I added a stable tag as follows. Thomas
and/or Jason, and you ack this?

Hum, I think I would actually prefer something like:

if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ continue;

So that we're explicit with the fact that we only care about I/O and
MEM32 resource types.

Thanks,

Thomas

[Arnd Bergmann]

Post by Geert Uytterhoeven
if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ continue;
So that we're explicit with the fact that we only care about I/O and
MEM32 resource types.

Agreed, that looks better than my patch as well.

Arnd

[Bjorn Helgaas]

Post by Arnd Bergmann

Post by Geert Uytterhoeven
if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ continue;
So that we're explicit with the fact that we only care about I/O and
MEM32 resource types.

Agreed, that looks better than my patch as well.

I like it better, too, but we still need the "range += rangesz" part,
so I don't think it will work. I suppose that could be moved to the
update expression of the "for" loop. Or, since we don't use "i" in
the loop at all, maybe we could do something like this:

for (; range < rend; range += rangesz)

[Bjorn Helgaas]

Post by Bjorn Helgaas

Post by Arnd Bergmann

Post by Geert Uytterhoeven
if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ continue;
So that we're explicit with the fact that we only care about I/O and
MEM32 resource types.

Agreed, that looks better than my patch as well.

I like it better, too, but we still need the "range += rangesz" part,
so I don't think it will work. I suppose that could be moved to the
update expression of the "for" loop. Or, since we don't use "i" in
for (; range < rend; range += rangesz)

Any more input on this? I don't think I've seen anything actually acked by
Thomas or Jason.

[Thomas Petazzoni]

Geert Uytterhoeven reported a warning when building pci-mvebu:

drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (slot == PCI_SLOT(devfn) && type == rtype) {
^

And indeed, the code of mvebu_get_tgt_attr() may lead to the usage of
rtype when being uninitialized, even though it would only happen if we
had entries other than I/O space and 32 bits memory space.

This commit fixes that by simply skipping the current DT range being
considered, if it doesn't match the resource type we're looking for.

Signed-off-by: Thomas Petazzoni <***@free-electrons.com>
Reported-by: Geert Uytterhoeven <geert+***@glider.be>
---
This patch is a different solution than the one proposed by Geert. It
(hopefully) matches the discussion we had with Bjorn and Arnd.

Signed-off-by: Thomas Petazzoni <***@free-electrons.com>
---
drivers/pci/host/pci-mvebu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a..b1315e1 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -873,7 +873,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
rangesz = pna + na + ns;
nranges = rlen / sizeof(__be32) / rangesz;

- for (i = 0; i < nranges; i++) {
+ for (i = 0; i < nranges; i++, range += rangesz) {
u32 flags = of_read_number(range, 1);
u32 slot = of_read_number(range + 1, 1);
u64 cpuaddr = of_read_number(range + na, pna);
@@ -883,14 +883,14 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ continue;

if (slot == PCI_SLOT(devfn) && type == rtype) {
*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
return 0;
}
-
- range += rangesz;
}

return -ENOENT;

[Bjorn Helgaas]

Post by Geert Uytterhoeven
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (slot == PCI_SLOT(devfn) && type == rtype) {
^
And indeed, the code of mvebu_get_tgt_attr() may lead to the usage of
rtype when being uninitialized, even though it would only happen if we
had entries other than I/O space and 32 bits memory space.
This commit fixes that by simply skipping the current DT range being
considered, if it doesn't match the resource type we're looking for.
---
This patch is a different solution than the one proposed by Geert. It
(hopefully) matches the discussion we had with Bjorn and Arnd.

Applied to pci/host-mvebu for v3.18, thanks! Oh, and I added a stable tag
for v3.12+, since mvebu_get_tgt_attr() appeared in v3.12.

Post by Geert Uytterhoeven
---
drivers/pci/host/pci-mvebu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a..b1315e1 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -873,7 +873,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
rangesz = pna + na + ns;
nranges = rlen / sizeof(__be32) / rangesz;
- for (i = 0; i < nranges; i++) {
+ for (i = 0; i < nranges; i++, range += rangesz) {
u32 flags = of_read_number(range, 1);
u32 slot = of_read_number(range + 1, 1);
u64 cpuaddr = of_read_number(range + na, pna);
@@ -883,14 +883,14 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
rtype = IORESOURCE_IO;
else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
rtype = IORESOURCE_MEM;
+ else
+ continue;
if (slot == PCI_SLOT(devfn) && type == rtype) {
*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
return 0;
}
-
- range += rangesz;
}
return -ENOENT;
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html