Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices

2015-12-18 Thread Aaron Sierra 
- Original Message -
> From: "Wolfram Sang" 
> Sent: Friday, December 11, 2015 7:19:40 AM
> 
> On Mon, Nov 16, 2015 at 06:02:19PM -0600, Aaron Sierra wrote:
> > Previously, the at24 driver would bail out in the case of a 16-bit
> > addressable EEPROM attached to an SMBus controller. This is because
> > SMBus block reads and writes don't map to I2C multi-byte reads and
> > writes when the offset portion is 2 bytes.
> > 
> > Instead of bailing out, this patch settles for functioning with single
> > byte read SMBus cycles. Writes can be block or single-byte, depending
> > on SMBus controller features.
> > 
> > Read access is not without some risk. Multiple SMBus cycles are
> > required to read even one byte. If the SMBus has multiple masters and
> > one accesses this EEPROM between the dummy address write and the
> > subsequent current-address-read cycle(s), this driver will receive
> > data from the wrong address.
> > 
> > Functionality has been tested with the following devices:
> > 
> > AT24CM01 attached to Intel ISCH SMBus
> > AT24C512 attached to Intel I801 SMBus
> > 
> > Read performance:
> > 3.6 KB/s with 32-byte* access
> > 
> > *limited to 32-bytes by I2C_SMBUS_BLOCK_MAX.
> > 
> > Write performance:
> > 248 B/s with 1-byte page (default)
> > 3.9 KB/s with 128-byte* page (via platform data)
> > 
> > *limited to 31-bytes by I2C_SMBUS_BLOCK_MAX - 1.
> > 
> > Signed-off-by: Nate Case 
> > Signed-off-by: Aaron Sierra 
> > Reviewed-by: Jean Delvare 
> > ---
> >  v2 - Account for changes related to introduction of
> >   i2c_smbus_read_i2c_block_data_or_emulated()
> >  v3 - Consolidate three patches into one
> > - Expand comments regarding SMBus multi-master read risks.
> > - Rely on current-address-read for improved read performance (i.e. one
> >   dummy address write followed by multiple individual byte reads).
> >   This improves performance from 1.4 KiB/s to 3.6 KiB/s.
> > - Use struct at24_data's writebuf instead of kzalloc-ing
> > - Only limit write_max by 1-byte when accessing a 16-bit device with
> >   block writes instead of attempting to preserve a power-of-two.
> > - Style fixes (indentation, parentheses, unnecessary masking, etc.)
> >  v4 - Address 16-bit safety in Kconfig
> > - Set "count" to zero later in at24_smbus_read_block_data()
> > - Fix over-80-columns issues in at24_eeprom_read()
> > - Fix write_max off-by-one in at24_probe()
> > - Check SMBus functionality needed for 16-bit device reads
> > - Homogenize indentation of SMBus functionality checks for SMBus write
> >  v5 - 16-bit device read needs READ_BYTE not READ_BYTE_DATA
> > - Clarify write_max limiting with smbus_max
> > - Add X-ES copyright
> >  v6 - Update comment associated with SMBus functionality testing for 16-bit
> >   device read (READ_BYTE not READ_BYTE_DATA)
> > 
> >  drivers/misc/eeprom/Kconfig |   5 +-
> >  drivers/misc/eeprom/at24.c  | 132
> >  +++-
> >  2 files changed, 122 insertions(+), 15 deletions(-)
> 
> I remember I had a lengthy discussion with Guenter Roeck about 16 bit
> support via smbus. I don't have the bandwidth right now to recap the
> discussion and check if the concerns apply to your implementation as
> well. Could you check this thread
> 
> https://lkml.org/lkml/2015/2/4/436
> 
> and report back if it applies to your patch as well? This would speed up
> the review significantly.

Wolfram,
The real bus-level multi-master issue is still present. However, it is
documented via comments in the driver and warnings in the Kconfig.

The other (and apparently bigger) issue of a single master experiencing
corruption due to simultaneous access through i2c-dev and this driver
appears to have been resolved. The i2cdump, i2cget, and i2cset utilities
refuse to access any device that has a driver bound to it. My attempts
to cause corruption using i2cdump were met with the following message
until I unregistered the at24 driver from the device:

   # i2cdump 8 0x54
   No size specified (using byte-data access)
   Error: Could not set address to 0x54: Device or resource busy

