BUG: support for at24 multi-slave-address eeproms is broken.

BUG: support for at24 multi-slave-address eeproms is broken.

[Sven Van Asbroeck]

Summary
-------
Some at24 eeproms have multiple i2c slave addresses. A patch introduced
between 4.14-rc5 and 4.14-rc6 breaks support for these eeproms:
reads/writes which start outside the first slave no longer work.

98e8201039afad5d2af87df9ac682f62f69c0c2f
(eeprom: at24: enable runtime pm support)

How to reproduce
----------------
- I'm using the latest mainline at24 driver (4.15-rc1)
- I have a 24AA16/24LC16B on board, which is a 2048-byte eeprom,
	made up of 8x 256-byte internal eeproms, all with their own i2c slave
	address. Base i2c address is 0x50, so this results in the following
	slave addresses: 0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57 of
	256 bytes each.

$ ls -l /sys/bus/i2c/devices/1-0050/eeprom
-rw-------    1 root     root          2048 Nov 30 13:16 /sys/bus/i2c/devices/1-0050/eeprom

Reading from the beginning to the end in one go works just fine:
$ hexdump -C /sys/bus/i2c/devices/1-0050/eeprom 
00000000  f8 6e cf 01 ff 01 00 00  03 00 58 41 18 00 00 00  |.n........XA....|
00000010  53 61 74 20 4d 61 72 20  31 32 20 31 32 3a 35 35  |Sat Mar 12 12:55|
00000020  3a 31 32 20 32 30 31 36  01 00 58 41 0f 00 00 00  |:12 2016..XA....|
00000030  41 58 4d 2d 55 50 33 32  39 32 2d 30 38 32 34 00  |AXM-UP3292-0824.|
00000040  02 00 58 41 0d 00 00 00  41 58 53 2d 55 50 33 4b  |..XA....AXS-UP3K|
00000050  2d 30 32 30 34 00 00 00  04 00 58 41 09 00 00 00  |-0204.....XA....|
00000060  55 4e 54 31 34 30 30 42  34 00 00 00 0a 00 58 41  |UNT1400B4.....XA|
00000070  01 00 00 00 32 00 00 00  09 00 58 41 01 00 00 00  |....2.....XA....|
00000080  31 00 00 00 01 00 00 00  32 00 00 00 01 00 00 00  |1.......2.......|
00000090  32 00 00 00 ff ff ff ff  ff ff ff ff ff ff ff ff  |2...............|
000000a0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
*
00000800

Reading from somewhere in the middle (outside of first slave) gives
EACCESS:
$ dd if=/sys/bus/i2c/devices/1-0050/eeprom bs=1 skip=520 count=20
dd: /sys/bus/i2c/devices/1-0050/eeprom: Permission denied

Reading from somewhere in the middle (INSIDE of first slave) still works:
$ dd if=/sys/bus/i2c/devices/1-0050/eeprom bs=1 skip=20 count=20 | hexdump -C
00000000  4d 61 72 20 31 32 20 31  32 3a 35 35 3a 31 32 20  |Mar 12 12:55:12 |
00000010  32 30 31 36                                       |2016|
00000014

Additional questions for the patch authors
------------------------------------------
1. why is there a need to add pm_runtime support to a series of chips
(at24) which do not have any power management registers, hardware or
support ?

2. why is the at24's pm_runtime support operating on its parent i2c bus?
Why can't the parent i2c bus driver be in charge of its own runtime pm?

Re: BUG: support for at24 multi-slave-address eeproms is broken.

[Sakari Ailus]

Hi Sven,

