linux-kernel - Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

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: <20060807140222.GG4032@ucw.cz>
Date:	Mon, 7 Aug 2006 14:02:23 +0000
From:	Pavel Machek <pavel@...e.cz>
To:	Shem Multinymous <multinymous@...il.com>
Cc:	Robert Love <rlove@...ve.org>, Jean Delvare <khali@...ux-fr.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org, hdaps-devel@...ts.sourceforge.net
Subject: Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

Hi!

> The current hdaps driver queries the hardware on (almost) any sysfs
> read, reading just the information it needs and discarding the rest
> This is inefficient, because every hardware query actually gives all 
> information. It also means we're losing data, because readouts are
> offered by the hardware at a constant rate and each query "eats up"
> a readout. It also results in unnecessarily complex code.
> 
> This patch moves all hardware value reading+parsing to a single 
> function, __hdaps_update(). All values are cached, and easily 
> referenced afterwards. This function is still invoked on every sysfs 
> read. This will be fixed in a later patch.
> 
> Signed-off-by: Shem Multinymous <multinymous@...il.com>
Signed-off-by: Pavel Machek <pavel@...e.cz>

> +/* __hdaps_update - read current state and update global state variables.
> + * Also prefetches the next read, to reduce udelay busy-waiting.
> + * If fast!=0, do one quick attempt without retries.
> + * Caller must hold controller lock. 
>   */

Linuxdoc, please.

> +	/* Parse position data: */
> +	pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1) * (hdaps_invert?-1:1);
> +	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1) * (hdaps_invert?-1:1);
> +
> +	/* Parse so-called "variance" data: */
> +	var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2) * (hdaps_invert?-1:1);
> +	var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2) * (hdaps_invert?-1:1);

Perhaps hdaps_invert should already have 1/-1 values.

>  {
> -	int ret = thinkpad_ec_lock();
> +	int ret;
> +	ret = thinkpad_ec_lock();

I actually liked the previous version more, and this change does not
really belong here.

						Pavel
-- 
Thanks for all the (sleeping) penguins.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/