[PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

This reverts commit 34806fc4e7090b34e32fa1110d546ab5ce01a6a0. It was
developed against an experimental kernel. The regular kernel does not
update the new message length to userspace, so the check is always false
positive. We can't change the kernel behaviour because it would break
the ABI. So revert this commit.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Very embarrasing :( I am sorry for this. Jean, maybe this is worth a
4.2.1. release?

 tools/i2ctransfer.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
index f2a4df8..b0e8d43 100644
--- a/tools/i2ctransfer.c
+++ b/tools/i2ctransfer.c
@@ -88,12 +88,7 @@ static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
 		int recv_len = msgs[i].flags & I2C_M_RECV_LEN;
 		int print_buf = (read && (flags & PRINT_READ_BUF)) ||
 				(!read && (flags & PRINT_WRITE_BUF));
-		__u16 len = msgs[i].len;
-
-		if (recv_len && print_buf && len != msgs[i].buf[0] + 1) {
-			fprintf(stderr, "Correcting wrong msg length after recv_len! Please fix the I2C driver and/or report.\n");
-			len = msgs[i].buf[0] + 1;
-		}
+		__u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len;
 
 		if (flags & PRINT_HEADER) {
 			fprintf(output, "msg %u: addr 0x%02x, %s, len ",
-- 
2.28.0

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Jean Delvare]

Hi Wolfram,

On Tue,  9 Feb 2021 12:05:56 +0100, Wolfram Sang wrote:
> This reverts commit 34806fc4e7090b34e32fa1110d546ab5ce01a6a0. It was
> developed against an experimental kernel. The regular kernel does not
> update the new message length to userspace, so the check is always false
> positive. We can't change the kernel behaviour because it would break
> the ABI. So revert this commit.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Very embarrasing :( I am sorry for this. Jean, maybe this is worth a
> 4.2.1. release?

Sorry for not catching this, I must say I did not remember exactly what
the API was supposed to be, and I did not have any device to test the
change.

We don't usually do minor version updates for bug fixes. Instead, what
I do is maintain a list of such "must have" fixes, that package
maintainers can refer to. Look for "Recommended patches" at:

https://i2c.wiki.kernel.org/index.php/I2C_Tools

There's no section for version 4.2 yet, but we can add one as soon as
the commit hits the public repository.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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


> We don't usually do minor version updates for bug fixes. Instead, what
> I do is maintain a list of such "must have" fixes, that package
> maintainers can refer to. Look for "Recommended patches" at:
> 
> https://i2c.wiki.kernel.org/index.php/I2C_Tools
> 
> There's no section for version 4.2 yet, but we can add one as soon as
> the commit hits the public repository.

I added a section now for the 4.2 release. And (finally!) started
cleaning up the wiki a little.


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

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <wsa+renesas@sang-engineering.com> writes:

 >> We don't usually do minor version updates for bug fixes. Instead, what
 >> I do is maintain a list of such "must have" fixes, that package
 >> maintainers can refer to. Look for "Recommended patches" at:
 >> 
 >> https://i2c.wiki.kernel.org/index.php/I2C_Tools
 >> 
 >> There's no section for version 4.2 yet, but we can add one as soon as
 >> the commit hits the public repository.

 > I added a section now for the 4.2 release. And (finally!) started
 > cleaning up the wiki a little.

Thanks! As a packager, I must say that this way of handling bugfixes
isn't great - I only just noticed this now by accident.

What is the issue with making bugfix releases?

-- 
Bye, Peter Korsgaard

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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


>  > I added a section now for the 4.2 release. And (finally!) started
>  > cleaning up the wiki a little.
> 
> Thanks! As a packager, I must say that this way of handling bugfixes
> isn't great - I only just noticed this now by accident.
> 
> What is the issue with making bugfix releases?

Instead of a minor 4.2.1 we could maybe also simply do a 4.3?


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

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <wsa+renesas@sang-engineering.com> writes:

 >> > I added a section now for the 4.2 release. And (finally!) started
 >> > cleaning up the wiki a little.
 >> 
 >> Thanks! As a packager, I must say that this way of handling bugfixes
 >> isn't great - I only just noticed this now by accident.
 >> 
 >> What is the issue with making bugfix releases?

 > Instead of a minor 4.2.1 we could maybe also simply do a 4.3?

Sure, that is also fine by me.

-- 
Bye, Peter Korsgaard

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Jean Delvare]

Hi Wolfram,

On Tue, 13 Apr 2021 14:54:33 +0200, Wolfram Sang wrote:
> >  > I added a section now for the 4.2 release. And (finally!) started
> >  > cleaning up the wiki a little.  
> > 
> > Thanks! As a packager, I must say that this way of handling bugfixes
> > isn't great - I only just noticed this now by accident.
> > 
> > What is the issue with making bugfix releases?  
> 
> Instead of a minor 4.2.1 we could maybe also simply do a 4.3?

Sounds reasonable, saves the need to change the process. Sure it feels
strange to make a new release with only 2 commits since the previous
release, OTOH we have a regression in one of the tools, which is
something rare (thankfully) and problematic enough to justify a rush of
schedule.

I'll take care of that ASAP if that's OK with you. If you have any
other change that should make it into 4.3, now is the time to commit it.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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


> release, OTOH we have a regression in one of the tools, which is
> something rare (thankfully) and problematic enough to justify a rush of
> schedule.

Yeah. Sorry about the regression!

> I'll take care of that ASAP if that's OK with you. If you have any
> other change that should make it into 4.3, now is the time to commit it.

Thank you. I don't have any patches left but I will review the ones
waiting in patchwork right now.


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

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Jean Delvare]

Hi Wolfram,

On Tue, 25 May 2021 22:36:16 +0200, Wolfram Sang wrote:
> > I'll take care of that ASAP if that's OK with you. If you have any
> > other change that should make it into 4.3, now is the time to commit it.  
> 
> Thank you. I don't have any patches left but I will review the ones
> waiting in patchwork right now.

I've committed mine. Do you see anything left in patchwork, or are we
ready for a release?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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

Hi Jean,

> I've committed mine. Do you see anything left in patchwork, or are we
> ready for a release?

Well, I thought mainly about this patch "tools: i2cbusses: Handle bus
names like /dev/i2c-0" but you took care of it faster than I did.

There is only one patch left in patchwork from 2016:

http://patchwork.ozlabs.org/project/linux-i2c/patch/044b3af6a47dfa92e047f0ff57e39a5b61e00058.1463165295.git.leonard.crestez@intel.com/
"[i2c-tools] i2cget: Add support for i2c block data"

That's all what's left in patchwork.

Dunno if you care about the old patch, but I think we are good to go for
a release.

Thanks!

   Wolfram


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

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Jean Delvare]

On Wed, 2 Jun 2021 18:25:21 +0200, Wolfram Sang wrote:
> Well, I thought mainly about this patch "tools: i2cbusses: Handle bus
> names like /dev/i2c-0" but you took care of it faster than I did.
> 
> There is only one patch left in patchwork from 2016:
> 
> http://patchwork.ozlabs.org/project/linux-i2c/patch/044b3af6a47dfa92e047f0ff57e39a5b61e00058.1463165295.git.leonard.crestez@intel.com/
> "[i2c-tools] i2cget: Add support for i2c block data"
> 
> That's all what's left in patchwork.
> 
> Dunno if you care about the old patch, but I think we are good to go for
> a release.

Yes I do care. Apparently I wasn't Cc'd on that thread so I wasn't even
aware that patch existed. It would be very nice if you could bump the
thread to me (unless there's a way to retrieve it from patchwork that I
didn't find?)

I read the thread, my initial answer would have matched Guenter's (as
always...) but the contributor has a point, the range option is not
supported by i2cdump in I2C block mode. Whether it's better to add
support for it there, or in i2cget, I'll see.

I think i2cget has my preference, because i2cdump pretty much assumes
that I2C block reads retrieve successive 8-bit register values, so you
can have block boundaries anywhere without changing the result. As a
matter of fact, while i2cdump attempts to make 32-byte I2C block reads,
it will transparently handle short reads by issuing additional block
reads at arbitrary offsets. This works nicely for EEPROMs but could
fail for other devices.

If we only want *one* I2C block read at a specified offset then i2cget
seems more appropriate indeed.

Looking at the code, I see that i2cdump already supports SMBus block
mode, in a way which is very similar to what the contributor asked for,
i.e. it doesn't really dump the chip's registers, but merely reads one
SMBus block at a specific offset. It's questionable why this should
belong to i2cdump in the first place, beyond the fact that it returns
several values when i2cget typically returns only one value.

So one option would be get rid of "s" mode in i2cdump, and add support
for both I2C and SMBus block read to i2cget.

Might be possible to add support for range limitation to I2C block
reads in i2cdump nevertheless, as it could be useful to read only
portions of EEPROMs.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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


> Yes I do care. Apparently I wasn't Cc'd on that thread so I wasn't even
> aware that patch existed. It would be very nice if you could bump the
> thread to me (unless there's a way to retrieve it from patchwork that I
> didn't find?)

Sadly, I can't send the whole thread. I thought a had a local archive
collected via gmane, but it seems this is not as complete as I hoped for
:( You can download the patch itself as mbox, however. Check the upper
right corner of the patchwork page. Or search for "mbox".

> If we only want *one* I2C block read at a specified offset then i2cget
> seems more appropriate indeed.

I don't have a clear opinion here. On the one hand, if we want just one
block read, this is more a "get" than a "dump". On the other hand,
i2cdump already has a range-parameter. So, from a user perspective,
keeping i2cget to single values and update the range parameter in
i2cdump might be least confusing?


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

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Jean Delvare]

On Fri, 4 Jun 2021 21:21:17 +0200, Wolfram Sang wrote:
> > If we only want *one* I2C block read at a specified offset then i2cget
> > seems more appropriate indeed.  
> 
> I don't have a clear opinion here. On the one hand, if we want just one
> block read, this is more a "get" than a "dump". On the other hand,
> i2cdump already has a range-parameter. So, from a user perspective,
> keeping i2cget to single values and update the range parameter in
> i2cdump might be least confusing?

The feature doesn't clearly fit in either tool but could be made to fit
in both ;-) Which isn't a situation I like.

I think this is really a conceptual difference. For certain devices
(like EEPROMs) I2C Block read means reading from a range of register
addresses, which is i2cdump's realm. But for other devices, I2C Block
read means reading multiple register values from a single address
(which is typically what _SMBus_ Block reads are always about, but
nothing prevents devices from using I2C Block reads for the same
purpose). Which is more like i2cget's realm.

The problem is that, depending on which device our users are working
with, they will *expect* the feature to be in one tool or the other.
And most probably won't even have the idea of trying the other. Of
course we could add a redirection note in the manual page, but users
will have wasted time already. If they read the manual page at all. And
even then, the way the data is presented could be confusing if they
need to use the "other" tool.

As a matter of fact, I have at least two more records of people asking
for this very feature without realizing it was already (partially)
available in another tool (Aleksandr Amelkin in 2015 and Gina Ko in
2019). The fact that i2cset does support Block writes (since 2011!)
when i2cget does not support Block reads is definitely confusing.

So I'm actually tempted to add the feature to *both* tools. Crestez's
patch would be the base for the i2cget implementation, to which I would
happily add SMBus block read support. For i2cdump, it's about adding
support for register range to the "i" mode. I took a quick look already
and it seems fairly trivial to implement.

Stay tuned,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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


> So I'm actually tempted to add the feature to *both* tools. Crestez's
> patch would be the base for the i2cget implementation, to which I would
> happily add SMBus block read support. For i2cdump, it's about adding
> support for register range to the "i" mode. I took a quick look already
> and it seems fairly trivial to implement.

Sounds good. Let's take this road then. I am convinced :)


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

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Jean Delvare]

