linux-kernel - Re: [PATCH 8/8 v3] i2c: i801: enable irq for byte_by

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] [day] [month] [year] [list]

Message-ID: <20120708135337.4fb879b2@endymion.delvare>
Date:	Sun, 8 Jul 2012 13:53:37 +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 8/8 v3] i2c: i801: enable irq for byte_by_byte
 transactions

One more note...

On Wed, 27 Jun 2012 21:54:15 +0800, Daniel Kurtz wrote:
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> (...)
> +static void i801_isr_byte_done(struct i801_priv *priv)
> +{
> +	/* For SMBus block reads, length is first byte read */
> +	if (priv->is_read && ((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
> +	    (priv->count == 0)) {
> +		priv->len = inb_p(SMBHSTDAT0(priv));
> +		if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
> +			dev_err(&priv->pci_dev->dev,
> +				"Illegal SMBus block read size %d\n",
> +				priv->len);
> +			/* FIXME: Recover */
> +			priv->len = I2C_SMBUS_BLOCK_MAX;
> +		} else {
> +			dev_dbg(&priv->pci_dev->dev,
> +				"SMBus block read size is %d\n",
> +				priv->len);
> +		}
> +		priv->data[-1] = priv->len;
> +	} else if (priv->is_read) {
> +		priv->data[priv->count++] = inb(SMBBLKDAT(priv));
> +		/* Set LAST_BYTE for last byte of read transaction */
> +		if (priv->count == priv->len - 1)
> +			priv->cmd |= SMBHSTCNT_LAST_BYTE;
> +		outb_p(priv->cmd, SMBHSTCNT(priv));

This register write doesn't seem to be needed, except when
SMBHSTCNT_LAST_BYTE must be added.

> +	} else if (priv->count < priv->len - 1) {
> +		/* Write next byte, except for IRQ after last byte */
> +		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
> +		outb_p(priv->cmd, SMBHSTCNT(priv));

Same here. I know we do write to SMBHSTCNT after for every byte in
i801_block_transaction_byte_by_byte(), that must be the reason why you
did it in the interrupt path too, but I don't know why we did that and
I don't think we actually had to.

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