[PATCH] i2c-i801: Consolidate polling

[PATCH] i2c-i801: Consolidate polling

[Jean Delvare]

Come up with a consistent, driver-wide strategy for event polling. For
intermediate steps of byte-by-byte block transactions, check for
BYTE_DONE or any error flag being set. At the end of every transaction,
check for both BUSY being cleared and INTR or any error flag being set.
This avoids having to wait twice for the same event.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Daniel, this is what I had in mind. This applies on top of your first 6
patches (i.e. all the preliminary cleanup patches, not the interrupt
patches.) It solves the performance regression problem and makes the
code look better IMHO. Tested OK on ICH10 and ICH3-M. What do you
think? Comments and suggestions for improvements are very welcome.

It may be possible to combine i801_wait_intr() and
i801_wait_byte_done() into a single function, as they do pretty much
the same, but I'll only do that if it doesn't hurt readability. My
first attempt was horrible.

 drivers/i2c/busses/i2c-i801.c |   88 ++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 45 deletions(-)

--- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c	2012-07-02 18:09:59.000000000 +0200
+++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c	2012-07-03 10:07:42.254632389 +0200
@@ -207,13 +207,13 @@ static int i801_check_pre(struct i801_pr
 }
 
 /* Convert the status register to an error code, and clear it. */
-static int i801_check_post(struct i801_priv *priv, int status, int timeout)
+static int i801_check_post(struct i801_priv *priv, int status)
 {
 	int result = 0;
+	int clear;
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout) {
-		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
+	if (unlikely(status < 0)) {
 		/* try to stop the current command */
 		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
 		outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
@@ -245,42 +245,60 @@ static int i801_check_post(struct i801_p
 		dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
 	}
 
-	if (result) {
-		/* Clear error flags */
-		outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
-		status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
-		if (status) {
-			dev_warn(&priv->pci_dev->dev, "Failed clearing status "
-				 "flags at end of transaction (%02x)\n",
-				 status);
-		}
-	}
+	/* Clear status flags except BYTE_DONE, to be cleared by caller */
+	clear = STATUS_ERROR_FLAGS;
+	if (!(status & SMBHSTSTS_BYTE_DONE))
+		clear |= SMBHSTSTS_INTR;
+	outb_p(status & clear, SMBHSTSTS(priv));
 
 	return result;
 }
 
-/* wait for INTR bit as advised by Intel */
-static void i801_wait_intr(struct i801_priv *priv)
+/* Wait for BUSY being cleared and either INTR or an error flag being set */
+static int i801_wait_intr(struct i801_priv *priv)
 {
 	int timeout = 0;
 	int status;
 
+	/* We will always wait for a fraction of a second! */
 	do {
 		usleep_range(250, 500);
 		status = inb_p(SMBHSTSTS(priv));
-	} while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
+	} while (((status & SMBHSTSTS_HOST_BUSY) ||
+		  !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
+		 (timeout++ < MAX_RETRIES));
 
-	if (timeout > MAX_RETRIES)
+	if (timeout > MAX_RETRIES) {
 		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
+		return -ETIMEDOUT;
+	}
+	return status;
+}
 
-	outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
+/* Wait for either BYTE_DONE or an error flag being set */
+static int i801_wait_byte_done(struct i801_priv *priv)
+{
+	int timeout = 0;
+	int status;
+
+	/* We will always wait for a fraction of a second! */
+	do {
+		usleep_range(250, 500);
+		status = inb_p(SMBHSTSTS(priv));
+	} while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
+		 (timeout++ < MAX_RETRIES));
+
+	if (timeout > MAX_RETRIES) {
+		dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
+		return -ETIMEDOUT;
+	}
+	return status;
 }
 
 static int i801_transaction(struct i801_priv *priv, int xact)
 {
 	int status;
 	int result;
-	int timeout = 0;
 
 	result = i801_check_pre(priv);
 	if (result < 0)
@@ -290,19 +308,8 @@ static int i801_transaction(struct i801_
 	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
-	/* We will always wait for a fraction of a second! */
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-	} while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_RETRIES));
-
-	result = i801_check_post(priv, status, timeout > MAX_RETRIES);
-	if (result < 0)
-		return result;
-
-	i801_wait_intr(priv);
-
-	return 0;
+	status = i801_wait_intr(priv);
+	return i801_check_post(priv, status);
 }
 
 static int i801_block_transaction_by_block(struct i801_priv *priv,
@@ -353,7 +360,6 @@ static int i801_block_transaction_byte_b
 	int smbcmd;
 	int status;
 	int result;
-	int timeout;
 
 	result = i801_check_pre(priv);
 	if (result < 0)
@@ -381,15 +387,8 @@ static int i801_block_transaction_byte_b
 			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
 			       SMBHSTCNT(priv));
 
-		/* We will always wait for a fraction of a second! */
-		timeout = 0;
-		do {
-			usleep_range(250, 500);
-			status = inb_p(SMBHSTSTS(priv));
-		} while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
-			 && (timeout++ < MAX_RETRIES));
-
-		result = i801_check_post(priv, status, timeout > MAX_RETRIES);
+		status = i801_wait_byte_done(priv);
+		result = i801_check_post(priv, status);
 		if (result < 0)
 			return result;
 
@@ -421,9 +420,8 @@ static int i801_block_transaction_byte_b
 		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 	}
 
-	i801_wait_intr(priv);
-
-	return 0;
+	status = i801_wait_intr(priv);
+	return i801_check_post(priv, status);
 }
 
 static int i801_set_block_buffer_mode(struct i801_priv *priv)


-- 
Jean Delvare

Re: [PATCH] i2c-i801: Consolidate polling

[Daniel Kurtz]