Hi Peter,

On Sat, 10 Apr 2021 10:14:31 +0200, Peter Korsgaard wrote:
> >>>>> "Wolfram" == Wolfram Sang <wsa+renesas@sang-engineering.com> writes:  
> 
>  >> We don't usually do minor version updates for bug fixes. Instead, what
>  >> I do is maintain a list of such "must have" fixes, that package
>  >> maintainers can refer to. Look for "Recommended patches" at:
>  >> 
>  >> https://i2c.wiki.kernel.org/index.php/I2C_Tools
>  >> 
>  >> There's no section for version 4.2 yet, but we can add one as soon as
>  >> the commit hits the public repository.  
> 
>  > I added a section now for the 4.2 release. And (finally!) started
>  > cleaning up the wiki a little.  
> 
> Thanks! As a packager, I must say that this way of handling bugfixes
> isn't great - I only just noticed this now by accident.
> 
> What is the issue with making bugfix releases?

The main issue is that making releases (bugfix or not) takes time. Even
though it's partly automated, there are still a number of manual steps
involved, and you can't afford getting them wrong. So as far as I am
concerned, making a release is always a time of stress.

Making a bugfix release would also require a change in my process, as
we would need a new branch in git to cherry pick the fixes that need to
go into the bugfix release.

Second issue is that I chose to go for 2-number versions for i2c tools
v4. Doing bugfix releases with 3 numbers now means mixing 2-number
versions with 3-number versions which can lead to confusion (for
sorting order, if nothing else). Maybe I should have gone for 3-number
versions, but as I did not have the intention to make bugfix releases
in the first place, this seemed overkill. And I still think it is, as I
believe - if memory serves - it is the first time we face a regression
in i2c-tools since I started managing the project. i2c-tools is a small
project with few commits, I simply don't want to make the process
around it more complex than needed.

I must say I'm surprised to see requests for more frequent and/or
bugfix releases when the world has moved to CD/CI and some projects
have abandoned the very notion of version number. If you can't be
bothered with checking the recommended fixes on top of the latest
release, then maybe just use the latest git snapshot always?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

[Wolfram Sang]

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

On Tue, Feb 09, 2021 at 12:05:56PM +0100, Wolfram Sang wrote:
> This reverts commit 34806fc4e7090b34e32fa1110d546ab5ce01a6a0. It was
> developed against an experimental kernel. The regular kernel does not
> update the new message length to userspace, so the check is always false
> positive. We can't change the kernel behaviour because it would break
> the ABI. So revert this commit.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to master.


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