Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
On Mon, Nov 19, 2007 at 02:32:41PM -0800, David Miller wrote: > From: David Brownell <[EMAIL PROTECTED]> > Date: Mon, 19 Nov 2007 11:59:55 -0800 > > > > This addition certainly won't hurt. Did we ever get any feedback as to > > > whether it actually helped? > > > > ISTR that davem refused to try it, after reporting an > > intermittent failure on the original patch (which only > > addresses one end of that hardware race). > > > > So, no ... but given we know that closing only one side > > of the race isn't quite enough, I think we need it anyway. > > I think we should put all these changes in because at a minimum > it's a step in the right direction even if it doesn't close > the hole %100. > > So please let's merge the mutex and the msleep() change in > ASAP, thanks. The msleep one is queued up for the next Linus pull, and after it goes in, I'll add it to the -stable trees as well. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
From: David Brownell <[EMAIL PROTECTED]> Date: Mon, 19 Nov 2007 11:59:55 -0800 > > This addition certainly won't hurt. Did we ever get any feedback as to > > whether it actually helped? > > ISTR that davem refused to try it, after reporting an > intermittent failure on the original patch (which only > addresses one end of that hardware race). > > So, no ... but given we know that closing only one side > of the race isn't quite enough, I think we need it anyway. I think we should put all these changes in because at a minimum it's a step in the right direction even if it doesn't close the hole %100. So please let's merge the mutex and the msleep() change in ASAP, thanks. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
> This addition certainly won't hurt. Did we ever get any feedback as to > whether it actually helped? ISTR that davem refused to try it, after reporting an intermittent failure on the original patch (which only addresses one end of that hardware race). So, no ... but given we know that closing only one side of the race isn't quite enough, I think we need it anyway. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
On Mon, 19 Nov 2007, David Brownell wrote: > Still needs the following update, IMO ... > > ==CUT HERE > A recent patch added software synchronization during EHCI startup, > so ports aren't switched away from the companion controllers after > resets have started. This patch adds a short delay letting hardware > get a chance to finish that port switching before any new resets begin. > > That is, both sides of that hardware race window need to be closed. > > Signed-off-by: David Brownell <[EMAIL PROTECTED]> > --- > drivers/usb/host/ehci-hcd.c |5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > --- g26.orig/drivers/usb/host/ehci-hcd.c 2007-11-13 16:07:12.0 > -0800 > +++ g26/drivers/usb/host/ehci-hcd.c 2007-11-13 16:13:01.0 -0800 > @@ -594,12 +594,15 @@ static int ehci_run (struct usb_hcd *hcd >* from the companions to the EHCI controller. If any of the >* companions are in the middle of a port reset at the time, it >* could cause trouble. Write-locking ehci_cf_port_reset_rwsem > - * guarantees that no resets are in progress. > + * guarantees that no resets are in progress. After we set CF, > + * a short delay lets the hardware catch up; new resets shouldn't > + * start before the port switching actions complete. >*/ > down_write(_cf_port_reset_rwsem); > hcd->state = HC_STATE_RUNNING; > ehci_writel(ehci, FLAG_CF, >regs->configured_flag); > ehci_readl(ehci, >regs->command); /* unblock posted writes */ > + msleep(5); > up_write(_cf_port_reset_rwsem); > > temp = HC_VERSION(ehci_readl(ehci, >caps->hc_capbase)); This addition certainly won't hurt. Did we ever get any feedback as to whether it actually helped? Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
On Mon, Nov 19, 2007 at 10:41:28AM -0800, David Brownell wrote: > Still needs the following update, IMO ... Yes, but that patch isn't in Linus's tree yet, so I can't add it to the -stable trees. It's in my queue to send to Linus, but that's not going to happen until next week at the earliest... thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
Still needs the following update, IMO ... == CUT HERE A recent patch added software synchronization during EHCI startup, so ports aren't switched away from the companion controllers after resets have started. This patch adds a short delay letting hardware get a chance to finish that port switching before any new resets begin. That is, both sides of that hardware race window need to be closed. Signed-off-by: David Brownell <[EMAIL PROTECTED]> --- drivers/usb/host/ehci-hcd.c |5 - 1 file changed, 4 insertions(+), 1 deletion(-) --- g26.orig/drivers/usb/host/ehci-hcd.c2007-11-13 16:07:12.0 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2007-11-13 16:13:01.0 -0800 @@ -594,12 +594,15 @@ static int ehci_run (struct usb_hcd *hcd * from the companions to the EHCI controller. If any of the * companions are in the middle of a port reset at the time, it * could cause trouble. Write-locking ehci_cf_port_reset_rwsem -* guarantees that no resets are in progress. +* guarantees that no resets are in progress. After we set CF, +* a short delay lets the hardware catch up; new resets shouldn't +* start before the port switching actions complete. */ down_write(_cf_port_reset_rwsem); hcd->state = HC_STATE_RUNNING; ehci_writel(ehci, FLAG_CF, >regs->configured_flag); ehci_readl(ehci, >regs->command); /* unblock posted writes */ + msleep(5); up_write(_cf_port_reset_rwsem); temp = HC_VERSION(ehci_readl(ehci, >caps->hc_capbase)); - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[patch 19/26] USB: mutual exclusion for EHCI init and port resets
2.6.22-stable review patch. If anyone has any objections, please let us
know.
--
From: Alan Stern <[EMAIL PROTECTED]>
patch 32fe01985aa2cb2562f6fc171e526e279abe10db in mainline.
This patch (as999) fixes a problem that sometimes shows up when host
controller driver modules are loaded in the wrong order. If ehci-hcd
happens to initialize an EHCI controller while the companion OHCI or
UHCI controller is in the middle of a port reset, the reset can fail
and the companion may get very confused. The patch adds an
rw-semaphore and uses it to keep EHCI initialization and port resets
mutually exclusive.
Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
Acked-by: David Brownell <[EMAIL PROTECTED]>
Cc: David Miller <[EMAIL PROTECTED]>
Cc: Dely L Sy <[EMAIL PROTECTED]>
Signed-off-by: Greg Kroah-Hartman <[EMAIL PROTECTED]>
---
drivers/usb/core/hcd.h |8 +++-
drivers/usb/core/hub.c | 15 ++-
drivers/usb/host/ehci-hcd.c |8
3 files changed, 29 insertions(+), 2 deletions(-)
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -19,6 +19,8 @@
#ifdef __KERNEL__
+#include
+
/* This file contains declarations of usbcore internals that are mostly
* used or exposed by Host Controller Drivers.
*/
@@ -464,5 +466,9 @@ static inline void usbmon_urb_complete(s
: (in_interrupt () ? "in_interrupt" : "can sleep"))
-#endif /* __KERNEL__ */
+/* This rwsem is for use only by the hub driver and ehci-hcd.
+ * Nobody else should touch it.
+ */
+extern struct rw_semaphore ehci_cf_port_reset_rwsem;
+#endif /* __KERNEL__ */
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -117,6 +117,12 @@ MODULE_PARM_DESC(use_both_schemes,
"try the other device initialization scheme if the "
"first one fails");
+/* Mutual exclusion for EHCI CF initialization. This interferes with
+ * port reset on some companion controllers.
+ */
+DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
+EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
+
static inline char *portspeed(int portstatus)
{
@@ -1513,6 +1519,11 @@ static int hub_port_reset(struct usb_hub
{
int i, status;
+ /* Block EHCI CF initialization during the port reset.
+* Some companion controllers don't like it when they mix.
+*/
+ down_read(_cf_port_reset_rwsem);
+
/* Reset the port */
for (i = 0; i < PORT_RESET_TRIES; i++) {
status = set_port_feature(hub->hdev,
@@ -1543,7 +1554,7 @@ static int hub_port_reset(struct usb_hub
usb_set_device_state(udev, status
? USB_STATE_NOTATTACHED
: USB_STATE_DEFAULT);
- return status;
+ goto done;
}
dev_dbg (hub->intfdev,
@@ -1556,6 +1567,8 @@ static int hub_port_reset(struct usb_hub
"Cannot enable port %i. Maybe the USB cable is bad?\n",
port1);
+ done:
+ up_read(_cf_port_reset_rwsem);
return status;
}
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -566,10 +566,18 @@ static int ehci_run (struct usb_hcd *hcd
* are explicitly handed to companion controller(s), so no TT is
* involved with the root hub. (Except where one is integrated,
* and there's no companion controller unless maybe for USB OTG.)
+*
+* Turning on the CF flag will transfer ownership of all ports
+* from the companions to the EHCI controller. If any of the
+* companions are in the middle of a port reset at the time, it
+* could cause trouble. Write-locking ehci_cf_port_reset_rwsem
+* guarantees that no resets are in progress.
*/
+ down_write(_cf_port_reset_rwsem);
hcd->state = HC_STATE_RUNNING;
ehci_writel(ehci, FLAG_CF, >regs->configured_flag);
ehci_readl(ehci, >regs->command); /* unblock posted writes */
+ up_write(_cf_port_reset_rwsem);
temp = HC_VERSION(ehci_readl(ehci, >caps->hc_capbase));
ehci_info (ehci,
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 19/26] USB: mutual exclusion for EHCI init and port resets
2.6.22-stable review patch. If anyone has any objections, please let us
know.
--
From: Alan Stern [EMAIL PROTECTED]
patch 32fe01985aa2cb2562f6fc171e526e279abe10db in mainline.
This patch (as999) fixes a problem that sometimes shows up when host
controller driver modules are loaded in the wrong order. If ehci-hcd
happens to initialize an EHCI controller while the companion OHCI or
UHCI controller is in the middle of a port reset, the reset can fail
and the companion may get very confused. The patch adds an
rw-semaphore and uses it to keep EHCI initialization and port resets
mutually exclusive.
Signed-off-by: Alan Stern [EMAIL PROTECTED]
Acked-by: David Brownell [EMAIL PROTECTED]
Cc: David Miller [EMAIL PROTECTED]
Cc: Dely L Sy [EMAIL PROTECTED]
Signed-off-by: Greg Kroah-Hartman [EMAIL PROTECTED]
---
drivers/usb/core/hcd.h |8 +++-
drivers/usb/core/hub.c | 15 ++-
drivers/usb/host/ehci-hcd.c |8
3 files changed, 29 insertions(+), 2 deletions(-)
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -19,6 +19,8 @@
#ifdef __KERNEL__
+#include linux/rwsem.h
+
/* This file contains declarations of usbcore internals that are mostly
* used or exposed by Host Controller Drivers.
*/
@@ -464,5 +466,9 @@ static inline void usbmon_urb_complete(s
: (in_interrupt () ? in_interrupt : can sleep))
-#endif /* __KERNEL__ */
+/* This rwsem is for use only by the hub driver and ehci-hcd.
+ * Nobody else should touch it.
+ */
+extern struct rw_semaphore ehci_cf_port_reset_rwsem;
+#endif /* __KERNEL__ */
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -117,6 +117,12 @@ MODULE_PARM_DESC(use_both_schemes,
try the other device initialization scheme if the
first one fails);
+/* Mutual exclusion for EHCI CF initialization. This interferes with
+ * port reset on some companion controllers.
+ */
+DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
+EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
+
static inline char *portspeed(int portstatus)
{
@@ -1513,6 +1519,11 @@ static int hub_port_reset(struct usb_hub
{
int i, status;
+ /* Block EHCI CF initialization during the port reset.
+* Some companion controllers don't like it when they mix.
+*/
+ down_read(ehci_cf_port_reset_rwsem);
+
/* Reset the port */
for (i = 0; i PORT_RESET_TRIES; i++) {
status = set_port_feature(hub-hdev,
@@ -1543,7 +1554,7 @@ static int hub_port_reset(struct usb_hub
usb_set_device_state(udev, status
? USB_STATE_NOTATTACHED
: USB_STATE_DEFAULT);
- return status;
+ goto done;
}
dev_dbg (hub-intfdev,
@@ -1556,6 +1567,8 @@ static int hub_port_reset(struct usb_hub
Cannot enable port %i. Maybe the USB cable is bad?\n,
port1);
+ done:
+ up_read(ehci_cf_port_reset_rwsem);
return status;
}
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -566,10 +566,18 @@ static int ehci_run (struct usb_hcd *hcd
* are explicitly handed to companion controller(s), so no TT is
* involved with the root hub. (Except where one is integrated,
* and there's no companion controller unless maybe for USB OTG.)
+*
+* Turning on the CF flag will transfer ownership of all ports
+* from the companions to the EHCI controller. If any of the
+* companions are in the middle of a port reset at the time, it
+* could cause trouble. Write-locking ehci_cf_port_reset_rwsem
+* guarantees that no resets are in progress.
*/
+ down_write(ehci_cf_port_reset_rwsem);
hcd-state = HC_STATE_RUNNING;
ehci_writel(ehci, FLAG_CF, ehci-regs-configured_flag);
ehci_readl(ehci, ehci-regs-command); /* unblock posted writes */
+ up_write(ehci_cf_port_reset_rwsem);
temp = HC_VERSION(ehci_readl(ehci, ehci-caps-hc_capbase));
ehci_info (ehci,
--
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
Still needs the following update, IMO ... == CUT HERE A recent patch added software synchronization during EHCI startup, so ports aren't switched away from the companion controllers after resets have started. This patch adds a short delay letting hardware get a chance to finish that port switching before any new resets begin. That is, both sides of that hardware race window need to be closed. Signed-off-by: David Brownell [EMAIL PROTECTED] --- drivers/usb/host/ehci-hcd.c |5 - 1 file changed, 4 insertions(+), 1 deletion(-) --- g26.orig/drivers/usb/host/ehci-hcd.c2007-11-13 16:07:12.0 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2007-11-13 16:13:01.0 -0800 @@ -594,12 +594,15 @@ static int ehci_run (struct usb_hcd *hcd * from the companions to the EHCI controller. If any of the * companions are in the middle of a port reset at the time, it * could cause trouble. Write-locking ehci_cf_port_reset_rwsem -* guarantees that no resets are in progress. +* guarantees that no resets are in progress. After we set CF, +* a short delay lets the hardware catch up; new resets shouldn't +* start before the port switching actions complete. */ down_write(ehci_cf_port_reset_rwsem); hcd-state = HC_STATE_RUNNING; ehci_writel(ehci, FLAG_CF, ehci-regs-configured_flag); ehci_readl(ehci, ehci-regs-command); /* unblock posted writes */ + msleep(5); up_write(ehci_cf_port_reset_rwsem); temp = HC_VERSION(ehci_readl(ehci, ehci-caps-hc_capbase)); - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
On Mon, Nov 19, 2007 at 10:41:28AM -0800, David Brownell wrote: Still needs the following update, IMO ... Yes, but that patch isn't in Linus's tree yet, so I can't add it to the -stable trees. It's in my queue to send to Linus, but that's not going to happen until next week at the earliest... thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
On Mon, 19 Nov 2007, David Brownell wrote: Still needs the following update, IMO ... ==CUT HERE A recent patch added software synchronization during EHCI startup, so ports aren't switched away from the companion controllers after resets have started. This patch adds a short delay letting hardware get a chance to finish that port switching before any new resets begin. That is, both sides of that hardware race window need to be closed. Signed-off-by: David Brownell [EMAIL PROTECTED] --- drivers/usb/host/ehci-hcd.c |5 - 1 file changed, 4 insertions(+), 1 deletion(-) --- g26.orig/drivers/usb/host/ehci-hcd.c 2007-11-13 16:07:12.0 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2007-11-13 16:13:01.0 -0800 @@ -594,12 +594,15 @@ static int ehci_run (struct usb_hcd *hcd * from the companions to the EHCI controller. If any of the * companions are in the middle of a port reset at the time, it * could cause trouble. Write-locking ehci_cf_port_reset_rwsem - * guarantees that no resets are in progress. + * guarantees that no resets are in progress. After we set CF, + * a short delay lets the hardware catch up; new resets shouldn't + * start before the port switching actions complete. */ down_write(ehci_cf_port_reset_rwsem); hcd-state = HC_STATE_RUNNING; ehci_writel(ehci, FLAG_CF, ehci-regs-configured_flag); ehci_readl(ehci, ehci-regs-command); /* unblock posted writes */ + msleep(5); up_write(ehci_cf_port_reset_rwsem); temp = HC_VERSION(ehci_readl(ehci, ehci-caps-hc_capbase)); This addition certainly won't hurt. Did we ever get any feedback as to whether it actually helped? Alan Stern - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
This addition certainly won't hurt. Did we ever get any feedback as to whether it actually helped? ISTR that davem refused to try it, after reporting an intermittent failure on the original patch (which only addresses one end of that hardware race). So, no ... but given we know that closing only one side of the race isn't quite enough, I think we need it anyway. - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
From: David Brownell [EMAIL PROTECTED] Date: Mon, 19 Nov 2007 11:59:55 -0800 This addition certainly won't hurt. Did we ever get any feedback as to whether it actually helped? ISTR that davem refused to try it, after reporting an intermittent failure on the original patch (which only addresses one end of that hardware race). So, no ... but given we know that closing only one side of the race isn't quite enough, I think we need it anyway. I think we should put all these changes in because at a minimum it's a step in the right direction even if it doesn't close the hole %100. So please let's merge the mutex and the msleep() change in ASAP, thanks. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 19/26] USB: mutual exclusion for EHCI init and port resets
On Mon, Nov 19, 2007 at 02:32:41PM -0800, David Miller wrote: From: David Brownell [EMAIL PROTECTED] Date: Mon, 19 Nov 2007 11:59:55 -0800 This addition certainly won't hurt. Did we ever get any feedback as to whether it actually helped? ISTR that davem refused to try it, after reporting an intermittent failure on the original patch (which only addresses one end of that hardware race). So, no ... but given we know that closing only one side of the race isn't quite enough, I think we need it anyway. I think we should put all these changes in because at a minimum it's a step in the right direction even if it doesn't close the hole %100. So please let's merge the mutex and the msleep() change in ASAP, thanks. The msleep one is queued up for the next Linus pull, and after it goes in, I'll add it to the -stable trees as well. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
