So a month or two ago I was having trouble with Atmel’s SAMA5 microprocessor running Linux. Most of the time the entire system would work but there were times the system mysteriously kernel panic or freeze without warning. Actually finding the cause was a bit of adventure but started off when I broke a SMBus slave in the system which crashed the system immediately each time.
SMBus Block Reads
Normally, a SMBus block read is just a combination of two i2c operations with minor logic which consists of
- Write
- Transmit write address
- transmit command byte
- Read
- Transmit read address
- Read length byte
- Read x number of bytes based on length
This is a straight forward I2C operation especially on Linux where you could just combine write and read i2c messages. The lower-level i2c-core does this automatically with write and read message packets when the i2c_smbus_read_block_data function is used. The length byte is the defining item of SMBus blocks as this indicates the data length which can be variable and denotes where the optional checksum exists in the packet. The SMBus specification also defines the maximum block length is 32. It also notes that 0 is not a valid block length.
Digging through i2c-core
First thing I did was to stare off at the functions inside /drivers/i2c/i2c-core.c We were using the userspace interface to the i2c subsystem via ioctl. ioctl is basically a file descriptor wrapper for the kernel functions. The wrapper is located in i2c-dev.c as the function below.
static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
unsigned long arg)
{
struct i2c_smbus_ioctl_data data_arg;
union i2c_smbus_data temp;
int datasize, res;
....
/* Note that command values are always valid! */
if ((data_arg.size == I2C_SMBUS_QUICK) ||
((data_arg.size == I2C_SMBUS_BYTE) &&
(data_arg.read_write == I2C_SMBUS_WRITE)))
/* These are special: we do not use data */
return i2c_smbus_xfer(client->adapter, client->addr,
client->flags, data_arg.read_write,
data_arg.command, data_arg.size, NULL);The relevant part is i2c_smbus_xfer being called after some verification on the data and function being called. Then looking at i2c_smbus_xfer, it is going to call i2c_smbus_xfer_emulated because the i2c-at91 driver does not have hardware smbus. Now the xfer_emulated function does real work.
case I2C_SMBUS_BLOCK_DATA:
if (read_write == I2C_SMBUS_READ) {
msg[1].flags |= I2C_M_RECV_LEN;
msg[1].len = 1; /* block length will be added by
the underlying bus driver */
} else {
...
}Ok so I2C_M_RECV_LEN is set when we have an smbus block read. But what does it do when we are done?
case I2C_SMBUS_BLOCK_DATA:
case I2C_SMBUS_BLOCK_PROC_CALL:
for (i = 0; i < msgbuf1[0] + 1; i++)
data->block[i] = msgbuf1[i];
break;It simply does a basic for loop to copy from msgbuf[1] to the data->block which is returned back to the higher level i2c_smbus_read_block_data function. This is where it got interesting, none of the i2c-core functions validated that the value returned from msgbuf1[0] was actually a valid value. Huh. So the i2c bus driver is responsible for validating that the byte length is valid that gets sent up to i2c-core. That’s nice.
##Digging through i2c-at91 I had to now examine the i2c-at91 driver. The function that gets called is at91_twi_xfer which validataes the i2c message struct passed is valid for the bus driver to execute. The only interesting bit is what the dev->buf and dev->buf_len variables get set to.
....
if (msg->flags & I2C_M_RD) {
dev_err(dev->dev, "first transfer must be write.\n");
return -EINVAL;
}
if (msg->len > 3) {
dev_err(dev->dev, "first message size must be <= 3.\n");
return -EINVAL;
}
/* 1st msg is put into the internal address, start with 2nd */
m_start = &msg[1];
for (i = 0; i < msg->len; ++i) {
const unsigned addr = msg->buf[msg->len - 1 - i];
internal_address |= addr << (8 * i);
int_addr_flag += AT91_TWI_IADRSZ_1;
}
....
dev->buf_len = m_start->len;
dev->buf = m_start->buf;
dev->msg = m_start;
ret = at91_do_twi_transfer(dev);now at91_do_twi_transfer does the bulk of the setup work in the i2c-at91 driver. Inside the driver we will enter this code path since we are doing a read.
} else if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
if (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY) {
dev_err(dev->dev, "RXRDY still set!");
at91_twi_read(dev, AT91_TWI_RHR);
}
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
at91_twi_write(dev, AT91_TWI_CR, start_flags);
/*
* When using dma, the last byte has to be read manually in
* order to not send the stop command too late and then
* to receive extra data. In practice, there are some issues
* if you use the dma to read n-1 bytes because of latency.
* Reading n-2 bytes with dma and the two last ones manually
* seems to be the best solution.
*/
if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
at91_twi_read_data_dma(dev);
/*
* It is important to enable TXCOMP irq here because
* doing it only when transferring the last two bytes
* will mask NACK errors since TXCOMP is set when a
* NACK occurs.
*/
at91_twi_write(dev, AT91_TWI_IER,
AT91_TWI_TXCOMP);
} else
at91_twi_write(dev, AT91_TWI_IER,
AT91_TWI_TXCOMP | AT91_TWI_RXRDY);From that we can tell that we are NOT using the DMA because our buf_len is set to 1. This means it is purely interrupt based fetching. (i.e. TWI_RXRDY being enabled triggers the interrupt). Now to find the interrupt handler which is atmel_twi_interrupt
else if (irqstatus & AT91_TWI_RXRDY)
at91_twi_read_next_byte(dev);Ok, lets look at at91_twi_read_next_byte
static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
{
if (dev->buf_len <= 0)
return;
*dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
--dev->buf_len;
/* handle I2C_SMBUS_BLOCK_DATA */
if (unlikely(dev->msg->flags & I2C_M_RECV_LEN)) {
dev->msg->flags &= ~I2C_M_RECV_LEN;
dev->buf_len += *dev->buf;
dev->msg->len = dev->buf_len + 1;
dev_dbg(dev->dev, "received block length %d\n", dev->buf_len);
}
/* send stop if second but last byte has been read */
if (dev->buf_len == 1)
at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
dev_dbg(dev->dev, "read 0x%x, to go %d\n", *dev->buf, dev->buf_len);
++dev->buf;
}O NO, it really isn’t doing what I think it is, is it??? Yes, it’s just accepting the first byte…blindly. This is going to go well, let’s test the theory!
Testing
I grabbed a package of tools called i2c-tools off github https://github.com/groeck/i2c-tools And quickly hacked it to do smbus_block_reads via block reads.
Now for some fun. I had an SMBus battery attached, it conforms for the most part to the SMBUS 1.1 spec.
So the first thing is validate my smbus read words, read address 0x08 and I expected voltage. Get back 0x0baa and the read is fine.
Then I move onto a valid read string command, read 0x20 and get back 8 bytes which make up a string. All is good.
Now to break read block! I noticed address 0x08 returned 0xAA as the LSB of the word in the first test. Perfect, let’s try read block on that.
Read block on 0x20…. KERNEL PANIC Yep, that’s bad.
Let’s reset and test one more thought, what if the byte length was 0?
Read command 0x00 which I knew would return 0 at the time….. System froze. Oooo, that explains alot.
Solution
In a perfect world, an SMBUS compliant device would always return valid data. Unfortunately that ideal is a stretch and thus data should always be validated. There can even be noise interfering with i2c communications in some systems that may make data unreliable (but there are checksums to take care of that issue).
Implementing a fix was relatively easy, there was just one note about how the I2C peripheral works in the SAMA5. The SAMA5 TWI peripheral has a sort of “eager” approach to i2c handling, where conditions like ACKs/NACKs and stops must be set the interrupt before the last data byte. It can’t just be stopped arbitrarily. The solution was just to modify the function to validate length and just read one more byte(which is discarded). Patch sent to mailing lists and maintainers.