[PATCH i2c-tools 0/2] tools: improvements to handling restricted addresses

[PATCH i2c-tools 0/2] tools: improvements to handling restricted addresses

[Wolfram Sang]

For debugging, I needed to use the restricted address range. That's when I
noticed that the docs for i2ctransfer were missing an update (patch 1). But
even more surprising, I noticed that the reserved addresses 0x03-0x07 were not
restricted. It seems we all are so used to our lovely i2c-tools that nobody
(including me) never questioned that :) But it should be fixed (patch 2), I
think.

Open for comments. Happy hacking!


Wolfram Sang (2):
  tools: i2ctransfer: man: mention -a everywhere
  tools: restrict all addresses defined by the standard

 CHANGES             |  2 ++
 tools/i2cbusses.c   |  2 +-
 tools/i2cdetect.8   |  2 +-
 tools/i2cdetect.c   |  2 +-
 tools/i2cdump.8     |  4 ++--
 tools/i2cdump.c     |  2 +-
 tools/i2cget.8      |  4 ++--
 tools/i2cget.c      |  2 +-
 tools/i2cset.8      |  4 ++--
 tools/i2cset.c      |  2 +-
 tools/i2ctransfer.8 | 12 +++++++-----
 11 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.11.0

[PATCH i2c-tools 1/2] tools: i2ctransfer: consequently document -a

[Wolfram Sang]

The text describing the address to be used did not reflect the recent
change where some of the functionality of '-f' has been seperated into
'-a'.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 CHANGES             | 1 +
 tools/i2ctransfer.8 | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/CHANGES b/CHANGES
index e2ea015..e9d46b5 100644
--- a/CHANGES
+++ b/CHANGES
@@ -5,6 +5,7 @@ master
   tools: Consistently use snprintf instead of sprintf
   decode-dimms: Print SPD revision for DDR3 too
                 Move SDR-specific code
+  i2ctransfer: Mention '-a' everywhere in the manpage
 
 4.1 (2018-11-30)
   Makefile: Make STRIP, DESTDIR and PREFIX overridable
diff --git a/tools/i2ctransfer.8 b/tools/i2ctransfer.8
index 5d3cc27..1f8ed82 100644
--- a/tools/i2ctransfer.8
+++ b/tools/i2ctransfer.8
@@ -96,10 +96,12 @@ It is parsed as an unsigned 16 bit integer, but note that the Linux Kernel appli
 specifies the 7-bit address of the chip to be accessed for this message, and is an integer.
 If omitted, reuse the previous address.
 Normally, addresses outside the range of 0x03-0x77 and addresses with a kernel driver attached to them will be blocked.
-With
+This can be overridden with
+.I -a
+(all) or
 .I -f
-(force), all addresses can be used.
-Be very careful when using that!
+(force).
+Be very careful when using these!
 10-bit addresses are currently not supported at all.
 
 .PP
-- 
2.11.0

Re: [PATCH i2c-tools 1/2] tools: i2ctransfer: consequently document -a

[Jean Delvare]

Hi Wolfram,

On Mon, 11 Mar 2019 23:33:34 +0100, Wolfram Sang wrote:
> The text describing the address to be used did not reflect the recent
> change where some of the functionality of '-f' has been seperated into
> '-a'.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  CHANGES             | 1 +
>  tools/i2ctransfer.8 | 8 +++++---
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/CHANGES b/CHANGES
> index e2ea015..e9d46b5 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -5,6 +5,7 @@ master
>    tools: Consistently use snprintf instead of sprintf
>    decode-dimms: Print SPD revision for DDR3 too
>                  Move SDR-specific code
> +  i2ctransfer: Mention '-a' everywhere in the manpage

Not sure how "once" qualifies as "everywhere" ;-)

>  
>  4.1 (2018-11-30)
>    Makefile: Make STRIP, DESTDIR and PREFIX overridable
> diff --git a/tools/i2ctransfer.8 b/tools/i2ctransfer.8
> index 5d3cc27..1f8ed82 100644
> --- a/tools/i2ctransfer.8
> +++ b/tools/i2ctransfer.8
> @@ -96,10 +96,12 @@ It is parsed as an unsigned 16 bit integer, but note that the Linux Kernel appli
>  specifies the 7-bit address of the chip to be accessed for this message, and is an integer.
>  If omitted, reuse the previous address.
>  Normally, addresses outside the range of 0x03-0x77 and addresses with a kernel driver attached to them will be blocked.
> -With
> +This can be overridden with
> +.I -a
> +(all) or
>  .I -f
> -(force), all addresses can be used.
> -Be very careful when using that!
> +(force).
> +Be very careful when using these!
>  10-bit addresses are currently not supported at all.
>  
>  .PP