On Thu, Nov 30, 2017 at 02:03:23PM -0500, Sven Van Asbroeck wrote:
> Summary
> -------
> Some at24 eeproms have multiple i2c slave addresses. A patch introduced
> between 4.14-rc5 and 4.14-rc6 breaks support for these eeproms:
> reads/writes which start outside the first slave no longer work.
> 
> 98e8201039afad5d2af87df9ac682f62f69c0c2f
> (eeprom: at24: enable runtime pm support)
> 
> How to reproduce
> ----------------
> - I'm using the latest mainline at24 driver (4.15-rc1)
> - I have a 24AA16/24LC16B on board, which is a 2048-byte eeprom,
> 	made up of 8x 256-byte internal eeproms, all with their own i2c slave
> 	address. Base i2c address is 0x50, so this results in the following
> 	slave addresses: 0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57 of
> 	256 bytes each.

Thank you for reporting this.

> 
> $ ls -l /sys/bus/i2c/devices/1-0050/eeprom
> -rw-------    1 root     root          2048 Nov 30 13:16 /sys/bus/i2c/devices/1-0050/eeprom
> 
> Reading from the beginning to the end in one go works just fine:
> $ hexdump -C /sys/bus/i2c/devices/1-0050/eeprom 
> 00000000  f8 6e cf 01 ff 01 00 00  03 00 58 41 18 00 00 00  |.n........XA....|
> 00000010  53 61 74 20 4d 61 72 20  31 32 20 31 32 3a 35 35  |Sat Mar 12 12:55|
> 00000020  3a 31 32 20 32 30 31 36  01 00 58 41 0f 00 00 00  |:12 2016..XA....|
> 00000030  41 58 4d 2d 55 50 33 32  39 32 2d 30 38 32 34 00  |AXM-UP3292-0824.|
> 00000040  02 00 58 41 0d 00 00 00  41 58 53 2d 55 50 33 4b  |..XA....AXS-UP3K|
> 00000050  2d 30 32 30 34 00 00 00  04 00 58 41 09 00 00 00  |-0204.....XA....|
> 00000060  55 4e 54 31 34 30 30 42  34 00 00 00 0a 00 58 41  |UNT1400B4.....XA|
> 00000070  01 00 00 00 32 00 00 00  09 00 58 41 01 00 00 00  |....2.....XA....|
> 00000080  31 00 00 00 01 00 00 00  32 00 00 00 01 00 00 00  |1.......2.......|
> 00000090  32 00 00 00 ff ff ff ff  ff ff ff ff ff ff ff ff  |2...............|
> 000000a0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> *
> 00000800
> 
> Reading from somewhere in the middle (outside of first slave) gives
> EACCESS:
> $ dd if=/sys/bus/i2c/devices/1-0050/eeprom bs=1 skip=520 count=20
> dd: /sys/bus/i2c/devices/1-0050/eeprom: Permission denied
> 
> Reading from somewhere in the middle (INSIDE of first slave) still works:
> $ dd if=/sys/bus/i2c/devices/1-0050/eeprom bs=1 skip=20 count=20 | hexdump -C
> 00000000  4d 61 72 20 31 32 20 31  32 3a 35 35 3a 31 32 20  |Mar 12 12:55:12 |
> 00000010  32 30 31 36                                       |2016|
> 00000014
> 
> Additional questions for the patch authors
> ------------------------------------------
> 1. why is there a need to add pm_runtime support to a series of chips
> (at24) which do not have any power management registers, hardware or
> support ?

Power management is not a property of the chip itself, it's external to it.

The motivation for the patch was that the chip is powered together with
other chips in a camera module. If it is not powered off, all other devices
in the module will remain powered. This is undesirable.

> 
> 2. why is the at24's pm_runtime support operating on its parent i2c bus?
> Why can't the parent i2c bus driver be in charge of its own runtime pm?

This is device specific, the I²C bus driver doesn't know about it.

I have a patch that should address the problem, let me know if it works for
you.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH 1/1] at24: Fix I²C device selection for runtime PM

[Sakari Ailus]

The at24 driver creates dummy I²C devices to access offsets in the chip
that are outside the area supported using a single I²C address. It is not
meaningful to use runtime PM to such devices; the system firmware (ACPI)
does not know about these devices nor runtime PM was enabled for them.
Always use the real device instead of the dummy ones.