-Aaron S.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices

2015-12-18 Thread Jean Delvare 
On Fri, 18 Dec 2015 13:47:50 -0600 (CST), Aaron Sierra wrote:
> - Original Message -
> > From: "Wolfram Sang" 
> > Sent: Friday, December 11, 2015 7:19:40 AM
> >
> > I remember I had a lengthy discussion with Guenter Roeck about 16 bit
> > support via smbus. I don't have the bandwidth right now to recap the
> > discussion and check if the concerns apply to your implementation as
> > well. Could you check this thread
> > 
> > https://lkml.org/lkml/2015/2/4/436
> > 
> > and report back if it applies to your patch as well? This would speed up
> > the review significantly.
> 
> The real bus-level multi-master issue is still present. However, it is
> documented via comments in the driver and warnings in the Kconfig.
> 
> The other (and apparently bigger) issue of a single master experiencing
> corruption due to simultaneous access through i2c-dev and this driver
> appears to have been resolved. The i2cdump, i2cget, and i2cset utilities
> refuse to access any device that has a driver bound to it. My attempts
> to cause corruption using i2cdump were met with the following message
> until I unregistered the at24 driver from the device:
> 
># i2cdump 8 0x54
>No size specified (using byte-data access)
>Error: Could not set address to 0x54: Device or resource busy

You could always bypass the security check and force the access with
option -f. But then there are plenty of ways to corrupt any device that
way, regardless of the driver (or even without any driver bound to the
device, if you run multiple i2c-dev-based user-space tools in
parallel.) So I'd say this is not relevant.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices

2015-12-11 Thread Wolfram Sang 
On Mon, Nov 16, 2015 at 06:02:19PM -0600, Aaron Sierra wrote:
> Previously, the at24 driver would bail out in the case of a 16-bit
> addressable EEPROM attached to an SMBus controller. This is because
> SMBus block reads and writes don't map to I2C multi-byte reads and
> writes when the offset portion is 2 bytes.
> 
> Instead of bailing out, this patch settles for functioning with single
> byte read SMBus cycles. Writes can be block or single-byte, depending
> on SMBus controller features.
> 
> Read access is not without some risk. Multiple SMBus cycles are
> required to read even one byte. If the SMBus has multiple masters and
> one accesses this EEPROM between the dummy address write and the
> subsequent current-address-read cycle(s), this driver will receive
> data from the wrong address.
> 
> Functionality has been tested with the following devices:
> 
> AT24CM01 attached to Intel ISCH SMBus
> AT24C512 attached to Intel I801 SMBus
> 
> Read performance:
> 3.6 KB/s with 32-byte* access
> 
> *limited to 32-bytes by I2C_SMBUS_BLOCK_MAX.
> 
> Write performance:
> 248 B/s with 1-byte page (default)
> 3.9 KB/s with 128-byte* page (via platform data)
> 
> *limited to 31-bytes by I2C_SMBUS_BLOCK_MAX - 1.
> 
> Signed-off-by: Nate Case 
> Signed-off-by: Aaron Sierra 
> Reviewed-by: Jean Delvare 
> ---
>  v2 - Account for changes related to introduction of
>   i2c_smbus_read_i2c_block_data_or_emulated()
>  v3 - Consolidate three patches into one
> - Expand comments regarding SMBus multi-master read risks.
> - Rely on current-address-read for improved read performance (i.e. one
>   dummy address write followed by multiple individual byte reads).
>   This improves performance from 1.4 KiB/s to 3.6 KiB/s.
> - Use struct at24_data's writebuf instead of kzalloc-ing
> - Only limit write_max by 1-byte when accessing a 16-bit device with
>   block writes instead of attempting to preserve a power-of-two.
> - Style fixes (indentation, parentheses, unnecessary masking, etc.)
>  v4 - Address 16-bit safety in Kconfig
> - Set "count" to zero later in at24_smbus_read_block_data()
> - Fix over-80-columns issues in at24_eeprom_read()
> - Fix write_max off-by-one in at24_probe()
> - Check SMBus functionality needed for 16-bit device reads
> - Homogenize indentation of SMBus functionality checks for SMBus write
>  v5 - 16-bit device read needs READ_BYTE not READ_BYTE_DATA
> - Clarify write_max limiting with smbus_max
> - Add X-ES copyright
>  v6 - Update comment associated with SMBus functionality testing for 16-bit
>   device read (READ_BYTE not READ_BYTE_DATA)
> 
>  drivers/misc/eeprom/Kconfig |   5 +-
>  drivers/misc/eeprom/at24.c  | 132 
> +++-
>  2 files changed, 122 insertions(+), 15 deletions(-)

