| 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
| ||
|
Message-ID: <20071103114653.57e0b479@hyperion.delvare>
Date: Sat, 3 Nov 2007 11:46:53 +0100
From: Jean Delvare <khali@...ux-fr.org>
To: Bryan Wu <bryan.wu@...log.com>
Cc: i2c@...sensors.org, linux-kernel@...r.kernel.org,
Sonic Zhang <sonic.zhang@...log.com>
Subject: Re: [PATCH 1/4] Blackfin I2C/TWI driver: Add repeat start feature
to avoid break of a bundle of i2c master xfer operation.
Hi Bryan,
Thanks for splitting the patches as requested. Here's a more complete
review:
On Fri, 2 Nov 2007 14:24:21 +0800, Bryan Wu wrote:
> From: Sonic Zhang <sonic.zhang@...log.com>
>
> - Create a new mode TWI_I2C_MODE_REPEAT.
> - No change to smbus operation.
I don't understand the difference between TWI_I2C_MODE_COMBINED and
TWI_I2C_MODE_REPEAT. Both use RSTART to send multiple data chunks at
once. Can you please explain the difference?
>
> Signed-off-by: Sonic Zhang <sonic.zhang@...log.com>
> Signed-off-by: Bryan Wu <bryan.wu@...log.com>
> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 179 +++++++++++++++++++++++--------------
> 1 files changed, 111 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 67224a4..6535852 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -42,6 +42,7 @@
> #define TWI_I2C_MODE_STANDARD 0x01
> #define TWI_I2C_MODE_STANDARDSUB 0x02
> #define TWI_I2C_MODE_COMBINED 0x04
> +#define TWI_I2C_MODE_REPEAT 0x08
This is strange (and possibly confusing) to use values that make the
mode look like a bitfield, when all the modes are actually mutually
exclusive and can't be combined. Admittedly these values are arbitrary
and there's no bug, but it would still make more sense to number the
modes linearly.
>
> struct bfin_twi_iface {
> int irq;
> @@ -58,6 +59,9 @@ struct bfin_twi_iface {
> struct timer_list timeout_timer;
> struct i2c_adapter adap;
> struct completion complete;
> + struct i2c_msg *pmsg;
> + int msg_num;
> + int cur_msg;
> };
>
> static struct bfin_twi_iface twi_iface;
> @@ -76,12 +80,16 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> /* start receive immediately after complete sending in
> * combine mode.
> */
> - else if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
> + else if (iface->cur_mode == TWI_I2C_MODE_COMBINED)
> bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> | MDIR | RSTART);
> - } else if (iface->manual_stop)
> + else if (iface->manual_stop)
> bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> | STOP);
> + else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> + iface->cur_msg+1 < iface->msg_num)
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> + | RSTART);
> SSYNC();
> /* Clear status */
> bfin_write_TWI_INT_STAT(XMTSERV);
> @@ -108,6 +116,11 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> | STOP);
> SSYNC();
> + } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> + iface->cur_msg+1 < iface->msg_num) {
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> + | RSTART);
> + SSYNC();
> }
> /* Clear interrupt source */
> bfin_write_TWI_INT_STAT(RCVSERV);
> @@ -119,7 +132,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> bfin_write_TWI_MASTER_STAT(0x3e);
> bfin_write_TWI_MASTER_CTL(0);
> SSYNC();
> - iface->result = -1;
> + iface->result = -EIO;
> /* if both err and complete int stats are set, return proper
> * results.
> */
> @@ -170,6 +183,42 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() |
> MEN | MDIR);
> SSYNC();
> + } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> + iface->cur_msg+1 < iface->msg_num) {
> + iface->cur_msg++;
> + iface->transPtr = iface->pmsg[iface->cur_msg].buf;
> + iface->writeNum = iface->readNum =
> + iface->pmsg[iface->cur_msg].len;
> + /* Set Transmit device address */
> + bfin_write_TWI_MASTER_ADDR(
> + iface->pmsg[iface->cur_msg].addr);
> + if (iface->pmsg[iface->cur_msg].flags & I2C_M_RD)
> + iface->read_write = I2C_SMBUS_READ;
> + else {
> + iface->read_write = I2C_SMBUS_WRITE;
> + /* Transmit first data */
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8(
> + *(iface->transPtr++));
> + iface->writeNum--;
> + SSYNC();
> + }
> + }
> +
> + if (iface->pmsg[iface->cur_msg].len <= 255)
> + bfin_write_TWI_MASTER_CTL(
> + iface->pmsg[iface->cur_msg].len << 6);
> + else if (iface->pmsg[iface->cur_msg].len > 255) {
This test will always succeed, so a simple "else" is enough.
> + bfin_write_TWI_MASTER_CTL(0xff << 6);
> + iface->manual_stop = 1;
> + }
Does this mean that the Blackfin TWI controller doesn't support
messages of more than 255 bytes? If so, you should return an error
right away when this happens, rather than silently truncating the
message.
> + /* remove restart bit and enable master receive */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() &
> + ~RSTART);
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() |
> + MEN | ((iface->read_write == I2C_SMBUS_READ) ?
> + MDIR : 0));
> + SSYNC();
> } else {
> iface->result = 1;
> bfin_write_TWI_INT_MASK(0);
> @@ -221,7 +270,6 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
> {
> struct bfin_twi_iface *iface = adap->algo_data;
> struct i2c_msg *pmsg;
> - int i, ret;
> int rc = 0;
>
> if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> @@ -231,81 +279,76 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
> yield();
> }
>
> - ret = 0;
> - for (i = 0; rc >= 0 && i < num; i++) {
> - pmsg = &msgs[i];
> - if (pmsg->flags & I2C_M_TEN) {
> - dev_err(&(adap->dev), "i2c-bfin-twi: 10 bits addr "
> - "not supported !\n");
> - rc = -EINVAL;
> - break;
> - }
> + iface->pmsg = msgs;
> + iface->msg_num = num;
> + iface->cur_msg = 0;
>
> - iface->cur_mode = TWI_I2C_MODE_STANDARD;
> - iface->manual_stop = 0;
> - iface->transPtr = pmsg->buf;
> - iface->writeNum = iface->readNum = pmsg->len;
> - iface->result = 0;
> - iface->timeout_count = 10;
> - /* Set Transmit device address */
> - bfin_write_TWI_MASTER_ADDR(pmsg->addr);
> -
> - /* FIFO Initiation. Data in FIFO should be
> - * discarded before start a new operation.
> - */
> - bfin_write_TWI_FIFO_CTL(0x3);
> - SSYNC();
> - bfin_write_TWI_FIFO_CTL(0);
> - SSYNC();
> + pmsg = &msgs[0];
> + if (pmsg->flags & I2C_M_TEN) {
> + dev_err(&adap->dev, "10 bits addr not supported!\n");
> + return -EINVAL;
> + }
>
> - if (pmsg->flags & I2C_M_RD)
> - iface->read_write = I2C_SMBUS_READ;
> - else {
> - iface->read_write = I2C_SMBUS_WRITE;
> - /* Transmit first data */
> - if (iface->writeNum > 0) {
> - bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> - iface->writeNum--;
> - SSYNC();
> - }
> + iface->cur_mode = TWI_I2C_MODE_REPEAT;
> + iface->manual_stop = 0;
> + iface->transPtr = pmsg->buf;
> + iface->writeNum = iface->readNum = pmsg->len;
> + iface->result = 0;
> + iface->timeout_count = 10;
> + /* Set Transmit device address */
> + bfin_write_TWI_MASTER_ADDR(pmsg->addr);
> +
> + /* FIFO Initiation. Data in FIFO should be
> + * discarded before start a new operation.
> + */
> + bfin_write_TWI_FIFO_CTL(0x3);
> + SSYNC();
> + bfin_write_TWI_FIFO_CTL(0);
> + SSYNC();
> +
> + if (pmsg->flags & I2C_M_RD)
> + iface->read_write = I2C_SMBUS_READ;
> + else {
> + iface->read_write = I2C_SMBUS_WRITE;
> + /* Transmit first data */
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> + iface->writeNum--;
> + SSYNC();
> }
> + }
>
> - /* clear int stat */
> - bfin_write_TWI_INT_STAT(MERR|MCOMP|XMTSERV|RCVSERV);
> + /* clear int stat */
> + bfin_write_TWI_INT_STAT(MERR | MCOMP | XMTSERV | RCVSERV);
>
> - /* Interrupt mask . Enable XMT, RCV interrupt */
> - bfin_write_TWI_INT_MASK(MCOMP | MERR |
> - ((iface->read_write == I2C_SMBUS_READ)?
> - RCVSERV : XMTSERV));
> - SSYNC();
> + /* Interrupt mask . Enable XMT, RCV interrupt */
> + bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
> + SSYNC();
>
> - if (pmsg->len > 0 && pmsg->len <= 255)
> - bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
> - else if (pmsg->len > 255) {
> - bfin_write_TWI_MASTER_CTL(0xff << 6);
> - iface->manual_stop = 1;
> - } else
> - break;
> + if (pmsg->len <= 255)
> + bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
> + else if (pmsg->len > 255) {
> + bfin_write_TWI_MASTER_CTL(0xff << 6);
> + iface->manual_stop = 1;
> + }
Same as above: if you can't achieve the transaction, return an error.
>
> - iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
> - add_timer(&iface->timeout_timer);
> + iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
> + add_timer(&iface->timeout_timer);
>
> - /* Master enable */
> - bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> - ((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
> - ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
> - SSYNC();
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> + ((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
> + ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
> + SSYNC();
>
> - wait_for_completion(&iface->complete);
> + wait_for_completion(&iface->complete);
>
> - rc = iface->result;
> - if (rc == 1)
> - ret++;
> - else if (rc == -1)
> - break;
> - }
> + rc = iface->result;
>
> - return ret;
> + if (rc == 1)
> + return num;
> + else
> + return rc;
> }
>
> /*
--
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/
Powered by blists - more mailing lists