[PATCH 0/2] w1: DS2408 fixes

[PATCH 0/2] w1: DS2408 fixes

[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

[PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write()

[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

[PATCH 2/2] w1: fix the resume command API

[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

Re: [PATCH 2/2] w1: fix the resume command API

[Jean-Francois Dagenais]

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
> 

Re: [PATCH 2/2] w1: fix the resume command API

[Evgeniy Polyakov]

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

Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write()

[Jean-Francois Dagenais]

> 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
> 

Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write()

[Jean-Francois Dagenais]

> 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.

Re: [PATCH 2/2] w1: fix the resume command API

[Mariusz Bialonczyk]

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

[PATCH v2] w1: fix the resume command API

[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

Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in output_write()

[Mariusz Bialonczyk]

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

[PATCH v2] w1: ds2408: reset on output_write retry with readback

[Jean-Francois Dagenais]

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

Re: [PATCH v2] w1: ds2408: reset on output_write retry with readback

[Greg Kroah-Hartman]

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

Re: [PATCH v2] w1: ds2408: reset on output_write retry with readback

[Jean-Francois Dagenais]

** 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!

Re: [PATCH v2] w1: ds2408: reset on output_write retry with readback

[Mariusz Bialonczyk]

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