On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> Come up with a consistent, driver-wide strategy for event polling. For
> intermediate steps of byte-by-byte block transactions, check for
> BYTE_DONE or any error flag being set. At the end of every transaction,
> check for both BUSY being cleared and INTR or any error flag being set.
> This avoids having to wait twice for the same event.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Daniel, this is what I had in mind. This applies on top of your first 6
> patches (i.e. all the preliminary cleanup patches, not the interrupt
> patches.) It solves the performance regression problem and makes the
> code look better IMHO. Tested OK on ICH10 and ICH3-M. What do you
> think? Comments and suggestions for improvements are very welcome.
>
> It may be possible to combine i801_wait_intr() and
> i801_wait_byte_done() into a single function, as they do pretty much
> the same, but I'll only do that if it doesn't hurt readability. My
> first attempt was horrible.
>
>  drivers/i2c/busses/i2c-i801.c |   88 ++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 45 deletions(-)
>
> --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c    2012-07-02 18:09:59.000000000 +0200
> +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03 10:07:42.254632389 +0200
> @@ -207,13 +207,13 @@ static int i801_check_pre(struct i801_pr
>  }
>
>  /* Convert the status register to an error code, and clear it. */
> -static int i801_check_post(struct i801_priv *priv, int status, int timeout)
> +static int i801_check_post(struct i801_priv *priv, int status)
>  {
>         int result = 0;
> +       int clear;
>
>         /* If the SMBus is still busy, we give up */
> -       if (timeout) {
> -               dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> +       if (unlikely(status < 0)) {
>                 /* try to stop the current command */
>                 dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
>                 outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> @@ -245,42 +245,60 @@ static int i801_check_post(struct i801_p
>                 dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
>         }
>
> -       if (result) {
> -               /* Clear error flags */
> -               outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> -               status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
> -               if (status) {
> -                       dev_warn(&priv->pci_dev->dev, "Failed clearing status "
> -                                "flags at end of transaction (%02x)\n",
> -                                status);
> -               }
> -       }
> +       /* Clear status flags except BYTE_DONE, to be cleared by caller */
> +       clear = STATUS_ERROR_FLAGS;
> +       if (!(status & SMBHSTSTS_BYTE_DONE))
> +               clear |= SMBHSTSTS_INTR;
> +       outb_p(status & clear, SMBHSTSTS(priv));

This becomes simpler (see below...), since status is already masked by
(STATUS_ERROR_FLAGS | SMBHSTSTS_INTR).

+       outb_p(status, SMBHSTSTS(priv));

>
>         return result;
>  }
>
> -/* wait for INTR bit as advised by Intel */
> -static void i801_wait_intr(struct i801_priv *priv)
> +/* Wait for BUSY being cleared and either INTR or an error flag being set */
> +static int i801_wait_intr(struct i801_priv *priv)
>  {
>         int timeout = 0;
>         int status;
>
> +       /* We will always wait for a fraction of a second! */
>         do {
>                 usleep_range(250, 500);
>                 status = inb_p(SMBHSTSTS(priv));
> -       } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
> +       } while (((status & SMBHSTSTS_HOST_BUSY) ||
> +                 !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
> +                (timeout++ < MAX_RETRIES));


>From your experiments, I think you saw that the final status (ERROR |
INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
loop need only check HOST_BUSY, and then just return the final status
bits of the transfer (ie, ERROR_FLAGS | INTR).  So, the function name
could be something like:

  i801_wait_xfer_status()

> -       if (timeout > MAX_RETRIES)
> +       if (timeout > MAX_RETRIES) {
>                 dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> +               return -ETIMEDOUT;
> +       }
> +       return status;
> +}
>
> -       outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> +/* Wait for either BYTE_DONE or an error flag being set */
> +static int i801_wait_byte_done(struct i801_priv *priv)
> +{
> +       int timeout = 0;
> +       int status;
> +
> +       /* We will always wait for a fraction of a second! */
> +       do {
> +               usleep_range(250, 500);
> +               status = inb_p(SMBHSTSTS(priv));
> +       } while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
> +                (timeout++ < MAX_RETRIES));
> +
> +       if (timeout > MAX_RETRIES) {
> +               dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
> +               return -ETIMEDOUT;
> +       }
> +       return status;
>  }

For i801_wait_byte_done(), on success I would return 0, if an error
was set, return (status & ERROR_FLAG), and of course return -ETIMEDOUT
on timeout.
Then, we only need to call i801_check_post() if i801_wait_byte_done()
!= 0... see below...

>
>  static int i801_transaction(struct i801_priv *priv, int xact)
>  {
>         int status;
>         int result;
> -       int timeout = 0;
>
>         result = i801_check_pre(priv);
>         if (result < 0)
> @@ -290,19 +308,8 @@ static int i801_transaction(struct i801_
>          * SMBSCMD are passed in xact */
>         outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>
> -       /* We will always wait for a fraction of a second! */
> -       do {
> -               usleep_range(250, 500);
> -               status = inb_p(SMBHSTSTS(priv));
> -       } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_RETRIES));
> -
> -       result = i801_check_post(priv, status, timeout > MAX_RETRIES);
> -       if (result < 0)
> -               return result;
> -
> -       i801_wait_intr(priv);
> -
> -       return 0;
> +       status = i801_wait_intr(priv);
> +       return i801_check_post(priv, status);
>  }
>
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
> @@ -353,7 +360,6 @@ static int i801_block_transaction_byte_b
>         int smbcmd;
>         int status;
>         int result;
> -       int timeout;
>
>         result = i801_check_pre(priv);
>         if (result < 0)
> @@ -381,15 +387,8 @@ static int i801_block_transaction_byte_b
>                         outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>                                SMBHSTCNT(priv));
>
> -               /* We will always wait for a fraction of a second! */
> -               timeout = 0;
> -               do {
> -                       usleep_range(250, 500);
> -                       status = inb_p(SMBHSTSTS(priv));
> -               } while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
> -                        && (timeout++ < MAX_RETRIES));
> -
> -               result = i801_check_post(priv, status, timeout > MAX_RETRIES);

+               status = i801_wait_byte_done(priv);
+                 if (status)
+                         goto done;

> @@ -421,9 +420,8 @@ static int i801_block_transaction_byte_b
>                 outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>         }
>
> -       i801_wait_intr(priv);
> -
> -       return 0;
> +       status = i801_wait_intr(priv);

done:
> +       return i801_check_post(priv, status);
>  }
>
>  static int i801_set_block_buffer_mode(struct i801_priv *priv)
>
>
> --
> Jean Delvare

While we are at this, let's clean up the "Illegal SMBus block read
size" error path, as well.
Why not use the "KILL" bit to terminate the transaction?

-Daniel

Re: [PATCH] i2c-i801: Consolidate polling

[Jean Delvare]

Hi Daniel,

Thanks for the feedback.

On Tue, 3 Jul 2012 17:50:13 +0800, Daniel Kurtz wrote:
> On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >
> > Come up with a consistent, driver-wide strategy for event polling. For
> > intermediate steps of byte-by-byte block transactions, check for
> > BYTE_DONE or any error flag being set. At the end of every transaction,
> > check for both BUSY being cleared and INTR or any error flag being set.
> > This avoids having to wait twice for the same event.
> >
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Cc: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> > Daniel, this is what I had in mind. This applies on top of your first 6
> > patches (i.e. all the preliminary cleanup patches, not the interrupt
> > patches.) It solves the performance regression problem and makes the
> > code look better IMHO. Tested OK on ICH10 and ICH3-M. What do you
> > think? Comments and suggestions for improvements are very welcome.
> >
> > It may be possible to combine i801_wait_intr() and
> > i801_wait_byte_done() into a single function, as they do pretty much
> > the same, but I'll only do that if it doesn't hurt readability. My
> > first attempt was horrible.
> >
> >  drivers/i2c/busses/i2c-i801.c |   88 ++++++++++++++++++++---------------------
> >  1 file changed, 43 insertions(+), 45 deletions(-)
> >
> > --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c    2012-07-02 18:09:59.000000000 +0200
> > +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03 10:07:42.254632389 +0200
> > (...)
> > @@ -245,42 +245,60 @@ static int i801_check_post(struct i801_p
> >                 dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
> >         }
> >
> > -       if (result) {
> > -               /* Clear error flags */
> > -               outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> > -               status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
> > -               if (status) {
> > -                       dev_warn(&priv->pci_dev->dev, "Failed clearing status "
> > -                                "flags at end of transaction (%02x)\n",
> > -                                status);
> > -               }
> > -       }
> > +       /* Clear status flags except BYTE_DONE, to be cleared by caller */
> > +       clear = STATUS_ERROR_FLAGS;
> > +       if (!(status & SMBHSTSTS_BYTE_DONE))
> > +               clear |= SMBHSTSTS_INTR;
> > +       outb_p(status & clear, SMBHSTSTS(priv));
> 
> This becomes simpler (see below...), since status is already masked by
> (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR).
> 
> +       outb_p(status, SMBHSTSTS(priv));

