| Message ID | 20210209110556.18814-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | [i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver" | expand |
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
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.
> 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.
>>>>> "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
> > 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?
>>>>> "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
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
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
> 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.
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
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
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
> 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?
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
> 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 :)
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 ",
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(-)