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'.
> }
> 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.
Mathieu
> +
> + if (IS_ERR(pmc->base)) {
> + err = PTR_ERR(pmc->base);
> + goto error;
> + }
>
> pmc->clk = devm_clk_get(&pdev->dev, "pclk");
> if (IS_ERR(pmc->clk)) {
> @@ -853,7 +877,9 @@ static int tegra_pmc_probe(struct platform_device *pdev)
> return 0;
>
> error:
> + mutex_lock(&pmc->powergates_lock);
> pmc->base = base;
> + mutex_unlock(&pmc->powergates_lock);
>
> return err;
> }
> --
> 2.1.4
>
> --
> 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
--
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