You're correct, I had not thought of doing it this way.

> >
> >         return result;
> >  }
> >
> > -/* wait for INTR bit as advised by Intel */
> > -static void i801_wait_intr(struct i801_priv *priv)
> > +/* Wait for BUSY being cleared and either INTR or an error flag being set */
> > +static int i801_wait_intr(struct i801_priv *priv)
> >  {
> >         int timeout = 0;
> >         int status;
> >
> > +       /* We will always wait for a fraction of a second! */
> >         do {
> >                 usleep_range(250, 500);
> >                 status = inb_p(SMBHSTSTS(priv));
> > -       } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
> > +       } while (((status & SMBHSTSTS_HOST_BUSY) ||
> > +                 !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
> > +                (timeout++ < MAX_RETRIES));
> 
> 
> From your experiments, I think you saw that the final status (ERROR |
> INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
> loop need only check HOST_BUSY, and then just return the final status
> bits of the transfer (ie, ERROR_FLAGS | INTR).  So, the function name
> could be something like:
> 
>   i801_wait_xfer_status()

You're right. But I did these tests out of curiosity, maybe I shouldn't
have published the results, because now I feel reluctant to use the
results. The reason is that this is not specified in the datasheet, so
I have no guarantee that the same holds for all chips or all
transaction types.

Furthermore, I see no reason to not play it safe. What costs time and
CPU here is the sleep and the inb_p. The extra test is pretty cheap
IMHO.

> > (...)
> > +/* Wait for either BYTE_DONE or an error flag being set */
> > +static int i801_wait_byte_done(struct i801_priv *priv)
> > +{
> > +       int timeout = 0;
> > +       int status;
> > +
> > +       /* We will always wait for a fraction of a second! */
> > +       do {
> > +               usleep_range(250, 500);
> > +               status = inb_p(SMBHSTSTS(priv));
> > +       } while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
> > +                (timeout++ < MAX_RETRIES));
> > +
> > +       if (timeout > MAX_RETRIES) {
> > +               dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
> > +               return -ETIMEDOUT;
> > +       }
> > +       return status;
> >  }
> 
> For i801_wait_byte_done(), on success I would return 0, if an error
> was set, return (status & ERROR_FLAG), and of course return -ETIMEDOUT
> on timeout.
> Then, we only need to call i801_check_post() if i801_wait_byte_done()
> != 0... see below...

Good idea, yes, this simplifies the code somewhat.

> >
> >  static int i801_transaction(struct i801_priv *priv, int xact)
> >  {
> >         int status;
> >         int result;
> > -       int timeout = 0;
> >
> >         result = i801_check_pre(priv);
> >         if (result < 0)
> > @@ -290,19 +308,8 @@ static int i801_transaction(struct i801_
> >          * SMBSCMD are passed in xact */
> >         outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
> >
> > -       /* We will always wait for a fraction of a second! */
> > -       do {
> > -               usleep_range(250, 500);
> > -               status = inb_p(SMBHSTSTS(priv));
> > -       } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_RETRIES));
> > -
> > -       result = i801_check_post(priv, status, timeout > MAX_RETRIES);
> > -       if (result < 0)
> > -               return result;
> > -
> > -       i801_wait_intr(priv);
> > -
> > -       return 0;
> > +       status = i801_wait_intr(priv);
> > +       return i801_check_post(priv, status);
> >  }
> >
> >  static int i801_block_transaction_by_block(struct i801_priv *priv,
> > @@ -353,7 +360,6 @@ static int i801_block_transaction_byte_b
> >         int smbcmd;
> >         int status;
> >         int result;
> > -       int timeout;
> >
> >         result = i801_check_pre(priv);
> >         if (result < 0)
> > @@ -381,15 +387,8 @@ static int i801_block_transaction_byte_b
> >                         outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> >                                SMBHSTCNT(priv));
> >
> > -               /* We will always wait for a fraction of a second! */
> > -               timeout = 0;
> > -               do {
> > -                       usleep_range(250, 500);
> > -                       status = inb_p(SMBHSTSTS(priv));
> > -               } while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
> > -                        && (timeout++ < MAX_RETRIES));
> > -
> > -               result = i801_check_post(priv, status, timeout > MAX_RETRIES);
> 
> +               status = i801_wait_byte_done(priv);
> +                 if (status)
> +                         goto done;

That's not really different from my own code, I guess it depends if one
likes gotos or not.

> > @@ -421,9 +420,8 @@ static int i801_block_transaction_byte_b
> >                 outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> >         }
> >
> > -       i801_wait_intr(priv);
> > -
> > -       return 0;
> > +       status = i801_wait_intr(priv);
> 
> done:
> > +       return i801_check_post(priv, status);
> >  }
> >
> >  static int i801_set_block_buffer_mode(struct i801_priv *priv)
> 
> While we are at this, let's clean up the "Illegal SMBus block read
> size" error path, as well.
> Why not use the "KILL" bit to terminate the transaction?

I have no idea why it was done that way, although I have to admit I'm
the author of the code (commit cf898dc5.) If using KILL works too and
this makes the core more simple, I'm all for it. This is easy enough to
test.

With your suggested changes, I have the following (tested) incremental
changes which I could merge into my patch. Please let me know if I
missed or misunderstood anything.

---
 drivers/i2c/busses/i2c-i801.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

--- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c	2012-07-03 15:00:21.000000000 +0200
+++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c	2012-07-03 15:25:50.759652731 +0200
@@ -206,11 +206,14 @@ static int i801_check_pre(struct i801_pr
 	return 0;
 }
 
-/* Convert the status register to an error code, and clear it. */
+/*
+ * Convert the status register to an error code, and clear it.
+ * Note that status only contains the bits we want to clear, not the
+ * actual register value.
+ */
 static int i801_check_post(struct i801_priv *priv, int status)
 {
 	int result = 0;
-	int clear;
 
 	/* If the SMBus is still busy, we give up */
 	if (unlikely(status < 0)) {
@@ -246,10 +249,7 @@ static int i801_check_post(struct i801_p
 	}
 
 	/* Clear status flags except BYTE_DONE, to be cleared by caller */
-	clear = STATUS_ERROR_FLAGS;
-	if (!(status & SMBHSTSTS_BYTE_DONE))
-		clear |= SMBHSTSTS_INTR;
-	outb_p(status & clear, SMBHSTSTS(priv));
+	outb_p(status, SMBHSTSTS(priv));
 
 	return result;
 }
@@ -272,7 +272,7 @@ static int i801_wait_intr(struct i801_pr
 		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
 		return -ETIMEDOUT;
 	}
-	return status;
+	return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
 }
 
 /* Wait for either BYTE_DONE or an error flag being set */