I remember I had a lengthy discussion with Guenter Roeck about 16 bit
support via smbus. I don't have the bandwidth right now to recap the
discussion and check if the concerns apply to your implementation as
well. Could you check this thread

https://lkml.org/lkml/2015/2/4/436

and report back if it applies to your patch as well? This would speed up
the review significantly.

Thanks,

   Wolfram



signature.asc
Description: Digital signature

Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices

2015-11-17 Thread Jean Delvare 
On Mon, 16 Nov 2015 18:02:19 -0600 (CST), Aaron Sierra wrote:
> Previously, the at24 driver would bail out in the case of a 16-bit
> addressable EEPROM attached to an SMBus controller. This is because
> SMBus block reads and writes don't map to I2C multi-byte reads and
> writes when the offset portion is 2 bytes.
> 
> Instead of bailing out, this patch settles for functioning with single
> byte read SMBus cycles. Writes can be block or single-byte, depending
> on SMBus controller features.
> 
> Read access is not without some risk. Multiple SMBus cycles are
> required to read even one byte. If the SMBus has multiple masters and
> one accesses this EEPROM between the dummy address write and the
> subsequent current-address-read cycle(s), this driver will receive
> data from the wrong address.
> 
> Functionality has been tested with the following devices:
> 
> AT24CM01 attached to Intel ISCH SMBus
> AT24C512 attached to Intel I801 SMBus
> 
> Read performance:
> 3.6 KB/s with 32-byte* access
> 
> *limited to 32-bytes by I2C_SMBUS_BLOCK_MAX.
> 
> Write performance:
> 248 B/s with 1-byte page (default)
> 3.9 KB/s with 128-byte* page (via platform data)
> 
> *limited to 31-bytes by I2C_SMBUS_BLOCK_MAX - 1.
> 
> Signed-off-by: Nate Case 
> Signed-off-by: Aaron Sierra 
> Reviewed-by: Jean Delvare 
> ---
>  v2 - Account for changes related to introduction of
>   i2c_smbus_read_i2c_block_data_or_emulated()
>  v3 - Consolidate three patches into one
> - Expand comments regarding SMBus multi-master read risks.
> - Rely on current-address-read for improved read performance (i.e. one
>   dummy address write followed by multiple individual byte reads).
>   This improves performance from 1.4 KiB/s to 3.6 KiB/s.
> - Use struct at24_data's writebuf instead of kzalloc-ing
> - Only limit write_max by 1-byte when accessing a 16-bit device with
>   block writes instead of attempting to preserve a power-of-two.
> - Style fixes (indentation, parentheses, unnecessary masking, etc.)
>  v4 - Address 16-bit safety in Kconfig
> - Set "count" to zero later in at24_smbus_read_block_data()
> - Fix over-80-columns issues in at24_eeprom_read()
> - Fix write_max off-by-one in at24_probe()
> - Check SMBus functionality needed for 16-bit device reads
> - Homogenize indentation of SMBus functionality checks for SMBus write
>  v5 - 16-bit device read needs READ_BYTE not READ_BYTE_DATA
> - Clarify write_max limiting with smbus_max
> - Add X-ES copyright
>  v6 - Update comment associated with SMBus functionality testing for 16-bit
>   device read (READ_BYTE not READ_BYTE_DATA)
> 
>  drivers/misc/eeprom/Kconfig |   5 +-
>  drivers/misc/eeprom/at24.c  | 132 
> +++-
>  2 files changed, 122 insertions(+), 15 deletions(-)
> (...)

Sweet, thanks for your patience.

Now it's up to Wolfram.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6] at24: Support SMBus read/write of 16-bit devices

2015-11-16 Thread Aaron Sierra 
Previously, the at24 driver would bail out in the case of a 16-bit
addressable EEPROM attached to an SMBus controller. This is because
SMBus block reads and writes don't map to I2C multi-byte reads and
writes when the offset portion is 2 bytes.

