[PATCH v4 RFC 1/2] [media] em28xx: retry I2C write ops if failed by timeout

[PATCH v4 RFC 1/2] [media] em28xx: retry I2C write ops if failed by timeout

[Mauro Carvalho Chehab]

At least on HVR-950, sometimes an I2C operation fails.

This seems to be more frequent when the device is connected
into an USB 3.0 port.

Instead of report an error, try to repeat it, for up to
20 ms. That makes the code more reliable.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 6cd3d909bb3a..35d6808aa9ff 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -189,6 +189,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	 * Zero length reads always succeed, even if no device is connected
 	 */
 
+retry:
 	/* Write to i2c device */
 	ret = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
 	if (ret != len) {
@@ -208,26 +209,24 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 		ret = dev->em28xx_read_reg(dev, 0x05);
 		if (ret == 0) /* success */
 			return len;
-		if (ret == 0x10) {
-			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
-				    addr);
-			return -EREMOTEIO;
-		}
+		if (ret == 0x10)
+			goto retry;
 		if (ret < 0) {
 			em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
 				    ret);
 			return ret;
 		}
 		msleep(5);
-		/*
-		 * NOTE: do we really have to wait for success ?
-		 * Never seen anything else than 0x00 or 0x10
-		 * (even with high payload) ...
-		 */
 	}
 
-	if (i2c_debug)
-		em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
+	if (ret == 0x10) {
+		if (i2c_debug)
+			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
+				    addr);
+	} else {
+		em28xx_warn("write to i2c device at 0x%x timed out (ret=0x%02x)\n",
+			    addr, ret);
+	}
 	return -EREMOTEIO;
 }
 
-- 
1.8.3.1

[PATCH v4 RFC 2/2] [media] em28xx: USB: adjust for changed 3.8 USB API

[Mauro Carvalho Chehab]

The recent changes in the USB API ("implement new semantics for
URB_ISO_ASAP") made the former meaning of the URB_ISO_ASAP flag the
default, and changed this flag to mean that URBs can be delayed.
This is not the behaviour wanted by any of the audio drivers because
it leads to discontinuous playback with very small period sizes.
Therefore, our URBs need to be submitted without this flag.

This patch implements the same fix as found at snd-usb-audio driver
(commit c75c5ab575af7db707689cdbb5a5c458e9a034bb)

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/em28xx/em28xx-audio.c | 2 +-
 drivers/media/usb/em28xx/em28xx-core.c  | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 30ee389a07f0..084ccb8bbab6 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -705,7 +705,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 		urb->dev = dev->udev;
 		urb->context = dev;
 		urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
-		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
+		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
 		urb->transfer_buffer = dev->adev.transfer_buffer[i];
 		urb->interval = 1;
 		urb->complete = em28xx_audio_isocirq;
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 97cc83c3c287..3fae119fc064 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -953,8 +953,7 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
 			usb_fill_int_urb(urb, dev->udev, pipe,
 					 usb_bufs->transfer_buffer[i], sb_size,
 					 em28xx_irq_callback, dev, 1);
-			urb->transfer_flags = URB_ISO_ASAP |
-					      URB_NO_TRANSFER_DMA_MAP;
+			urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
 			k = 0;
 			for (j = 0; j < usb_bufs->num_packets; j++) {
 				urb->iso_frame_desc[j].offset = k;
-- 
1.8.3.1

Re: [PATCH v4 RFC 1/2] [media] em28xx: retry I2C write ops if failed by timeout

[Markus Rechberger]

Did you trace the i2c messages on the bus? This seems like papering
the actual bug.

USB 3.0 is a disaster with Linux, maybe your hardware or your
controller driver is not okay?
There are other bugreports out there which are USB 3.0 related, some
of our customers reported that since 3.6.0 is okay while 3.7.10 give
them a complete system lock up also with the driver in question here.


On Sat, Jan 4, 2014 at 12:09 PM, Mauro Carvalho Chehab
<m.chehab@samsung.com> wrote:
> At least on HVR-950, sometimes an I2C operation fails.
>
> This seems to be more frequent when the device is connected
> into an USB 3.0 port.
>
> Instead of report an error, try to repeat it, for up to
> 20 ms. That makes the code more reliable.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 6cd3d909bb3a..35d6808aa9ff 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -189,6 +189,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>          * Zero length reads always succeed, even if no device is connected
>          */
>
> +retry:
>         /* Write to i2c device */
>         ret = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
>         if (ret != len) {
> @@ -208,26 +209,24 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>                 ret = dev->em28xx_read_reg(dev, 0x05);
>                 if (ret == 0) /* success */
>                         return len;
> -               if (ret == 0x10) {
> -                       em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> -                                   addr);
> -                       return -EREMOTEIO;
> -               }
> +               if (ret == 0x10)
> +                       goto retry;
>                 if (ret < 0) {
>                         em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
>                                     ret);
>                         return ret;
>                 }
>                 msleep(5);
> -               /*
> -                * NOTE: do we really have to wait for success ?
> -                * Never seen anything else than 0x00 or 0x10
> -                * (even with high payload) ...
> -                */
>         }
>
> -       if (i2c_debug)
> -               em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
> +       if (ret == 0x10) {
> +               if (i2c_debug)
> +                       em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> +                                   addr);
> +       } else {
> +               em28xx_warn("write to i2c device at 0x%x timed out (ret=0x%02x)\n",
> +                           addr, ret);
> +       }
>         return -EREMOTEIO;
>  }
>
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 RFC 1/2] [media] em28xx: retry I2C write ops if failed by timeout