Looks good to me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools 1/2] tools: i2ctransfer: consequently document -a

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]


> > +  i2ctransfer: Mention '-a' everywhere in the manpage
> 
> Not sure how "once" qualifies as "everywhere" ;-)

Yes, this patch changes it only once but after that it is everywhere :)
(It was mentioned in another place already)

> Looks good to me.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i2c-tools 1/2] tools: i2ctransfer: consequently document -a

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Mon, Mar 11, 2019 at 11:33:34PM +0100, Wolfram Sang wrote:
> The text describing the address to be used did not reflect the recent
> change where some of the functionality of '-f' has been seperated into
> '-a'.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to master.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH i2c-tools 2/2] tools: restrict all addresses defined by the standard

[Wolfram Sang]

The I2C standard reserves addresses 0x03-0x07. Adapt our tools to that.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 CHANGES             | 1 +
 tools/i2cbusses.c   | 2 +-
 tools/i2cdetect.8   | 2 +-
 tools/i2cdetect.c   | 2 +-
 tools/i2cdump.8     | 4 ++--
 tools/i2cdump.c     | 2 +-
 tools/i2cget.8      | 4 ++--
 tools/i2cget.c      | 2 +-
 tools/i2cset.8      | 4 ++--
 tools/i2cset.c      | 2 +-
 tools/i2ctransfer.8 | 4 ++--
 11 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/CHANGES b/CHANGES
index e9d46b5..e3ff3a0 100644
--- a/CHANGES
+++ b/CHANGES
@@ -3,6 +3,7 @@ i2c-tools CHANGES
 
 master
   tools: Consistently use snprintf instead of sprintf
+         Restrict addresses 0x03-0x07, too (defined by I2C standard)
   decode-dimms: Print SPD revision for DDR3 too
                 Move SDR-specific code
   i2ctransfer: Mention '-a' everywhere in the manpage
