* [PATCH 0/6] Cleanup reads/writes to Line 6 device memory
@ 2015-02-11 5:03 Chris Rorvick
2015-02-11 5:03 ` [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces Chris Rorvick
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
The primary fix here is to throttle the requests for status after a
read or write operation. In some cases I was seeing more than 1000
before the device was ready with a response.
The remainder are smaller improvements related to the read/write logic.
All of this seems pretty straightforward with the possible exception of
changing the returned error from EINVAL to EIO. The EINVAL seems wrong
to me but maybe not?
Regards,
Chris
Chris Rorvick (6):
ALSA: line6: Improve line6_read/write_data() interfaces
ALSA: line6: Add delay before reading status
ALSA: line6: Return error if device not responding
ALSA: line6: Return EIO if read/write not successful
ALSA: line6: Use explicit type for serial number
ALSA: line6: toneport: Use explicit type for firmware version
sound/usb/line6/driver.c | 51 +++++++++++++++++++++++++++++++++-------------
sound/usb/line6/driver.h | 10 ++++-----
sound/usb/line6/pod.c | 4 ++--
sound/usb/line6/toneport.c | 4 ++--
4 files changed, 46 insertions(+), 23 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
@ 2015-02-11 5:03 ` Chris Rorvick
2015-02-11 9:33 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 2/6] ALSA: line6: Add delay before reading status Chris Rorvick
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
Use explicit types to reflect the range of valid values.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
sound/usb/line6/driver.c | 10 +++++-----
sound/usb/line6/driver.h | 8 ++++----
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index 1e58e92..aac1e35 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -299,8 +299,8 @@ static void line6_data_received(struct urb *urb)
/*
Read data from device.
*/
-int line6_read_data(struct usb_line6 *line6, int address, void *data,
- size_t datalen)
+int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
+ u8 datalen)
{
struct usb_device *usbdev = line6->usbdev;
int ret;
@@ -309,7 +309,7 @@ int line6_read_data(struct usb_line6 *line6, int address, void *data,
/* query the serial number: */
ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67,
USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
- (datalen << 8) | 0x21, address,
+ ((u16) datalen << 8) | 0x21, address,
NULL, 0, LINE6_TIMEOUT * HZ);
if (ret < 0) {
@@ -357,8 +357,8 @@ EXPORT_SYMBOL_GPL(line6_read_data);
/*
Write data to device.
*/
-int line6_write_data(struct usb_line6 *line6, int address, void *data,
- size_t datalen)
+int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
+ u16 datalen)
{
struct usb_device *usbdev = line6->usbdev;
int ret;
diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
index 8247a6b..603bdc4 100644
--- a/sound/usb/line6/driver.h
+++ b/sound/usb/line6/driver.h
@@ -147,8 +147,8 @@ struct usb_line6 {
extern char *line6_alloc_sysex_buffer(struct usb_line6 *line6, int code1,
int code2, int size);
-extern int line6_read_data(struct usb_line6 *line6, int address, void *data,
- size_t datalen);
+extern int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
+ u8 datalen);
extern int line6_read_serial_number(struct usb_line6 *line6,
int *serial_number);
extern int line6_send_raw_message_async(struct usb_line6 *line6,
@@ -161,8 +161,8 @@ extern void line6_start_timer(struct timer_list *timer, unsigned int msecs,
void (*function)(unsigned long),
unsigned long data);
extern int line6_version_request_async(struct usb_line6 *line6);
-extern int line6_write_data(struct usb_line6 *line6, int address, void *data,
- size_t datalen);
+extern int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
+ u16 datalen);
int line6_probe(struct usb_interface *interface,
const struct usb_device_id *id,
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/6] ALSA: line6: Add delay before reading status
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
2015-02-11 5:03 ` [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces Chris Rorvick
@ 2015-02-11 5:03 ` Chris Rorvick
2015-02-11 9:34 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 3/6] ALSA: line6: Return error if device not responding Chris Rorvick
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
The device indicates the result of a read/write operation by making the
status available on a subsequent request from the driver. This is not
ready immediately, though, so the driver is currently slamming the
device with hundreds of pointless requests before getting the expected
response. Add a two millisecond delay before each attempt. This is
approximately the behavior observed with version 4.2.7.1 of the Windows
driver.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
sound/usb/line6/driver.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index aac1e35..222c14f 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -296,6 +296,8 @@ static void line6_data_received(struct urb *urb)
line6_start_listen(line6);
}
+#define LINE6_READ_WRITE_STATUS_DELAY 2 /* milliseconds */
+
/*
Read data from device.
*/
@@ -319,6 +321,8 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
/* Wait for data length. We'll get 0xff until length arrives. */
do {
+ mdelay(LINE6_READ_WRITE_STATUS_DELAY);
+
ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67,
USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
@@ -376,6 +380,8 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
}
do {
+ mdelay(LINE6_READ_WRITE_STATUS_DELAY);
+
ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
0x67,
USB_TYPE_VENDOR | USB_RECIP_DEVICE |
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/6] ALSA: line6: Return error if device not responding
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
2015-02-11 5:03 ` [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces Chris Rorvick
2015-02-11 5:03 ` [PATCH 2/6] ALSA: line6: Add delay before reading status Chris Rorvick
@ 2015-02-11 5:03 ` Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 4/6] ALSA: line6: Return EIO if read/write not successful Chris Rorvick
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
Put an upper bound on how long we will wait for the device to respond to
a read/write request (i.e., 100 milliseconds) and return an error if
this is reached.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
sound/usb/line6/driver.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index 222c14f..2a33f3e 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -297,6 +297,7 @@ static void line6_data_received(struct urb *urb)
}
#define LINE6_READ_WRITE_STATUS_DELAY 2 /* milliseconds */
+#define LINE6_READ_WRITE_MAX_RETRIES 50
/*
Read data from device.
@@ -307,6 +308,7 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
struct usb_device *usbdev = line6->usbdev;
int ret;
unsigned char len;
+ unsigned count;
/* query the serial number: */
ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67,
@@ -320,7 +322,7 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
}
/* Wait for data length. We'll get 0xff until length arrives. */
- do {
+ for (count = 0; count < LINE6_READ_WRITE_MAX_RETRIES; count++) {
mdelay(LINE6_READ_WRITE_STATUS_DELAY);
ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67,
@@ -333,9 +335,16 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
"receive length failed (error %d)\n", ret);
return ret;
}
- } while (len == 0xff);
- if (len != datalen) {
+ if (len != 0xff)
+ break;
+ }
+
+ if (len == 0xff) {
+ dev_err(line6->ifcdev, "read failed after %d retries\n",
+ count);
+ return -EIO;
+ } else if (len != datalen) {
/* should be equal or something went wrong */
dev_err(line6->ifcdev,
"length mismatch (expected %d, got %d)\n",
@@ -367,6 +376,7 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
struct usb_device *usbdev = line6->usbdev;
int ret;
unsigned char status;
+ int count;
ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67,
USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
@@ -379,7 +389,7 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
return ret;
}
- do {
+ for (count = 0; count < LINE6_READ_WRITE_MAX_RETRIES; count++) {
mdelay(LINE6_READ_WRITE_STATUS_DELAY);
ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
@@ -394,9 +404,16 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
"receiving status failed (error %d)\n", ret);
return ret;
}
- } while (status == 0xff);
- if (status != 0) {
+ if (status != 0xff)
+ break;
+ }
+
+ if (status == 0xff) {
+ dev_err(line6->ifcdev, "write failed after %d retries\n",
+ count);
+ return -EIO;
+ } else if (status != 0) {
dev_err(line6->ifcdev, "write failed (error %d)\n", ret);
return -EINVAL;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/6] ALSA: line6: Return EIO if read/write not successful
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
` (2 preceding siblings ...)
2015-02-11 5:03 ` [PATCH 3/6] ALSA: line6: Return error if device not responding Chris Rorvick
@ 2015-02-11 5:03 ` Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 5/6] ALSA: line6: Use explicit type for serial number Chris Rorvick
2015-02-11 5:03 ` [PATCH 6/6] ALSA: line6: toneport: Use explicit type for firmware version Chris Rorvick
5 siblings, 1 reply; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
sound/usb/line6/driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index 2a33f3e..f8e2eb0 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -349,7 +349,7 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
dev_err(line6->ifcdev,
"length mismatch (expected %d, got %d)\n",
(int)datalen, (int)len);
- return -EINVAL;
+ return -EIO;
}
/* receive the result: */
@@ -415,7 +415,7 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
return -EIO;
} else if (status != 0) {
dev_err(line6->ifcdev, "write failed (error %d)\n", ret);
- return -EINVAL;
+ return -EIO;
}
return 0;
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/6] ALSA: line6: Use explicit type for serial number
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
` (3 preceding siblings ...)
2015-02-11 5:03 ` [PATCH 4/6] ALSA: line6: Return EIO if read/write not successful Chris Rorvick
@ 2015-02-11 5:03 ` Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 6/6] ALSA: line6: toneport: Use explicit type for firmware version Chris Rorvick
5 siblings, 1 reply; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
The serial number (aka ESN) is a 32-bit value.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
sound/usb/line6/driver.c | 2 +-
sound/usb/line6/driver.h | 2 +-
sound/usb/line6/pod.c | 4 ++--
sound/usb/line6/toneport.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index f8e2eb0..d31ceb8 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -426,7 +426,7 @@ EXPORT_SYMBOL_GPL(line6_write_data);
Read Line 6 device serial number.
(POD, TonePort, GuitarPort)
*/
-int line6_read_serial_number(struct usb_line6 *line6, int *serial_number)
+int line6_read_serial_number(struct usb_line6 *line6, u32 *serial_number)
{
return line6_read_data(line6, 0x80d0, serial_number,
sizeof(*serial_number));
diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
index 603bdc4..b281bff 100644
--- a/sound/usb/line6/driver.h
+++ b/sound/usb/line6/driver.h
@@ -150,7 +150,7 @@ extern char *line6_alloc_sysex_buffer(struct usb_line6 *line6, int code1,
extern int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
u8 datalen);
extern int line6_read_serial_number(struct usb_line6 *line6,
- int *serial_number);
+ u32 *serial_number);
extern int line6_send_raw_message_async(struct usb_line6 *line6,
const char *buffer, int size);
extern int line6_send_sysex_message(struct usb_line6 *line6,
diff --git a/sound/usb/line6/pod.c b/sound/usb/line6/pod.c
index c4246ad..425d22d 100644
--- a/sound/usb/line6/pod.c
+++ b/sound/usb/line6/pod.c
@@ -73,7 +73,7 @@ struct usb_line6_pod {
int startup_progress;
/* Serial number of device */
- int serial_number;
+ u32 serial_number;
/* Firmware version (x 100) */
int firmware_version;
@@ -247,7 +247,7 @@ static ssize_t serial_number_show(struct device *dev,
struct usb_interface *interface = to_usb_interface(dev);
struct usb_line6_pod *pod = usb_get_intfdata(interface);
- return sprintf(buf, "%d\n", pod->serial_number);
+ return sprintf(buf, "%u\n", pod->serial_number);
}
/*
diff --git a/sound/usb/line6/toneport.c b/sound/usb/line6/toneport.c
index 1a0a485..ddf7368 100644
--- a/sound/usb/line6/toneport.c
+++ b/sound/usb/line6/toneport.c
@@ -49,7 +49,7 @@ struct usb_line6_toneport {
int source;
/* Serial number of device */
- int serial_number;
+ u32 serial_number;
/* Firmware version (x 100) */
int firmware_version;
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/6] ALSA: line6: toneport: Use explicit type for firmware version
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
` (4 preceding siblings ...)
2015-02-11 5:03 ` [PATCH 5/6] ALSA: line6: Use explicit type for serial number Chris Rorvick
@ 2015-02-11 5:03 ` Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
5 siblings, 1 reply; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 5:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Chris Rorvick, alsa-devel, linux-kernel, Stefan Hajnoczi
The firmware version is a single byte so have the variable type agree.
Since the address to this member is passed to the read function, using
an int is not even portable.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
sound/usb/line6/toneport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/usb/line6/toneport.c b/sound/usb/line6/toneport.c
index ddf7368..6d4c50c 100644
--- a/sound/usb/line6/toneport.c
+++ b/sound/usb/line6/toneport.c
@@ -52,7 +52,7 @@ struct usb_line6_toneport {
u32 serial_number;
/* Firmware version (x 100) */
- int firmware_version;
+ u8 firmware_version;
/* Timer for delayed PCM startup */
struct timer_list timer;
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces
2015-02-11 5:03 ` [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces Chris Rorvick
@ 2015-02-11 9:33 ` Takashi Iwai
2015-02-11 11:58 ` Chris Rorvick
0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2015-02-11 9:33 UTC (permalink / raw)
To: Chris Rorvick; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
At Tue, 10 Feb 2015 23:03:12 -0600,
Chris Rorvick wrote:
>
> Use explicit types to reflect the range of valid values.
Well, this isn't necessarily to be changed at these places. If we
want to be safer, there should be proper range checks instead.
Also, readers may wonder when they see the different types between
read and write without explanations.
Though, I agree that size_t is superfluous, especially for 64bit,
too.
thanks,
Takashi
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
> ---
> sound/usb/line6/driver.c | 10 +++++-----
> sound/usb/line6/driver.h | 8 ++++----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> index 1e58e92..aac1e35 100644
> --- a/sound/usb/line6/driver.c
> +++ b/sound/usb/line6/driver.c
> @@ -299,8 +299,8 @@ static void line6_data_received(struct urb *urb)
> /*
> Read data from device.
> */
> -int line6_read_data(struct usb_line6 *line6, int address, void *data,
> - size_t datalen)
> +int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> + u8 datalen)
> {
> struct usb_device *usbdev = line6->usbdev;
> int ret;
> @@ -309,7 +309,7 @@ int line6_read_data(struct usb_line6 *line6, int address, void *data,
> /* query the serial number: */
> ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> - (datalen << 8) | 0x21, address,
> + ((u16) datalen << 8) | 0x21, address,
> NULL, 0, LINE6_TIMEOUT * HZ);
>
> if (ret < 0) {
> @@ -357,8 +357,8 @@ EXPORT_SYMBOL_GPL(line6_read_data);
> /*
> Write data to device.
> */
> -int line6_write_data(struct usb_line6 *line6, int address, void *data,
> - size_t datalen)
> +int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> + u16 datalen)
> {
> struct usb_device *usbdev = line6->usbdev;
> int ret;
> diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
> index 8247a6b..603bdc4 100644
> --- a/sound/usb/line6/driver.h
> +++ b/sound/usb/line6/driver.h
> @@ -147,8 +147,8 @@ struct usb_line6 {
>
> extern char *line6_alloc_sysex_buffer(struct usb_line6 *line6, int code1,
> int code2, int size);
> -extern int line6_read_data(struct usb_line6 *line6, int address, void *data,
> - size_t datalen);
> +extern int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> + u8 datalen);
> extern int line6_read_serial_number(struct usb_line6 *line6,
> int *serial_number);
> extern int line6_send_raw_message_async(struct usb_line6 *line6,
> @@ -161,8 +161,8 @@ extern void line6_start_timer(struct timer_list *timer, unsigned int msecs,
> void (*function)(unsigned long),
> unsigned long data);
> extern int line6_version_request_async(struct usb_line6 *line6);
> -extern int line6_write_data(struct usb_line6 *line6, int address, void *data,
> - size_t datalen);
> +extern int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> + u16 datalen);
>
> int line6_probe(struct usb_interface *interface,
> const struct usb_device_id *id,
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/6] ALSA: line6: Add delay before reading status
2015-02-11 5:03 ` [PATCH 2/6] ALSA: line6: Add delay before reading status Chris Rorvick
@ 2015-02-11 9:34 ` Takashi Iwai
0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2015-02-11 9:34 UTC (permalink / raw)
To: Chris Rorvick; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
At Tue, 10 Feb 2015 23:03:13 -0600,
Chris Rorvick wrote:
>
> The device indicates the result of a read/write operation by making the
> status available on a subsequent request from the driver. This is not
> ready immediately, though, so the driver is currently slamming the
> device with hundreds of pointless requests before getting the expected
> response. Add a two millisecond delay before each attempt. This is
> approximately the behavior observed with version 4.2.7.1 of the Windows
> driver.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
Applied, thanks.
Takashi
> ---
> sound/usb/line6/driver.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> index aac1e35..222c14f 100644
> --- a/sound/usb/line6/driver.c
> +++ b/sound/usb/line6/driver.c
> @@ -296,6 +296,8 @@ static void line6_data_received(struct urb *urb)
> line6_start_listen(line6);
> }
>
> +#define LINE6_READ_WRITE_STATUS_DELAY 2 /* milliseconds */
> +
> /*
> Read data from device.
> */
> @@ -319,6 +321,8 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
>
> /* Wait for data length. We'll get 0xff until length arrives. */
> do {
> + mdelay(LINE6_READ_WRITE_STATUS_DELAY);
> +
> ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE |
> USB_DIR_IN,
> @@ -376,6 +380,8 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> }
>
> do {
> + mdelay(LINE6_READ_WRITE_STATUS_DELAY);
> +
> ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> 0x67,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE |
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] ALSA: line6: Return error if device not responding
2015-02-11 5:03 ` [PATCH 3/6] ALSA: line6: Return error if device not responding Chris Rorvick
@ 2015-02-11 9:43 ` Takashi Iwai
0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2015-02-11 9:43 UTC (permalink / raw)
To: Chris Rorvick; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
At Tue, 10 Feb 2015 23:03:14 -0600,
Chris Rorvick wrote:
>
> Put an upper bound on how long we will wait for the device to respond to
> a read/write request (i.e., 100 milliseconds) and return an error if
> this is reached.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
Applied, thanks.
Takashi
> ---
> sound/usb/line6/driver.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> index 222c14f..2a33f3e 100644
> --- a/sound/usb/line6/driver.c
> +++ b/sound/usb/line6/driver.c
> @@ -297,6 +297,7 @@ static void line6_data_received(struct urb *urb)
> }
>
> #define LINE6_READ_WRITE_STATUS_DELAY 2 /* milliseconds */
> +#define LINE6_READ_WRITE_MAX_RETRIES 50
>
> /*
> Read data from device.
> @@ -307,6 +308,7 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> struct usb_device *usbdev = line6->usbdev;
> int ret;
> unsigned char len;
> + unsigned count;
>
> /* query the serial number: */
> ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67,
> @@ -320,7 +322,7 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> }
>
> /* Wait for data length. We'll get 0xff until length arrives. */
> - do {
> + for (count = 0; count < LINE6_READ_WRITE_MAX_RETRIES; count++) {
> mdelay(LINE6_READ_WRITE_STATUS_DELAY);
>
> ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), 0x67,
> @@ -333,9 +335,16 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> "receive length failed (error %d)\n", ret);
> return ret;
> }
> - } while (len == 0xff);
>
> - if (len != datalen) {
> + if (len != 0xff)
> + break;
> + }
> +
> + if (len == 0xff) {
> + dev_err(line6->ifcdev, "read failed after %d retries\n",
> + count);
> + return -EIO;
> + } else if (len != datalen) {
> /* should be equal or something went wrong */
> dev_err(line6->ifcdev,
> "length mismatch (expected %d, got %d)\n",
> @@ -367,6 +376,7 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> struct usb_device *usbdev = line6->usbdev;
> int ret;
> unsigned char status;
> + int count;
>
> ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x67,
> USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> @@ -379,7 +389,7 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> return ret;
> }
>
> - do {
> + for (count = 0; count < LINE6_READ_WRITE_MAX_RETRIES; count++) {
> mdelay(LINE6_READ_WRITE_STATUS_DELAY);
>
> ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> @@ -394,9 +404,16 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> "receiving status failed (error %d)\n", ret);
> return ret;
> }
> - } while (status == 0xff);
>
> - if (status != 0) {
> + if (status != 0xff)
> + break;
> + }
> +
> + if (status == 0xff) {
> + dev_err(line6->ifcdev, "write failed after %d retries\n",
> + count);
> + return -EIO;
> + } else if (status != 0) {
> dev_err(line6->ifcdev, "write failed (error %d)\n", ret);
> return -EINVAL;
> }
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/6] ALSA: line6: Return EIO if read/write not successful
2015-02-11 5:03 ` [PATCH 4/6] ALSA: line6: Return EIO if read/write not successful Chris Rorvick
@ 2015-02-11 9:43 ` Takashi Iwai
0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2015-02-11 9:43 UTC (permalink / raw)
To: Chris Rorvick; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
At Tue, 10 Feb 2015 23:03:15 -0600,
Chris Rorvick wrote:
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
Applied, thanks.
Takashi
> ---
> sound/usb/line6/driver.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> index 2a33f3e..f8e2eb0 100644
> --- a/sound/usb/line6/driver.c
> +++ b/sound/usb/line6/driver.c
> @@ -349,7 +349,7 @@ int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> dev_err(line6->ifcdev,
> "length mismatch (expected %d, got %d)\n",
> (int)datalen, (int)len);
> - return -EINVAL;
> + return -EIO;
> }
>
> /* receive the result: */
> @@ -415,7 +415,7 @@ int line6_write_data(struct usb_line6 *line6, u16 address, void *data,
> return -EIO;
> } else if (status != 0) {
> dev_err(line6->ifcdev, "write failed (error %d)\n", ret);
> - return -EINVAL;
> + return -EIO;
> }
>
> return 0;
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] ALSA: line6: Use explicit type for serial number
2015-02-11 5:03 ` [PATCH 5/6] ALSA: line6: Use explicit type for serial number Chris Rorvick
@ 2015-02-11 9:43 ` Takashi Iwai
0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2015-02-11 9:43 UTC (permalink / raw)
To: Chris Rorvick; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
At Tue, 10 Feb 2015 23:03:16 -0600,
Chris Rorvick wrote:
>
> The serial number (aka ESN) is a 32-bit value.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
Applied, thanks.
Takashi
> ---
> sound/usb/line6/driver.c | 2 +-
> sound/usb/line6/driver.h | 2 +-
> sound/usb/line6/pod.c | 4 ++--
> sound/usb/line6/toneport.c | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> index f8e2eb0..d31ceb8 100644
> --- a/sound/usb/line6/driver.c
> +++ b/sound/usb/line6/driver.c
> @@ -426,7 +426,7 @@ EXPORT_SYMBOL_GPL(line6_write_data);
> Read Line 6 device serial number.
> (POD, TonePort, GuitarPort)
> */
> -int line6_read_serial_number(struct usb_line6 *line6, int *serial_number)
> +int line6_read_serial_number(struct usb_line6 *line6, u32 *serial_number)
> {
> return line6_read_data(line6, 0x80d0, serial_number,
> sizeof(*serial_number));
> diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
> index 603bdc4..b281bff 100644
> --- a/sound/usb/line6/driver.h
> +++ b/sound/usb/line6/driver.h
> @@ -150,7 +150,7 @@ extern char *line6_alloc_sysex_buffer(struct usb_line6 *line6, int code1,
> extern int line6_read_data(struct usb_line6 *line6, u16 address, void *data,
> u8 datalen);
> extern int line6_read_serial_number(struct usb_line6 *line6,
> - int *serial_number);
> + u32 *serial_number);
> extern int line6_send_raw_message_async(struct usb_line6 *line6,
> const char *buffer, int size);
> extern int line6_send_sysex_message(struct usb_line6 *line6,
> diff --git a/sound/usb/line6/pod.c b/sound/usb/line6/pod.c
> index c4246ad..425d22d 100644
> --- a/sound/usb/line6/pod.c
> +++ b/sound/usb/line6/pod.c
> @@ -73,7 +73,7 @@ struct usb_line6_pod {
> int startup_progress;
>
> /* Serial number of device */
> - int serial_number;
> + u32 serial_number;
>
> /* Firmware version (x 100) */
> int firmware_version;
> @@ -247,7 +247,7 @@ static ssize_t serial_number_show(struct device *dev,
> struct usb_interface *interface = to_usb_interface(dev);
> struct usb_line6_pod *pod = usb_get_intfdata(interface);
>
> - return sprintf(buf, "%d\n", pod->serial_number);
> + return sprintf(buf, "%u\n", pod->serial_number);
> }
>
> /*
> diff --git a/sound/usb/line6/toneport.c b/sound/usb/line6/toneport.c
> index 1a0a485..ddf7368 100644
> --- a/sound/usb/line6/toneport.c
> +++ b/sound/usb/line6/toneport.c
> @@ -49,7 +49,7 @@ struct usb_line6_toneport {
> int source;
>
> /* Serial number of device */
> - int serial_number;
> + u32 serial_number;
>
> /* Firmware version (x 100) */
> int firmware_version;
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/6] ALSA: line6: toneport: Use explicit type for firmware version
2015-02-11 5:03 ` [PATCH 6/6] ALSA: line6: toneport: Use explicit type for firmware version Chris Rorvick
@ 2015-02-11 9:43 ` Takashi Iwai
0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2015-02-11 9:43 UTC (permalink / raw)
To: Chris Rorvick; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
At Tue, 10 Feb 2015 23:03:17 -0600,
Chris Rorvick wrote:
>
> The firmware version is a single byte so have the variable type agree.
> Since the address to this member is passed to the read function, using
> an int is not even portable.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
Applied, thanks.
Takashi
> ---
> sound/usb/line6/toneport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/usb/line6/toneport.c b/sound/usb/line6/toneport.c
> index ddf7368..6d4c50c 100644
> --- a/sound/usb/line6/toneport.c
> +++ b/sound/usb/line6/toneport.c
> @@ -52,7 +52,7 @@ struct usb_line6_toneport {
> u32 serial_number;
>
> /* Firmware version (x 100) */
> - int firmware_version;
> + u8 firmware_version;
>
> /* Timer for delayed PCM startup */
> struct timer_list timer;
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces
2015-02-11 9:33 ` Takashi Iwai
@ 2015-02-11 11:58 ` Chris Rorvick
0 siblings, 0 replies; 14+ messages in thread
From: Chris Rorvick @ 2015-02-11 11:58 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, linux-kernel, Stefan Hajnoczi
On Wed, Feb 11, 2015 at 3:33 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Tue, 10 Feb 2015 23:03:12 -0600,
> Chris Rorvick wrote:
>>
>> Use explicit types to reflect the range of valid values.
>
> Well, this isn't necessarily to be changed at these places. If we
> want to be safer, there should be proper range checks instead.
> Also, readers may wonder when they see the different types between
> read and write without explanations.
Thanks, I'll send a v2 shortly.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-02-11 11:58 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-11 5:03 [PATCH 0/6] Cleanup reads/writes to Line 6 device memory Chris Rorvick
2015-02-11 5:03 ` [PATCH 1/6] ALSA: line6: Improve line6_read/write_data() interfaces Chris Rorvick
2015-02-11 9:33 ` Takashi Iwai
2015-02-11 11:58 ` Chris Rorvick
2015-02-11 5:03 ` [PATCH 2/6] ALSA: line6: Add delay before reading status Chris Rorvick
2015-02-11 9:34 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 3/6] ALSA: line6: Return error if device not responding Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 4/6] ALSA: line6: Return EIO if read/write not successful Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 5/6] ALSA: line6: Use explicit type for serial number Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
2015-02-11 5:03 ` [PATCH 6/6] ALSA: line6: toneport: Use explicit type for firmware version Chris Rorvick
2015-02-11 9:43 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).