Fixes: 98e8201039af ("eeprom: at24: enable runtime pm support")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/eeprom/at24.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index e0b4b36ef010..07b6b61b96ee 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -561,18 +561,16 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
 static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 {
 	struct at24_data *at24 = priv;
-	struct i2c_client *client;
+	struct device *dev = &at24->client[0]->dev;
 	char *buf = val;
 	int ret;
 
 	if (unlikely(!count))
 		return count;
 
-	client = at24_translate_offset(at24, &off);
-
-	ret = pm_runtime_get_sync(&client->dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
-		pm_runtime_put_noidle(&client->dev);
+		pm_runtime_put_noidle(dev);
 		return ret;
 	}
 
@@ -588,7 +586,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 		status = at24->read_func(at24, buf, off, count);
 		if (status < 0) {
 			mutex_unlock(&at24->lock);
-			pm_runtime_put(&client->dev);
+			pm_runtime_put(dev);
 			return status;
 		}
 		buf += status;
@@ -598,7 +596,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 
 	mutex_unlock(&at24->lock);
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_put(dev);
 
 	return 0;
 }
@@ -606,18 +604,16 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 {
 	struct at24_data *at24 = priv;
-	struct i2c_client *client;
+	struct device *dev = &at24->client[0]->dev;
 	char *buf = val;
 	int ret;
 
 	if (unlikely(!count))
 		return -EINVAL;
 
-	client = at24_translate_offset(at24, &off);
-
-	ret = pm_runtime_get_sync(&client->dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
-		pm_runtime_put_noidle(&client->dev);
+		pm_runtime_put_noidle(dev);
 		return ret;
 	}
 
@@ -633,7 +629,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 		status = at24->write_func(at24, buf, off, count);
 		if (status < 0) {
 			mutex_unlock(&at24->lock);
-			pm_runtime_put(&client->dev);
+			pm_runtime_put(dev);
 			return status;
 		}
 		buf += status;
@@ -643,7 +639,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 
 	mutex_unlock(&at24->lock);
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_put(dev);
 
 	return 0;
 }
-- 
2.11.0

Re: BUG: support for at24 multi-slave-address eeproms is broken.

[Bartosz Golaszewski]