@@ -292,7 +292,7 @@ static int i801_wait_byte_done(struct i8
 		dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
 		return -ETIMEDOUT;
 	}
-	return status;
+	return status & STATUS_ERROR_FLAGS;
 }
 
 static int i801_transaction(struct i801_priv *priv, int xact)
@@ -388,9 +388,8 @@ static int i801_block_transaction_byte_b
 			       SMBHSTCNT(priv));
 
 		status = i801_wait_byte_done(priv);
-		result = i801_check_post(priv, status);
-		if (result < 0)
-			return result;
+		if (status)
+			goto exit;
 
 		if (i == 1 && read_write == I2C_SMBUS_READ
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
@@ -421,6 +420,7 @@ static int i801_block_transaction_byte_b
 	}
 
 	status = i801_wait_intr(priv);
+exit:
 	return i801_check_post(priv, status);
 }
 


-- 
Jean Delvare

Re: [PATCH] i2c-i801: Consolidate polling

[Daniel Kurtz]

On Tue, Jul 3, 2012 at 9:39 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi Daniel,
>
> Thanks for the feedback.
>
> On Tue, 3 Jul 2012 17:50:13 +0800, Daniel Kurtz wrote:
> > On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > >
> > > Come up with a consistent, driver-wide strategy for event polling. For
> > > intermediate steps of byte-by-byte block transactions, check for
> > > BYTE_DONE or any error flag being set. At the end of every
> > > transaction,
> > > check for both BUSY being cleared and INTR or any error flag being
> > > set.
> > > This avoids having to wait twice for the same event.
> > >
> > > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > > Cc: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > ---
> > > Daniel, this is what I had in mind. This applies on top of your first
> > > 6
> > > patches (i.e. all the preliminary cleanup patches, not the interrupt
> > > patches.) It solves the performance regression problem and makes the
> > > code look better IMHO. Tested OK on ICH10 and ICH3-M. What do you
> > > think? Comments and suggestions for improvements are very welcome.
> > >
> > > It may be possible to combine i801_wait_intr() and
> > > i801_wait_byte_done() into a single function, as they do pretty much
> > > the same, but I'll only do that if it doesn't hurt readability. My
> > > first attempt was horrible.
> > >
> > >  drivers/i2c/busses/i2c-i801.c |   88
> > > ++++++++++++++++++++---------------------
> > >  1 file changed, 43 insertions(+), 45 deletions(-)
> > >
> > > --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c    2012-07-02
> > > 18:09:59.000000000 +0200
> > > +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03
> > > 10:07:42.254632389 +0200
> > > (...)
> > > @@ -245,42 +245,60 @@ static int i801_check_post(struct i801_p
> > >                 dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
> > >         }
> > >
> > > -       if (result) {
> > > -               /* Clear error flags */
> > > -               outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> > > -               status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
> > > -               if (status) {
> > > -                       dev_warn(&priv->pci_dev->dev, "Failed clearing
> > > status "
> > > -                                "flags at end of transaction
> > > (%02x)\n",
> > > -                                status);
> > > -               }
> > > -       }
> > > +       /* Clear status flags except BYTE_DONE, to be cleared by
> > > caller */
> > > +       clear = STATUS_ERROR_FLAGS;
> > > +       if (!(status & SMBHSTSTS_BYTE_DONE))
> > > +               clear |= SMBHSTSTS_INTR;
> > > +       outb_p(status & clear, SMBHSTSTS(priv));
> >
> > This becomes simpler (see below...), since status is already masked by
> > (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR).
> >
> > +       outb_p(status, SMBHSTSTS(priv));
>
> You're correct, I had not thought of doing it this way.
>
> > >
> > >         return result;
> > >  }
> > >
> > > -/* wait for INTR bit as advised by Intel */
> > > -static void i801_wait_intr(struct i801_priv *priv)
> > > +/* Wait for BUSY being cleared and either INTR or an error flag being
> > > set */
> > > +static int i801_wait_intr(struct i801_priv *priv)
> > >  {
> > >         int timeout = 0;
> > >         int status;
> > >
> > > +       /* We will always wait for a fraction of a second! */
> > >         do {
> > >                 usleep_range(250, 500);
> > >                 status = inb_p(SMBHSTSTS(priv));
> > > -       } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ <
> > > MAX_RETRIES));
> > > +       } while (((status & SMBHSTSTS_HOST_BUSY) ||
> > > +                 !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR)))
> > > &&
> > > +                (timeout++ < MAX_RETRIES));
> >
> >
> > From your experiments, I think you saw that the final status (ERROR |
> > INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
> > loop need only check HOST_BUSY, and then just return the final status
> > bits of the transfer (ie, ERROR_FLAGS | INTR).  So, the function name
> > could be something like:
> >
> >   i801_wait_xfer_status()
>
> You're right. But I did these tests out of curiosity, maybe I shouldn't
> have published the results, because now I feel reluctant to use the
> results. The reason is that this is not specified in the datasheet, so
> I have no guarantee that the same holds for all chips or all
> transaction types.
>
> Furthermore, I see no reason to not play it safe. What costs time and
> CPU here is the sleep and the inb_p. The extra test is pretty cheap
> IMHO.

Sorry, I forgot to make my actual point last time.  This loop actually
terminates *early* now, since it ends when an error occur, without
waiting for the transaction to finish.  I think it would be safer to
wait for the transaction to finish (HOST_BUSY -> 0) first, and then
return the status.  Otherwise, you might end up trying to clear the
error status while HOST_BUSY is set, which is technically a violation
of the spec:

From ICH 10 datasheet:

HOST_BUSY — R/WC.
0 = Cleared by the ICH10 when the current transaction is completed.
1 = Indicates that the ICH10 is running a command from the host
interface. No SMB registers should be accessed while this bit is set,
except the BLOCK DATA BYTE Register. The BLOCK DATA BYTE Register can
be accessed when this bit is set only when the SMB_CMD bits in the
Host Control Register are programmed for Block command or I2C Read
command. This is necessary in order to check the DONE_STS bit.

>
> > > (...)
> > > +/* Wait for either BYTE_DONE or an error flag being set */
> > > +static int i801_wait_byte_done(struct i801_priv *priv)
> > > +{
> > > +       int timeout = 0;
> > > +       int status;
> > > +
> > > +       /* We will always wait for a fraction of a second! */
> > > +       do {
> > > +               usleep_range(250, 500);
> > > +               status = inb_p(SMBHSTSTS(priv));
> > > +       } while (!(status & (STATUS_ERROR_FLAGS |
> > > SMBHSTSTS_BYTE_DONE)) &&
> > > +                (timeout++ < MAX_RETRIES));
> > > +
> > > +       if (timeout > MAX_RETRIES) {
> > > +               dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
> > > +               return -ETIMEDOUT;
> > > +       }
> > > +       return status;
> > >  }
> >
> > For i801_wait_byte_done(), on success I would return 0, if an error
> > was set, return (status & ERROR_FLAG), and of course return -ETIMEDOUT
> > on timeout.
> > Then, we only need to call i801_check_post() if i801_wait_byte_done()
> > != 0... see below...
>
> Good idea, yes, this simplifies the code somewhat.
>
> > >
> > >  static int i801_transaction(struct i801_priv *priv, int xact)
> > >  {
> > >         int status;
> > >         int result;
> > > -       int timeout = 0;
> > >
> > >         result = i801_check_pre(priv);
> > >         if (result < 0)
> > > @@ -290,19 +308,8 @@ static int i801_transaction(struct i801_
> > >          * SMBSCMD are passed in xact */
> > >         outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
> > >
> > > -       /* We will always wait for a fraction of a second! */
> > > -       do {
> > > -               usleep_range(250, 500);
> > > -               status = inb_p(SMBHSTSTS(priv));
> > > -       } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ <
> > > MAX_RETRIES));
> > > -
> > > -       result = i801_check_post(priv, status, timeout > MAX_RETRIES);
> > > -       if (result < 0)
> > > -               return result;
> > > -
> > > -       i801_wait_intr(priv);
> > > -
> > > -       return 0;
> > > +       status = i801_wait_intr(priv);
> > > +       return i801_check_post(priv, status);
> > >  }
> > >
> > >  static int i801_block_transaction_by_block(struct i801_priv *priv,
> > > @@ -353,7 +360,6 @@ static int i801_block_transaction_byte_b
> > >         int smbcmd;
> > >         int status;
> > >         int result;
> > > -       int timeout;
> > >
> > >         result = i801_check_pre(priv);
> > >         if (result < 0)
> > > @@ -381,15 +387,8 @@ static int i801_block_transaction_byte_b
> > >                         outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> > >                                SMBHSTCNT(priv));
> > >
> > > -               /* We will always wait for a fraction of a second! */
> > > -               timeout = 0;
> > > -               do {
> > > -                       usleep_range(250, 500);
> > > -                       status = inb_p(SMBHSTSTS(priv));
> > > -               } while (!(status & (SMBHSTSTS_BYTE_DONE |
> > > STATUS_ERROR_FLAGS))
> > > -                        && (timeout++ < MAX_RETRIES));
> > > -
> > > -               result = i801_check_post(priv, status, timeout >
> > > MAX_RETRIES);
> >
> > +               status = i801_wait_byte_done(priv);
> > +                 if (status)
> > +                         goto done;
>
> That's not really different from my own code, I guess it depends if one
> likes gotos or not.
>
> > > @@ -421,9 +420,8 @@ static int i801_block_transaction_byte_b
> > >                 outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> > >         }
> > >
> > > -       i801_wait_intr(priv);
> > > -
> > > -       return 0;
> > > +       status = i801_wait_intr(priv);
> >
> > done:
> > > +       return i801_check_post(priv, status);
> > >  }
> > >
> > >  static int i801_set_block_buffer_mode(struct i801_priv *priv)
> >
> > While we are at this, let's clean up the "Illegal SMBus block read
> > size" error path, as well.
> > Why not use the "KILL" bit to terminate the transaction?
>
> I have no idea why it was done that way, although I have to admit I'm
> the author of the code (commit cf898dc5.) If using KILL works too and
> this makes the core more simple, I'm all for it. This is easy enough to
> test.
>
> With your suggested changes, I have the following (tested) incremental
> changes which I could merge into my patch. Please let me know if I
> missed or misunderstood anything.
>
> ---
>  drivers/i2c/busses/i2c-i801.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c    2012-07-03
> 15:00:21.000000000 +0200
> +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03
> 15:25:50.759652731 +0200
> @@ -206,11 +206,14 @@ static int i801_check_pre(struct i801_pr
>         return 0;
>  }
>
> -/* Convert the status register to an error code, and clear it. */
> +/*
> + * Convert the status register to an error code, and clear it.
> + * Note that status only contains the bits we want to clear, not the
> + * actual register value.
> + */
>  static int i801_check_post(struct i801_priv *priv, int status)
>  {
>         int result = 0;
> -       int clear;
>
>         /* If the SMBus is still busy, we give up */
>         if (unlikely(status < 0)) {
> @@ -246,10 +249,7 @@ static int i801_check_post(struct i801_p
>         }
>
>         /* Clear status flags except BYTE_DONE, to be cleared by caller */
> -       clear = STATUS_ERROR_FLAGS;
> -       if (!(status & SMBHSTSTS_BYTE_DONE))
> -               clear |= SMBHSTSTS_INTR;
> -       outb_p(status & clear, SMBHSTSTS(priv));
> +       outb_p(status, SMBHSTSTS(priv));
>
>         return result;
>  }
> @@ -272,7 +272,7 @@ static int i801_wait_intr(struct i801_pr

"i801_wait_intr()" isn't such a good name anymore.  I recommend
i801_wait_xfer_status(), or i801_wait_host_idle().

>                 dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");

"Timed out waiting for transaction to complete!\n"

>                 return -ETIMEDOUT;
>         }
> -       return status;
> +       return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
>  }
>
>  /* Wait for either BYTE_DONE or an error flag being set */
> @@ -292,7 +292,7 @@ static int i801_wait_byte_done(struct i8
>                 dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
>                 return -ETIMEDOUT;
>         }
> -       return status;
> +       return status & STATUS_ERROR_FLAGS;
>  }
>
>  static int i801_transaction(struct i801_priv *priv, int xact)
> @@ -388,9 +388,8 @@ static int i801_block_transaction_byte_b
>                                SMBHSTCNT(priv));
>
>                 status = i801_wait_byte_done(priv);
> -               result = i801_check_post(priv, status);
> -               if (result < 0)
> -                       return result;
> +               if (status)
> +                       goto exit;
>
>                 if (i == 1 && read_write == I2C_SMBUS_READ
>                  && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> @@ -421,6 +420,7 @@ static int i801_block_transaction_byte_b
>         }
>
>         status = i801_wait_intr(priv);
> +exit:
>         return i801_check_post(priv, status);
>  }
>
>
>
> --
> Jean Delvare

