linux-kernel - Re: [PATCH v6 09/12] drm/tegra: Reset the SOR on probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150324102447.GA18115@ulmo.nvidia.com>
Date:	Tue, 24 Mar 2015 11:24:49 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	linux-tegra@...r.kernel.org,
	Terje Bergström <tbergstrom@...dia.com>,
	David Airlie <airlied@...ux.ie>,
	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 09/12] drm/tegra: Reset the SOR on probe

On Wed, Mar 18, 2015 at 10:52:24AM +0100, Tomeu Vizoso wrote:
> As there isn't a way for the firmware on the Nyan chromebooks to hand
> over the display to the kernel.

Perhaps this should be more explicit. I'm assuming this somehow breaks
on Nyan for you because we try to reprogram the SOR from an already
initialized state and that doesn't actually work? Your current commit
message doesn't make this sound like a fix for an actual problem.

> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> Acked-by: Stephen Warren <swarren@...dia.com>
> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
> ---
>  drivers/gpu/drm/tegra/sor.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 2afe478..e6caacc 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -1458,6 +1458,20 @@ static int tegra_sor_probe(struct platform_device *pdev)
>  
>  	mutex_init(&sor->lock);
>  
> +	err = reset_control_assert(sor->rst);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to assert SOR reset: %d\n", err);
> +		return err;
> +	}
> +
> +	msleep(20);
> +
> +	err = reset_control_deassert(sor->rst);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to deassert SOR reset: %d\n", err);
> +		return err;
> +	}

If you do this in ->probe() there's a potentially long time where the
screen will remain black. A better location for this is probably in the
tegra_sor_init() function. That puts it closer to where the output is
reinitialized and potentially reduces the time where no display is
available.

Also it is a more logical place for this code, since the clocks are also
enabled in tegra_sor_init(). Technically resets on Tegra are synchronous
and therefore the reset should happen across the clk_prepare_enable()
call of the primary module clock. So I'd expect something like this:

	err = reset_control_assert(sor->rst);
	...

	err = clk_prepare_enable(sor->clk);
	...

	/* sleep */

	err = reset_control_deassert(sor->rst);
	...

Where sleep can probably be significantly shorter than 20 ms. I think
the documentation says that 2 us are typically enough. That said, I'm
fine with leaving in 20 ms, it shouldn't matter much in the overall
boot-time.

Also, since this will need to be revisited once we have proper hand-over
from firmware to kernel, do you mind adding a comment along these lines:

	/*
	 * XXX: Remove this reset once proper hand-over from firmware to
	 * kernel is possible.
	 */

?

Thanks,
Thierry

Content of type "application/pgp-signature" skipped