2017-11-30 23:34 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> Hi Sven,
>
> On Thu, Nov 30, 2017 at 02:03:23PM -0500, Sven Van Asbroeck wrote:
>> Summary
>> -------
>> Some at24 eeproms have multiple i2c slave addresses. A patch introduced
>> between 4.14-rc5 and 4.14-rc6 breaks support for these eeproms:
>> reads/writes which start outside the first slave no longer work.
>>
>> 98e8201039afad5d2af87df9ac682f62f69c0c2f
>> (eeprom: at24: enable runtime pm support)
>>
>> How to reproduce
>> ----------------
>> - I'm using the latest mainline at24 driver (4.15-rc1)
>> - I have a 24AA16/24LC16B on board, which is a 2048-byte eeprom,
>>       made up of 8x 256-byte internal eeproms, all with their own i2c slave
>>       address. Base i2c address is 0x50, so this results in the following
>>       slave addresses: 0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57 of
>>       256 bytes each.
>
> Thank you for reporting this.
>
>>
>> $ ls -l /sys/bus/i2c/devices/1-0050/eeprom
>> -rw-------    1 root     root          2048 Nov 30 13:16 /sys/bus/i2c/devices/1-0050/eeprom
>>
>> Reading from the beginning to the end in one go works just fine:
>> $ hexdump -C /sys/bus/i2c/devices/1-0050/eeprom
>> 00000000  f8 6e cf 01 ff 01 00 00  03 00 58 41 18 00 00 00  |.n........XA....|
>> 00000010  53 61 74 20 4d 61 72 20  31 32 20 31 32 3a 35 35  |Sat Mar 12 12:55|
>> 00000020  3a 31 32 20 32 30 31 36  01 00 58 41 0f 00 00 00  |:12 2016..XA....|
>> 00000030  41 58 4d 2d 55 50 33 32  39 32 2d 30 38 32 34 00  |AXM-UP3292-0824.|
>> 00000040  02 00 58 41 0d 00 00 00  41 58 53 2d 55 50 33 4b  |..XA....AXS-UP3K|
>> 00000050  2d 30 32 30 34 00 00 00  04 00 58 41 09 00 00 00  |-0204.....XA....|
>> 00000060  55 4e 54 31 34 30 30 42  34 00 00 00 0a 00 58 41  |UNT1400B4.....XA|
>> 00000070  01 00 00 00 32 00 00 00  09 00 58 41 01 00 00 00  |....2.....XA....|
>> 00000080  31 00 00 00 01 00 00 00  32 00 00 00 01 00 00 00  |1.......2.......|
>> 00000090  32 00 00 00 ff ff ff ff  ff ff ff ff ff ff ff ff  |2...............|
>> 000000a0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
>> *
>> 00000800
>>
>> Reading from somewhere in the middle (outside of first slave) gives
>> EACCESS:
>> $ dd if=/sys/bus/i2c/devices/1-0050/eeprom bs=1 skip=520 count=20
>> dd: /sys/bus/i2c/devices/1-0050/eeprom: Permission denied
>>
>> Reading from somewhere in the middle (INSIDE of first slave) still works:
>> $ dd if=/sys/bus/i2c/devices/1-0050/eeprom bs=1 skip=20 count=20 | hexdump -C
>> 00000000  4d 61 72 20 31 32 20 31  32 3a 35 35 3a 31 32 20  |Mar 12 12:55:12 |
>> 00000010  32 30 31 36                                       |2016|
>> 00000014
>>
>> Additional questions for the patch authors
>> ------------------------------------------
>> 1. why is there a need to add pm_runtime support to a series of chips
>> (at24) which do not have any power management registers, hardware or
>> support ?
>
> Power management is not a property of the chip itself, it's external to it.
>
> The motivation for the patch was that the chip is powered together with
> other chips in a camera module. If it is not powered off, all other devices
> in the module will remain powered. This is undesirable.
>
>>
>> 2. why is the at24's pm_runtime support operating on its parent i2c bus?
>> Why can't the parent i2c bus driver be in charge of its own runtime pm?
>
> This is device specific, the I²C bus driver doesn't know about it.
>
> I have a patch that should address the problem, let me know if it works for
> you.
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

Thanks for taking care of it!

@Sven: it would be great if you could test it and add your Tested-by
to the patch. I'll then queue it for 4.15.

Thanks,
Bartosz

Re: [PATCH 1/1] at24: Fix I²C device selection for runtime PM

[Sven Van Asbroeck]

Thank you, it fixes the issue on the multi-address eeprom that I have access to.

Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com>

One very minor remark:

+       struct device *dev = &at24->client[0]->dev;

It is sufficiently clear to others (and us a few months down the line)
why we are
using only client[0] for power management? Could it benefit from a separate
function with comments?

struct device *dev = get_pm_device(at24);

static struct device *get_pm_device(struct at24_data *at24)
{
    /* explain why we use client[0] and not any of the dummies */
    return &at24->client[0]->dev;
}

Re: [PATCH 1/1] at24: Fix I²C device selection for runtime PM

[Sakari Ailus]

Hi Sven,