Re: [PATCH] i2c-i801: Consolidate polling

[Jean Delvare]

Hi Daniel,

Thanks again for the continued feedback.

On Wed, 4 Jul 2012 14:12:26 +0800, Daniel Kurtz wrote:
> On Tue, Jul 3, 2012 at 9:39 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Tue, 3 Jul 2012 17:50:13 +0800, Daniel Kurtz wrote:
> > > On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > > > -/* wait for INTR bit as advised by Intel */
> > > > -static void i801_wait_intr(struct i801_priv *priv)
> > > > +/* Wait for BUSY being cleared and either INTR or an error flag being set */
> > > > +static int i801_wait_intr(struct i801_priv *priv)
> > > >  {
> > > >         int timeout = 0;
> > > >         int status;
> > > >
> > > > +       /* We will always wait for a fraction of a second! */
> > > >         do {
> > > >                 usleep_range(250, 500);
> > > >                 status = inb_p(SMBHSTSTS(priv));
> > > > -       } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
> > > > +       } while (((status & SMBHSTSTS_HOST_BUSY) ||
> > > > +                 !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
> > > > +                (timeout++ < MAX_RETRIES));
> > >
> > > From your experiments, I think you saw that the final status (ERROR |
> > > INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
> > > loop need only check HOST_BUSY, and then just return the final status
> > > bits of the transfer (ie, ERROR_FLAGS | INTR).  So, the function name
> > > could be something like:
> > >
> > >   i801_wait_xfer_status()
> >
> > You're right. But I did these tests out of curiosity, maybe I shouldn't
> > have published the results, because now I feel reluctant to use the
> > results. The reason is that this is not specified in the datasheet, so
> > I have no guarantee that the same holds for all chips or all
> > transaction types.
> >
> > Furthermore, I see no reason to not play it safe. What costs time and
> > CPU here is the sleep and the inb_p. The extra test is pretty cheap
> > IMHO.
> 
> Sorry, I forgot to make my actual point last time.  This loop actually
> terminates *early* now, since it ends when an error occur, without
> waiting for the transaction to finish.

This wasn't my intent, and also not what my code is doing, unless I am
reading it wrong.

> I think it would be safer to
> wait for the transaction to finish (HOST_BUSY -> 0) first, and then
> return the status.  

Which is what I'm doing, as far as I can see. Leaving the timeout case
aside for a moment, the loop keeps running if the following condition
is met:

	(status & SMBHSTSTS_HOST_BUSY) || !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))

