Message ID | 20130412093003.GE30416@pengutronix.de |
---|---|
State | RFC |
Headers | show |
Hi Uwe, please prefer wsa@the-dreams.de for kernel stuff. > - Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without > supporting I2C_M_RECV_LEN? Yes. Check the #define and you'll see that it does not contain BLOCK_READ and BLOCK_PROC_CALL. All the best, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 12, 2013 at 05:37:57PM +0200, Wolfram Sang wrote: > please prefer wsa@the-dreams.de for kernel stuff. The reason I used wolfram@... is probably: $ for addr in sa olfram; do printf "%s: " $addr; git log --oneline --grep=w$addr@the-dreams.de v3.9-rc6 | wc -l; done sa: 7 olfram: 35 But I see the recent commits use "sa" so probably I will do that next time, too. > > - Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without > > supporting I2C_M_RECV_LEN? > > Yes. Check the #define and you'll see that it does not contain > BLOCK_READ and BLOCK_PROC_CALL. Ah, ok. But then there is a different problem: Even though "my" driver only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling i2c_smbus_read_block_data in userspace results in .master_xfer being called with I2C_M_RECV_LEN set. Best regards Uwe
Hi Uwe, > The reason I used wolfram@... is probably: Looking up MAINTAINERS would have done it :) > Ah, ok. But then there is a different problem: Even though "my" driver > only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling > i2c_smbus_read_block_data in userspace results in .master_xfer being > called with I2C_M_RECV_LEN set. From Documentation/i2c/functionality: Because not every I2C or SMBus adapter implements everything in the I2C specifications, a client can not trust that everything it needs is implemented when it is given the option to attach to an adapter: the client needs some way to check whether an adapter has the needed functionality... Regards, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Uwe, CCing Lucas Stach too, are you two not in the same office now ? ;-) > Hello, > > I'm currently trying to teach the i2c-mxs driver to handle the > I2C_M_RECV_LEN flag but failed up to now. Maybe someone has an good idea > how to make it running? > > My current patch looks as follows: Try basing your patch on top of Lucas's, they looked very reasonable. I think WS should have applied them by now. Wolfram, what's the status of those three mxs- i2c patches from Lucas, can you tell? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wolfram, On Sun, Apr 14, 2013 at 01:57:58PM +0200, Wolfram Sang wrote: > > Ah, ok. But then there is a different problem: Even though "my" driver > > only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling > > i2c_smbus_read_block_data in userspace results in .master_xfer being > > called with I2C_M_RECV_LEN set. > > From Documentation/i2c/functionality: > > Because not every I2C or SMBus adapter implements everything in the > I2C specifications, a client can not trust that everything it needs > is implemented when it is given the option to attach to an adapter: > the client needs some way to check whether an adapter has the needed > functionality... While add support for I2C_M_RECV_LEN I forgot to write the length data to the first byte in the message buffer which happend to be initialized with 0xff. This made i2c_smbus_xfer_emulated copy 255 bytes to data->block overflowing the array and so resulting in stack curruption. I think the same could be accomplished with a non-broken driver (e.g. by calling i2c_smbus_read_block_data for an eeprom that is interpreted as a 1 byte read by the i2c bus driver. If the read byte is big enough the same stack curruption occurs). So IMHO the i2c core should be a bit more careful here and either not let i2c_smbus_xfer_emulated call the xfer callback of a driver that is not capable to handle I2C_M_RECV_LEN with a message that has this bit set or at least assert that data->block isn't written to out of bounds. Best regards Uwe
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -1,3 +1,4 @@ +#define DEBUG /* * Freescale MXS I2C bus driver * @@ -289,14 +290,23 @@ write_init_pio_fail: static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c) { unsigned long timeout = jiffies + msecs_to_jiffies(1000); + u32 reg, reg_old; + int first = 1; - while (!(readl(i2c->regs + MXS_I2C_DEBUG0) & + while (!(reg = readl(i2c->regs + MXS_I2C_DEBUG0) & MXS_I2C_DEBUG0_DMAREQ)) { + if (first || reg != reg_old) { + pr_debug("%s: I2C_DEBUG = 0x%08x\n", __func__, reg); + first = 0; + reg_old = reg; + } if (time_after(jiffies, timeout)) return -ETIMEDOUT; cond_resched(); } + pr_debug("%s: I2C_DEBUG = 0x%08x\n", __func__, reg); + writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR); return 0; @@ -338,6 +348,9 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR); if (msg->flags & I2C_M_RD) { + int readlen = msg->len; + size_t buf_offset = 0; + addr_data |= I2C_SMBUS_READ; /* SELECT command. */ @@ -354,19 +367,39 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, if (ret) return ret; + if (msg->flags & I2C_M_RECV_LEN) { + pr_debug("read first byte\n"); + writel(MXS_I2C_CTRL0_RUN | MXS_I2C_CTRL0_MASTER_MODE | + MXS_I2C_CTRL0_XFER_COUNT(1), + i2c->regs + MXS_I2C_CTRL0); + + ret = mxs_i2c_pio_wait_dmareq(i2c); + if (ret) + return ret; + + data = readl(i2c->regs + MXS_I2C_DATA) & 0xff; + if (data > I2C_SMBUS_BLOCK_MAX) + return -EPROTO; + + msg->len += data; + readlen += data - 1; + } + + pr_debug("%s: data = %u, msg->len = %u, readlen = %u\n", __func__, data, msg->len, readlen); + /* READ command. */ writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags | - MXS_I2C_CTRL0_XFER_COUNT(msg->len), + MXS_I2C_CTRL0_XFER_COUNT(readlen), i2c->regs + MXS_I2C_CTRL0); - for (i = 0; i < msg->len; i++) { + for (i = 0; i < readlen; i++) { if ((i & 3) == 0) { ret = mxs_i2c_pio_wait_dmareq(i2c); if (ret) return ret; data = readl(i2c->regs + MXS_I2C_DATA); } - msg->buf[i] = data & 0xff; + msg->buf[buf_offset + i] = data & 0xff; data >>= 8; } } else { @@ -438,6 +471,9 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, * is set to 8 bytes, transfers shorter than 8 bytes are transfered * using PIO mode while longer transfers use DMA. The 8 byte border is * based on this empirical measurement and a lot of previous frobbing. + * + * Note that only mxs_i2c_pio_setup_xfer has support for I2C_M_RECV_LEN. + * That is OK as msg->len is <= 2 when that flag is set. */ if (msg->len < 8) { ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); @@ -497,6 +533,8 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id) struct mxs_i2c_dev *i2c = dev_id; u32 stat = readl(i2c->regs + MXS_I2C_CTRL1) & MXS_I2C_IRQ_MASK; + pr_debug("%s: stat = 0x%08x\n", __func__, stat); + if (!stat) return IRQ_NONE;