> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <ma...@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 <ma...@skyboo.net>
> Cc: Jean-Francois Dagenais <jeff.dagen...@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
>
