Re: [PATCH 1/1] usb,uhci: add a new tag for virtual uhci devices

2013-06-27 Thread Li, Zhen-Hua (USL-China) 

There was a problem, the warning Controller not stopped yet.
And your last patch for this problem does a wrong thing:
It prevents all HP uhci devices from auto-stop, which make HP uhci 
devices waste more

power.
This is another new problem.

I think this should be corrected, so I want to apply it.

Thanks
Zhen-Hua

On 06/27/2013 11:52 PM, Alan Stern wrote:

On Thu, 27 Jun 2013, Li, Zhen-Hua (USL-China) wrote:


Hi Alan,

I don't have a machine that this makes action different.

Then why do you want to apply the patch?


No matter whether it makes different, there is one thing will never change:
We create a patch to FIX a problem, not to avoid a problem.
Only when we can not fix it, we try to avoid it.

But in this case, there is no problem to fix or to avoid, right?  After
all, how can there be a problem if no machines are affected?

Alan Stern

.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] usb,uhci: add a new tag for virtual uhci devices

2013-06-26 Thread Li, Zhen-Hua 
From: Li, Zhen-Hua zhen-h...@hp.com

There's another patch trying to fix this warning:
 Controller not stopped yet!. 
It is : 997ff893603c6455da4c5e26ba1d0f81adfecdfc .

I don't think it is appropriate to avoid auto-stop for all HP uhci
devices. So add one  tag for the virtual uhci devices, it is used
to replace wait_for_hp in the auto-stop case.

Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com
---
 drivers/usb/host/uhci-hcd.h |1 +
 drivers/usb/host/uhci-hub.c |2 +-
 drivers/usb/host/uhci-pci.c |   21 +
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
index 6f986d8..915d5df 100644
--- a/drivers/usb/host/uhci-hcd.h
+++ b/drivers/usb/host/uhci-hcd.h
@@ -425,6 +425,7 @@ struct uhci_hcd {
/* Silicon quirks */
unsigned int oc_low:1;  /* OverCurrent bit active low */
unsigned int wait_for_hp:1; /* Wait for HP port reset */
+   unsigned int virtual_device:1;  /* For some virtual devices */
unsigned int big_endian_mmio:1; /* Big endian registers */
unsigned int big_endian_desc:1; /* Big endian descriptors */
 
diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
index 9189bc9..da00754 100644
--- a/drivers/usb/host/uhci-hub.c
+++ b/drivers/usb/host/uhci-hub.c
@@ -226,7 +226,7 @@ static int uhci_hub_status_data(struct usb_hcd *hcd, char 
*buf)
if (any_ports_active(uhci))
uhci-rh_state = UHCI_RH_RUNNING;
else if (time_after_eq(jiffies, uhci-auto_stop_time) 
-   !uhci-wait_for_hp)
+   !uhci-virtual_device)
suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
break;
 
diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
index c300bd2f7..68e6d92 100644
--- a/drivers/usb/host/uhci-pci.c
+++ b/drivers/usb/host/uhci-pci.c
@@ -110,6 +110,24 @@ static int uhci_pci_global_suspend_mode_is_broken(struct 
uhci_hcd *uhci)
return 0;
 }
 
+static int uhci_virtual_device_ids[][2] = {
+   {0x103c, 0x3300},
+   {0, 0},
+};
+static int uhci_is_virtual_device(struct uhci_hcd *uhci)
+{
+   int i;
+   struct pci_dev *dev;
+
+   dev = to_pci_dev(uhci_dev(uhci));
+   for (i = 0; uhci_virtual_device_ids[i][0] != 0; i++) {
+   if (dev-vendor == uhci_virtual_device_ids[i][0]
+dev-device == uhci_virtual_device_ids[i][1])
+   return 1;
+   }
+   return 0;
+}
+
 static int uhci_pci_init(struct usb_hcd *hcd)
 {
struct uhci_hcd *uhci = hcd_to_uhci(hcd);
@@ -129,6 +147,9 @@ static int uhci_pci_init(struct usb_hcd *hcd)
if (to_pci_dev(uhci_dev(uhci))-vendor == PCI_VENDOR_ID_HP)
uhci-wait_for_hp = 1;
 
+   if (uhci_is_virtual_device(uhci))
+   uhci-virtual_device = 1;
+
/* Set up pointers to PCI-specific functions */
uhci-reset_hc = uhci_pci_reset_hc;
uhci-check_and_reset_hc = uhci_pci_check_and_reset_hc;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] usb,uhci: add a new tag for virtual uhci devices

2013-06-26 Thread Li, Zhen-Hua (USL-China) 

Hi Alan,

I don't have a machine that this makes action different.

No matter whether it makes different, there is one thing will never change:
We create a patch to FIX a problem, not to avoid a problem.
Only when we can not fix it, we try to avoid it.

