Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH 1/2] w1: ds2408: add a missing reset when retrying in
 output_write()
From:   Jean-Francois Dagenais <jeff.dagenais@gmail.com>
In-Reply-To: <20190318092737.8170-2-manio@skyboo.net>
Date:   Tue, 19 Mar 2019 10:21:32 -0400
Cc:     linux-kernel@vger.kernel.org, Evgeniy Polyakov <zbr@ioremap.net>,
        Greg Kroah-Hartman <greg@kroah.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <DF2ABF87-116D-4287-8A18-89C2C59124AF@gmail.com>
References: <20190318092737.8170-1-manio@skyboo.net>
 <20190318092737.8170-2-manio@skyboo.net>
To:     Mariusz Bialonczyk <manio@skyboo.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <manio@skyboo.net> =
wrote:
>=20
> 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.
>=20
> 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.
>=20
> 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(-)
>=20
> 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;
>=20
> 	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);
 }
=20
+#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] =3D W1_F29_FUNC_READ_PIO_REGS;
+	w1_buf[1] =3D W1_F29_REG_OUTPUT_LATCH_STATE;
+	w1_buf[2] =3D 0;
+	w1_write_block(sl->master, w1_buf, 3);
+
+	/* read the result of the READ_PIO_REGS command */
+	return w1_read_8(sl->master) =3D=3D 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 =3D W1_F29_RETRIES;
+	ssize_t bytes_written =3D -EIO;
=20
 	if (count !=3D 1 || off !=3D 0)
 		return -EFAULT;
@@ -155,9 +184,9 @@ static ssize_t output_write(struct file *filp, =
struct kobject *kobj,
 	dev_dbg(&sl->dev, "mutex locked");
=20
 	if (w1_reset_select_slave(sl))
-		goto error;
+		goto out;
=20
-	while (retries--) {
+	do {
 		w1_buf[0] =3D W1_F29_FUNC_CHANN_ACCESS_WRITE;
 		w1_buf[1] =3D *buf;
 		w1_buf[2] =3D ~(*buf);
@@ -165,44 +194,23 @@ static ssize_t output_write(struct file *filp, =
struct kobject *kobj,
=20
 		readBack =3D w1_read_8(sl->master);
=20
-		if (readBack !=3D 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 =3D=3D W1_F29_SUCCESS_CONFIRM_BYTE &&
+		    optional_read_back_valid(sl, *buf)) {
+			bytes_written =3D 1;
+			goto out;
 		}
=20
-#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] =3D W1_F29_FUNC_READ_PIO_REGS;
-		w1_buf[1] =3D W1_F29_REG_OUTPUT_LATCH_STATE;
-		w1_buf[2] =3D 0;
-		w1_write_block(sl->master, w1_buf, 3);
-		/* read the result of the READ_PIO_REGS command */
-		if (w1_read_8(sl->master) =3D=3D *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);
=20
-	return -EIO;
+	dev_dbg(&sl->dev, "%s, mutex unlocked retries:%d\n",
+		(bytes_written > 0) ? "succeeded" : "error", retries);
+
+	return bytes_written;
 }
=20

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] =3D W1_F29_FUNC_CHANN_ACCESS_WRITE;
> 		w1_buf[1] =3D *buf;
> 		w1_buf[2] =3D ~(*buf);
> @@ -165,12 +172,8 @@ static ssize_t output_write(struct file *filp, =
struct kobject *kobj,
>=20
> 		readBack =3D w1_read_8(sl->master);
>=20
> -		if (readBack !=3D 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 !=3D W1_F29_SUCCESS_CONFIRM_BYTE)
> 			continue;
> -		}
>=20
> #ifdef CONFIG_W1_SLAVE_DS2408_READBACK
> 		/* here the master could read another byte which
> --=20
> 2.19.0.rc1
>=20