[PATCH] [RFC] cmd: i2c: fix default address len for DM_I2C
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?). --- cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index bd04b14024be..c57271479e81 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -118,17 +118,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES; #endif
#define DISP_LINE_LEN 16 - -/* - * Default for driver model is to use the chip's existing address length. - * For legacy code, this is not stored, so we need to use a suitable - * default. - */ -#if CONFIG_IS_ENABLED(DM_I2C) -#define DEFAULT_ADDR_LEN (-1) -#else #define DEFAULT_ADDR_LEN 1 -#endif
#if CONFIG_IS_ENABLED(DM_I2C) static struct udevice *i2c_cur_bus;
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote:
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?).
cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
diff --git a/cmd/i2c.c b/cmd/i2c.c index bd04b14024be..c57271479e81 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -118,17 +118,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES; #endif
#define DISP_LINE_LEN 16
-/*
- Default for driver model is to use the chip's existing address length.
- For legacy code, this is not stored, so we need to use a suitable
- default.
- */
-#if CONFIG_IS_ENABLED(DM_I2C) -#define DEFAULT_ADDR_LEN (-1) -#else #define DEFAULT_ADDR_LEN 1 -#endif
#if CONFIG_IS_ENABLED(DM_I2C) static struct udevice *i2c_cur_bus; -- 2.25.1
Regards, Simon
On Sat, Aug 13, 2022 at 7:59 AM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote:
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?).
cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
Simon,
Here's some annotated debug prints added where chip_offset is passed/set: Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit DRAM: 1 GiB i2c_chip_of_to_plat gsc@20 offset_len=1 i2c_chip_of_to_plat pmic@69 offset_len=1 i2c_get_chip i2c@30a20000 0x51 offset_len=1 i2c_bind_driver i2c@30a20000 offset_len=1 i2c_set_chip_offset_len generic_51 offset_len=1 dm_i2c_read generic_51 offset=0 offset_len=1 i2c_setup_offset 0x51 offset=0 offset_len=1 Core: 209 devices, 27 uclasses, devicetree: separate ... u-boot=> i2c dev 0 && i2c md 0x51 0 50 Setting bus to 0 i2c - I2C sub-system
Usage: i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info ...
So the chip I noticed this issue with is 0x51 which an atmel,24c02 compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of (i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above i2c1-0x51 (i2c1=i2c@30a20000) is accessed early as it holds the board model information and at that point in time it's accessed with alen=1 (which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but by the time the 'i2c md 0x51 0 50' comes around i2c_get_chip_offset_len() returns 0 for this device. The other eeprom devices on i2c1 are never accessed by a driver so they would never have a 'default' alen set.
Where is the initial setting of alen supposed to have come?
Best Regards,
Tim
diff --git a/cmd/i2c.c b/cmd/i2c.c index bd04b14024be..c57271479e81 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -118,17 +118,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES; #endif
#define DISP_LINE_LEN 16
-/*
- Default for driver model is to use the chip's existing address length.
- For legacy code, this is not stored, so we need to use a suitable
- default.
- */
-#if CONFIG_IS_ENABLED(DM_I2C) -#define DEFAULT_ADDR_LEN (-1) -#else #define DEFAULT_ADDR_LEN 1 -#endif
#if CONFIG_IS_ENABLED(DM_I2C) static struct udevice *i2c_cur_bus; -- 2.25.1
Regards, Simon
Hi Tim,
On Mon, 15 Aug 2022 at 11:48, Tim Harvey tharvey@gateworks.com wrote:
On Sat, Aug 13, 2022 at 7:59 AM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote:
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?).
cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
Simon,
Here's some annotated debug prints added where chip_offset is passed/set: Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit DRAM: 1 GiB i2c_chip_of_to_plat gsc@20 offset_len=1 i2c_chip_of_to_plat pmic@69 offset_len=1 i2c_get_chip i2c@30a20000 0x51 offset_len=1 i2c_bind_driver i2c@30a20000 offset_len=1 i2c_set_chip_offset_len generic_51 offset_len=1 dm_i2c_read generic_51 offset=0 offset_len=1 i2c_setup_offset 0x51 offset=0 offset_len=1 Core: 209 devices, 27 uclasses, devicetree: separate ... u-boot=> i2c dev 0 && i2c md 0x51 0 50 Setting bus to 0 i2c - I2C sub-system
Usage: i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info ...
So the chip I noticed this issue with is 0x51 which an atmel,24c02 compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of (i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above i2c1-0x51 (i2c1=i2c@30a20000) is accessed early as it holds the board model information and at that point in time it's accessed with alen=1 (which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but by the time the 'i2c md 0x51 0 50' comes around i2c_get_chip_offset_len() returns 0 for this device. The other eeprom devices on i2c1 are never accessed by a driver so they would never have a 'default' alen set.
OK I see,
Where is the initial setting of alen supposed to have come?
The "u-boot,i2c-offset-len" property in the device tree.
Regards, Simon
Best Regards,
Tim
diff --git a/cmd/i2c.c b/cmd/i2c.c index bd04b14024be..c57271479e81 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -118,17 +118,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES; #endif
#define DISP_LINE_LEN 16
-/*
- Default for driver model is to use the chip's existing address length.
- For legacy code, this is not stored, so we need to use a suitable
- default.
- */
-#if CONFIG_IS_ENABLED(DM_I2C) -#define DEFAULT_ADDR_LEN (-1) -#else #define DEFAULT_ADDR_LEN 1 -#endif
#if CONFIG_IS_ENABLED(DM_I2C) static struct udevice *i2c_cur_bus; -- 2.25.1
Regards, Simon
On Mon, Aug 15, 2022 at 3:16 PM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Mon, 15 Aug 2022 at 11:48, Tim Harvey tharvey@gateworks.com wrote:
On Sat, Aug 13, 2022 at 7:59 AM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote:
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?).
cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
Simon,
Here's some annotated debug prints added where chip_offset is passed/set: Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit DRAM: 1 GiB i2c_chip_of_to_plat gsc@20 offset_len=1 i2c_chip_of_to_plat pmic@69 offset_len=1 i2c_get_chip i2c@30a20000 0x51 offset_len=1 i2c_bind_driver i2c@30a20000 offset_len=1 i2c_set_chip_offset_len generic_51 offset_len=1 dm_i2c_read generic_51 offset=0 offset_len=1 i2c_setup_offset 0x51 offset=0 offset_len=1 Core: 209 devices, 27 uclasses, devicetree: separate ... u-boot=> i2c dev 0 && i2c md 0x51 0 50 Setting bus to 0 i2c - I2C sub-system
Usage: i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info ...
So the chip I noticed this issue with is 0x51 which an atmel,24c02 compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of (i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above i2c1-0x51 (i2c1=i2c@30a20000) is accessed early as it holds the board model information and at that point in time it's accessed with alen=1 (which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but by the time the 'i2c md 0x51 0 50' comes around i2c_get_chip_offset_len() returns 0 for this device. The other eeprom devices on i2c1 are never accessed by a driver so they would never have a 'default' alen set.
OK I see,
Where is the initial setting of alen supposed to have come?
The "u-boot,i2c-offset-len" property in the device tree.
Simon,
I see what happened here. The issue is caused by commit 8f8c04bf1ebbd ("i2c: fix stack buffer overflow vulnerability in i2c md command") which changed alen from int to uint (yet its still getting set to DEFAULT_ADDR_LEN which is -1) thus the 'if (alen > 3)' now returns CMD_RET_USAGE.
I'm not sure what the best fix is at this point - revert all or part of 8f8c04bf1ebbd
Nicolas, what is your opinion? To summarize commit 8f8c04bf1ebbd broke the ability to pass a -1 default address length to do_i2c_* such that something as common as 'i2c md 0x50 0 50' fails with a usage error.
Best Regards,
Tim
Tim
Regards, Simon
Best Regards,
Tim
diff --git a/cmd/i2c.c b/cmd/i2c.c index bd04b14024be..c57271479e81 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -118,17 +118,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES; #endif
#define DISP_LINE_LEN 16
-/*
- Default for driver model is to use the chip's existing address length.
- For legacy code, this is not stored, so we need to use a suitable
- default.
- */
-#if CONFIG_IS_ENABLED(DM_I2C) -#define DEFAULT_ADDR_LEN (-1) -#else #define DEFAULT_ADDR_LEN 1 -#endif
#if CONFIG_IS_ENABLED(DM_I2C) static struct udevice *i2c_cur_bus; -- 2.25.1
Regards, Simon
Hi Tim,
On Tue, 16 Aug 2022 at 13:50, Tim Harvey tharvey@gateworks.com wrote:
On Mon, Aug 15, 2022 at 3:16 PM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Mon, 15 Aug 2022 at 11:48, Tim Harvey tharvey@gateworks.com wrote:
On Sat, Aug 13, 2022 at 7:59 AM Simon Glass sjg@chromium.org wrote:
Hi Tim,
On Thu, 11 Aug 2022 at 11:57, Tim Harvey tharvey@gateworks.com wrote:
According to the comment block "The default {addr} parameter is one byte (.1) which works well for memories and registers with 8 bits of address space."
While this is true for legacy I2C a default length of -1 is being passed for DM_I2C which results in a usage error.
Restore the documented behavior by always using a default alen of 1.
Signed-off-by: Tim Harvey tharvey@gateworks.com
This is an RFC as I'm unclear if we want to restore the legacy usage or enforce a new usage (in which case the comment block should be updated) and I'm not clear if this is documented in other places. If the decision is to enforce a new usage then it is unclear to me how to specifiy the default alen as there is no command for that (i2c alen [len]?).
cmd/i2c.c | 10 ---------- 1 file changed, 10 deletions(-)
Can you dig into this a little more on your board? DEFAULT_ADDR_LEN is set to -1 so that by default it does not get set by the command, and the existing alen is used.
This is necessary for driver model, since the alen can be set by the peripheral device and we don't want to overwrite it:
if (!ret && alen != -1) ret = i2c_set_chip_offset_len(dev, alen);
Simon,
Here's some annotated debug prints added where chip_offset is passed/set: Model: Gateworks Venice GW73xx-0x i.MX8MM Development Kit DRAM: 1 GiB i2c_chip_of_to_plat gsc@20 offset_len=1 i2c_chip_of_to_plat pmic@69 offset_len=1 i2c_get_chip i2c@30a20000 0x51 offset_len=1 i2c_bind_driver i2c@30a20000 offset_len=1 i2c_set_chip_offset_len generic_51 offset_len=1 dm_i2c_read generic_51 offset=0 offset_len=1 i2c_setup_offset 0x51 offset=0 offset_len=1 Core: 209 devices, 27 uclasses, devicetree: separate ... u-boot=> i2c dev 0 && i2c md 0x51 0 50 Setting bus to 0 i2c - I2C sub-system
Usage: i2c bus [muxtype:muxaddr:muxchannel] - show I2C bus info ...
So the chip I noticed this issue with is 0x51 which an atmel,24c02 compatible eeprom which imx8mm-venice-gw700x.dtsi defines 4 of (i2c1-0x50, i2c1-0x51, i2c1-0x52, i2c1-0x53). You can see above i2c1-0x51 (i2c1=i2c@30a20000) is accessed early as it holds the board model information and at that point in time it's accessed with alen=1 (which I specify in board/gateworks/venice/eeprom.c:eeprom_read()) but by the time the 'i2c md 0x51 0 50' comes around i2c_get_chip_offset_len() returns 0 for this device. The other eeprom devices on i2c1 are never accessed by a driver so they would never have a 'default' alen set.
OK I see,
Where is the initial setting of alen supposed to have come?
The "u-boot,i2c-offset-len" property in the device tree.
Simon,
I see what happened here. The issue is caused by commit 8f8c04bf1ebbd ("i2c: fix stack buffer overflow vulnerability in i2c md command") which changed alen from int to uint (yet its still getting set to DEFAULT_ADDR_LEN which is -1) thus the 'if (alen > 3)' now returns CMD_RET_USAGE.
I'm not sure what the best fix is at this point - revert all or part of 8f8c04bf1ebbd
Nicolas, what is your opinion? To summarize commit 8f8c04bf1ebbd broke the ability to pass a -1 default address length to do_i2c_* such that something as common as 'i2c md 0x50 0 50' fails with a usage error.
Ah, ok. Well first we should add a test to dm/test/i2c.c to cover they failure you are seeing.
Yes, revert part of it and then add checks for -ve values?
Regards, Simon
participants (2)
-
Simon Glass -
Tim Harvey