Thanks
ZhenHua

On 06/27/2013 03:17 AM, Alan Stern wrote:

On Wed, 26 Jun 2013, Li, Zhen-Hua wrote:


From: Li, Zhen-Hua zhen-h...@hp.com

There's another patch trying to fix this warning:
  Controller not stopped yet!.
It is : 997ff893603c6455da4c5e26ba1d0f81adfecdfc .

I don't think it is appropriate to avoid auto-stop for all HP uhci
devices. So add one  tag for the virtual uhci devices, it is used
to replace wait_for_hp in the auto-stop case.

Do you have any machines where this makes a difference?

Alan Stern

.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

2013-05-05 Thread Li, Zhen-Hua (USL-China) 

On 04/29/2013 02:55 AM, Alan Stern wrote:

On Sun, 28 Apr 2013, ZhenHua wrote:


In fact, the patch is so easy that I am including it below.  Please
test this (without either of your patches) to see if it works.

Alan Stern




Index: usb-3.9/drivers/usb/host/uhci-hub.c
===
--- usb-3.9.orig/drivers/usb/host/uhci-hub.c
+++ usb-3.9/drivers/usb/host/uhci-hub.c
@@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
/* auto-stop if nothing connected for 1 second */
if (any_ports_active(uhci))
uhci-rh_state = UHCI_RH_RUNNING;
-   else if (time_after_eq(jiffies, uhci-auto_stop_time))
+   else if (time_after_eq(jiffies, uhci-auto_stop_time) 
+   !uhci-wait_for_hp)
suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
break;



I have tested the UHCI_RH_RUNNING_NODEVS case yeasterday, and it works.
But the function suspend_rh is also called in other places, so I think
it only fixes
the warning when auto stop is called, but not fix the warning when
uhci's bus_suspend
is called,  it will come out again.

Have you tried this?  I expect the warning will not occur when the
bus_suspend routine is called, because then there will be a 1-ms delay,
not just a 400-us delay.

I tested this, and the warning is gone.  Is this patch committed ?
I need to paste the link to suse bugzilla.
Not joking. I tested both of them , uhci-wait_for_hp and no_auto_stop , 
for the UHCI_RH_RUNNING_NODEVS
 case. And as uhci-wait_for_hp is 1 on my system, so they got the same 
result.



You must be joking.  I wrote that patch while composing the email
message to you, and nobody except you has tested it.

Since you confirm that it works, I will submit it.  But new patches
like this won't be accepted until after the upcoming merge window
closes, which won't be for more than two weeks.





And if you add uhci-wait_for_hp check in the UHCI_RH_RUNNING_NODEVS case,
all hp uhci devices will not auto stop, not only the virtual devices. I
guess it may waste resource.

If you want, you can add a new flag specifically for virtual
controllers.  But it shouldn't matter -- as long as your kernels are
built with CONFIG_PM_RUNTIME enabled, there won't be any significant
waste of resources.

Alan Stern


I think we can check the product id to determine whether a device is
virtual.
Do you know if there is another way to check this?

I don't know anything at all about your virtual UHCI controllers, other
than what you have told me.  In particular, I don't know what product
IDs are used by HP's virtual and non-virtual controllers.  Maybe
somebody else at HP can tell you.

Alan Stern

.

So I can only check the product IDs.

Thanks for helping me on this bug.


Regards
Zhen-Hua

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

2013-04-26 Thread Li, Zhen-Hua 
This patch is trying to fix this bug on SLES11 SP2:
https://bugzilla.novell.com/show_bug.cgi?id=817035

On a large HP system with 64T memory and 60 logical cpus, when usb
driver inits the iLo Virtual USB Controller, there comes a warning
Controller not stopped yet!. It is because the HP iLo virtual usb
device requires a longer delay.

Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com
---
 drivers/usb/host/uhci-hcd.c |5 +
 drivers/usb/host/uhci-hub.c |   12 +++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 4a86b63..af30517 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -355,6 +355,11 @@ __acquires(uhci-lock)
if (uhci-dead)
return;
}
+
+   /* HP's iLo Virtual USB Controller requires a longer delay. */
+   if (uhci-wait_for_hp)
+   wait_for_HP(uhci, USBSTS, USBSTS_HCH, 1000);
+
if (!(uhci_readw(uhci, USBSTS)  USBSTS_HCH))
dev_warn(uhci_dev(uhci), Controller not stopped yet!\n);
 
diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
index f87bee6..c3f772c 100644
--- a/drivers/usb/host/uhci-hub.c
+++ b/drivers/usb/host/uhci-hub.c
@@ -120,14 +120,15 @@ static void uhci_finish_suspend(struct uhci_hcd *uhci, 
int port,
 }
 
 /* Wait for the UHCI controller in HP's iLO2 server management chip.
- * It can take up to 250 us to finish a reset and set the CSC bit.
+ * It can take up to max_wait us to finish the operation.
  */
