On Tue, 29 Jan 2013 10:32:53 -0500, Neil Horman wrote:
> On Tue, Jan 29, 2013 at 03:05:51PM +0100, Jean Delvare wrote:
> > On Mon, 28 Jan 2013 14:43:52 -0500, Neil Horman wrote:
> > > + if (size != I2C_SMBUS_QUICK)
> > > + memcpy(data, dma_buffer,
> > > + min(sizeof(*data), (size_t)I2C_SMBUS_BLOCK_MAX));
> >
> > This computation looks wrong. We already know that sizeof(*data) ==
> > sizeof(union i2c_smbus_data) > I2C_SMBUS_BLOCK_MAX, so the min() above
> > will always return I2C_SMBUS_BLOCK_MAX. Which may be insufficient as
> > the first byte holds the length, and at the same time it will be more
> > than needed in most cases.
>
> Gah! Sorry, you're right, I wasn't paying attention and though that data was
> just an allocated buffer, I need to fix that.
>
> > My original suggestion to limit the size of the copy was meant as a
> > run-time optimization, i.e. copying exactly the number of bytes that
> > were received from the slave. That would be 1 or 2 for non-block reads,
> > and the block length for block reads. Unfortunately I'm not sure is the
> > block length is available. See my comment about this below in
> > ismt_access().
>
> We don't currently have the dma_size, no, but the only calling function computes
> it (either from the block header or sets it appropriately for non-block
> transfers). We can pass that into the process_desc function and fix this.
This won't fix it, unless the hardware happens to update dma_size when
receiving a block from a slave. If not then dma_size will always be
I2C_SMBUS_BLOCK_MAX, while what we need is the number of bytes actually
received.
> > > +static int ismt_access(struct i2c_adapter *adap, u16 addr,
> > > + unsigned short flags, char read_write, u8 command,
> > > + int size, union i2c_smbus_data *data)
> > > +{
> > > + int ret;
> > > + dma_addr_t dma_addr = 0; /* address of the data buffer */
> > > + u8 dma_size = 0;
> > > + enum dma_data_direction dma_direction = 0;
> > > + struct ismt_desc *desc;
> > > + struct ismt_priv *priv = i2c_get_adapdata(adap);
> > > + struct device *dev = &priv->pci_dev->dev;
> > > +
> > > + desc = &priv->hw[priv->head];
> > > +
> > > + /* Initialize the descriptor */
> > > + memset(desc, 0, sizeof(struct ismt_desc));
> > > + desc->tgtaddr_rw = ISMT_DESC_ADDR_RW(addr, read_write);
> > > +
> > > + /* Create a temporary buffer for the DMA transaction */
> > > + /* and insert the command at the beginning of the buffer */
> > > + if ((read_write == I2C_SMBUS_WRITE) &&
> > > + (size != I2C_SMBUS_QUICK)) {
> >
> > If I read the code below properly, this can be skipped for size ==
> > I2C_SMBUS_BYTE too, right?
> I don't think so. The command field contains the data, and that is right below
> the memcpy in that if clause. We can certainly optimize this however, given
> that I need to fix the memcpy sizes anyway.
Precisely, I'm not sure the priv->dma_buffer[0] = command is needed in
this specific case, because later on we have:
desc->control |= ISMT_DESC_CWRL;
desc->wr_len_cmd = command;
and dma_size isn't set, which suggests that priv->dma_buffer won't even
be used. Honestly this "presetting" of priv->dma_buffer causes more
confusion than it helps, and performance-wise it makes little sense, so
I wouldn't object to you exploding it to the relevant code paths in the
switch statement.
I'll take a look at the datasheet tomorrow to check if I got it right
or you did. Or neither of us ^^ I'll also look for test hardware, I
don't have that at home obviously, but at work maybe.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
