On 14/01/16 13:45, Thierry Reding wrote:

* PGP Signed by an unknown key

On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote:

quoted

During early initialisation, the PMC registers are mapped and the PMC SoC
data is populated in the PMC data structure. This allows other drivers
access the PMC register space, via the public tegra PMC APIs, prior to
probing the PMC device.

When the PMC device is probed, the PMC registers are mapped again and if
successful the initial mapping is freed. If the probing of the PMC device
fails after the registers are remapped, then the registers will be
unmapped and hence the pointer to the PMC registers will be invalid. This
could lead to a potential crash, because once the PMC SoC data pointer is
populated, the driver assumes that the PMC register mapping is also valid
and a user calling any of the public tegra PMC APIs could trigger an
exception because these APIs don't check that the mapping is still valid.

Rather than adding a test to see if the PMC register mapping is valid,
fix this by removing the second mapping of the PMC registers and reserve
the memory region for the PMC registers during early initialisation where
the initial mapping is created. During the probing of the PMC simply check
that the PMC registers have been mapped.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

devm_ioremap_resource() was used on purpose to make sure we get a proper
name assigned to the memory region in /proc/iomem. As it is, there will
be no name associated with it, which I think is unfortunate. As such I'd
prefer keeping that call and instead fix the issue with the invalid
mapping by making sure that pmc->base is assigned only after nothing can
fail in probe anymore.

Yes, however, you still get a valid name in /proc/iomem with this
change. I made sure I tested that ...

/ # cat /proc/iomem
6000d000-6000dfff : /gpio@0,6000d000
60020000-600213ff : /dma@0,60020000
700008d4-70000b6f : /pinmux@0,700008d4
70003000-70003293 : /pinmux@0,700008d4
70006000-7000603f : serial
7000d100-7000d1ff : /i2c@0,7000d100
7000e400-7000e7ff : /pmc@0,7000e400
...

The only expection might be the non-DT case, but I am not sure we care
about that for most boards? Hmm ... I wonder if I need to set
"regs.name" for the non-DT case? I probably should ...

quoted

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e60fc2d33c94..fdd1a8d0940f 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c

@@ -807,22 +807,17 @@ out:
 
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
-	void __iomem *base = pmc->base;

The alternative that I proposed above would entail not setting this...

quoted

-	struct resource *res;
 	int err;
 
+	if (!pmc->base) {
+		dev_err(&pdev->dev, "base address is not configured\n");
+		return -ENXIO;
+	}
+
 	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
 	if (err < 0)
 		return err;
 
-	/* take over the memory region from the early initialization */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pmc->base = devm_ioremap_resource(&pdev->dev, res);

... and storing the result of the mapping in "base" instead...

quoted

-	if (IS_ERR(pmc->base))
-		return PTR_ERR(pmc->base);
-
-	iounmap(base);

... and move the unmap to the very end of the probe function, which
would look something like:

	/* take over the memory region from the early initialization */
	iounmap(pmc->base);
	pmc->base = base;

That way the mapping in "base" will automatically be undone upon error
and the pmc->base will only be overridden when it's certain that the
probe will succeed.

What do you think?

I thought about that, but it still seems racy. You probably want to
assign the new base before freeing the old and so you would need another
tmp variable. Even so, I was not sure if there could be a race here.
Ideally, you would lock, but then you need to lock everywhere that you
use base. Given that my patch still provides a /proc/iomem entry with a
valid name, it seems best to me.

Cheers
Jon