Which, per De Morgan's laws [1], means the loop _exit_ condition is:

	!(status & SMBHSTSTS_HOST_BUSY) && (status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))

That is, we only exit the loop if the BUSY bit is cleared _and_ either
INTR or an error flag is set. In other words this is a late exit. Are
you OK with it now, or am I really missing something?

[1] http://en.wikipedia.org/wiki/De_Morgan%27s_laws

> > (...)
> > @@ -272,7 +272,7 @@ static int i801_wait_intr(struct i801_pr
> 
> "i801_wait_intr()" isn't such a good name anymore.  I recommend
> i801_wait_xfer_status(), or i801_wait_host_idle().

i801_wait_intr() isn't such a bad name IMHO, we are still ideally hoping
that INTR will be set, as this means the transaction will be
successful. The name has the advantage of being symmetric with
i801_wait_byte_done(), so it's clear which one of the status bits we
are expecting.

"i801_wait_host_idle" would be a good name too, but the actual name
doesn't really matter anyway, as the next patch merges both functions
into a single one. I had written this patch before you posted your
proposed changes, but it can still be applied on top of these (with the
usual context adjustment.) Unless you really don't like it? If so,
please tell me why.

> >                 dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> 
> "Timed out waiting for transaction to complete!\n"

My next patch already changes this to:

		dev_err(&priv->pci_dev->dev,
			"Timeout waiting for %02x! (%02x)\n", wait_set, status);

My message is more generic because the function is now generic, but the
information provided is more or less the same. If you think wording can
be improved, just let me know.

---
 drivers/i2c/busses/i2c-i801.c |   50 ++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

--- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c	2012-07-03 16:49:42.000000000 +0200
+++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c	2012-07-03 17:10:37.062356531 +0200
@@ -254,29 +254,16 @@ static int i801_check_post(struct i801_p
 	return result;
 }
 
-/* Wait for BUSY being cleared and either INTR or an error flag being set */
-static int i801_wait_intr(struct i801_priv *priv)
-{
-	int timeout = 0;
-	int status;
-
-	/* We will always wait for a fraction of a second! */
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-	} while (((status & SMBHSTSTS_HOST_BUSY) ||
-		  !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
-		 (timeout++ < MAX_RETRIES));
-
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
-		return -ETIMEDOUT;
-	}
-	return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
-}
-
-/* Wait for either BYTE_DONE or an error flag being set */
-static int i801_wait_byte_done(struct i801_priv *priv)
+/*
+ * Wait for some status register bits being cleared and others being set.
+ * Typical usage:
+ *  - Wait for BUSY being cleared and either INTR or an error flag being set
+ *    (end of transaction)
+ *  - Wait for either BYTE_DONE or an error flag being set (step of
+ *    byte-by-byte block transaction)
+ * Return the status flags which should be cleared automatically.
+ */
+static int i801_wait(struct i801_priv *priv, int wait_cleared, int wait_set)
 {
 	int timeout = 0;
 	int status;
@@ -285,14 +272,17 @@ static int i801_wait_byte_done(struct i8
 	do {
 		usleep_range(250, 500);
 		status = inb_p(SMBHSTSTS(priv));
-	} while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
+	} while (((status & wait_cleared) ||
+		  !(status & (STATUS_ERROR_FLAGS | wait_set))) &&
 		 (timeout++ < MAX_RETRIES));
 
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
+	if (unlikely(timeout > MAX_RETRIES)) {
+		dev_err(&priv->pci_dev->dev,
+			"Timeout waiting for %02x! (%02x)\n", wait_set, status);
 		return -ETIMEDOUT;
 	}
-	return status & STATUS_ERROR_FLAGS;
+	status &= ~SMBHSTSTS_BYTE_DONE;	/* Must be cleared explicitly */
+	return status & (STATUS_ERROR_FLAGS | wait_set);
 }
 
 static int i801_transaction(struct i801_priv *priv, int xact)
@@ -308,7 +298,7 @@ static int i801_transaction(struct i801_
 	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
-	status = i801_wait_intr(priv);
+	status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
 	return i801_check_post(priv, status);
 }
 
@@ -387,7 +377,7 @@ static int i801_block_transaction_byte_b
 			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
 			       SMBHSTCNT(priv));
 
-		status = i801_wait_byte_done(priv);
+		status = i801_wait(priv, 0, SMBHSTSTS_BYTE_DONE);
 		if (status)
 			goto exit;
 
@@ -419,7 +409,7 @@ static int i801_block_transaction_byte_b
 		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 	}
 
-	status = i801_wait_intr(priv);
+	status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
 exit:
 	return i801_check_post(priv, status);
 }


-- 
Jean Delvare

Re: [PATCH] i2c-i801: Consolidate polling

[Daniel Kurtz]

