RE: [V2] usb: gadget: storage: Remove warning message
> > You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT. And when > that's done, it will no longer need to set common->new_fsg to NULL either. > Yes, we should change fsg_unbind() as well. > This is sort of a band-aid approach. The real problem is that the original > design > of the driver never intended for there to be more than one outstanding > CONFIG_CHANGE event, so naturally there are scenarios where it doesn't work > right when that assumption is violated. This patch addresses one of those > scenarios, but there may be others remaining. > > Ultimately the problem boils down to synchronization with the composite core. Thanks for your reviewing, I agree with your point. > Some of the callbacks made by the core take time to fully process, so what > should happen if the core makes a second callback before the first one is > completely processed? > > On the other hand, I can't think of anything better at the moment. > > Alan Stern Actually, composite core have tried to deal with this case by using a spinlock. (please refer to the cdev->lock) The problem here is that the callback of fsg will delegate the request to fsg_main_thread. That is, the handling of fsg request will run in parallel with composite core. In my opinion, this issue can be avoided if we handle these request directly in the callback of fsg instead of handing it over to fsg_main_thread. But this might take too much time in the interrupt context, which is not good for system performance. Please correct me if there is anything wrong. Thanks EJ --nvpublic
[PATCH v2] usb: dwc2: Use generic PHY width in params setup
Setting params.phy_utmi_width in dwc2_lowlevel_hw_init() is pointless since
it's value will be overwritten by dwc2_init_params().
This change make sure to take in account the generic PHY width information
during paraminitialisation, done in dwc2_set_param_phy_utmi_width().
By doing so, the phy_utmi_width params can still be overrided by
devicetree specific params and will also be checked against hardware
capabilities.
Fixes: 707d80f0a3c5 ("usb: dwc2: gadget: Replace phyif with phy_utmi_width")
Tested-by: Marek Szyprowski
Signed-off-by: Jules Maselbas
---
v2: Fix typo in commit message. Add Fixes and Tested-by tags.
---
drivers/usb/dwc2/params.c | 9 +
drivers/usb/dwc2/platform.c | 9 -
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 6900eea57526..5949262ff669 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -253,6 +253,15 @@ static void dwc2_set_param_phy_utmi_width(struct
dwc2_hsotg *hsotg)
val = (hsotg->hw_params.utmi_phy_data_width ==
GHWCFG4_UTMI_PHY_DATA_WIDTH_8) ? 8 : 16;
+ if (hsotg->phy) {
+ /*
+* If using the generic PHY framework, check if the PHY bus
+* width is 8-bit and set the phyif appropriately.
+*/
+ if (phy_get_bus_width(hsotg->phy) == 8)
+ val = 8;
+ }
+
hsotg->params.phy_utmi_width = val;
}
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index d10a7f8daec3..e98d7812da2d 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -271,15 +271,6 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
hsotg->plat = dev_get_platdata(hsotg->dev);
- if (hsotg->phy) {
- /*
-* If using the generic PHY framework, check if the PHY bus
-* width is 8-bit and set the phyif appropriately.
-*/
- if (phy_get_bus_width(hsotg->phy) == 8)
- hsotg->params.phy_utmi_width = 8;
- }
-
/* Clock */
hsotg->clk = devm_clk_get_optional(hsotg->dev, "otg");
if (IS_ERR(hsotg->clk)) {
--
2.21.0.196.g041f5ea
[PATCH 3/4] rio500: simplify locking
Admitting that there can be only one device allows us
to drop any pretense about locking one device or
a table of devices.
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/rio500.c | 43 ---
1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index a1832c195b5f..a0574f54738f 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -51,7 +51,6 @@ struct rio_usb_data {
char *obuf, *ibuf; /* transfer buffers */
char bulk_in_ep, bulk_out_ep; /* Endpoint assignments */
wait_queue_head_t wait_q; /* for timeouts */
- struct mutex lock; /* general race avoidance */
};
static DEFINE_MUTEX(rio500_mutex);
@@ -63,10 +62,8 @@ static int open_rio(struct inode *inode, struct file *file)
/* against disconnect() */
mutex_lock(&rio500_mutex);
- mutex_lock(&(rio->lock));
if (rio->isopen || !rio->present) {
- mutex_unlock(&(rio->lock));
mutex_unlock(&rio500_mutex);
return -EBUSY;
}
@@ -74,7 +71,6 @@ static int open_rio(struct inode *inode, struct file *file)
init_waitqueue_head(&rio->wait_q);
- mutex_unlock(&(rio->lock));
dev_info(&rio->rio_dev->dev, "Rio opened.\n");
mutex_unlock(&rio500_mutex);
@@ -88,7 +84,6 @@ static int close_rio(struct inode *inode, struct file *file)
/* against disconnect() */
mutex_lock(&rio500_mutex);
- mutex_lock(&(rio->lock));
rio->isopen = 0;
if (!rio->present) {
@@ -100,7 +95,6 @@ static int close_rio(struct inode *inode, struct file *file)
} else {
dev_info(&rio->rio_dev->dev, "Rio closed.\n");
}
- mutex_unlock(&(rio->lock));
mutex_unlock(&rio500_mutex);
return 0;
}
@@ -115,7 +109,7 @@ static long ioctl_rio(struct file *file, unsigned int cmd,
unsigned long arg)
int retries;
int retval=0;
- mutex_lock(&(rio->lock));
+ mutex_lock(&rio500_mutex);
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
retval = -ENODEV;
@@ -259,7 +253,7 @@ static long ioctl_rio(struct file *file, unsigned int cmd,
unsigned long arg)
err_out:
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return retval;
}
@@ -279,12 +273,12 @@ write_rio(struct file *file, const char __user *buffer,
int errn = 0;
int intr;
- intr = mutex_lock_interruptible(&(rio->lock));
+ intr = mutex_lock_interruptible(&rio500_mutex);
if (intr)
return -EINTR;
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return -ENODEV;
}
@@ -307,7 +301,7 @@ write_rio(struct file *file, const char __user *buffer,
goto error;
}
if (signal_pending(current)) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return bytes_written ? bytes_written : -EINTR;
}
@@ -345,12 +339,12 @@ write_rio(struct file *file, const char __user *buffer,
buffer += copy_size;
} while (count > 0);
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return bytes_written ? bytes_written : -EIO;
error:
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return errn;
}
@@ -367,12 +361,12 @@ read_rio(struct file *file, char __user *buffer, size_t
count, loff_t * ppos)
char *ibuf;
int intr;
- intr = mutex_lock_interruptible(&(rio->lock));
+ intr = mutex_lock_interruptible(&rio500_mutex);
if (intr)
return -EINTR;
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return -ENODEV;
}
@@ -383,11 +377,11 @@ read_rio(struct file *file, char __user *buffer, size_t
count, loff_t * ppos)
while (count > 0) {
if (signal_pending(current)) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return read_count ? read_count : -EINTR;
}
if (!rio->rio_dev) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return -ENODEV;
}
[PATCH 1/4] rio500: refuse more than one device at a time
This driver is using a global variable. It cannot handle more than
one device at a time. The issue has been existing since the dawn
of the driver.
V2: Fixed locking in probe()
Fixed Documentation
Signed-off-by: Oliver Neukum
Reported-by: syzbot+35f04d136fc975a70...@syzkaller.appspotmail.com
---
drivers/usb/misc/rio500.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index 13e4889bc34f..fa921ae60ffa 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -447,15 +447,23 @@ static int probe_rio(struct usb_interface *intf,
{
struct usb_device *dev = interface_to_usbdev(intf);
struct rio_usb_data *rio = &rio_instance;
- int retval;
+ int retval = 0;
- dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
+ mutex_lock(&rio500_mutex);
+ if (rio->present) {
+ dev_info(&intf->dev, "Second USB Rio at address %d refused\n",
dev->devnum);
+ retval = -EBUSY;
+ goto bail_out;
+ } else {
+ dev_info(&intf->dev, "USB Rio found at address %d\n",
dev->devnum);
+ }
retval = usb_register_dev(intf, &usb_rio_class);
if (retval) {
dev_err(&dev->dev,
"Not able to get a minor for this device.\n");
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto bail_out;
}
rio->rio_dev = dev;
@@ -464,7 +472,8 @@ static int probe_rio(struct usb_interface *intf,
dev_err(&dev->dev,
"probe_rio: Not enough memory for the output buffer\n");
usb_deregister_dev(intf, &usb_rio_class);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto bail_out;
}
dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
@@ -473,7 +482,8 @@ static int probe_rio(struct usb_interface *intf,
"probe_rio: Not enough memory for the input buffer\n");
usb_deregister_dev(intf, &usb_rio_class);
kfree(rio->obuf);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto bail_out;
}
dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
@@ -481,8 +491,10 @@ static int probe_rio(struct usb_interface *intf,
usb_set_intfdata (intf, rio);
rio->present = 1;
+bail_out:
+ mutex_unlock(&rio500_mutex);
- return 0;
+ return retval;
}
static void disconnect_rio(struct usb_interface *intf)
--
2.16.4
[PATCH 2/4] rio500: fix memeory leak in close after disconnect
If a disconnected device is closed, rio_close() must free
the buffers.
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/rio500.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index fa921ae60ffa..a1832c195b5f 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -86,9 +86,22 @@ static int close_rio(struct inode *inode, struct file *file)
{
struct rio_usb_data *rio = &rio_instance;
- rio->isopen = 0;
+ /* against disconnect() */
+ mutex_lock(&rio500_mutex);
+ mutex_lock(&(rio->lock));
- dev_info(&rio->rio_dev->dev, "Rio closed.\n");
+ rio->isopen = 0;
+ if (!rio->present) {
+ /* cleanup has been delayed */
+ kfree(rio->ibuf);
+ kfree(rio->obuf);
+ rio->ibuf = NULL;
+ rio->obuf = NULL;
+ } else {
+ dev_info(&rio->rio_dev->dev, "Rio closed.\n");
+ }
+ mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return 0;
}
--
2.16.4
[PATCHv3 2/4] rio500: fix memeory leak in close after disconnect
If a disconnected device is closed, rio_close() must free
the buffers.
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/rio500.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index fa921ae60ffa..a1832c195b5f 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -86,9 +86,22 @@ static int close_rio(struct inode *inode, struct file *file)
{
struct rio_usb_data *rio = &rio_instance;
- rio->isopen = 0;
+ /* against disconnect() */
+ mutex_lock(&rio500_mutex);
+ mutex_lock(&(rio->lock));
- dev_info(&rio->rio_dev->dev, "Rio closed.\n");
+ rio->isopen = 0;
+ if (!rio->present) {
+ /* cleanup has been delayed */
+ kfree(rio->ibuf);
+ kfree(rio->obuf);
+ rio->ibuf = NULL;
+ rio->obuf = NULL;
+ } else {
+ dev_info(&rio->rio_dev->dev, "Rio closed.\n");
+ }
+ mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return 0;
}
--
2.16.4
[PATCHv3 4/4] USB: rio500: update Documentation
Added the newly added limit and updated the text a bit Signed-off-by: Oliver Neukum --- Documentation/usb/rio.txt | 66 ++- 1 file changed, 13 insertions(+), 53 deletions(-) diff --git a/Documentation/usb/rio.txt b/Documentation/usb/rio.txt index ca9adcf56355..ea73475471db 100644 --- a/Documentation/usb/rio.txt +++ b/Documentation/usb/rio.txt @@ -76,70 +76,30 @@ Additional Information and userspace tools Requirements -A host with a USB port. Ideally, either a UHCI (Intel) or OHCI -(Compaq and others) hardware port should work. +A host with a USB port running a Linux kernel with RIO 500 support enabled. -A Linux development kernel (2.3.x) with USB support enabled or a -backported version to linux-2.2.x. See http://www.linux-usb.org for -more information on accomplishing this. +The driver is a module called rio500, which should be automatically loaded +as you plug in your device. If that fails you can manually load it with -A Linux kernel with RIO 500 support enabled. + modprobe rio500 -'lspci' which is only needed to determine the type of USB hardware -available in your machine. - -Configuration - -Using `lspci -v`, determine the type of USB hardware available. - - If you see something like:: - -USB Controller: .. -Flags: . -I/O ports at - - Then you have a UHCI based controller. - - If you see something like:: - - USB Controller: . - Flags: - Memory at . - - Then you have a OHCI based controller. - -Using `make menuconfig` or your preferred method for configuring the -kernel, select 'Support for USB', 'OHCI/UHCI' depending on your -hardware (determined from the steps above), 'USB Diamond Rio500 support', and -'Preliminary USB device filesystem'. Compile and install the modules -(you may need to execute `depmod -a` to update the module -dependencies). - -Add a device for the USB rio500:: +Udev should automatically create a device node as soon as plug in your device. +If that fails, you can manually add a device for the USB rio500:: mknod /dev/usb/rio500 c 180 64 -Set appropriate permissions for /dev/usb/rio500 (don't forget about -group and world permissions). Both read and write permissions are +In that case, set appropriate permissions for /dev/usb/rio500 (don't forget +about group and world permissions). Both read and write permissions are required for proper operation. -Load the appropriate modules (if compiled as modules): - - OHCI:: - -modprobe usbcore -modprobe usb-ohci -modprobe rio500 - - UHCI:: - -modprobe usbcore -modprobe usb-uhci (or uhci) -modprobe rio500 - That's it. The Rio500 Utils at: http://rio500.sourceforge.net should be able to access the rio500. +Limits +== + +You can use only a single rio500 device at a time with your computer. + Bugs -- 2.16.4
[PATCHv3 1/4] rio500: refuse more than one device at a time
This driver is using a global variable. It cannot handle more than
one device at a time. The issue has been existing since the dawn
of the driver.
V2: Fixed locking in probe()
Fixed Documentation
V3: Fixed formatting of Documentation
Signed-off-by: Oliver Neukum
Reported-by: syzbot+35f04d136fc975a70...@syzkaller.appspotmail.com
---
drivers/usb/misc/rio500.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index 13e4889bc34f..fa921ae60ffa 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -447,15 +447,23 @@ static int probe_rio(struct usb_interface *intf,
{
struct usb_device *dev = interface_to_usbdev(intf);
struct rio_usb_data *rio = &rio_instance;
- int retval;
+ int retval = 0;
- dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
+ mutex_lock(&rio500_mutex);
+ if (rio->present) {
+ dev_info(&intf->dev, "Second USB Rio at address %d refused\n",
dev->devnum);
+ retval = -EBUSY;
+ goto bail_out;
+ } else {
+ dev_info(&intf->dev, "USB Rio found at address %d\n",
dev->devnum);
+ }
retval = usb_register_dev(intf, &usb_rio_class);
if (retval) {
dev_err(&dev->dev,
"Not able to get a minor for this device.\n");
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto bail_out;
}
rio->rio_dev = dev;
@@ -464,7 +472,8 @@ static int probe_rio(struct usb_interface *intf,
dev_err(&dev->dev,
"probe_rio: Not enough memory for the output buffer\n");
usb_deregister_dev(intf, &usb_rio_class);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto bail_out;
}
dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
@@ -473,7 +482,8 @@ static int probe_rio(struct usb_interface *intf,
"probe_rio: Not enough memory for the input buffer\n");
usb_deregister_dev(intf, &usb_rio_class);
kfree(rio->obuf);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto bail_out;
}
dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
@@ -481,8 +491,10 @@ static int probe_rio(struct usb_interface *intf,
usb_set_intfdata (intf, rio);
rio->present = 1;
+bail_out:
+ mutex_unlock(&rio500_mutex);
- return 0;
+ return retval;
}
static void disconnect_rio(struct usb_interface *intf)
--
2.16.4
[PATCHv3 3/4] rio500: simplify locking
Admitting that there can be only one device allows us
to drop any pretense about locking one device or
a table of devices.
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/rio500.c | 43 ---
1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index a1832c195b5f..a0574f54738f 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -51,7 +51,6 @@ struct rio_usb_data {
char *obuf, *ibuf; /* transfer buffers */
char bulk_in_ep, bulk_out_ep; /* Endpoint assignments */
wait_queue_head_t wait_q; /* for timeouts */
- struct mutex lock; /* general race avoidance */
};
static DEFINE_MUTEX(rio500_mutex);
@@ -63,10 +62,8 @@ static int open_rio(struct inode *inode, struct file *file)
/* against disconnect() */
mutex_lock(&rio500_mutex);
- mutex_lock(&(rio->lock));
if (rio->isopen || !rio->present) {
- mutex_unlock(&(rio->lock));
mutex_unlock(&rio500_mutex);
return -EBUSY;
}
@@ -74,7 +71,6 @@ static int open_rio(struct inode *inode, struct file *file)
init_waitqueue_head(&rio->wait_q);
- mutex_unlock(&(rio->lock));
dev_info(&rio->rio_dev->dev, "Rio opened.\n");
mutex_unlock(&rio500_mutex);
@@ -88,7 +84,6 @@ static int close_rio(struct inode *inode, struct file *file)
/* against disconnect() */
mutex_lock(&rio500_mutex);
- mutex_lock(&(rio->lock));
rio->isopen = 0;
if (!rio->present) {
@@ -100,7 +95,6 @@ static int close_rio(struct inode *inode, struct file *file)
} else {
dev_info(&rio->rio_dev->dev, "Rio closed.\n");
}
- mutex_unlock(&(rio->lock));
mutex_unlock(&rio500_mutex);
return 0;
}
@@ -115,7 +109,7 @@ static long ioctl_rio(struct file *file, unsigned int cmd,
unsigned long arg)
int retries;
int retval=0;
- mutex_lock(&(rio->lock));
+ mutex_lock(&rio500_mutex);
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
retval = -ENODEV;
@@ -259,7 +253,7 @@ static long ioctl_rio(struct file *file, unsigned int cmd,
unsigned long arg)
err_out:
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return retval;
}
@@ -279,12 +273,12 @@ write_rio(struct file *file, const char __user *buffer,
int errn = 0;
int intr;
- intr = mutex_lock_interruptible(&(rio->lock));
+ intr = mutex_lock_interruptible(&rio500_mutex);
if (intr)
return -EINTR;
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return -ENODEV;
}
@@ -307,7 +301,7 @@ write_rio(struct file *file, const char __user *buffer,
goto error;
}
if (signal_pending(current)) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return bytes_written ? bytes_written : -EINTR;
}
@@ -345,12 +339,12 @@ write_rio(struct file *file, const char __user *buffer,
buffer += copy_size;
} while (count > 0);
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return bytes_written ? bytes_written : -EIO;
error:
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return errn;
}
@@ -367,12 +361,12 @@ read_rio(struct file *file, char __user *buffer, size_t
count, loff_t * ppos)
char *ibuf;
int intr;
- intr = mutex_lock_interruptible(&(rio->lock));
+ intr = mutex_lock_interruptible(&rio500_mutex);
if (intr)
return -EINTR;
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return -ENODEV;
}
@@ -383,11 +377,11 @@ read_rio(struct file *file, char __user *buffer, size_t
count, loff_t * ppos)
while (count > 0) {
if (signal_pending(current)) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return read_count ? read_count : -EINTR;
}
if (!rio->rio_dev) {
- mutex_unlock(&(rio->lock));
+ mutex_unlock(&rio500_mutex);
return -ENODEV;
}
[PATCH 4/4] USB: rio500: update Documentation
Added the newly added limit and updated the text a bit Signed-off-by: Oliver Neukum --- Documentation/usb/rio.txt | 66 ++- 1 file changed, 13 insertions(+), 53 deletions(-) diff --git a/Documentation/usb/rio.txt b/Documentation/usb/rio.txt index ca9adcf56355..ea73475471db 100644 --- a/Documentation/usb/rio.txt +++ b/Documentation/usb/rio.txt @@ -76,70 +76,30 @@ Additional Information and userspace tools Requirements -A host with a USB port. Ideally, either a UHCI (Intel) or OHCI -(Compaq and others) hardware port should work. +A host with a USB port running a Linux kernel with RIO 500 support enabled. -A Linux development kernel (2.3.x) with USB support enabled or a -backported version to linux-2.2.x. See http://www.linux-usb.org for -more information on accomplishing this. +The driver is a module called rio500, which should be automatically loaded +as you plug in your device. If that fails you can manually load it with -A Linux kernel with RIO 500 support enabled. + modprobe rio500 -'lspci' which is only needed to determine the type of USB hardware -available in your machine. - -Configuration - -Using `lspci -v`, determine the type of USB hardware available. - - If you see something like:: - -USB Controller: .. -Flags: . -I/O ports at - - Then you have a UHCI based controller. - - If you see something like:: - - USB Controller: . - Flags: - Memory at . - - Then you have a OHCI based controller. - -Using `make menuconfig` or your preferred method for configuring the -kernel, select 'Support for USB', 'OHCI/UHCI' depending on your -hardware (determined from the steps above), 'USB Diamond Rio500 support', and -'Preliminary USB device filesystem'. Compile and install the modules -(you may need to execute `depmod -a` to update the module -dependencies). - -Add a device for the USB rio500:: +Udev should automatically create a device node as soon as plug in your device. +If that fails, you can manually add a device for the USB rio500:: mknod /dev/usb/rio500 c 180 64 -Set appropriate permissions for /dev/usb/rio500 (don't forget about -group and world permissions). Both read and write permissions are +In that case, set appropriate permissions for /dev/usb/rio500 (don't forget +about group and world permissions). Both read and write permissions are required for proper operation. -Load the appropriate modules (if compiled as modules): - - OHCI:: - -modprobe usbcore -modprobe usb-ohci -modprobe rio500 - - UHCI:: - -modprobe usbcore -modprobe usb-uhci (or uhci) -modprobe rio500 - That's it. The Rio500 Utils at: http://rio500.sourceforge.net should be able to access the rio500. +Limits +== + +You can use only a single rio500 device at a time with your computer. + Bugs -- 2.16.4
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
Dear All, On 2019-04-26 15:23, Mathias Nyman wrote: > From: Nicolas Saenz Julienne > > Immediate data transfers (IDT) allow the HCD to copy small chunks of > data (up to 8bytes) directly into its output transfer TRBs. This avoids > the somewhat expensive DMA mappings that are performed by default on > most URBs submissions. > > In the case an URB was suitable for IDT. The data is directly copied > into the "Data Buffer Pointer" region of the TRB and the IDT flag is > set. Instead of triggering memory accesses the HC will use the data > directly. > > The implementation could cover all kind of output endpoints. Yet > Isochronous endpoints are bypassed as I was unable to find one that > matched IDT's constraints. As we try to bypass the default DMA mappings > on URB buffers we'd need to find a Isochronous device with an > urb->transfer_buffer_length <= 8 bytes. > > The implementation takes into account that the 8 byte buffers provided > by the URB will never cross a 64KB boundary. > > Signed-off-by: Nicolas Saenz Julienne > Reviewed-by: Felipe Balbi > Signed-off-by: Mathias Nyman I've noticed that this patch causes regression on various Samsung Exynos 5420/5422/5800 boards, which have USB3.0 host ports provided by DWC3/XHCI hardware module. The regression can be observed with ASIX USB 2.0 ethernet dongle, which stops working after applying this patch (eth0 interface is registered, but no packets are transmitted/received). I can provide more debugging information or do some tests, just let me know what do you need. Reverting this commit makes ASIX USB ethernet dongle operational again. Here are some more information from one of my test systems: # lsusb Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub Bus 005 Device 002: ID 0b95:772b ASIX Electronics Corp. AX88772B Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 001 Device 002: ID 2232:1056 Silicon Motion Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub # lsusb -t /: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M |__ Port 1: Dev 2, If 0, Class=Vendor Specific Class, Driver=asix, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=exynos-ohci/3p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=exynos-ehci/3p, 480M |__ Port 1: Dev 2, If 0, Class=Video, Driver=, 480M |__ Port 1: Dev 2, If 1, Class=Video, Driver=, 480M # dmesg | grep usb [ 1.484210] reg-fixed-voltage regulator-usb300: GPIO lookup for consumer (null) [ 1.484219] reg-fixed-voltage regulator-usb300: using device tree for GPIO lookup [ 1.484241] of_get_named_gpiod_flags: can't parse 'gpios' property of node '/regulator-usb300[0]' [ 1.484276] of_get_named_gpiod_flags: parsed 'gpio' property of node '/regulator-usb300[0]' - status (0) [ 1.484749] reg-fixed-voltage regulator-usb301: GPIO lookup for consumer (null) [ 1.484758] reg-fixed-voltage regulator-usb301: using device tree for GPIO lookup [ 1.484777] of_get_named_gpiod_flags: can't parse 'gpios' property of node '/regulator-usb301[0]' [ 1.484807] of_get_named_gpiod_flags: parsed 'gpio' property of node '/regulator-usb301[0]' - status (0) [ 1.490275] usbcore: registered new interface driver usbfs [ 1.495521] usbcore: registered new interface driver hub [ 1.500897] usbcore: registered new device driver usb [ 2.014966] samsung-usb2-phy 1213.phy: 1213.phy supply vbus not found, using dummy regulator [ 2.024093] exynos5_usb3drd_phy 1210.phy: 1210.phy supply vbus-boost not found, using dummy regulator [ 2.033232] exynos5_usb3drd_phy 1250.phy: 1250.phy supply vbus-boost not found, using dummy regulator [ 2.347306] usbcore: registered new interface driver r8152 [ 2.352427] usbcore: registered new interface driver asix [ 2.357877] usbcore: registered new interface driver ax88179_178a [ 2.363860] usbcore: registered new interface driver cdc_ether [ 2.369730] usbcore: registered new interface driver smsc75xx [ 2.375421] usbcore: registered new interface driver smsc95xx [ 2.381198] usbcore: registered new interface driver net1080 [ 2.386760] usbcore: registered new interface driver cdc_subset [ 2.392699] usbcore: registered new interface driver zaurus [ 2.398280] usbcore: registered new interface driver cdc_ncm [ 2.404280] exynos-dwc3 soc:usb3-0: soc:usb3-0 supply vdd33 not found, using dummy regulator [ 2.412485] exynos-dwc3 soc:usb3-0: soc:usb3-0 supply vdd10 not found, using dummy regulator [ 2.427570] exynos-dwc3 soc:usb3-1: soc:usb3-1 supply vdd33 not fo
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
On 9.5.2019 13.32, Marek Szyprowski wrote: Dear All, On 2019-04-26 15:23, Mathias Nyman wrote: From: Nicolas Saenz Julienne Immediate data transfers (IDT) allow the HCD to copy small chunks of data (up to 8bytes) directly into its output transfer TRBs. This avoids the somewhat expensive DMA mappings that are performed by default on most URBs submissions. In the case an URB was suitable for IDT. The data is directly copied into the "Data Buffer Pointer" region of the TRB and the IDT flag is set. Instead of triggering memory accesses the HC will use the data directly. The implementation could cover all kind of output endpoints. Yet Isochronous endpoints are bypassed as I was unable to find one that matched IDT's constraints. As we try to bypass the default DMA mappings on URB buffers we'd need to find a Isochronous device with an urb->transfer_buffer_length <= 8 bytes. The implementation takes into account that the 8 byte buffers provided by the URB will never cross a 64KB boundary. Signed-off-by: Nicolas Saenz Julienne Reviewed-by: Felipe Balbi Signed-off-by: Mathias Nyman I've noticed that this patch causes regression on various Samsung Exynos 5420/5422/5800 boards, which have USB3.0 host ports provided by DWC3/XHCI hardware module. The regression can be observed with ASIX USB 2.0 ethernet dongle, which stops working after applying this patch (eth0 interface is registered, but no packets are transmitted/received). I can provide more debugging information or do some tests, just let me know what do you need. Reverting this commit makes ASIX USB ethernet dongle operational again. Thanks for reporting. Would it be possible to check if your ASIX ethernet dongle works on some desktop/laptop setup with this same IDT patch? Also Exynos xhci traces could help, they would show the content of the TRBs using IDT. Maybe byte order gets messed up? Take traces with: mount -t debugfs none /sys/kernel/debug echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable send /sys/kernel/debug/tracing/trace content to me If we can't get this fixed I'll revert the IDT patch Thanks Mathias
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
On Thu, 2019-05-09 at 14:40 +0300, Mathias Nyman wrote: > On 9.5.2019 13.32, Marek Szyprowski wrote: > > Dear All, > > > > On 2019-04-26 15:23, Mathias Nyman wrote: > > > From: Nicolas Saenz Julienne > > > > > > Immediate data transfers (IDT) allow the HCD to copy small chunks of > > > data (up to 8bytes) directly into its output transfer TRBs. This avoids > > > the somewhat expensive DMA mappings that are performed by default on > > > most URBs submissions. > > > > > > In the case an URB was suitable for IDT. The data is directly copied > > > into the "Data Buffer Pointer" region of the TRB and the IDT flag is > > > set. Instead of triggering memory accesses the HC will use the data > > > directly. > > > > > > The implementation could cover all kind of output endpoints. Yet > > > Isochronous endpoints are bypassed as I was unable to find one that > > > matched IDT's constraints. As we try to bypass the default DMA mappings > > > on URB buffers we'd need to find a Isochronous device with an > > > urb->transfer_buffer_length <= 8 bytes. > > > > > > The implementation takes into account that the 8 byte buffers provided > > > by the URB will never cross a 64KB boundary. > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > Reviewed-by: Felipe Balbi > > > Signed-off-by: Mathias Nyman > > > > I've noticed that this patch causes regression on various Samsung Exynos > > 5420/5422/5800 boards, which have USB3.0 host ports provided by > > DWC3/XHCI hardware module. The regression can be observed with ASIX USB > > 2.0 ethernet dongle, which stops working after applying this patch (eth0 > > interface is registered, but no packets are transmitted/received). I can > > provide more debugging information or do some tests, just let me know > > what do you need. Reverting this commit makes ASIX USB ethernet dongle > > operational again. > > > > Thanks for reporting. > > Would it be possible to check if your ASIX ethernet dongle works on some > desktop/laptop setup with this same IDT patch? > > Also Exynos xhci traces could help, they would show the content of the TRBs > using IDT. > Maybe byte order gets messed up? > > Take traces with: > > mount -t debugfs none /sys/kernel/debug > echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb > echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable > > > > send /sys/kernel/debug/tracing/trace content to me > > If we can't get this fixed I'll revert the IDT patch Hi Matthias, thanks for your help. I'll also be looking into it, so please send me the logs too. Regards, Nicolas signature.asc Description: This is a digitally signed message part
[PATCH] USB: sisusbvga: fix oops in error path of sisusb_probe
The pointer used to log a failure of usb_register_dev() must
be set before the error is logged.
Signed-off-by: oliver Neukum
Reported-by: syzbot+a0cbdbd6d169020c8...@syzkaller.appspotmail.com
Fixes: 7b5cd5fefbe02 ("USB: SisUSB2VGA: Convert printk to dev_* macros")
---
drivers/usb/misc/sisusbvga/sisusb.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
b/drivers/usb/misc/sisusbvga/sisusb.c
index 9560fde621ee..23af255831c6 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -3029,6 +3029,14 @@ static int sisusb_probe(struct usb_interface *intf,
mutex_init(&(sisusb->lock));
+ sisusb->sisusb_dev = dev;
+ sisusb->minor = intf->minor;
+ sisusb->vrambase = SISUSB_PCI_MEMBASE;
+ sisusb->mmiobase = SISUSB_PCI_MMIOBASE;
+ sisusb->mmiosize = SISUSB_PCI_MMIOSIZE;
+ sisusb->ioportbase = SISUSB_PCI_IOPORTBASE;
+ /* Everything else is zero */
+
/* Register device */
retval = usb_register_dev(intf, &usb_sisusb_class);
if (retval) {
@@ -3039,14 +3047,6 @@ static int sisusb_probe(struct usb_interface *intf,
goto error_1;
}
- sisusb->sisusb_dev = dev;
- sisusb->minor = intf->minor;
- sisusb->vrambase = SISUSB_PCI_MEMBASE;
- sisusb->mmiobase = SISUSB_PCI_MMIOBASE;
- sisusb->mmiosize = SISUSB_PCI_MMIOSIZE;
- sisusb->ioportbase = SISUSB_PCI_IOPORTBASE;
- /* Everything else is zero */
-
/* Allocate buffers */
sisusb->ibufsize = SISUSB_IBUF_SIZE;
sisusb->ibuf = kmalloc(SISUSB_IBUF_SIZE, GFP_KERNEL);
--
2.16.4
[PATCHv2] USB: sisusbvga: fix oops in error path of sisusb_probe
The pointer used to log a failure of usb_register_dev() must
be set before the error is logged.
v2: fix that minor is not available before registration
Signed-off-by: oliver Neukum
Reported-by: syzbot+a0cbdbd6d169020c8...@syzkaller.appspotmail.com
Fixes: 7b5cd5fefbe02 ("USB: SisUSB2VGA: Convert printk to dev_* macros")
---
drivers/usb/misc/sisusbvga/sisusb.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
b/drivers/usb/misc/sisusbvga/sisusb.c
index 9560fde621ee..ea06f1fed6fa 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -3029,6 +3029,13 @@ static int sisusb_probe(struct usb_interface *intf,
mutex_init(&(sisusb->lock));
+ sisusb->sisusb_dev = dev;
+ sisusb->vrambase = SISUSB_PCI_MEMBASE;
+ sisusb->mmiobase = SISUSB_PCI_MMIOBASE;
+ sisusb->mmiosize = SISUSB_PCI_MMIOSIZE;
+ sisusb->ioportbase = SISUSB_PCI_IOPORTBASE;
+ /* Everything else is zero */
+
/* Register device */
retval = usb_register_dev(intf, &usb_sisusb_class);
if (retval) {
@@ -3039,13 +3046,7 @@ static int sisusb_probe(struct usb_interface *intf,
goto error_1;
}
- sisusb->sisusb_dev = dev;
- sisusb->minor = intf->minor;
- sisusb->vrambase = SISUSB_PCI_MEMBASE;
- sisusb->mmiobase = SISUSB_PCI_MMIOBASE;
- sisusb->mmiosize = SISUSB_PCI_MMIOSIZE;
- sisusb->ioportbase = SISUSB_PCI_IOPORTBASE;
- /* Everything else is zero */
+ sisusb->minor = intf->minor;
/* Allocate buffers */
sisusb->ibufsize = SISUSB_IBUF_SIZE;
--
2.16.4
RE: [V2] usb: gadget: storage: Remove warning message
On Thu, 9 May 2019, EJ Hsu wrote: > > You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT. And when > > that's done, it will no longer need to set common->new_fsg to NULL either. > > > > Yes, we should change fsg_unbind() as well. > > > This is sort of a band-aid approach. The real problem is that the original > > design > > of the driver never intended for there to be more than one outstanding > > CONFIG_CHANGE event, so naturally there are scenarios where it doesn't work > > right when that assumption is violated. This patch addresses one of those > > scenarios, but there may be others remaining. > > > > Ultimately the problem boils down to synchronization with the composite > > core. > > Thanks for your reviewing, I agree with your point. > > > Some of the callbacks made by the core take time to fully process, so what > > should happen if the core makes a second callback before the first one is > > completely processed? > > > > On the other hand, I can't think of anything better at the moment. > > > > Alan Stern > > Actually, composite core have tried to deal with this case by using a > spinlock. > (please refer to the cdev->lock) > The problem here is that the callback of fsg will delegate the request to > fsg_main_thread. That is, the handling of fsg request will run in parallel > with > composite core. > In my opinion, this issue can be avoided if we handle these request directly > in the callback of fsg instead of handing it over to fsg_main_thread. But > this > might take too much time in the interrupt context, which is not good for > system performance. > > Please correct me if there is anything wrong. Thanks In theory, the mass-storage function could also be fixed to be more careful about locking and synchronization. For example, never set or read common->next_fsg unless you're holding the fsg lock, and don't raise a CONFIG_CHANGE exception if another one is already pending. But I think your patch will be good enough for now, once you have fixed up the two issues mentioned earlier. Alan Stern
Re: [PATCH -next] usb: gadget: udc: lpc32xx: fix return value check in lpc32xx_udc_probe()
Thanks
Acked-by: Sylvain Lemieux
On Sat, May 4, 2019 at 8:17 AM Vladimir Zapolskiy wrote:
>
> Hi Wei Yongjun,
>
> On 05/04/2019 10:04 AM, Wei Yongjun wrote:
> > In case of error, the function devm_ioremap_resource() returns ERR_PTR()
> > and never returns NULL. The NULL test in the return value check should
> > be replaced with IS_ERR().
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Fixes: 408b56ca5c8e ("usb: gadget: udc: lpc32xx: simplify probe")
> > Signed-off-by: Wei Yongjun
> > ---
> > drivers/usb/gadget/udc/lpc32xx_udc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c
> > b/drivers/usb/gadget/udc/lpc32xx_udc.c
> > index d8f1c60793ed..00fb79c6d025 100644
> > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c
> > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
> > @@ -3070,9 +3070,9 @@ static int lpc32xx_udc_probe(struct platform_device
> > *pdev)
> > }
> >
> > udc->udp_baseaddr = devm_ioremap_resource(dev, res);
> > - if (!udc->udp_baseaddr) {
> > + if (IS_ERR(udc->udp_baseaddr)) {
> > dev_err(udc->dev, "IO map failure\n");
> > - return -ENOMEM;
> > + return PTR_ERR(udc->udp_baseaddr);
> > }
> >
> > /* Get USB device clock */
>
> thank you for the change, it is a correct fix.
>
> I do suppose that dev_err() in the context can be evenly removed, but
> likely it should be another change.
>
> Acked-by: Vladimir Zapolskiy
>
> --
> Best wishes,
> Vladimir
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
On 9.5.2019 14.51, Nicolas Saenz Julienne wrote:
On Thu, 2019-05-09 at 14:40 +0300, Mathias Nyman wrote:
On 9.5.2019 13.32, Marek Szyprowski wrote:
Dear All,
On 2019-04-26 15:23, Mathias Nyman wrote:
From: Nicolas Saenz Julienne
Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bytes) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.
In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.
The implementation could cover all kind of output endpoints. Yet
Isochronous endpoints are bypassed as I was unable to find one that
matched IDT's constraints. As we try to bypass the default DMA mappings
on URB buffers we'd need to find a Isochronous device with an
urb->transfer_buffer_length <= 8 bytes.
The implementation takes into account that the 8 byte buffers provided
by the URB will never cross a 64KB boundary.
Signed-off-by: Nicolas Saenz Julienne
Reviewed-by: Felipe Balbi
Signed-off-by: Mathias Nyman
I've noticed that this patch causes regression on various Samsung Exynos
5420/5422/5800 boards, which have USB3.0 host ports provided by
DWC3/XHCI hardware module. The regression can be observed with ASIX USB
2.0 ethernet dongle, which stops working after applying this patch (eth0
interface is registered, but no packets are transmitted/received). I can
provide more debugging information or do some tests, just let me know
what do you need. Reverting this commit makes ASIX USB ethernet dongle
operational again.
Thanks for reporting.
Would it be possible to check if your ASIX ethernet dongle works on some
desktop/laptop setup with this same IDT patch?
Also Exynos xhci traces could help, they would show the content of the TRBs
using IDT.
Maybe byte order gets messed up?
Take traces with:
mount -t debugfs none /sys/kernel/debug
echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb
echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable
send /sys/kernel/debug/tracing/trace content to me
If we can't get this fixed I'll revert the IDT patch
Hi Matthias, thanks for your help.
I'll also be looking into it, so please send me the logs too.
Got the logs off list, thanks
The "Buffer" data in Control transfer Data stage look suspicious.
grep "flags I:" trace_fail | grep Data
kworker/0:2-124 [000] d..163.092399: xhci_queue_trb: CTRL: Buffer
18b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429 [005] d..193.181231: xhci_queue_trb: CTRL: Buffer
18b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429 [007] dn.293.182050: xhci_queue_trb: CTRL: Buffer
length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429 [007] d..293.182499: xhci_queue_trb: CTRL: Buffer
8000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429 [007] d..293.182736: xhci_queue_trb: CTRL: Buffer
8000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
kworker/0:3-1409 [000] d..393.382630: xhci_queue_trb: CTRL: Buffer
8000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
First guess would be that in case URB has URB_NO_TRANSFER_DMA_MAP set then data
will be mapped and urb->transfer_dma is already set.
The IDT patch uses urb->trabfer_dma as a temporary buffer, and copies the
urb->transfer_buffer there.
if transfer buffer is already dma mapped the urb->transfer_buffer can be
garbage,
(shouldn't, but it can be)
Below code avoids IDT if URB_NO_TRANSFER_DMA_MAP is set, and doesn't touch
urb->transfer_dma (patch attached)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fed3385..f080054 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3423,11 +3423,14 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
if (urb->transfer_buffer_length > 0) {
u32 length_field, remainder;
+ u64 addr;
if (xhci_urb_suitable_for_idt(urb)) {
- memcpy(&urb->transfer_dma, urb->transfer_buffer,
+ memcpy(&addr, urb->transfer_buffer,
urb->transfer_buffer_length);
field |= TRB_IDT;
+ } else {
+ addr = (u64) urb->transfer_dma;
}
remainder = xhci_td_remainder(xhci, 0,
@@ -3440,8 +3443,8 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
if (setup->bRequestType & USB_DIR_IN)
field |= TRB_DIR_IN;
queue_trb(xhci, ep_ring, true,
- lower_32_bits(urb->
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
Hi Matthias, thanks for spending the time debugging this :)
On Thu, 2019-05-09 at 18:10 +0300, Mathias Nyman wrote:
> Got the logs off list, thanks
>
> The "Buffer" data in Control transfer Data stage look suspicious.
>
> grep "flags I:" trace_fail | grep Data
> kworker/0:2-124 [000] d..163.092399: xhci_queue_trb: CTRL: Buffer
> 18b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [005] d..193.181231: xhci_queue_trb: CTRL: Buffer
> 18b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [007] dn.293.182050: xhci_queue_trb: CTRL: Buffer
> length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [007] d..293.182499: xhci_queue_trb: CTRL: Buffer
> 8000 length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [007] d..293.182736: xhci_queue_trb: CTRL: Buffer
> 8000 length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> kworker/0:3-1409 [000] d..393.382630: xhci_queue_trb: CTRL: Buffer
> 8000 length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
>
> First guess would be that in case URB has URB_NO_TRANSFER_DMA_MAP set then
> data
> will be mapped and urb->transfer_dma is already set.
> The IDT patch uses urb->trabfer_dma as a temporary buffer, and copies the
> urb->transfer_buffer there.
> if transfer buffer is already dma mapped the urb->transfer_buffer can be
> garbage,
> (shouldn't, but it can be)
>
> Below code avoids IDT if URB_NO_TRANSFER_DMA_MAP is set, and doesn't touch
> urb->transfer_dma (patch attached)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index fed3385..f080054 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3423,11 +3423,14 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> mem_flags,
>
> if (urb->transfer_buffer_length > 0) {
> u32 length_field, remainder;
> + u64 addr;
>
> if (xhci_urb_suitable_for_idt(urb)) {
> - memcpy(&urb->transfer_dma, urb->transfer_buffer,
> + memcpy(&addr, urb->transfer_buffer,
> urb->transfer_buffer_length);
> field |= TRB_IDT;
> + } else {
> + addr = (u64) urb->transfer_dma;
> }
>
> remainder = xhci_td_remainder(xhci, 0,
> @@ -3440,8 +3443,8 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> mem_flags,
> if (setup->bRequestType & USB_DIR_IN)
> field |= TRB_DIR_IN;
> queue_trb(xhci, ep_ring, true,
> - lower_32_bits(urb->transfer_dma),
> - upper_32_bits(urb->transfer_dma),
> + lower_32_bits(addr),
> + upper_32_bits(addr),
> length_field,
> field | ep_ring->cycle_state);
> }
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index a450a99..7f8b950 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -2160,7 +2160,8 @@ static inline bool xhci_urb_suitable_for_idt(struct urb
> *urb)
> {
> if (!usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb)
> &&
> usb_endpoint_maxp(&urb->ep->desc) >= TRB_IDT_MAX_SIZE &&
> - urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE)
> + urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE &&
> + !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
> return true;
>
> return false;
>
This could be it, I broadly checked and assumed everyone was playing nice with
the transfer_dma pointer, but I guess I might have missed something.
> If that doesn't help, then it's possible DATA trbs in control transfer can't
> use IDT at all. IDT is supported for Normal TRBs, which have a different trb
> type than DATA trbs in control transfers.
>
> Also xhci specs 4.11.7 limit IDT usage:
>
> "If the IDT flag is set in one TRB of a TD, then it shall be the only Transfer
> TRB of the TD"
>
> A whole control transfer is one TD, and it already contains a SETUP transfer
> TRB
> which is using the IDT flag.
>
> Following disables IDT for control transfers (testpatch attached as well)
This one I'm not so sure as the standard defines a control transfer as a 2 or 3
TD operation (see 4.11.2.2):
"The Control Transfer Ring may contain Setup Stage and Status Stage TDs, and
optionally Data Stage TDs."
Also, for what is worth, I spent some time testing that specific case on my
intel laptop while preparing the patch.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
On 9.5.2019 18.38, Nicolas Saenz Julienne wrote: Hi Mathias, thanks for spending the time debugging this :) On Thu, 2019-05-09 at 18:10 +0300, Mathias Nyman wrote: Got the logs off list, thanks The "Buffer" data in Control transfer Data stage look suspicious. First guess would be that in case URB has URB_NO_TRANSFER_DMA_MAP set then data will be mapped and urb->transfer_dma is already set. The IDT patch uses urb->trabfer_dma as a temporary buffer, and copies the urb->transfer_buffer there. if transfer buffer is already dma mapped the urb->transfer_buffer can be garbage, (shouldn't, but it can be) This could be it, I broadly checked and assumed everyone was playing nice with the transfer_dma pointer, but I guess I might have missed something. If that doesn't help, then it's possible DATA trbs in control transfer can't use IDT at all. IDT is supported for Normal TRBs, which have a different trb type than DATA trbs in control transfers. Also xhci specs 4.11.7 limit IDT usage: "If the IDT flag is set in one TRB of a TD, then it shall be the only Transfer TRB of the TD" A whole control transfer is one TD, and it already contains a SETUP transfer TRB which is using the IDT flag. This one I'm not so sure as the standard defines a control transfer as a 2 or 3 TD operation (see 4.11.2.2): "The Control Transfer Ring may contain Setup Stage and Status Stage TDs, and optionally Data Stage TDs." True, xhci driver treats a control transfer as one TD, but TRBs aren't chained so from hw point of view they are separate TDs Also, for what is worth, I spent some time testing that specific case on my intel laptop while preparing the patch. And a closer look at the spec shows that both Normal and Data Stage TRB support IDT. So this is not likely the cause. -Mathias
Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support
Hi Mathias,
On 2019-05-09 17:10, Mathias Nyman wrote:
> On 9.5.2019 14.51, Nicolas Saenz Julienne wrote:
>> On Thu, 2019-05-09 at 14:40 +0300, Mathias Nyman wrote:
>>> On 9.5.2019 13.32, Marek Szyprowski wrote:
Dear All,
On 2019-04-26 15:23, Mathias Nyman wrote:
> From: Nicolas Saenz Julienne
>
> Immediate data transfers (IDT) allow the HCD to copy small chunks of
> data (up to 8bytes) directly into its output transfer TRBs. This
> avoids
> the somewhat expensive DMA mappings that are performed by default on
> most URBs submissions.
>
> In the case an URB was suitable for IDT. The data is directly copied
> into the "Data Buffer Pointer" region of the TRB and the IDT flag is
> set. Instead of triggering memory accesses the HC will use the data
> directly.
>
> The implementation could cover all kind of output endpoints. Yet
> Isochronous endpoints are bypassed as I was unable to find one that
> matched IDT's constraints. As we try to bypass the default DMA
> mappings
> on URB buffers we'd need to find a Isochronous device with an
> urb->transfer_buffer_length <= 8 bytes.
>
> The implementation takes into account that the 8 byte buffers
> provided
> by the URB will never cross a 64KB boundary.
>
> Signed-off-by: Nicolas Saenz Julienne
> Reviewed-by: Felipe Balbi
> Signed-off-by: Mathias Nyman
I've noticed that this patch causes regression on various Samsung
Exynos
5420/5422/5800 boards, which have USB3.0 host ports provided by
DWC3/XHCI hardware module. The regression can be observed with ASIX
USB
2.0 ethernet dongle, which stops working after applying this patch
(eth0
interface is registered, but no packets are transmitted/received).
I can
provide more debugging information or do some tests, just let me know
what do you need. Reverting this commit makes ASIX USB ethernet dongle
operational again.
>>>
>>> Thanks for reporting.
>>>
>>> Would it be possible to check if your ASIX ethernet dongle works on
>>> some
>>> desktop/laptop setup with this same IDT patch?
>>>
>>> Also Exynos xhci traces could help, they would show the content of
>>> the TRBs
>>> using IDT.
>>> Maybe byte order gets messed up?
>>>
>>> Take traces with:
>>>
>>> mount -t debugfs none /sys/kernel/debug
>>> echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb
>>> echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable
>>>
>>>
>>>
>>> send /sys/kernel/debug/tracing/trace content to me
>>>
>>> If we can't get this fixed I'll revert the IDT patch
>>
>> Hi Matthias, thanks for your help.
>>
>> I'll also be looking into it, so please send me the logs too.
>>
>
> Got the logs off list, thanks
>
> The "Buffer" data in Control transfer Data stage look suspicious.
>
> grep "flags I:" trace_fail | grep Data
> kworker/0:2-124 [000] d..1 63.092399: xhci_queue_trb: CTRL:
> Buffer 18b65000 length 6 TD size 0 intr 0 type 'Data Stage'
> flags I:i:c:s:i:e:C
> ifconfig-1429 [005] d..1 93.181231: xhci_queue_trb: CTRL: Buffer
> 18b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [007] dn.2 93.182050: xhci_queue_trb: CTRL: Buffer
> length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [007] d..2 93.182499: xhci_queue_trb: CTRL: Buffer
> 8000 length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> ifconfig-1429 [007] d..2 93.182736: xhci_queue_trb: CTRL: Buffer
> 8000 length 8 TD size 0 intr 0 type 'Data Stage' flags
> I:i:c:s:i:e:C
> kworker/0:3-1409 [000] d..3 93.382630: xhci_queue_trb: CTRL:
> Buffer 8000 length 8 TD size 0 intr 0 type 'Data Stage'
> flags I:i:c:s:i:e:C
>
> First guess would be that in case URB has URB_NO_TRANSFER_DMA_MAP set
> then data
> will be mapped and urb->transfer_dma is already set.
> The IDT patch uses urb->trabfer_dma as a temporary buffer, and copies the
> urb->transfer_buffer there.
> if transfer buffer is already dma mapped the urb->transfer_buffer can
> be garbage,
> (shouldn't, but it can be)
>
> Below code avoids IDT if URB_NO_TRANSFER_DMA_MAP is set, and doesn't
> touch
> urb->transfer_dma (patch attached)
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index fed3385..f080054 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3423,11 +3423,14 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci,
> gfp_t mem_flags,
>
> if (urb->transfer_buffer_length > 0) {
> u32 length_field, remainder;
> + u64 addr;
>
> if (xhci_urb_suitable_for_idt(urb)) {
> - memcpy(&urb->transfer_dma, urb->transfer_buffer,
> + memcpy(&addr, urb->transfer_buffer,
> urb->transfer
