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  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]
Date:	Mon, 24 Mar 2014 23:34:45 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Alexandre Courbot <acourbot@...dia.com>
Cc:	Ben Skeggs <bskeggs@...hat.com>, nouveau@...ts.freedesktop.org,
	dri-devel@...ts.freedesktop.org, linux-tegra@...r.kernel.org,
	linux-kernel@...r.kernel.org, gnurou@...il.com
Subject: Re: [PATCH 06/12] drm/nouveau/ibus: add GK20A support

On Mon, Mar 24, 2014 at 05:42:28PM +0900, Alexandre Courbot wrote:
[...]
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
[...]
> +#include <subdev/ibus.h>
> +
> +struct nvea_ibus_priv {
> +	struct nouveau_ibus base;
> +};
> +
> +static void
> +nvea_ibus_init_priv_ring(struct nvea_ibus_priv *priv)
> +{
> +	nv_mask(priv, 0x137250, 0x3f, 0);
> +
> +	nv_mask(priv, 0x000200, 0x20, 0);
> +	udelay(20);

usleep_range()?

> +static void
> +nvea_ibus_intr(struct nouveau_subdev *subdev)
> +{
[...]
> +	/* Acknowledge interrupt */
> +	nv_mask(priv, 0x12004c, 0x2, 0x2);
> +
> +	while (--retry >= 0) {
> +		command = nv_rd32(priv, 0x12004c) & 0x3f;
> +		if (command == 0)
> +			break;
> +	}
> +
> +	if (retry < 0)
> +		nv_warn(priv, "timeout waiting for ringmaster ack\n");
> +}

Perhaps I'm being paranoid, but this loop now depends on the frequency
of the various clocks involved and therefore might break at some point
if the frequencies get sufficiently high.

So a slightly safer implementation would use a proper timeout using a
combination of msecs_to_jiffies(), time_before() and usleep_range(),
like so:

	timeout = jiffies + msecs_to_jiffies(...);

	while (time_before(jiffies, timeout)) {
		command = nv_rd32(...) & 0x3f;
		if (command == 0)
			break;

		usleep_range(...);
	}

	if (time_after(jiffies, timeout))
		nv_warn(...);

This assumes that there's some known timeout after which the ringmaster
is expected to have acked the interrupt. On that note, I wonder if the
warning is accurate here: it's my understanding that writing 0x2 to the
register does acknowledge the interrupt, so the ringmaster does in fact
"clear" it rather than "acknowledge" it, doesn't it?

Although now that I mention it I seem to remember that this write is
actually sending a command to the ring master and perhaps waiting for
the register to return to 0 is indeed waiting for an ACK of sorts. Maybe
adding a comment or so describing what this sequence does would be
appropriate here?

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists