| Message ID | 20241209060422.1021512-1-zhangjian.3032@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: slave-eeprom: add latch mode | expand |
On Mon, Dec 09, 2024 at 02:04:21PM +0800, Jian Zhang wrote: > The read operation is locked by byte, while the write operation is > locked by block (or based on the amount of data written). If we need to > ensure the integrity of a "block" of data that the other end can read, > then we need a latch mode, lock the buffer when a read operation is > requested. I don't really understand what you want to fix here. Does this patch really fix your issue because... > switch (event) { > case I2C_SLAVE_WRITE_RECEIVED: > + if (eeprom->latch) { > + spin_lock(&eeprom->buffer_lock); > + memcpy(eeprom->buffer_latch, eeprom->buffer, eeprom->bin.size); > + spin_unlock(&eeprom->buffer_lock); > + } ... what advantage brings you this memcpy of the buffer to a latch after every single byte is received? > + if (of_property_read_bool(client->adapter->dev.of_node, "use-latch")) { If there really is a problem, we don't need a binding for it but should use the fix in all cases.
On Tue, May 20, 2025 at 1:03 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > On Mon, Dec 09, 2024 at 02:04:21PM +0800, Jian Zhang wrote: > > The read operation is locked by byte, while the write operation is > > locked by block (or based on the amount of data written). If we need to > > ensure the integrity of a "block" of data that the other end can read, > > then we need a latch mode, lock the buffer when a read operation is > > requested. > > I don't really understand what you want to fix here. Does this patch > really fix your issue because... The scenario I’m dealing with: * I’m using this driver to simulate an EEPROM device. * One byte of the EEPROM content contains the CRC of the preceding data. * Each time I update the EEPROM data, I use i2c_slave_eeprom_bin_write to write the entire buffer, so the data in memory is always consistent. * I expect the I2C master (peer) to be able to perform a block read and get the full, correct data including a valid CRC. The problem I’ve encountered: * In i2c_slave_eeprom_slave_cb, a block read from the master triggers multiple callbacks, each returning one byte. This results in a sequence like: 1. Master sends a write 2. Master reads the first byte. 3. The EEPROM buffer is updated. 4, Master reads the second byte. This may lead to a mismatch during a single block read, where the master receives data that is partially from the, old buffer and partially from the new one—causing CRC validation to fail. > > > switch (event) { > > case I2C_SLAVE_WRITE_RECEIVED: > > + if (eeprom->latch) { > > + spin_lock(&eeprom->buffer_lock); > > + memcpy(eeprom->buffer_latch, eeprom->buffer, eeprom->bin.size); > > + spin_unlock(&eeprom->buffer_lock); > > + } > > ... what advantage brings you this memcpy of the buffer to a latch after > every single byte is received? If you agree that the scenario I described earlier is a valid case worth considering, I make a copy of the buffer at the beginning of a write operation, and then use this latched buffer for all subsequent reads until the STOP condition. Sorry, I think I placed the copy logic in the wrong spot earlier. Perhaps it would be more appropriate to do it in the I2C_SLAVE_WRITE_REQUESTED callback. > > > + if (of_property_read_bool(client->adapter->dev.of_node, "use-latch")) { > > If there really is a problem, we don't need a binding for it but should > use the fix in all cases. > i got it, thanks.
diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c index 5946c0d0aef9..29246ac7d350 100644 --- a/drivers/i2c/i2c-slave-eeprom.c +++ b/drivers/i2c/i2c-slave-eeprom.c @@ -33,7 +33,9 @@ struct eeprom_data { u16 address_mask; u8 num_address_bytes; u8 idx_write_cnt; + bool latch; bool read_only; + u8 *buffer_latch; u8 buffer[]; }; @@ -49,6 +51,11 @@ static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, switch (event) { case I2C_SLAVE_WRITE_RECEIVED: + if (eeprom->latch) { + spin_lock(&eeprom->buffer_lock); + memcpy(eeprom->buffer_latch, eeprom->buffer, eeprom->bin.size); + spin_unlock(&eeprom->buffer_lock); + } if (eeprom->idx_write_cnt < eeprom->num_address_bytes) { if (eeprom->idx_write_cnt == 0) eeprom->buffer_idx = 0; @@ -69,7 +76,10 @@ static int i2c_slave_eeprom_slave_cb(struct i2c_client *client, fallthrough; case I2C_SLAVE_READ_REQUESTED: spin_lock(&eeprom->buffer_lock); - *val = eeprom->buffer[eeprom->buffer_idx & eeprom->address_mask]; + if (eeprom->latch) + *val = eeprom->buffer_latch[eeprom->buffer_idx & eeprom->address_mask]; + else + *val = eeprom->buffer[eeprom->buffer_idx & eeprom->address_mask]; spin_unlock(&eeprom->buffer_lock); /* * Do not increment buffer_idx here, because we don't know if @@ -152,6 +162,17 @@ static int i2c_slave_eeprom_probe(struct i2c_client *client) if (!eeprom) return -ENOMEM; + if (of_property_read_bool(client->adapter->dev.of_node, "use-latch")) { + dev_info(&client->dev, "Using latch mode"); + eeprom->latch = true; + eeprom->buffer_latch = devm_kzalloc(&client->dev, size, GFP_KERNEL); + if (!eeprom->buffer_latch) + return -ENOMEM; + } else { + eeprom->buffer_latch = NULL; + eeprom->latch = false; + } + eeprom->num_address_bytes = flag_addr16 ? 2 : 1; eeprom->address_mask = size - 1; eeprom->read_only = FIELD_GET(I2C_SLAVE_FLAG_RO, id->driver_data);
The read operation is locked by byte, while the write operation is locked by block (or based on the amount of data written). If we need to ensure the integrity of a "block" of data that the other end can read, then we need a latch mode, lock the buffer when a read operation is requested. Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com> --- drivers/i2c/i2c-slave-eeprom.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)