Instead of bailing out, this patch settles for functioning with single
byte read SMBus cycles. Writes can be block or single-byte, depending
on SMBus controller features.

Read access is not without some risk. Multiple SMBus cycles are
required to read even one byte. If the SMBus has multiple masters and
one accesses this EEPROM between the dummy address write and the
subsequent current-address-read cycle(s), this driver will receive
data from the wrong address.

Functionality has been tested with the following devices:

AT24CM01 attached to Intel ISCH SMBus
AT24C512 attached to Intel I801 SMBus

Read performance:
3.6 KB/s with 32-byte* access

*limited to 32-bytes by I2C_SMBUS_BLOCK_MAX.

Write performance:
248 B/s with 1-byte page (default)
3.9 KB/s with 128-byte* page (via platform data)

*limited to 31-bytes by I2C_SMBUS_BLOCK_MAX - 1.

Signed-off-by: Nate Case 
Signed-off-by: Aaron Sierra 
Reviewed-by: Jean Delvare 
---
 v2 - Account for changes related to introduction of
  i2c_smbus_read_i2c_block_data_or_emulated()
 v3 - Consolidate three patches into one
- Expand comments regarding SMBus multi-master read risks.
- Rely on current-address-read for improved read performance (i.e. one
  dummy address write followed by multiple individual byte reads).
  This improves performance from 1.4 KiB/s to 3.6 KiB/s.
- Use struct at24_data's writebuf instead of kzalloc-ing
- Only limit write_max by 1-byte when accessing a 16-bit device with
  block writes instead of attempting to preserve a power-of-two.
- Style fixes (indentation, parentheses, unnecessary masking, etc.)
 v4 - Address 16-bit safety in Kconfig
- Set "count" to zero later in at24_smbus_read_block_data()
- Fix over-80-columns issues in at24_eeprom_read()
- Fix write_max off-by-one in at24_probe()
- Check SMBus functionality needed for 16-bit device reads
- Homogenize indentation of SMBus functionality checks for SMBus write
 v5 - 16-bit device read needs READ_BYTE not READ_BYTE_DATA
- Clarify write_max limiting with smbus_max
- Add X-ES copyright
 v6 - Update comment associated with SMBus functionality testing for 16-bit
  device read (READ_BYTE not READ_BYTE_DATA)

 drivers/misc/eeprom/Kconfig |   5 +-
 drivers/misc/eeprom/at24.c  | 132 +++-
 2 files changed, 122 insertions(+), 15 deletions(-)

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..cc0f86c 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -22,7 +22,10 @@ config EEPROM_AT24
 
  If you use this with an SMBus adapter instead of an I2C adapter,
  full functionality is not available.  Only smaller devices are
- supported (24c16 and below, max 4 kByte).
+ supported via block reads (24c16 and below, max 4 kByte).
+ Larger devices that use 16-bit addresses will only work with
+ individual byte reads, which is very slow in general and is unsafe
+ in multi-master SMBus topologies.
 
  This driver can also be built as a module.  If so, the module
  will be called at24.
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 5d7c090..52cb707 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2005-2007 David Brownell
  * Copyright (C) 2008 Wolfram Sang, Pengutronix
+ * Copyright (C) 2015 Extreme Engineering Solutions, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -50,7 +51,7 @@
  * Other than binding model, current differences from "eeprom" driver are
  * that this one handles write access and isn't restricted to 24c02 devices.
  * It also handles larger devices (32 kbit and up) with two-byte addresses,
- * which won't work on pure SMBus systems.
+ * which don't work without risks on pure SMBus systems.
  */
 
 struct at24_data {
@@ -141,6 +142,87 @@ MODULE_DEVICE_TABLE(acpi, at24_acpi_ids);
 /*-*/
 
 /*
+ * Write a byte to an AT24 device using SMBus cycles.
+ */
+static inline s32 at24_smbus_write_byte_data(struct at24_data *at24,
+   struct i2c_client *client, u16 offset, u8 value)
+{
+   if (!(at24->chip.flags & AT24_FLAG_ADDR16))
+   return i2c_smbus_write_byte_data(client, offset, value);
+
+   /*
+* Emulate I2C multi-byte write by using SMBus "write word"
+* cycle.  We split up the 16-bit offset among the "command"
+*