linux-kernel - Re: [PATCH 2/8 v3] i2c: i801: optimize waiting for HWPEC to finish

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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 27 Jun 2012 16:58:48 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Daniel Kurtz <djkurtz@...omium.org>
Cc:	Ben Dooks <ben-linux@...ff.org>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Seth Heasley <seth.heasley@...el.com>,
	Olof Johansson <olof@...om.net>,
	Benson Leung <bleung@...omium.org>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/8 v3] i2c: i801: optimize waiting for HWPEC to finish

Hi Daniel,

On Wed, 27 Jun 2012 21:54:09 +0800, Daniel Kurtz wrote:
> When a transaction has finished (including the PEC), the SMBus controller
> sets the INTR bit.
> Slightly optimize the polling loop by reading status before the first
> sleep.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@...omium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 51e11eb..8b74e1e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -291,11 +291,11 @@ static void i801_wait_hwpec(struct i801_priv *priv)
>  	int timeout = 0;
>  	int status;
>  
> -	do {
> +	status = inb_p(SMBHSTSTS(priv));
> +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>  		usleep_range(250, 500);
>  		status = inb_p(SMBHSTSTS(priv));
> -	} while ((!(status & SMBHSTSTS_INTR))
> -		 && (timeout++ < MAX_RETRIES));
> +	}
>  
>  	if (timeout > MAX_RETRIES)
>  		dev_dbg(&priv->pci_dev->dev, "PEC Timeout!\n");

Hmm, I would be very cautious here. Deep memories whisper to me
that we have sometimes done it that way in the past due to hardware
bugs. I think it was for the PIIX4 and not the 82801, but I wouldn't be
surprised if early 82801 chips had some hardware bugs as well.

Also note that optimizing the polling code isn't a priority when you're
about to enable interrupts for most chips supported by the driver. If
anything, leaving the polling code unchanged for now could even be a
goal, to make sure that users can disable interrupts if that doesn't
work for them, and things keep working as before.

Plus, PEC isn't used that much (neither you nor me can test it). And
lastly, are you really certain that there is a benefit? Are you sure
that the SMBus controller is always faster to receive and process the
PEC byte than the CPU is to reach the INTR check?

Unless you have actual numbers showing a significant improvement, I'd
rather postpone this change.

-- 
Jean Delvare
--
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/