diff --git a/tools/i2cbusses.c b/tools/i2cbusses.c
index 438f345..b4f00ae 100644
--- a/tools/i2cbusses.c
+++ b/tools/i2cbusses.c
@@ -381,7 +381,7 @@ int parse_i2c_address(const char *address_arg, int all_addrs)
 {
 	long address;
 	char *end;
-	long min_addr = 0x03;
+	long min_addr = 0x08;
 	long max_addr = 0x77;
 
 	address = strtol(address_arg, &end, 0);
diff --git a/tools/i2cdetect.8 b/tools/i2cdetect.8
index 81d0f97..14c1f18 100644
--- a/tools/i2cdetect.8
+++ b/tools/i2cdetect.8
@@ -26,7 +26,7 @@ outputs a table with the list of detected devices on the specified bus.
 \fIi2cbus\fR indicates the number or name of the I2C bus to be scanned, and
 should correspond to one of the busses listed by \fIi2cdetect -l\fR.
 The optional parameters \fIfirst\fR and \fIlast\fR restrict the scanning
-range (default: from 0x03 to 0x77).
+range (default: from 0x08 to 0x77).
 .PP
 As there is no standard I2C detection command, i2cdetect uses arbitrary
 SMBus commands (namely SMBus quick write and SMBus receive byte) to probe
diff --git a/tools/i2cdetect.c b/tools/i2cdetect.c
index 1804f84..0b9af48 100644
--- a/tools/i2cdetect.c
+++ b/tools/i2cdetect.c
@@ -204,7 +204,7 @@ int main(int argc, char *argv[])
 	char filename[20];
 	unsigned long funcs;
 	int mode = MODE_AUTO;
-	int first = 0x03, last = 0x77;
+	int first = 0x08, last = 0x77;
 	int flags = 0;
 	int yes = 0, version = 0, list = 0;
 
diff --git a/tools/i2cdump.8 b/tools/i2cdump.8
index 2240f3c..18bf600 100644
--- a/tools/i2cdump.8
+++ b/tools/i2cdump.8
@@ -43,12 +43,12 @@ will perform the operation directly. This is mainly meant to be used in
 scripts.
 .TP
 .B -a
-Allow using addresses between 0x00 - 0x02 and 0x78 - 0x7f. Not recommended.
+Allow using addresses between 0x00 - 0x07 and 0x78 - 0x7f. Not recommended.
 .PP
 At least two options must be provided to i2cdump. \fIi2cbus\fR indicates the
 number or name of the I2C bus to be scanned. This number should correspond to one
 of the busses listed by \fIi2cdetect -l\fR. \fIaddress\fR indicates the
-address to be scanned on that bus, and is an integer between 0x03 and 0x77.
+address to be scanned on that bus, and is an integer between 0x08 and 0x77.
 .PP
 The \fImode\fR parameter, if specified, is one of the letters \fBb\fP, \fBw\fP,
 \fBs\fP, or \fBi\fP, corresponding to a read size of a single byte, a 16-bit
diff --git a/tools/i2cdump.c b/tools/i2cdump.c
index 3bd2077..70cecb9 100644
--- a/tools/i2cdump.c
+++ b/tools/i2cdump.c
@@ -38,7 +38,7 @@ static void help(void)
 	fprintf(stderr,
 		"Usage: i2cdump [-f] [-y] [-r first-last] [-a] I2CBUS ADDRESS [MODE [BANK [BANKREG]]]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
-		"  ADDRESS is an integer (0x03 - 0x77, or 0x00 - 0x7f if -a is given)\n"
+		"  ADDRESS is an integer (0x08 - 0x77, or 0x00 - 0x7f if -a is given)\n"
 		"  MODE is one of:\n"
 		"    b (byte, default)\n"
 		"    w (word)\n"
diff --git a/tools/i2cget.8 b/tools/i2cget.8
index 8b48ad7..680279f 100644
--- a/tools/i2cget.8
+++ b/tools/i2cget.8
@@ -37,12 +37,12 @@ will perform the operation directly. This is mainly meant to be used in
 scripts. Use with caution.
 .TP
 .B -a
-Allow using addresses between 0x00 - 0x02 and 0x78 - 0x7f. Not recommended.
+Allow using addresses between 0x00 - 0x07 and 0x78 - 0x7f. Not recommended.
 .PP
 There are two required options to i2cget. \fIi2cbus\fR indicates the number
 or name of the I2C bus to be scanned.  This number should correspond to one of
 the busses listed by \fIi2cdetect -l\fR. \fIchip-address\fR specifies the
-address of the chip on that bus, and is an integer between 0x03 and 0x77.
+address of the chip on that bus, and is an integer between 0x08 and 0x77.
 .PP
 \fIdata-address\fR specifies the address on that chip to read from, and is
 an integer between 0x00 and 0xFF. If omitted, the currently active register
diff --git a/tools/i2cget.c b/tools/i2cget.c
index d2ed56a..1db4f39 100644
--- a/tools/i2cget.c
+++ b/tools/i2cget.c
@@ -43,7 +43,7 @@ static void help(void)
 	fprintf(stderr,
 		"Usage: i2cget [-f] [-y] [-a] I2CBUS CHIP-ADDRESS [DATA-ADDRESS [MODE]]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
-		"  ADDRESS is an integer (0x03 - 0x77, or 0x00 - 0x7f if -a is given)\n"
+		"  ADDRESS is an integer (0x08 - 0x77, or 0x00 - 0x7f if -a is given)\n"
 		"  MODE is one of:\n"
 		"    b (read byte data, default)\n"
 		"    w (read word data)\n"
diff --git a/tools/i2cset.8 b/tools/i2cset.8
index cd53aba..8c73c60 100644
--- a/tools/i2cset.8
+++ b/tools/i2cset.8
@@ -57,12 +57,12 @@ value written. This used to be the default behavior. The same limitations
 apply as those of option \fB-m\fR.
 .TP
 .B -a
-Allow using addresses between 0x00 - 0x02 and 0x78 - 0x7f. Not recommended.
+Allow using addresses between 0x00 - 0x07 and 0x78 - 0x7f. Not recommended.
 .PP
 There are three required options to i2cset. \fIi2cbus\fR indicates the number
 or name of the I2C bus to be scanned.  This number should correspond to one of
 the busses listed by \fIi2cdetect -l\fR. \fIchip-address\fR specifies the
-address of the chip on that bus, and is an integer between 0x03 and 0x77.
+address of the chip on that bus, and is an integer between 0x08 and 0x77.
 \fIdata-address\fR specifies the address on that chip to write to, and is an
 integer between 0x00 and 0xFF.
 .PP
diff --git a/tools/i2cset.c b/tools/i2cset.c
index e82dc52..f0430f2 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -40,7 +40,7 @@ static void help(void)
 	fprintf(stderr,
 		"Usage: i2cset [-f] [-y] [-m MASK] [-r] [-a] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
-		"  ADDRESS is an integer (0x03 - 0x77, or 0x00 - 0x7f if -a is given)\n"
+		"  ADDRESS is an integer (0x08 - 0x77, or 0x00 - 0x7f if -a is given)\n"
 		"  MODE is one of:\n"
 		"    c (byte, no value)\n"
 		"    b (byte data, default)\n"
diff --git a/tools/i2ctransfer.8 b/tools/i2ctransfer.8
index 1f8ed82..d16e34e 100644
--- a/tools/i2ctransfer.8
+++ b/tools/i2ctransfer.8
@@ -64,7 +64,7 @@ It will print infos about all messages sent, i.e. not only for read messages but
 Display the version and exit.
 .TP
 .B -a
-Allow using addresses between 0x00 - 0x02 and 0x78 - 0x7f. Not recommended.
+Allow using addresses between 0x00 - 0x07 and 0x78 - 0x7f. Not recommended.
 
 .SH ARGUMENTS
 .PP
@@ -95,7 +95,7 @@ It is parsed as an unsigned 16 bit integer, but note that the Linux Kernel appli
 .B [@address]
 specifies the 7-bit address of the chip to be accessed for this message, and is an integer.
 If omitted, reuse the previous address.
-Normally, addresses outside the range of 0x03-0x77 and addresses with a kernel driver attached to them will be blocked.
+Normally, addresses outside the range of 0x08-0x77 and addresses with a kernel driver attached to them will be blocked.
 This can be overridden with
 .I -a
 (all) or
-- 
2.11.0

Re: [PATCH i2c-tools 2/2] tools: restrict all addresses defined by the standard

[Jean Delvare]

Hi Wolfram,

On Mon, 11 Mar 2019 23:33:35 +0100, Wolfram Sang wrote:
> The I2C standard reserves addresses 0x03-0x07. Adapt our tools to that.

I have a different interpretation of the specification. Addresses
0x04-0x07 are marked as "Hs-mode master code". They are not explicitly
marked as "reserved" (although admittedly the table is named "Reserved
addresses"). My understanding is that HS-mode will "use" these
addresses, which means that you can't use HS-mode _and_ slaves at
addresses 0x04-0x07 on the same I²C bus segment. I do not read this as
"non-HS-mode slaves can't use these addresses". But maybe this is just
me, and also I don't know that much about HS-mode really.

Address 0x03 is indeed reserved, so obviously that this really makes me
wonder how the original range of 0x03-0x77 was decided. Unfortunately
the git history doesn't go that far (this range was already used in
i2cget and i2cdetect at the time i2c-tools was split to its own
repository). But I was able to dig out an old lm-sensors commit:

commit e1c8fe76763bfa7e9261a5a024755d2274a8b14b
Author: Mark D. Studebaker
Date:   Sat Mar 20 17:23:31 2004 +0000

    scan address 0x03 again
    
    
    git-svn-id: http://lm-sensors.org/svn/lm-sensors/trunk@2377 7894878c-1315-0410-8ee3-d5d059ff63e0

diff --git a/prog/detect/i2cdetect.c b/prog/detect/i2cdetect.c
index 43bb704265ac..8d96591493aa 100644
--- a/prog/detect/i2cdetect.c
+++ b/prog/detect/i2cdetect.c
@@ -159,7 +159,7 @@ int main(int argc, char *argv[])
   for (i = 0; i < 128; i+=16) {
     printf("%02x: ",i);
     for(j = 0; j < 16; j++) {
-      if (!force && (i+j<0x04 || i+j>0x77)) {
+      if (!force && (i+j<0x03 || i+j>0x77)) {
         printf("   ");
         continue;
       }
diff --git a/prog/detect/sensors-detect b/prog/detect/sensors-detect
index adc6ef2f2fb9..2aa0575ce7b3 100755
--- a/prog/detect/sensors-detect
+++ b/prog/detect/sensors-detect
@@ -2418,7 +2418,7 @@ sub scan_adapter
   binmode FILE;
 
   # Now scan each address in turn
-  foreach $addr (0x04..0x77) {
+  foreach $addr (0x03..0x77) {
     # As the not_to_scan list is sorted, we can check it fast
     if (@not_to_scan and $not_to_scan[0] == $addr) {
       shift @not_to_scan;

Unfortunately no rationale in the log message, so this does not tell us
why address 0x03 was rehabilitated. I can only speculate that someone
had an actual device responding to address 0x03 back then.

That being said... In the end it doesn't really matter if addresses
0x03-0x07 are considered reserved but "should not", as long as the user
has an option to override that limitation. Now that such an option is
consistently available for all i2c tools, I have no objection to being
more conservative by default.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools 2/2] tools: restrict all addresses defined by the standard

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

Hi Jean,

> > The I2C standard reserves addresses 0x03-0x07. Adapt our tools to that.
> 
> I have a different interpretation of the specification. Addresses
> 0x04-0x07 are marked as "Hs-mode master code". They are not explicitly
> marked as "reserved" (although admittedly the table is named "Reserved
> addresses"). My understanding is that HS-mode will "use" these

Yes, my understanding is that the table lists all reserved addresses.

> addresses, which means that you can't use HS-mode _and_ slaves at
> addresses 0x04-0x07 on the same I²C bus segment. I do not read this as
> "non-HS-mode slaves can't use these addresses". But maybe this is just
> me, and also I don't know that much about HS-mode really.

From the spec, section 5.3.2: "Hs-mode master codes are reserved 8-bit
codes, which are not used for slave addressing or other purposes."

> Address 0x03 is indeed reserved, so obviously that this really makes me
> wonder how the original range of 0x03-0x77 was decided. Unfortunately
> the git history doesn't go that far (this range was already used in
> i2cget and i2cdetect at the time i2c-tools was split to its own
> repository).

My theory (only guessing, I didn't check): When introducing Hs-mode, the
I2C designers wanted to have the possibility for 8 Hs-masters. So, they
took addresses 0x04-0x07 (and the R/W bit) which allows for easy logic
in HW. And 0x03 was marked "reserved for future use" because it was in
the middle of those two reserved blocks. That would mean that before
Hs-mode, addresses 0x03-0x77 was in deed possible.

> That being said... In the end it doesn't really matter if addresses
> 0x03-0x07 are considered reserved but "should not", as long as the user
> has an option to override that limitation. Now that such an option is
> consistently available for all i2c tools, I have no objection to being
> more conservative by default.

Cool, I read this as an ack. Thanks!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i2c-tools 2/2] tools: restrict all addresses defined by the standard

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On Mon, Mar 11, 2019 at 11:33:35PM +0100, Wolfram Sang wrote:
> The I2C standard reserves addresses 0x03-0x07. Adapt our tools to that.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to master.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i2c-tools 0/2] tools: improvements to handling restricted addresses

[Niklas Söderlund]

Hi Wolfram,

Thanks for your work.

On 2019-03-11 23:33:33 +0100, Wolfram Sang wrote:
> For debugging, I needed to use the restricted address range. That's when I
> noticed that the docs for i2ctransfer were missing an update (patch 1). But
> even more surprising, I noticed that the reserved addresses 0x03-0x07 were not
> restricted. It seems we all are so used to our lovely i2c-tools that nobody
> (including me) never questioned that :) But it should be fixed (patch 2), I
> think.
> 
> Open for comments. Happy hacking!

I'm no expert in i2c but this matches my understanding of reserved 
addresses in the specification, feel free to add for the whole series

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> 
> Wolfram Sang (2):
>   tools: i2ctransfer: man: mention -a everywhere
>   tools: restrict all addresses defined by the standard
> 
>  CHANGES             |  2 ++
>  tools/i2cbusses.c   |  2 +-
>  tools/i2cdetect.8   |  2 +-
>  tools/i2cdetect.c   |  2 +-
>  tools/i2cdump.8     |  4 ++--
>  tools/i2cdump.c     |  2 +-
>  tools/i2cget.8      |  4 ++--
>  tools/i2cget.c      |  2 +-
>  tools/i2cset.8      |  4 ++--
>  tools/i2cset.c      |  2 +-
>  tools/i2ctransfer.8 | 12 +++++++-----
>  11 files changed, 21 insertions(+), 17 deletions(-)
> 
> -- 
> 2.11.0
> 

-- 
Regards,
Niklas Söderlund