* [PATCH 0/2] w1: DS2408 fixes @ 2019-03-18 9:27 Mariusz Bialonczyk 2019-03-18 9:27 ` [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() Mariusz Bialonczyk ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-03-18 9:27 UTC (permalink / raw) To: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman, Jean-Francois Dagenais Cc: Mariusz Bialonczyk [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=y, Size: 1235 bytes --] Hi, I prepared a two fixes related with 1wire and DS2408. In short the problem was that a single DS2408 slave was properly setting the output value to a channel access write but the command was returning error. Moreover when there was a write of a 0xff value (all inputs off, there was also no problem - it's now also understandable for me after looking at the transmission log) ;) This problem doesn't occur on a multidrop bus. Finally I was able to catch the problems recently after analyzing the oscilloscope waveforms in PulseView. After those fixes all is now working perfectly fine - no matter if it is multidrop or single slave bus :) ps. if you are merging this please also take into account a patch which I've sent recently but it is still not merged: https://lore.kernel.org/lkml/20190304112336.21397-1-manio@skyboo.net/ regards, Mariusz Białończyk | xmpp/e-mail: manio@skyboo.net https://skyboo.net | https://github.com/manio Mariusz Bialonczyk (2): w1: ds2408: add a missing reset when retrying in output_write() w1: fix the resume command API drivers/w1/slaves/w1_ds2408.c | 13 ++++++++----- drivers/w1/w1_io.c | 11 +++++++++-- 2 files changed, 17 insertions(+), 7 deletions(-) -- 2.19.0.rc1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() 2019-03-18 9:27 [PATCH 0/2] w1: DS2408 fixes Mariusz Bialonczyk @ 2019-03-18 9:27 ` Mariusz Bialonczyk 2019-03-19 14:21 ` Jean-Francois Dagenais 2019-03-21 15:18 ` [PATCH v2] w1: ds2408: reset on output_write retry with readback Jean-Francois Dagenais 2019-03-18 9:27 ` [PATCH 2/2] w1: fix the resume command API Mariusz Bialonczyk 2019-03-21 10:52 ` [PATCH v2] " Mariusz Bialonczyk 2 siblings, 2 replies; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-03-18 9:27 UTC (permalink / raw) To: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman, Jean-Francois Dagenais Cc: Mariusz Bialonczyk When we have success in 'Channel Access Write' but reading back the latch state has failed, then the code continues but without doing a proper slave reset. This was leading to protocol errors as the slave treats the next 'Channel Access Write' as the continuation of previous command. This commit is fixing this, and because we have to reset no matter if the actual write or the readback checking is failing then the resetting is done on the beginning of the loop. Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net> Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/slaves/w1_ds2408.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index b535d5ec35b6..562ee2d861e8 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -158,6 +158,13 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, goto error; while (retries--) { + /* do a reset/resume on every new retry call + except the first one */ + if (retries < W1_F29_RETRIES - 1) { + if (w1_reset_resume_command(sl->master)) + goto error; + } + w1_buf[0] = W1_F29_FUNC_CHANN_ACCESS_WRITE; w1_buf[1] = *buf; w1_buf[2] = ~(*buf); @@ -165,12 +172,8 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, readBack = w1_read_8(sl->master); - if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) { - if (w1_reset_resume_command(sl->master)) - goto error; - /* try again, the slave is ready for a command */ + if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) continue; - } #ifdef CONFIG_W1_SLAVE_DS2408_READBACK /* here the master could read another byte which -- 2.19.0.rc1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() 2019-03-18 9:27 ` [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() Mariusz Bialonczyk @ 2019-03-19 14:21 ` Jean-Francois Dagenais 2019-03-19 14:25 ` Jean-Francois Dagenais 2019-03-21 10:55 ` Mariusz Bialonczyk 2019-03-21 15:18 ` [PATCH v2] w1: ds2408: reset on output_write retry with readback Jean-Francois Dagenais 1 sibling, 2 replies; 14+ messages in thread From: Jean-Francois Dagenais @ 2019-03-19 14:21 UTC (permalink / raw) To: Mariusz Bialonczyk; +Cc: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman > On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <manio@skyboo.net> wrote: > > When we have success in 'Channel Access Write' but reading back the latch > state has failed, then the code continues but without doing a proper > slave reset. This was leading to protocol errors as the slave treats > the next 'Channel Access Write' as the continuation of previous command. > > This commit is fixing this, and because we have to reset no matter if > the actual write or the readback checking is failing then the resetting > is done on the beginning of the loop. > > Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net> > Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com> > --- > drivers/w1/slaves/w1_ds2408.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c > index b535d5ec35b6..562ee2d861e8 100644 > --- a/drivers/w1/slaves/w1_ds2408.c > +++ b/drivers/w1/slaves/w1_ds2408.c > @@ -158,6 +158,13 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, > goto error; > > while (retries--) { > + /* do a reset/resume on every new retry call > + except the first one */ > + if (retries < W1_F29_RETRIES - 1) { > + if (w1_reset_resume_command(sl->master)) > + goto error; > + } > + The case being solved here is strictly restricted to the CONFIG_W1_SLAVE_DS2408_READBACK case and should be confined to this macro being defined. I think my original code here is to blame. Although I appreciate what you are trying to fix and that this does it, I don't really appreciate the resulting style as it puts the improbable case of the retry in the forefront of the loop using a non-obvious condition. This adds burden to the reader. Since this is an error handling case, it should like like so and be handled lower in the loop. May I suggest a cleaned up version my original klunky code with your fix in it (Note: this is untested, it compiles on arm64, that's all): drivers/w1/slaves/w1_ds2408.c | 78 ++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index b535d5ec35b6..bf308660f6ae 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -138,6 +138,34 @@ static ssize_t status_control_read(struct file *filp, struct kobject *kobj, W1_F29_REG_CONTROL_AND_STATUS, buf); } +#ifdef CONFIG_W1_SLAVE_DS2408_READBACK +static bool optional_read_back_valid(struct w1_slave *sl, u8 expected) +{ + u8 w1_buf[3]; + /* here the master could read another byte which + would be the PIO reg (the actual pin logic state) + since in this driver we don't know which pins are + in and outs, there's no value to read the state and + compare. with (*buf) so end this command abruptly: */ + if (w1_reset_resume_command(sl->master)) + return false; + /* go read back the output latches */ + /* (the direct effect of the write access) */ + w1_buf[0] = W1_F29_FUNC_READ_PIO_REGS; + w1_buf[1] = W1_F29_REG_OUTPUT_LATCH_STATE; + w1_buf[2] = 0; + w1_write_block(sl->master, w1_buf, 3); + + /* read the result of the READ_PIO_REGS command */ + return w1_read_8(sl->master) == expected; +} +#else +static bool optional_read_back_valid(struct w1_slave *sl, u8 expected) +{ + return true; +} +#endif + static ssize_t output_write(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -146,6 +174,7 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, u8 w1_buf[3]; u8 readBack; unsigned int retries = W1_F29_RETRIES; + ssize_t bytes_written = -EIO; if (count != 1 || off != 0) return -EFAULT; @@ -155,9 +184,9 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, dev_dbg(&sl->dev, "mutex locked"); if (w1_reset_select_slave(sl)) - goto error; + goto out; - while (retries--) { + do { w1_buf[0] = W1_F29_FUNC_CHANN_ACCESS_WRITE; w1_buf[1] = *buf; w1_buf[2] = ~(*buf); @@ -165,44 +194,23 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, readBack = w1_read_8(sl->master); - if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) { - if (w1_reset_resume_command(sl->master)) - goto error; - /* try again, the slave is ready for a command */ - continue; + if (readBack == W1_F29_SUCCESS_CONFIRM_BYTE && + optional_read_back_valid(sl, *buf)) { + bytes_written = 1; + goto out; } -#ifdef CONFIG_W1_SLAVE_DS2408_READBACK - /* here the master could read another byte which - would be the PIO reg (the actual pin logic state) - since in this driver we don't know which pins are - in and outs, there's no value to read the state and - compare. with (*buf) so end this command abruptly: */ if (w1_reset_resume_command(sl->master)) - goto error; - - /* go read back the output latches */ - /* (the direct effect of the write above) */ - w1_buf[0] = W1_F29_FUNC_READ_PIO_REGS; - w1_buf[1] = W1_F29_REG_OUTPUT_LATCH_STATE; - w1_buf[2] = 0; - w1_write_block(sl->master, w1_buf, 3); - /* read the result of the READ_PIO_REGS command */ - if (w1_read_8(sl->master) == *buf) -#endif - { - /* success! */ - mutex_unlock(&sl->master->bus_mutex); - dev_dbg(&sl->dev, - "mutex unlocked, retries:%d", retries); - return 1; - } - } -error: + goto out; /* unrecoverable error */ + /* try again, the slave is ready for a command */ + } while (--retries); +out: mutex_unlock(&sl->master->bus_mutex); - dev_dbg(&sl->dev, "mutex unlocked in error, retries:%d", retries); - return -EIO; + dev_dbg(&sl->dev, "%s, mutex unlocked retries:%d\n", + (bytes_written > 0) ? "succeeded" : "error", retries); + + return bytes_written; } I can do a proper patch submission if you guys provide positive response on this early RFC. Or better yet, you can take it and own it yourself as your v2 Mariusz. ;) > w1_buf[0] = W1_F29_FUNC_CHANN_ACCESS_WRITE; > w1_buf[1] = *buf; > w1_buf[2] = ~(*buf); > @@ -165,12 +172,8 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, > > readBack = w1_read_8(sl->master); > > - if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) { > - if (w1_reset_resume_command(sl->master)) > - goto error; > - /* try again, the slave is ready for a command */ > + if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) > continue; > - } > > #ifdef CONFIG_W1_SLAVE_DS2408_READBACK > /* here the master could read another byte which > -- > 2.19.0.rc1 > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() 2019-03-19 14:21 ` Jean-Francois Dagenais @ 2019-03-19 14:25 ` Jean-Francois Dagenais 2019-03-21 10:55 ` Mariusz Bialonczyk 1 sibling, 0 replies; 14+ messages in thread From: Jean-Francois Dagenais @ 2019-03-19 14:25 UTC (permalink / raw) To: Mariusz Bialonczyk; +Cc: open list, Evgeniy Polyakov, Greg Kroah-Hartman > On Mar 19, 2019, at 10:21, Jean-Francois Dagenais <jeff.dagenais@gmail.com> wrote: > > I can do a proper patch submission if you guys provide positive response on this > early RFC. Or better yet, you can take it and own it yourself as your v2 > Mariusz. ;) Just FYI, here's what the retry loop looks like after this suggested patch: do { w1_buf[0] = W1_F29_FUNC_CHANN_ACCESS_WRITE; w1_buf[1] = *buf; w1_buf[2] = ~(*buf); w1_write_block(sl->master, w1_buf, 3); readBack = w1_read_8(sl->master); if (readBack == W1_F29_SUCCESS_CONFIRM_BYTE && optional_read_back_valid(sl, *buf)) { bytes_written = 1; goto out; } if (w1_reset_resume_command(sl->master)) goto out; /* unrecoverable error */ /* try again, the slave is ready for a command */ } while (--retries); Much better on the eyes. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() 2019-03-19 14:21 ` Jean-Francois Dagenais 2019-03-19 14:25 ` Jean-Francois Dagenais @ 2019-03-21 10:55 ` Mariusz Bialonczyk 1 sibling, 0 replies; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-03-21 10:55 UTC (permalink / raw) To: Jean-Francois Dagenais; +Cc: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman On Tue, 19 Mar 2019 10:21:32 -0400 Jean-Francois Dagenais <jeff.dagenais@gmail.com> wrote: > May I suggest a cleaned up version my original klunky code with your fix in it > (Note: this is untested, it compiles on arm64, that's all): Thank you Jean! I just tested your version - and it is working as expected :) > I can do a proper patch submission if you guys provide positive response on this > early RFC. Or better yet, you can take it and own it yourself as your v2 > Mariusz. ;) No, it's your code, please just submit v2 as yours (I've already sent simplified version of the patch 1/2 as you suggested). Just drop the following and all will be fine :) Reported-by: Mariusz Bialonczyk <manio@skyboo.net> Thanks! regards, -- Mariusz Białończyk | xmpp/e-mail: manio@skyboo.net https://skyboo.net | https://github.com/manio ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] w1: ds2408: reset on output_write retry with readback 2019-03-18 9:27 ` [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() Mariusz Bialonczyk 2019-03-19 14:21 ` Jean-Francois Dagenais @ 2019-03-21 15:18 ` Jean-Francois Dagenais 2019-03-27 16:53 ` Greg Kroah-Hartman 1 sibling, 1 reply; 14+ messages in thread From: Jean-Francois Dagenais @ 2019-03-21 15:18 UTC (permalink / raw) To: open list, Evgeniy Polyakov; +Cc: Greg Kroah-Hartman, Mariusz Bialonczyk Originally Reported-by: Mariusz Bialonczyk <manio@skyboo.net> When we have success in 'Channel Access Write' but reading back latch states fails, a write is retried without doing a proper slave reset. This leads to protocol errors as the slave treats the next 'Channel Access Write' as the continuation of previous command. This commit is fixing this by making sure if the retry loop re-runs, a reset is performed, whatever the failure (CONFIRM_BYTE or the read back). The loop was quite due for a cleanup and this change mandated it. By isolating the CONFIG_W1_SLAVE_DS2408_READBACK case into it's own function, we vastly reduce the visual and branching(runtime and compile-time) noise. Tested-by: Mariusz Bialonczyk <manio@skyboo.net> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/slaves/w1_ds2408.c | 76 ++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index b535d5ec35b6..92e8f0755b9a 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -138,14 +138,37 @@ static ssize_t status_control_read(struct file *filp, struct kobject *kobj, W1_F29_REG_CONTROL_AND_STATUS, buf); } +#ifdef fCONFIG_W1_SLAVE_DS2408_READBACK +static bool optional_read_back_valid(struct w1_slave *sl, u8 expected) +{ + u8 w1_buf[3]; + + if (w1_reset_resume_command(sl->master)) + return false; + + w1_buf[0] = W1_F29_FUNC_READ_PIO_REGS; + w1_buf[1] = W1_F29_REG_OUTPUT_LATCH_STATE; + w1_buf[2] = 0; + + w1_write_block(sl->master, w1_buf, 3); + + return (w1_read_8(sl->master) == expected); +} +#else +static bool optional_read_back_valid(struct w1_slave *sl, u8 expected) +{ + return true; +} +#endif + static ssize_t output_write(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) { struct w1_slave *sl = kobj_to_w1_slave(kobj); u8 w1_buf[3]; - u8 readBack; unsigned int retries = W1_F29_RETRIES; + ssize_t bytes_written = -EIO; if (count != 1 || off != 0) return -EFAULT; @@ -155,54 +178,33 @@ static ssize_t output_write(struct file *filp, struct kobject *kobj, dev_dbg(&sl->dev, "mutex locked"); if (w1_reset_select_slave(sl)) - goto error; + goto out; - while (retries--) { + do { w1_buf[0] = W1_F29_FUNC_CHANN_ACCESS_WRITE; w1_buf[1] = *buf; w1_buf[2] = ~(*buf); - w1_write_block(sl->master, w1_buf, 3); - readBack = w1_read_8(sl->master); + w1_write_block(sl->master, w1_buf, 3); - if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) { - if (w1_reset_resume_command(sl->master)) - goto error; - /* try again, the slave is ready for a command */ - continue; + if (w1_read_8(sl->master) == W1_F29_SUCCESS_CONFIRM_BYTE && + optional_read_back_valid(sl, *buf)) { + bytes_written = 1; + goto out; } -#ifdef CONFIG_W1_SLAVE_DS2408_READBACK - /* here the master could read another byte which - would be the PIO reg (the actual pin logic state) - since in this driver we don't know which pins are - in and outs, there's no value to read the state and - compare. with (*buf) so end this command abruptly: */ if (w1_reset_resume_command(sl->master)) - goto error; + goto out; /* unrecoverable error */ + /* try again, the slave is ready for a command */ + } while (--retries); - /* go read back the output latches */ - /* (the direct effect of the write above) */ - w1_buf[0] = W1_F29_FUNC_READ_PIO_REGS; - w1_buf[1] = W1_F29_REG_OUTPUT_LATCH_STATE; - w1_buf[2] = 0; - w1_write_block(sl->master, w1_buf, 3); - /* read the result of the READ_PIO_REGS command */ - if (w1_read_8(sl->master) == *buf) -#endif - { - /* success! */ - mutex_unlock(&sl->master->bus_mutex); - dev_dbg(&sl->dev, - "mutex unlocked, retries:%d", retries); - return 1; - } - } -error: +out: mutex_unlock(&sl->master->bus_mutex); - dev_dbg(&sl->dev, "mutex unlocked in error, retries:%d", retries); - return -EIO; + dev_dbg(&sl->dev, "%s, mutex unlocked retries:%d\n", + (bytes_written > 0) ? "succeeded" : "error", retries); + + return bytes_written; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] w1: ds2408: reset on output_write retry with readback 2019-03-21 15:18 ` [PATCH v2] w1: ds2408: reset on output_write retry with readback Jean-Francois Dagenais @ 2019-03-27 16:53 ` Greg Kroah-Hartman 2019-03-28 12:17 ` Jean-Francois Dagenais 0 siblings, 1 reply; 14+ messages in thread From: Greg Kroah-Hartman @ 2019-03-27 16:53 UTC (permalink / raw) To: Jean-Francois Dagenais; +Cc: open list, Evgeniy Polyakov, Mariusz Bialonczyk On Thu, Mar 21, 2019 at 11:18:27AM -0400, Jean-Francois Dagenais wrote: > Originally > Reported-by: Mariusz Bialonczyk <manio@skyboo.net> That needs to go down in the signed-off-by area. Also, please resend this series in a way that I can apply it. You didn't say what the order was for the v2 patches. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] w1: ds2408: reset on output_write retry with readback 2019-03-27 16:53 ` Greg Kroah-Hartman @ 2019-03-28 12:17 ` Jean-Francois Dagenais 2019-04-03 8:33 ` Mariusz Bialonczyk 0 siblings, 1 reply; 14+ messages in thread From: Jean-Francois Dagenais @ 2019-03-28 12:17 UTC (permalink / raw) To: open list ** resending to list only because of bounce ** Hi Greg, > On Mar 27, 2019, at 12:53, Greg Kroah-Hartman <greg@kroah.com> wrote: > > On Thu, Mar 21, 2019 at 11:18:27AM -0400, Jean-Francois Dagenais wrote: >> Originally >> Reported-by: Mariusz Bialonczyk <manio@skyboo.net> > > That needs to go down in the signed-off-by area. Noted. > > Also, please resend this series in a way that I can apply it. You > didn't say what the order was for the v2 patches. Mariusz can chime in but I believe although it was the same exercise that led to his discoveries, each patch can stand alone and be applied in any order. I will send a v3 without a reply-to-id. Mariusz' patch "[PATCH v2] w1: fix the resume command API" can be applied as is unless you need it changed or resent. Cheers! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] w1: ds2408: reset on output_write retry with readback 2019-03-28 12:17 ` Jean-Francois Dagenais @ 2019-04-03 8:33 ` Mariusz Bialonczyk 0 siblings, 0 replies; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-04-03 8:33 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: open list, Jean-Francois Dagenais Hi Greg, Exactly as Jean-Francois stated: > Mariusz can chime in but I believe although it was the same exercise > that led to his discoveries, each patch can stand alone and be applied > in any order. > > I will send a v3 without a reply-to-id. Mariusz' patch "[PATCH v2] w1: > fix the resume command API" can be applied as is unless you need it > changed or resent. To sum it all - patches order is irrelevant and the patches to merge are: mine latest one: Subject: [PATCH v2] w1: fix the resume command API Date: Thu, 21 Mar 2019 11:52:55 +0100 Message-ID: <20190321105255.19126-1-manio@skyboo.net> and Jean's latest one: Subject: [PATCH v4] w1: ds2408: reset on output_write retry with readback Date: Thu, 28 Mar 2019 12:41:11 -0400 Message-Id: <20190328164111.24041-1-jeff.dagenais@gmail.com> regards, -- Mariusz Białończyk | xmpp/e-mail: manio@skyboo.net http://manio.skyboo.net | https://github.com/manio ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] w1: fix the resume command API 2019-03-18 9:27 [PATCH 0/2] w1: DS2408 fixes Mariusz Bialonczyk 2019-03-18 9:27 ` [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() Mariusz Bialonczyk @ 2019-03-18 9:27 ` Mariusz Bialonczyk 2019-03-19 13:21 ` Jean-Francois Dagenais 2019-03-21 10:52 ` [PATCH v2] " Mariusz Bialonczyk 2 siblings, 1 reply; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-03-18 9:27 UTC (permalink / raw) To: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman, Jean-Francois Dagenais Cc: Mariusz Bialonczyk From the DS2408 datasheet [1]: "Resume Command function checks the status of the RC flag and, if it is set, directly transfers control to the control functions, similar to a Skip ROM command. The only way to set the RC flag is through successfully executing the Match ROM, Search ROM, Conditional Search ROM, or Overdrive-Match ROM command" The function currently works perfectly fine in a multidrop bus, but when we have only a single slave connected, then only a Skip ROM is used and Match ROM is not called at all. This is leading to problems e.g. with single one DS2408 connected, as the Resume Command is not working properly and the device is responding with failing results after the Resume Command. This commit is fixing this by using a Skip ROM instead in those cases. The bandwidth / performance advantage is exactly the same. Refs: [1] https://datasheets.maximintegrated.com/en/ds/DS2408.pdf Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net> Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/w1_io.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c index 0364d3329c52..4697136b9027 100644 --- a/drivers/w1/w1_io.c +++ b/drivers/w1/w1_io.c @@ -432,8 +432,15 @@ int w1_reset_resume_command(struct w1_master *dev) if (w1_reset_bus(dev)) return -1; - /* This will make only the last matched slave perform a skip ROM. */ - w1_write_8(dev, W1_RESUME_CMD); + if (dev->slave_count == 1) { + /* Resume Command has to be preceeded with e.g. Match ROM which is + * not happening on single-slave buses, just do a Skip ROM instead + */ + w1_write_8(dev, W1_SKIP_ROM); + } else { + /* This will make only the last matched slave perform a skip ROM. */ + w1_write_8(dev, W1_RESUME_CMD); + } return 0; } EXPORT_SYMBOL_GPL(w1_reset_resume_command); -- 2.19.0.rc1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] w1: fix the resume command API 2019-03-18 9:27 ` [PATCH 2/2] w1: fix the resume command API Mariusz Bialonczyk @ 2019-03-19 13:21 ` Jean-Francois Dagenais 2019-03-19 14:21 ` Evgeniy Polyakov 0 siblings, 1 reply; 14+ messages in thread From: Jean-Francois Dagenais @ 2019-03-19 13:21 UTC (permalink / raw) To: Mariusz Bialonczyk; +Cc: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman Hi Mariusz, I appreciate your work on this. > On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <manio@skyboo.net> wrote: > > From the DS2408 datasheet [1]: > "Resume Command function checks the status of the RC flag and, if it is set, > directly transfers control to the control functions, similar to a Skip ROM > command. The only way to set the RC flag is through successfully executing > the Match ROM, Search ROM, Conditional Search ROM, or Overdrive-Match ROM > command" Indeed, figure 12-2 flow chart shows that SKIP_ROM resets RC to 0, then RESUME looks for RC==1 to enter the control function "mode". Nice find! I don't know however if other slaves are like that. Since the true impact of your suggested change is indeed null on the bus (bit count wise). I guess even this specific slave case is enough to warrant the change in the subsys. > > The function currently works perfectly fine in a multidrop bus, but when we > have only a single slave connected, then only a Skip ROM is used and Match > ROM is not called at all. This is leading to problems e.g. with single one > DS2408 connected, as the Resume Command is not working properly and the > device is responding with failing results after the Resume Command. > > This commit is fixing this by using a Skip ROM instead in those cases. > The bandwidth / performance advantage is exactly the same. > > Refs: > [1] https://datasheets.maximintegrated.com/en/ds/DS2408.pdf > > Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net> > Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com> Reviewed-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> > --- > drivers/w1/w1_io.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c > index 0364d3329c52..4697136b9027 100644 > --- a/drivers/w1/w1_io.c > +++ b/drivers/w1/w1_io.c > @@ -432,8 +432,15 @@ int w1_reset_resume_command(struct w1_master *dev) > if (w1_reset_bus(dev)) > return -1; > > - /* This will make only the last matched slave perform a skip ROM. */ > - w1_write_8(dev, W1_RESUME_CMD); > + if (dev->slave_count == 1) { > + /* Resume Command has to be preceeded with e.g. Match ROM which is > + * not happening on single-slave buses, just do a Skip ROM instead > + */ > + w1_write_8(dev, W1_SKIP_ROM); > + } else { > + /* This will make only the last matched slave perform a skip ROM. */ > + w1_write_8(dev, W1_RESUME_CMD); > + } This may be a subsys maintainer's style preference, but perhaps the verbose comments might be better suited for the git commit message. Could this then be shorted to if (dev->slave_count == 1) w1_write_8(dev, W1_SKIP_ROM); else w1_write_8(dev, W1_RESUME_CMD); Or maybe: w1_write_8(dev, dev->slave_count > 1 ? W1_RESUME_CMD : W1_SKIP_ROM); I am also ok with this proposed version, hence the "reviewed-by". > return 0; > } > EXPORT_SYMBOL_GPL(w1_reset_resume_command); > -- > 2.19.0.rc1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] w1: fix the resume command API 2019-03-19 13:21 ` Jean-Francois Dagenais @ 2019-03-19 14:21 ` Evgeniy Polyakov 2019-03-21 10:11 ` Mariusz Bialonczyk 0 siblings, 1 reply; 14+ messages in thread From: Evgeniy Polyakov @ 2019-03-19 14:21 UTC (permalink / raw) To: Jean-Francois Dagenais, Mariusz Bialonczyk Cc: linux-kernel, Greg Kroah-Hartman Hi everyone Mariusz, thank you for the patch, a small comment on it logic 19.03.2019, 16:21, "Jean-Francois Dagenais" <jeff.dagenais@gmail.com>: >> --- >> drivers/w1/w1_io.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c >> index 0364d3329c52..4697136b9027 100644 >> --- a/drivers/w1/w1_io.c >> +++ b/drivers/w1/w1_io.c >> @@ -432,8 +432,15 @@ int w1_reset_resume_command(struct w1_master *dev) >> if (w1_reset_bus(dev)) >> return -1; >> >> - /* This will make only the last matched slave perform a skip ROM. */ >> - w1_write_8(dev, W1_RESUME_CMD); >> + if (dev->slave_count == 1) { >> + /* Resume Command has to be preceeded with e.g. Match ROM which is >> + * not happening on single-slave buses, just do a Skip ROM instead >> + */ >> + w1_write_8(dev, W1_SKIP_ROM); Looks like this may break the search logic, can you check that with this patch applied some other single slave device will correctly 'tell' its id and it can be addressed via match rom (like, basically, just reading temperature or something like that)? >> + } else { >> + /* This will make only the last matched slave perform a skip ROM. */ >> + w1_write_8(dev, W1_RESUME_CMD); >> + } > > This may be a subsys maintainer's style preference, but perhaps the verbose comments > might be better suited for the git commit message. Could this then be shorted to > > if (dev->slave_count == 1) > w1_write_8(dev, W1_SKIP_ROM); > else > w1_write_8(dev, W1_RESUME_CMD); > > Or maybe: > > w1_write_8(dev, dev->slave_count > 1 ? W1_RESUME_CMD : W1_SKIP_ROM); > > I am also ok with this proposed version, hence the "reviewed-by". > >> return 0; >> } >> EXPORT_SYMBOL_GPL(w1_reset_resume_command); >> -- >> 2.19.0.rc1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] w1: fix the resume command API 2019-03-19 14:21 ` Evgeniy Polyakov @ 2019-03-21 10:11 ` Mariusz Bialonczyk 0 siblings, 0 replies; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-03-21 10:11 UTC (permalink / raw) To: Evgeniy Polyakov; +Cc: Jean-Francois Dagenais, linux-kernel, Greg Kroah-Hartman Hi Evgeniy, On Tue, 19 Mar 2019 17:21:09 +0300 Evgeniy Polyakov <zbr@ioremap.net> wrote: > Looks like this may break the search logic, can you check that with this patch applied > some other single slave device will correctly 'tell' its id and it can be addressed via match rom (like, basically, just reading temperature or something like that)? Yes, I have tested it in a mixed environment (regarding single slave/multidrop): /sys/devices/w1_bus_master5 # ll total 0 drwxr-xr-x 9 root 0 0 Mar 21 10:53 . drwxr-xr-x 17 root 0 0 Mar 2 16:49 .. drwxr-xr-x 4 root 0 0 Mar 21 10:55 28-00000921ff5e drwxr-xr-x 3 root 0 0 Mar 21 11:04 29-0000001246a4 drwxr-xr-x 3 root 0 0 Mar 21 11:04 29-0000001246b1 drwxr-xr-x 3 root 0 0 Mar 21 11:04 29-0000001246b9 drwxr-xr-x 3 root 0 0 Mar 21 10:54 29-0000001246bc drwxr-xr-x 3 root 0 0 Mar 21 10:53 3a-0000000f354f /sys/devices/w1_bus_master6 # ll total 0 drwxr-xr-x 4 root 0 0 Mar 21 10:54 . drwxr-xr-x 17 root 0 0 Mar 2 16:49 .. drwxr-xr-x 3 root 0 0 Mar 21 11:07 29-0000001246a9 No problem with searching and using other slaves on the bus. regards, -- Mariusz Białończyk | xmpp/e-mail: manio@skyboo.net https://skyboo.net | https://github.com/manio ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] w1: fix the resume command API 2019-03-18 9:27 [PATCH 0/2] w1: DS2408 fixes Mariusz Bialonczyk 2019-03-18 9:27 ` [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() Mariusz Bialonczyk 2019-03-18 9:27 ` [PATCH 2/2] w1: fix the resume command API Mariusz Bialonczyk @ 2019-03-21 10:52 ` Mariusz Bialonczyk 2 siblings, 0 replies; 14+ messages in thread From: Mariusz Bialonczyk @ 2019-03-21 10:52 UTC (permalink / raw) To: linux-kernel, Evgeniy Polyakov, Greg Kroah-Hartman, Jean-Francois Dagenais Cc: Mariusz Bialonczyk From the DS2408 datasheet [1]: "Resume Command function checks the status of the RC flag and, if it is set, directly transfers control to the control functions, similar to a Skip ROM command. The only way to set the RC flag is through successfully executing the Match ROM, Search ROM, Conditional Search ROM, or Overdrive-Match ROM command" The function currently works perfectly fine in a multidrop bus, but when we have only a single slave connected, then only a Skip ROM is used and Match ROM is not called at all. This is leading to problems e.g. with single one DS2408 connected, as the Resume Command is not working properly and the device is responding with failing results after the Resume Command. This commit is fixing this by using a Skip ROM instead in those cases. The bandwidth / performance advantage is exactly the same. Refs: [1] https://datasheets.maximintegrated.com/en/ds/DS2408.pdf Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net> Reviewed-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/w1_io.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c index 0364d3329c52..3516ce6718d9 100644 --- a/drivers/w1/w1_io.c +++ b/drivers/w1/w1_io.c @@ -432,8 +432,7 @@ int w1_reset_resume_command(struct w1_master *dev) if (w1_reset_bus(dev)) return -1; - /* This will make only the last matched slave perform a skip ROM. */ - w1_write_8(dev, W1_RESUME_CMD); + w1_write_8(dev, dev->slave_count > 1 ? W1_RESUME_CMD : W1_SKIP_ROM); return 0; } EXPORT_SYMBOL_GPL(w1_reset_resume_command); -- 2.19.0.rc1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-04-03 8:34 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-18 9:27 [PATCH 0/2] w1: DS2408 fixes Mariusz Bialonczyk 2019-03-18 9:27 ` [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write() Mariusz Bialonczyk 2019-03-19 14:21 ` Jean-Francois Dagenais 2019-03-19 14:25 ` Jean-Francois Dagenais 2019-03-21 10:55 ` Mariusz Bialonczyk 2019-03-21 15:18 ` [PATCH v2] w1: ds2408: reset on output_write retry with readback Jean-Francois Dagenais 2019-03-27 16:53 ` Greg Kroah-Hartman 2019-03-28 12:17 ` Jean-Francois Dagenais 2019-04-03 8:33 ` Mariusz Bialonczyk 2019-03-18 9:27 ` [PATCH 2/2] w1: fix the resume command API Mariusz Bialonczyk 2019-03-19 13:21 ` Jean-Francois Dagenais 2019-03-19 14:21 ` Evgeniy Polyakov 2019-03-21 10:11 ` Mariusz Bialonczyk 2019-03-21 10:52 ` [PATCH v2] " Mariusz Bialonczyk
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.