On Fri, Dec 01, 2017 at 10:20:41AM -0500, Sven Van Asbroeck wrote:
> Thank you, it fixes the issue on the multi-address eeprom that I have access to.
> 
> Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com>
> 
> One very minor remark:
> 
> +       struct device *dev = &at24->client[0]->dev;
> 
> It is sufficiently clear to others (and us a few months down the line)
> why we are
> using only client[0] for power management? Could it benefit from a separate
> function with comments?
> 
> struct device *dev = get_pm_device(at24);
> 
> static struct device *get_pm_device(struct at24_data *at24)
> {
>     /* explain why we use client[0] and not any of the dummies */
>     return &at24->client[0]->dev;
> }

There are no comments in assigning at24->client[0] either (or a helper
function). I think it should be rather evident when looking at the code
when you think about it. I certainly don't object adding a comment if you
insist or someone else thinks it's a good idea.

Thanks for testing!

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/1] at24: Fix I²C device selection for runtime PM

[Bartosz Golaszewski]

2017-12-01 16:35 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> Hi Sven,
>
> On Fri, Dec 01, 2017 at 10:20:41AM -0500, Sven Van Asbroeck wrote:
>> Thank you, it fixes the issue on the multi-address eeprom that I have access to.
>>
>> Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com>
>>
>> One very minor remark:
>>
>> +       struct device *dev = &at24->client[0]->dev;
>>
>> It is sufficiently clear to others (and us a few months down the line)
>> why we are
>> using only client[0] for power management? Could it benefit from a separate
>> function with comments?
>>
>> struct device *dev = get_pm_device(at24);
>>
>> static struct device *get_pm_device(struct at24_data *at24)
>> {
>>     /* explain why we use client[0] and not any of the dummies */
>>     return &at24->client[0]->dev;
>> }
>
> There are no comments in assigning at24->client[0] either (or a helper
> function). I think it should be rather evident when looking at the code
> when you think about it. I certainly don't object adding a comment if you
> insist or someone else thinks it's a good idea.
>
> Thanks for testing!
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

Pushed to at24/fixes, thanks!

@Saraki: there were some conflicts with the previous fixes queued for
4.15. Could you take a look if my rebase didn't break anything? You
can find my tree at
git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git

Best regards,
Bartosz

Re: [PATCH 1/1] at24: Fix I²C device selection for runtime PM

[Sven Van Asbroeck]

Bartosz wrote:
> Could you take a look if my rebase didn't break anything?

Unfortunately the rebase broke multi-address eeprom support ;(

I will post a corrected patch.

Re: [PATCH 1/1] at24: Fix I²C device selection for runtime PM

[Sakari Ailus]

On Fri, Dec 01, 2017 at 05:29:27PM +0100, Bartosz Golaszewski wrote:
> 2017-12-01 16:35 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> > Hi Sven,
> >
> > On Fri, Dec 01, 2017 at 10:20:41AM -0500, Sven Van Asbroeck wrote:
> >> Thank you, it fixes the issue on the multi-address eeprom that I have access to.
> >>
> >> Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com>
> >>
> >> One very minor remark:
> >>
> >> +       struct device *dev = &at24->client[0]->dev;
> >>
> >> It is sufficiently clear to others (and us a few months down the line)
> >> why we are
> >> using only client[0] for power management? Could it benefit from a separate
> >> function with comments?
> >>
> >> struct device *dev = get_pm_device(at24);
> >>
> >> static struct device *get_pm_device(struct at24_data *at24)
> >> {
> >>     /* explain why we use client[0] and not any of the dummies */
> >>     return &at24->client[0]->dev;
> >> }
> >
> > There are no comments in assigning at24->client[0] either (or a helper
> > function). I think it should be rather evident when looking at the code
> > when you think about it. I certainly don't object adding a comment if you
> > insist or someone else thinks it's a good idea.
> >
> > Thanks for testing!
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
> 
> Pushed to at24/fixes, thanks!
> 
> @Saraki: there were some conflicts with the previous fixes queued for
> 4.15. Could you take a look if my rebase didn't break anything? You
> can find my tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git

Seems fine to me. Thanks!!

-- 
Sakari Ailus
sakari.ailus@linux.intel.com