On 14/01/16 17:24, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Jan 14, 2016 at 04:35:28PM +0000, Jon Hunter wrote:

quoted

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

quoted

quoted

Old 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(-)

[snip]

Ah yes, of course. You could still do it with just the two pointers if
you keep the code as-is and revert back to the backed up value in case
of errors. Along this line:

	base = pmc->base;

	pmc->base = devm_ioremap_resource(&pdev->dev, res);

	/* on success */
	iounmap(base);

	/* on error */
	pmc->base = base;

These pointer assignments should be atomic, so no potential for races
there. I've attached a patch which should do the trick, though I have
not tested it.

Right, but I am concerned about someone calling tegra_powergate_set()
(which with patch 6 of this series) will poll for the state to change. I
am not sure we can guarantee the pointer does not change while this is
happening.

quoted

              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.

The primary reason for the "takeover" is that except for the extra
iounmap() and pointer swapping the tegra_pmc_probe() function is really
a standard driver implementation. The idea behind this had always been
that it should be possible to easily convert this to a proper driver if
either we ended up with PSCI exclusively for SMP or defer SMP setup
until the PMC driver had been probed, so that we could get rid of the
early_initcall().

I agree it is cleaner, however, I am still concerned there could still
be a race.

Jon