On 29/01/16 16:20, Mathieu Poirier wrote:
> On 28 January 2016 at 09:33, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>> The PMC base address pointer is initialised during early boot so that
>> early platform code may used the PMC public functions. During the probe
>> of the PMC driver the base address pointer is mapped again and the initial
>> mapping is freed. This exposes a window where a device accessing the PMC
>> registers via one of the public functions, could race with the updating
>> of the pointer and lead to a invalid access. Furthermore, the only
>> protection between multiple devices attempting to access the PMC registers
>> is when setting the powergate state to on or off. None of the other public
>> functions that access the PMC registers are protected.
>>
>> Use the existing mutex to protect paths that may race with regard to
>> accessing the PMC registers.
>>
>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>> ---
>> drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 85b4e166273a..f8cdb7ce9755 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id)
>> if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> return -EINVAL;
>>
>> + mutex_lock(&pmc->powergates_lock);
>> status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
>> + mutex_unlock(&pmc->powergates_lock);
>> +
>> return !!status;
>> }
>>
>> @@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id)
>> if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> return -EINVAL;
>>
>> + mutex_lock(&pmc->powergates_lock);
>> +
>> /*
>> * On Tegra124 and later, the clamps for the GPU are controlled by a
>> * separate register (with different semantics).
>> @@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id)
>> if (id == TEGRA_POWERGATE_3D) {
>> if (pmc->soc->has_gpu_clamps) {
>> tegra_pmc_writel(0, GPU_RG_CNTRL);
>> - return 0;
>> + goto out;
>> }
>> }
>>
>> @@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id)
>>
>> tegra_pmc_writel(mask, REMOVE_CLAMPING);
>>
>> +out:
>> + mutex_unlock(&pmc->powergates_lock);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(tegra_powergate_remove_clamping);
>> @@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id)
>> unsigned int bit, mask;
>> int err;
>>
>> + mutex_lock(&pmc->powergates_lock);
>> +
>> err = tegra_io_rail_prepare(id, &request, &status, &bit);
>> if (err < 0)
>> - return err;
>> + goto error;
>>
>> mask = 1 << bit;
>>
>> @@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id)
>> err = tegra_io_rail_poll(status, mask, 0, 250);
>> if (err < 0) {
>> pr_info("tegra_io_rail_poll() failed: %d\n", err);
>> - return err;
>> + goto error;
>> }
>>
>> tegra_io_rail_unprepare();
>>
>> - return 0;
>> +error:
>> + mutex_unlock(&pmc->powergates_lock);
>> +
>> + return err < 0 ? err : 0;
>
> Is this necessary? Why simply not returning 'err'? From what I see
> 'tegra_io_rail_power_on()' can only return a negative value or '0'.
Right, this is probably not necessary and so I could simplify this.
>> }
>> EXPORT_SYMBOL(tegra_io_rail_power_on);
>>
>> @@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id)
>> unsigned int bit, mask;
>> int err;
>>
>> + mutex_lock(&pmc->powergates_lock);
>> +
>> err = tegra_io_rail_prepare(id, &request, &status, &bit);
>> if (err < 0) {
>> pr_info("tegra_io_rail_prepare() failed: %d\n", err);
>> - return err;
>> + goto error;
>> }
>>
>> mask = 1 << bit;
>> @@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id)
>>
>> err = tegra_io_rail_poll(status, mask, mask, 250);
>> if (err < 0)
>> - return err;
>> + goto error;
>>
>> tegra_io_rail_unprepare();
>>
>> - return 0;
>> +error:
>> + mutex_unlock(&pmc->powergates_lock);
>> +
>> + return err < 0 ? err : 0;
>
> Same comment as above.
>
>> }
>> EXPORT_SYMBOL(tegra_io_rail_power_off);
>>
>> @@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>>
>> /* take over the memory region from the early initialization */
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + mutex_lock(&pmc->powergates_lock);
>> pmc->base = devm_ioremap_resource(&pdev->dev, res);
>> - if (IS_ERR(pmc->base))
>> - return PTR_ERR(pmc->base);
>> + mutex_unlock(&pmc->powergates_lock);
>
> Since the mutex is released there is a window of opportunity for
> devices to access an erroneous pointer. A better approach might be to
> use a temporary variable, do all the initialisation that is required
> and when things look good set pmc-base to that temporary variable.
Thanks. Not sure what I was thinking here. I will fix that.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