On Wed, Jul 4, 2012 at 3:49 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Daniel,
>
> Thanks again for the continued feedback.
>
> On Wed, 4 Jul 2012 14:12:26 +0800, Daniel Kurtz wrote:
>> On Tue, Jul 3, 2012 at 9:39 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> > On Tue, 3 Jul 2012 17:50:13 +0800, Daniel Kurtz wrote:
>> > > On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> > > > -/* wait for INTR bit as advised by Intel */
>> > > > -static void i801_wait_intr(struct i801_priv *priv)
>> > > > +/* Wait for BUSY being cleared and either INTR or an error flag being set */
>> > > > +static int i801_wait_intr(struct i801_priv *priv)
>> > > >  {
>> > > >         int timeout = 0;
>> > > >         int status;
>> > > >
>> > > > +       /* We will always wait for a fraction of a second! */
>> > > >         do {
>> > > >                 usleep_range(250, 500);
>> > > >                 status = inb_p(SMBHSTSTS(priv));
>> > > > -       } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
>> > > > +       } while (((status & SMBHSTSTS_HOST_BUSY) ||
>> > > > +                 !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
>> > > > +                (timeout++ < MAX_RETRIES));
>> > >
>> > > From your experiments, I think you saw that the final status (ERROR |
>> > > INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
>> > > loop need only check HOST_BUSY, and then just return the final status
>> > > bits of the transfer (ie, ERROR_FLAGS | INTR).  So, the function name
>> > > could be something like:
>> > >
>> > >   i801_wait_xfer_status()
>> >
>> > You're right. But I did these tests out of curiosity, maybe I shouldn't
>> > have published the results, because now I feel reluctant to use the
>> > results. The reason is that this is not specified in the datasheet, so
>> > I have no guarantee that the same holds for all chips or all
>> > transaction types.
>> >
>> > Furthermore, I see no reason to not play it safe. What costs time and
>> > CPU here is the sleep and the inb_p. The extra test is pretty cheap
>> > IMHO.
>>
>> Sorry, I forgot to make my actual point last time.  This loop actually
>> terminates *early* now, since it ends when an error occur, without
>> waiting for the transaction to finish.
>
> This wasn't my intent, and also not what my code is doing, unless I am
> reading it wrong.
>
>> I think it would be safer to
>> wait for the transaction to finish (HOST_BUSY -> 0) first, and then
>> return the status.
>
> Which is what I'm doing, as far as I can see. Leaving the timeout case
> aside for a moment, the loop keeps running if the following condition
> is met:
>
>         (status & SMBHSTSTS_HOST_BUSY) || !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))
>
> Which, per De Morgan's laws [1], means the loop _exit_ condition is:
>
>         !(status & SMBHSTSTS_HOST_BUSY) && (status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))
>
> That is, we only exit the loop if the BUSY bit is cleared _and_ either
> INTR or an error flag is set. In other words this is a late exit. Are
> you OK with it now, or am I really missing something?

Nope.  You are right, I was reading the code wrong.
We are in complete agreement about what it should be doing, and you
have convinced me that it is in fact doing it.

>
> [1] http://en.wikipedia.org/wiki/De_Morgan%27s_laws
>
>> > (...)
>> > @@ -272,7 +272,7 @@ static int i801_wait_intr(struct i801_pr
>>
>> "i801_wait_intr()" isn't such a good name anymore.  I recommend
>> i801_wait_xfer_status(), or i801_wait_host_idle().
>
> i801_wait_intr() isn't such a bad name IMHO, we are still ideally hoping
> that INTR will be set, as this means the transaction will be
> successful. The name has the advantage of being symmetric with
> i801_wait_byte_done(), so it's clear which one of the status bits we
> are expecting.
>
> "i801_wait_host_idle" would be a good name too, but the actual name
> doesn't really matter anyway, as the next patch merges both functions
> into a single one. I had written this patch before you posted your
> proposed changes, but it can still be applied on top of these (with the
> usual context adjustment.) Unless you really don't like it? If so,
> please tell me why.
>
>> >                 dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
>>
>> "Timed out waiting for transaction to complete!\n"
>
> My next patch already changes this to:
>
>                 dev_err(&priv->pci_dev->dev,
>                         "Timeout waiting for %02x! (%02x)\n", wait_set, status);
>
> My message is more generic because the function is now generic, but the
> information provided is more or less the same. If you think wording can
> be improved, just let me know.

I'm really not sure this function is that necessary.  The code seems
more confusing now with the two waits combined into a single function.
 Keeping them separate also means you don't have to do extra work like
passing "wait_cleared = 0" and making sure not to clear BYTE_DONE in
the BYTE_DONE case.

>
> ---
>  drivers/i2c/busses/i2c-i801.c |   50 ++++++++++++++++-------------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
>
> --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c    2012-07-03 16:49:42.000000000 +0200
> +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03 17:10:37.062356531 +0200
> @@ -254,29 +254,16 @@ static int i801_check_post(struct i801_p
>         return result;
>  }
>
> -/* Wait for BUSY being cleared and either INTR or an error flag being set */
> -static int i801_wait_intr(struct i801_priv *priv)
> -{
> -       int timeout = 0;
> -       int status;
> -
> -       /* We will always wait for a fraction of a second! */
> -       do {
> -               usleep_range(250, 500);
> -               status = inb_p(SMBHSTSTS(priv));
> -       } while (((status & SMBHSTSTS_HOST_BUSY) ||
> -                 !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
> -                (timeout++ < MAX_RETRIES));
> -
> -       if (timeout > MAX_RETRIES) {
> -               dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> -               return -ETIMEDOUT;
> -       }
> -       return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
> -}
> -
> -/* Wait for either BYTE_DONE or an error flag being set */
> -static int i801_wait_byte_done(struct i801_priv *priv)
> +/*
> + * Wait for some status register bits being cleared and others being set.
> + * Typical usage:
> + *  - Wait for BUSY being cleared and either INTR or an error flag being set
> + *    (end of transaction)
> + *  - Wait for either BYTE_DONE or an error flag being set (step of
> + *    byte-by-byte block transaction)
> + * Return the status flags which should be cleared automatically.
> + */
> +static int i801_wait(struct i801_priv *priv, int wait_cleared, int wait_set)
>  {
>         int timeout = 0;
>         int status;
> @@ -285,14 +272,17 @@ static int i801_wait_byte_done(struct i8
>         do {
>                 usleep_range(250, 500);
>                 status = inb_p(SMBHSTSTS(priv));
> -       } while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
> +       } while (((status & wait_cleared) ||
> +                 !(status & (STATUS_ERROR_FLAGS | wait_set))) &&
>                  (timeout++ < MAX_RETRIES));
>
> -       if (timeout > MAX_RETRIES) {
> -               dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
> +       if (unlikely(timeout > MAX_RETRIES)) {
> +               dev_err(&priv->pci_dev->dev,
> +                       "Timeout waiting for %02x! (%02x)\n", wait_set, status);
>                 return -ETIMEDOUT;
>         }
> -       return status & STATUS_ERROR_FLAGS;
> +       status &= ~SMBHSTSTS_BYTE_DONE; /* Must be cleared explicitly */
> +       return status & (STATUS_ERROR_FLAGS | wait_set);
>  }
>
>  static int i801_transaction(struct i801_priv *priv, int xact)
> @@ -308,7 +298,7 @@ static int i801_transaction(struct i801_
>          * SMBSCMD are passed in xact */
>         outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>
> -       status = i801_wait_intr(priv);
> +       status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
>         return i801_check_post(priv, status);
>  }
>
> @@ -387,7 +377,7 @@ static int i801_block_transaction_byte_b
>                         outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>                                SMBHSTCNT(priv));
>
> -               status = i801_wait_byte_done(priv);
> +               status = i801_wait(priv, 0, SMBHSTSTS_BYTE_DONE);
>                 if (status)
>                         goto exit;
>
> @@ -419,7 +409,7 @@ static int i801_block_transaction_byte_b
>                 outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>         }
>
> -       status = i801_wait_intr(priv);
> +       status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
>  exit:
>         return i801_check_post(priv, status);
>  }
>
> --
> Jean Delvare