-static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr)
+static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr,
+   u16 status, int max_wait)
 {
int i;
 
-   for (i = 10; i  250; i += 10) {
-   if (uhci_readw(uhci, port_addr)  USBPORTSC_CSC)
+   for (i = 10; i  max_wait; i += 10) {
+   if (uhci_readw(uhci, port_addr)  status)
return;
udelay(10);
}
@@ -151,7 +152,8 @@ static void uhci_check_ports(struct uhci_hcd *uhci)
/* HP's server management chip requires
 * a longer delay. */
if (uhci-wait_for_hp)
-   wait_for_HP(uhci, port_addr);
+   wait_for_HP(uhci, port_addr,
+   USBPORTSC_CSC, 250);
 
/* If the port was enabled before, turning
 * reset on caused a port enable change.
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

2013-04-25 Thread Li, Zhen-Hua 
This patch is trying to fix this bug on SLES11 SP2:
https://bugzilla.novell.com/show_bug.cgi?id=817035

On a large HP system with 64T memory and 60 logical cpus, when usb
driver inits the iLo Virtual USB Controller, there comes a warning
Controller not stopped yet!. It is because driver does not wait
enough time. So driver should wait and retry if found controller 
 not stopped.

Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com
Signed-off-by: Tom Vaden tom.va...@hp.com
---
 drivers/usb/host/uhci-hcd.c |   17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 4a86b63..d592e22 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd 
*uhci)
uhci-global_suspend_mode_is_broken(uhci) : 0;
 }
 
+#define UHCI_SUSPENDRH_RETRY_MAX  10
+#define UHCI_SUSPENDRH_RETRY_DELAY100
+
 static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
 __releases(uhci-lock)
 __acquires(uhci-lock)
@@ -284,6 +287,8 @@ __acquires(uhci-lock)
int auto_stop;
int int_enable, egsm_enable, wakeup_enable;
struct usb_device *rhdev = uhci_to_hcd(uhci)-self.root_hub;
+   int try;
+   u16 stopped;
 
auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
dev_dbg(rhdev-dev, %s%s\n, __func__,
@@ -355,7 +360,17 @@ __acquires(uhci-lock)
if (uhci-dead)
return;
}
-   if (!(uhci_readw(uhci, USBSTS)  USBSTS_HCH))
+
+   for (try = 0; try  UHCI_SUSPENDRH_RETRY_MAX; try++) {
+   /* For some devices, for example, HP iLo usb controller,
+* we need to wait for more time and retry. 
+*/
+   stopped = uhci_readw(uhci, USBSTS)  USBSTS_HCH;
+   if (stopped)
+   break;
+   udelay(UHCI_SUSPENDRH_RETRY_DELAY);
+   }
+   if (!stopped)
dev_warn(uhci_dev(uhci), Controller not stopped yet!\n);
 
uhci_get_current_frame_number(uhci);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

2013-04-23 Thread Li, Zhen-Hua 
From: Li, Zhen-Hua zhen-h...@hp.com

This patch is trying to fix bug QXCR1001261767.
On some HP platform, when usb driver inits the iLo Virtual USB Controller, 
there may be a warning Controller not stopped yet!. It is because driver does 
not wait enough time.
This patch adds more time waiting and retries.

Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com
---
 drivers/usb/host/uhci-hcd.c |   16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 4a86b63..514e9d7 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd 
*uhci)
uhci-global_suspend_mode_is_broken(uhci) : 0;
 }
 
+#define UHCI_SUSPENDRH_RETRY_MAX  10
+#define UHCI_SUSPENDRH_RETRY_DELAY100
+
 static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
 __releases(uhci-lock)
 __acquires(uhci-lock)
@@ -284,6 +287,7 @@ __acquires(uhci-lock)
int auto_stop;
int int_enable, egsm_enable, wakeup_enable;
struct usb_device *rhdev = uhci_to_hcd(uhci)-self.root_hub;
+   u16 try, stopped;
 
auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
dev_dbg(rhdev-dev, %s%s\n, __func__,
@@ -355,7 +359,17 @@ __acquires(uhci-lock)
if (uhci-dead)
return;
}
-   if (!(uhci_readw(uhci, USBSTS)  USBSTS_HCH))
+
+   for (try = 0; try  UHCI_SUSPENDRH_RETRY_MAX; try++) {
+   /*
+* Sometimes we may need to wait more time and try again.
+*/
+   stopped = uhci_readw(uhci, USBSTS)  USBSTS_HCH;
+   if (stopped)
+   break;
+   udelay(UHCI_SUSPENDRH_RETRY_DELAY);
+   }
+   if (!stopped)
dev_warn(uhci_dev(uhci), Controller not stopped yet!\n);
 
uhci_get_current_frame_number(uhci);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html