[Frank Schäfer]

Am 04.01.2014 18:58, schrieb Markus Rechberger:
> Did you trace the i2c messages on the bus? This seems like papering
> the actual bug.
The USB traces are clear:
i2c status 0x10 is treated as _final_ i2c transfer status and the driver
does _not_ retry.

Yes, it's papering over the actual bug.
We need to stop this crap and fix the actual bugs instead.

And it's wrong i2c adapter driver design.
Whether or retrying makes sense (and how often, with which delay)
depends on the context the i2c operation is used in.
That's why this decision is up to the i2c client driver.

Regards,
Frank

> USB 3.0 is a disaster with Linux, maybe your hardware or your
> controller driver is not okay?
> There are other bugreports out there which are USB 3.0 related, some
> of our customers reported that since 3.6.0 is okay while 3.7.10 give
> them a complete system lock up also with the driver in question here.
>
>
> On Sat, Jan 4, 2014 at 12:09 PM, Mauro Carvalho Chehab
> <m.chehab@samsung.com> wrote:
>> At least on HVR-950, sometimes an I2C operation fails.
>>
>> This seems to be more frequent when the device is connected
>> into an USB 3.0 port.
>>
>> Instead of report an error, try to repeat it, for up to
>> 20 ms. That makes the code more reliable.
>>
>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-i2c.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>> index 6cd3d909bb3a..35d6808aa9ff 100644
>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>> @@ -189,6 +189,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>          * Zero length reads always succeed, even if no device is connected
>>          */
>>
>> +retry:
>>         /* Write to i2c device */
>>         ret = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
>>         if (ret != len) {
>> @@ -208,26 +209,24 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>                 ret = dev->em28xx_read_reg(dev, 0x05);
>>                 if (ret == 0) /* success */
>>                         return len;
>> -               if (ret == 0x10) {
>> -                       em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
>> -                                   addr);
>> -                       return -EREMOTEIO;
>> -               }
>> +               if (ret == 0x10)
>> +                       goto retry;
>>                 if (ret < 0) {
>>                         em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
>>                                     ret);
>>                         return ret;
>>                 }
>>                 msleep(5);
>> -               /*
>> -                * NOTE: do we really have to wait for success ?
>> -                * Never seen anything else than 0x00 or 0x10
>> -                * (even with high payload) ...
>> -                */
>>         }
>>
>> -       if (i2c_debug)
>> -               em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
>> +       if (ret == 0x10) {
>> +               if (i2c_debug)
>> +                       em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
>> +                                   addr);
>> +       } else {
>> +               em28xx_warn("write to i2c device at 0x%x timed out (ret=0x%02x)\n",
>> +                           addr, ret);
>> +       }
>>         return -EREMOTEIO;
>>  }
>>
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html