Re: [PATCH] i2c-i801: Consolidate polling

[Jean Delvare]

On Wed, 4 Jul 2012 16:08:25 +0800, Daniel Kurtz wrote:
> On Wed, Jul 4, 2012 at 3:49 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Wed, 4 Jul 2012 14:12:26 +0800, Daniel Kurtz wrote:
> >> On Tue, Jul 3, 2012 at 9:39 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >> > @@ -272,7 +272,7 @@ static int i801_wait_intr(struct i801_pr
> >>
> >> "i801_wait_intr()" isn't such a good name anymore.  I recommend
> >> i801_wait_xfer_status(), or i801_wait_host_idle().
> >
> > i801_wait_intr() isn't such a bad name IMHO, we are still ideally hoping
> > that INTR will be set, as this means the transaction will be
> > successful. The name has the advantage of being symmetric with
> > i801_wait_byte_done(), so it's clear which one of the status bits we
> > are expecting.
> >
> > "i801_wait_host_idle" would be a good name too, but the actual name
> > doesn't really matter anyway, as the next patch merges both functions
> > into a single one. I had written this patch before you posted your
> > proposed changes, but it can still be applied on top of these (with the
> > usual context adjustment.) Unless you really don't like it? If so,
> > please tell me why.
> >
> >> >                 dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> >>
> >> "Timed out waiting for transaction to complete!\n"
> >
> > My next patch already changes this to:
> >
> >                 dev_err(&priv->pci_dev->dev,
> >                         "Timeout waiting for %02x! (%02x)\n", wait_set, status);
> >
> > My message is more generic because the function is now generic, but the
> > information provided is more or less the same. If you think wording can
> > be improved, just let me know.
> 
> I'm really not sure this function is that necessary.  The code seems
> more confusing now with the two waits combined into a single function.
>  Keeping them separate also means you don't have to do extra work like
> passing "wait_cleared = 0" and making sure not to clear BYTE_DONE in
> the BYTE_DONE case.

You have a point, and this is the reason why I originally wasn't sure
myself if I wanted to do it. But it also allows for further cleanups. I
have a patch moving the call to i801_wait into i801_check_post (which
allows gcc to inline i801_wait) and another one merging i801_wait into
i801_check_post (which saves some intermediate work.) Each step may
not be very appealing nor convincing individually, but if you stack all
three, the diffstat looks like:

 drivers/i2c/busses/i2c-i801.c |   86 ++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 56 deletions(-)

And the binary comparison shows 274 bytes saved on x86_64, which isn't
negligible.

That being said... The last two steps are unlikely to fit well with your
interrupt patches. So I will postpone these cleanup/optimization
patches of mine for the time being. I started working on this because
of the performance regression introduced by one of your patches. Now
that this is solved, I should resume to my original goal which was
to get all your patches reviewed, tested and in linux-next as quickly
as possible. We can always discuss the rest later.

Thanks again for your time, I'll now reorder and cleanup the current
patch series, retest, and then move on to the two interrupt patches.

-- 
Jean Delvare

Re: [PATCH] i2c-i801: Consolidate polling

[Jean Delvare]

On Tue, 3 Jul 2012 10:19:27 +0200, Jean Delvare wrote:
> It may be possible to combine i801_wait_intr() and
> i801_wait_byte_done() into a single function, as they do pretty much
> the same, but I'll only do that if it doesn't hurt readability. My
> first attempt was horrible.

This could look as follows, which also offers opportunities to further
simplify the code by either letting i801_check_post() call i801_wait(),
or even merging both functions together. Not sure yet if we want to do
either.

However this probably doesn't fit well with your suggested changes to
my initial patch, so I'll have to ponder the benefits of each cleanup.
You opinion is welcome in this respect as well.

---
 drivers/i2c/busses/i2c-i801.c |   46 +++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

--- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c	2012-07-03 10:07:42.000000000 +0200
+++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c	2012-07-03 10:31:07.945245984 +0200
@@ -254,29 +254,15 @@ static int i801_check_post(struct i801_p
 	return result;
 }
 
-/* Wait for BUSY being cleared and either INTR or an error flag being set */
-static int i801_wait_intr(struct i801_priv *priv)
-{
-	int timeout = 0;
-	int status;
-
-	/* We will always wait for a fraction of a second! */
-	do {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-	} while (((status & SMBHSTSTS_HOST_BUSY) ||
-		  !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
-		 (timeout++ < MAX_RETRIES));
-
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
-		return -ETIMEDOUT;
-	}
-	return status;
-}
-
-/* Wait for either BYTE_DONE or an error flag being set */
-static int i801_wait_byte_done(struct i801_priv *priv)
+/*
+ * Wait for some status register bits being cleared and others being set.
+ * Typical usage:
+ *  - Wait for BUSY being cleared and either INTR or an error flag being set
+ *    (end of transaction)
+ *  - Wait for either BYTE_DONE or an error flag being set (step of
+ *    byte-by-byte block transaction)
+ */
+static int i801_wait(struct i801_priv *priv, int wait_cleared, int wait_set)
 {
 	int timeout = 0;
 	int status;
@@ -285,11 +271,13 @@ static int i801_wait_byte_done(struct i8
 	do {
 		usleep_range(250, 500);
 		status = inb_p(SMBHSTSTS(priv));
-	} while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
+	} while (((status & wait_cleared) ||
+		  !(status & (STATUS_ERROR_FLAGS | wait_set))) &&
 		 (timeout++ < MAX_RETRIES));
 
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
+	if (unlikely(timeout > MAX_RETRIES)) {
+		dev_dbg(&priv->pci_dev->dev,
+			"Timeout waiting for %02x! (%02x)\n", wait_set, status);
 		return -ETIMEDOUT;
 	}
 	return status;
@@ -308,7 +296,7 @@ static int i801_transaction(struct i801_
 	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
-	status = i801_wait_intr(priv);
+	status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
 	return i801_check_post(priv, status);
 }
 
@@ -387,7 +375,7 @@ static int i801_block_transaction_byte_b
 			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
 			       SMBHSTCNT(priv));
 
-		status = i801_wait_byte_done(priv);
+		status = i801_wait(priv, 0, SMBHSTSTS_BYTE_DONE);
 		result = i801_check_post(priv, status);
 		if (result < 0)
 			return result;
@@ -420,7 +408,7 @@ static int i801_block_transaction_byte_b
 		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 	}
 
-	status = i801_wait_intr(priv);
+	status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
 	return i801_check_post(priv, status);
 }
 


-- 
Jean Delvare