USB UHCI portstatus correction

PATCH In review Aleš Nesrsta 2 participants 8 messages Jun 20, 2010 – Jul 13, 2010

lore

Share & tools

lore

Aleš Nesrsta

author

2010-06-20 09:23 UTC #0 lore raw

PATCH 1/1 USB UHCI portstatus correction

Hi,

I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.

Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...

Best regards
Ales

usb_patched/bus/usb/uhci.c renamed from usb/bus/usb/uhci.c +17 -2

diff -urB ./usb/bus/usb/uhci.c ./usb_patched/bus/usb/uhci.c
@@ -612,8 +612,23 @@
   status = grub_uhci_readreg16 (u, reg);
   grub_dprintf ("uhci", "detect=0x%02x\n", status);
 
+  if (!enable) /* We don't need reset port */
+    {
+      /* Disable the port.  */
+      grub_uhci_writereg16 (u, reg, 0 << 2);
+      grub_dprintf ("uhci", "waiting for the port to be disabled\n");
+      endtime = grub_get_time_ms () + 1000;
+      while ((grub_uhci_readreg16 (u, reg) & (1 << 2)))
+        if (grub_get_time_ms () > endtime)
+          return grub_error (GRUB_ERR_IO, "UHCI Timed out");
+
+      status = grub_uhci_readreg16 (u, reg);
+      grub_dprintf ("uhci", ">3detect=0x%02x\n", status);
+      return GRUB_ERR_NONE;
+    }
+    
   /* Reset the port.  */
-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);
 
   /* Wait for the reset to complete.  XXX: How long exactly?  */
   grub_millisleep (50); /* For root hub should be nominaly 50ms */

@@ -623,7 +638,7 @@
   grub_millisleep (10);
 
   /* Enable the port.  */
-  grub_uhci_writereg16 (u, reg, enable << 2);
+  grub_uhci_writereg16 (u, reg, 1 << 2);
   grub_millisleep (10);
 
   grub_dprintf ("uhci", "waiting for the port to be enabled\n");

Vladimir 'φ-coder/phcoder' Serbinenko

2010-06-25 19:45 UTC #1 lore raw

Quoted 9 lines · Aleš Nesrsta

Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...

This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.

Quoted 10 lines

Best regards
Ales
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko

Aleš Nesrsta

author

2010-06-25 22:03 UTC #2 lore raw

Quoted 6 lines · Vladimir 'φ-coder/phcoder' Serbinenko

Quoted 5 lines · Aleš Nesrsta
Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.

Quoted 7 lines

Quoted 4 lines
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...
This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.

Hi,
You are right according to some possible side-effects. But the lines
...
if (!enable) /* We don't need reset port */

    {
      /* Disable the port.  */
      grub_uhci_writereg16 (u, reg, 0 << 2);

...
should disable the port as the bit "Port Enable" is reset.

Port reset should be not necessary when disabling port - according to USB specification, reset of port should be done only to enable port and mainly to bring newly connected device to "Default" state (i.e. to state when device accepts communication via address 0).

Of course:
- I can be wrong
- it should be tested according to some side-effect

But in original code is real bug - if this function is called with enable=0, it reports false timeout as it is waiting for bit which will never set in this case.
This bug should be corrected in some way.

There is also question, why does the function attach_root_port (in usbhub.c) disable and enable of port before initialize connected device ?
Reset & enable of port (if new device is connected) should be enough, because, according to USB specification:
- hub should automatically disable the port if device is disconnected or port is not powered
- ports should be disabled by hub after power-up of hub
But maybe there are some special cases or buggy hubs/devices which needs such behavior (?) - I don't know, so I didn't touch this part of code.

Best regards
Ales

Quoted 16 lines

Quoted 10 lines
Best regards
Ales
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Vladimir 'φ-coder/phcoder' Serbinenko

2010-06-28 16:48 UTC #3 lore raw

Quoted 33 lines · Aleš Nesrsta

Quoted 7 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 5 lines · Aleš Nesrsta
Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.
Quoted 9 lines
Quoted 5 lines
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...
This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.
Hi,
You are right according to some possible side-effects. But the lines
...
if (!enable) /* We don't need reset port */
    {
      /* Disable the port.  */
      grub_uhci_writereg16 (u, reg, 0 << 2);
...
should disable the port as the bit "Port Enable" is reset.
Port reset should be not necessary when disabling port - according to USB specification, reset of port should be done only to enable port and mainly to bring newly connected device to "Default" state (i.e. to state when device accepts communication via address 0).

Quoted 10 lines

Of course:
- I can be wrong
- it should be tested according to some side-effect
But in original code is real bug - if this function is called with enable=0, it reports false timeout as it is waiting for bit which will never set in this case.
This bug should be corrected in some way.

I have nothing against that change. I was mainly referring to:

- grub_uhci_writereg16 (u, reg, enable << 9);
+ grub_uhci_writereg16 (u, reg, 1 << 9);
Which seems to always enable the port.

Quoted 12 lines

There is also question, why does the function attach_root_port (in usbhub.c) disable and enable of port before initialize connected device ?
Reset & enable of port (if new device is connected) should be enough, because, according to USB specification:
- hub should automatically disable the port if device is disconnected or port is not powered
- ports should be disabled by hub after power-up of hub
But maybe there are some special cases or buggy hubs/devices which needs such behavior (?) - I don't know, so I didn't touch this part of code.

It's the right strategy: if it doesn't bug and unlikely to, leave it alone.

Quoted 28 lines

Best regards
Ales
Quoted 12 lines
Quoted 11 lines
Best regards
Ales
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 5 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko

Aleš Nesrsta

author

2010-07-05 17:12 UTC #4 lore raw

Quoted 48 lines · Vladimir 'φ-coder/phcoder' Serbinenko

Quoted 33 lines · Aleš Nesrsta
Quoted 7 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 5 lines · Aleš Nesrsta
Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.
Quoted 9 lines
Quoted 5 lines
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...
This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.
Hi,
You are right according to some possible side-effects. But the lines
...
if (!enable) /* We don't need reset port */
    {
      /* Disable the port.  */
      grub_uhci_writereg16 (u, reg, 0 << 2);
...
should disable the port as the bit "Port Enable" is reset.
Port reset should be not necessary when disabling port - according to USB specification, reset of port should be done only to enable port and mainly to bring newly connected device to "Default" state (i.e. to state when device accepts communication via address 0).
Quoted 10 lines
Of course:
- I can be wrong
- it should be tested according to some side-effect
But in original code is real bug - if this function is called with enable=0, it reports false timeout as it is waiting for bit which will never set in this case.
This bug should be corrected in some way.
I have nothing against that change. I was mainly referring to:
usb_patched/bus/usb/uhci.c renamed from usb/bus/usb/uhci.c +1 -1
-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);

Quoted 2 lines

Which seems to always enable the port.

OK, I committed it into trunk as rev. 2522.
Regards, Ales

Quoted 48 lines

Quoted 13 lines
There is also question, why does the function attach_root_port (in usbhub.c) disable and enable of port before initialize connected device ?
Reset & enable of port (if new device is connected) should be enough, because, according to USB specification:
- hub should automatically disable the port if device is disconnected or port is not powered
- ports should be disabled by hub after power-up of hub
But maybe there are some special cases or buggy hubs/devices which needs such behavior (?) - I don't know, so I didn't touch this part of code.
It's the right strategy: if it doesn't bug and unlikely to, leave it alone.
Quoted 22 lines
Best regards
Ales
Quoted 12 lines
Quoted 11 lines
Best regards
Ales
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 5 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 6 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Vladimir 'φ-coder/phcoder' Serbinenko

2010-07-05 23:57 UTC #5 lore raw

Quoted 54 lines · Aleš Nesrsta

Quoted 52 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 37 lines · Aleš Nesrsta
Quoted 14 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 11 lines · Aleš Nesrsta
Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...
Quoted 5 lines
This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.
Hi,
You are right according to some possible side-effects. But the lines
...
if (!enable) /* We don't need reset port */
    {
      /* Disable the port.  */
      grub_uhci_writereg16 (u, reg, 0 << 2);
...
should disable the port as the bit "Port Enable" is reset.
Port reset should be not necessary when disabling port - according to USB specification, reset of port should be done only to enable port and mainly to bring newly connected device to "Default" state (i.e. to state when device accepts communication via address 0).
Quoted 11 lines
Of course:
- I can be wrong
- it should be tested according to some side-effect
But in original code is real bug - if this function is called with enable=0, it reports false timeout as it is waiting for bit which will never set in this case.
This bug should be corrected in some way.
I have nothing against that change. I was mainly referring to:

Quoted 9 lines

Quoted 5 lines
usb_patched/bus/usb/uhci.c renamed from usb/bus/usb/uhci.c +1 -1
-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);
Which seems to always enable the port.
OK, I committed it into trunk as rev. 2522.
Regards, Ales

I'm about to revert your patch. I never approved the patch as whole. I think you misunderstood me. When I approve patch I explicitly say "Go ahead for mainline" or "Go ahead for experimental". In this case I explicitly don't understand why you change (enable << 9) to (1 << 9). Could you explain this?

Quoted 59 lines

Quoted 47 lines
Quoted 14 lines
There is also question, why does the function attach_root_port (in usbhub.c) disable and enable of port before initialize connected device ?
Reset & enable of port (if new device is connected) should be enough, because, according to USB specification:
- hub should automatically disable the port if device is disconnected or port is not powered
- ports should be disabled by hub after power-up of hub
But maybe there are some special cases or buggy hubs/devices which needs such behavior (?) - I don't know, so I didn't touch this part of code.
It's the right strategy: if it doesn't bug and unlikely to, leave it alone.
Quoted 23 lines
Best regards
Ales
Quoted 12 lines
Quoted 5 lines
Best regards
Ales
Quoted 7 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 6 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 7 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 5 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko

Aleš Nesrsta

author

2010-07-06 17:19 UTC #6 lore raw

Vladimir 'φ-coder/phcoder' Serbinenko :

Quoted 64 lines · Aleš Nesrsta

Quoted 54 lines
Quoted 52 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 37 lines · Aleš Nesrsta
Quoted 14 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 11 lines · Aleš Nesrsta
Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...
Quoted 5 lines
This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.
Hi,
You are right according to some possible side-effects. But the lines
...
if (!enable) /* We don't need reset port */
    {
      /* Disable the port.  */
      grub_uhci_writereg16 (u, reg, 0 << 2);
...
should disable the port as the bit "Port Enable" is reset.
Port reset should be not necessary when disabling port - according to USB specification, reset of port should be done only to enable port and mainly to bring newly connected device to "Default" state (i.e. to state when device accepts communication via address 0).
Quoted 11 lines
Of course:
- I can be wrong
- it should be tested according to some side-effect
But in original code is real bug - if this function is called with enable=0, it reports false timeout as it is waiting for bit which will never set in this case.
This bug should be corrected in some way.
I have nothing against that change. I was mainly referring to:
Quoted 9 lines
Quoted 5 lines
usb_patched/bus/usb/uhci.c renamed from usb/bus/usb/uhci.c +1 -1
-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);
Which seems to always enable the port.
OK, I committed it into trunk as rev. 2522.
Regards, Ales

Quoted 5 lines

I'm about to revert your patch. I never approved the patch as whole. I think you misunderstood me. When I approve patch I explicitly say "Go ahead for mainline" or "Go ahead for experimental". In this case I explicitly don't understand why you change (enable << 9) to (1 << 9). Could you explain this?

Sorry about misunderstanding.

No problem to explain it - simply, this part code was changed only to prevent misunderstanding... :-)

If "enable" is FALSE, then no reset of USB port is requested/necessary and code goes via this way:
...
if (!enable) /* We don't need reset port */

    {
      /* Disable the port.  */
 ...
     return GRUB_ERR_NONE;
    }

I.e., Port Enable bit is cleared (and all another bits except WRC bits... - it should be not relevant in case of port disabling).

If "enable" is TRUE, it is necessary to do port reset, i.e. it is executed this code:
...
/* Reset the port. */
grub_uhci_writereg16 (u, reg, 1 << 9);
...
return GRUB_ERR_NONE;
}
But when this part of code is executed, "enable" is (should be..) always TRUE, so why to use "enable" instead of 1...? It can be confusing...

Note 1:
In both cases (set or reset of Port Enable bit) is made some waiting - it is because specification says: "Note that the bit status does not change until the port state actually changes and that there may be a delay in disabling or enabling a port if there is a transaction currently in progress on the USB".

Note 2:
As I wrote, this patch corrects the problem when function grub_uhci_portstatus is called with enable=FALSE.
What is maybe interesting, in "trunk" this function is never called with enable=FALSE, it is called only from this part of usbhub.c (grub_usb_root_hub):
...
for (i = 0; i < ports; i++)

    {
      grub_usb_speed_t speed = controller->dev->detect_dev (controller,

i);

      if (speed != GRUB_USB_SPEED_NONE)
	{
	  grub_usb_device_t dev;

	  /* Enable the port.  */
	  err = controller->dev->portstatus (controller, i, 1);
	  if (err)
	    continue;

	  /* Enable the port and create a device.  */
	  dev = grub_usb_hub_add_dev (controller, speed);
	  if (! dev)
	    continue;

	  /* If the device is a Hub, scan it for more devices.  */
	  if (dev->descdev.class == 0x09)
	    grub_usb_add_hub (dev);
	}
    }

...

In "usb" branch looks the same function differently:
It calls new function "attach_root_port" which contains:
...
/* Disable the port. XXX: Why? */
err = controller->dev->portstatus (controller, portno, 0); if (err)

    return;

  /* Enable the port.  */
  err = controller->dev->portstatus (controller, portno, 1);
  if (err)
    return;

/* Enable the port and create a device. */
dev = grub_usb_hub_add_dev (controller, speed);
if (! dev)

    return;

...
I.e. this functions first disables the port (I don't know why - as I wrote in text below from previous e-mail... - could it be some unwanted rest of some experiments?) and this causes timeout problem which is corrected in my patch.

Feel free to revert this patch or modify it as necessary...

Regards
Ales

Quoted 65 lines

Quoted 53 lines
Quoted 47 lines
Quoted 14 lines
There is also question, why does the function attach_root_port (in usbhub.c) disable and enable of port before initialize connected device ?
Reset & enable of port (if new device is connected) should be enough, because, according to USB specification:
- hub should automatically disable the port if device is disconnected or port is not powered
- ports should be disabled by hub after power-up of hub
But maybe there are some special cases or buggy hubs/devices which needs such behavior (?) - I don't know, so I didn't touch this part of code.
It's the right strategy: if it doesn't bug and unlikely to, leave it alone.
Quoted 23 lines
Best regards
Ales
Quoted 12 lines
Quoted 5 lines
Best regards
Ales
Quoted 7 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 6 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 7 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 5 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 6 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Vladimir 'φ-coder/phcoder' Serbinenko

2010-07-13 10:00 UTC #7 lore raw

Quoted 100 lines · Aleš Nesrsta

Vladimir 'φ-coder/phcoder' Serbinenko :
Quoted 74 lines · Aleš Nesrsta
Quoted 62 lines
Quoted 58 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 41 lines · Aleš Nesrsta
Quoted 16 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 12 lines · Aleš Nesrsta
Hi,
I found some mistake in uhci.c in grub_uhci_portstatus when enable=0. There is proposal of correction.
Without correction portstatus reported false timeout when enable=0 because it is waiting for reset to be done but none is performed...
Quoted 6 lines
This patch seems to change much more that you say. In particular enable=0 is a request to disable port and you seem to always enable it. This is likely to break other code.
Hi,
You are right according to some possible side-effects. But the lines
...
if (!enable) /* We don't need reset port */
    {
      /* Disable the port.  */
      grub_uhci_writereg16 (u, reg, 0 << 2);
...
should disable the port as the bit "Port Enable" is reset.
Port reset should be not necessary when disabling port - according to USB specification, reset of port should be done only to enable port and mainly to bring newly connected device to "Default" state (i.e. to state when device accepts communication via address 0).
Quoted 12 lines
Of course:
- I can be wrong
- it should be tested according to some side-effect
But in original code is real bug - if this function is called with enable=0, it reports false timeout as it is waiting for bit which will never set in this case.
This bug should be corrected in some way.
I have nothing against that change. I was mainly referring to:
Quoted 11 lines
Quoted 6 lines
(quoted diff) +1 -1
-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);
Which seems to always enable the port.
OK, I committed it into trunk as rev. 2522.
Regards, Ales
Quoted 6 lines
I'm about to revert your patch. I never approved the patch as whole. I think you misunderstood me. When I approve patch I explicitly say "Go ahead for mainline" or "Go ahead for experimental". In this case I explicitly don't understand why you change (enable << 9) to (1 << 9). Could you explain this?
Sorry about misunderstanding.
No problem to explain it - simply, this part code was changed only to prevent misunderstanding... :-)
If "enable" is FALSE, then no reset of USB port is requested/necessary and code goes via this way:
...
if (!enable) /* We don't need reset port */
    {
      /* Disable the port.  */
 ...
     return GRUB_ERR_NONE;
    }
I.e., Port Enable bit is cleared (and all another bits except WRC bits... - it should be not relevant in case of port disabling).

Quoted 12 lines

If "enable" is TRUE, it is necessary to do port reset, i.e. it is executed this code:
...
/* Reset the port. */
grub_uhci_writereg16 (u, reg, 1 << 9);
...
return GRUB_ERR_NONE;
}
But when this part of code is executed, "enable" is (should be..) always TRUE, so why to use "enable" instead of 1...? It can be confusing...

Oh, I missed the return statement. My bad. Now I have no problem with the patch going into usb branch. I'll remerge usb branch into trunk soon. Once I test it on Yeeloong

Quoted 150 lines

Note 1:
In both cases (set or reset of Port Enable bit) is made some waiting - it is because specification says: "Note that the bit status does not change until the port state actually changes and that there may be a delay in disabling or enabling a port if there is a transaction currently in progress on the USB".
Note 2:
As I wrote, this patch corrects the problem when function grub_uhci_portstatus is called with enable=FALSE.
What is maybe interesting, in "trunk" this function is never called with enable=FALSE, it is called only from this part of usbhub.c (grub_usb_root_hub):
...
for (i = 0; i < ports; i++)
    {
      grub_usb_speed_t speed = controller->dev->detect_dev (controller,
i);
      if (speed != GRUB_USB_SPEED_NONE)
	{
	  grub_usb_device_t dev;

	  /* Enable the port.  */
	  err = controller->dev->portstatus (controller, i, 1);
	  if (err)
	    continue;

	  /* Enable the port and create a device.  */
	  dev = grub_usb_hub_add_dev (controller, speed);
	  if (! dev)
	    continue;

	  /* If the device is a Hub, scan it for more devices.  */
	  if (dev->descdev.class == 0x09)
	    grub_usb_add_hub (dev);
	}
    }
...
In "usb" branch looks the same function differently:
It calls new function "attach_root_port" which contains:
...
/* Disable the port. XXX: Why? */
err = controller->dev->portstatus (controller, portno, 0); if (err)
    return;
  /* Enable the port.  */
  err = controller->dev->portstatus (controller, portno, 1);
  if (err)
    return;
/* Enable the port and create a device. */
dev = grub_usb_hub_add_dev (controller, speed);
if (! dev)
    return;
...
I.e. this functions first disables the port (I don't know why - as I wrote in text below from previous e-mail... - could it be some unwanted rest of some experiments?) and this causes timeout problem which is corrected in my patch.
Feel free to revert this patch or modify it as necessary...
Regards
Ales
Quoted 67 lines
Quoted 59 lines
Quoted 53 lines
Quoted 16 lines
There is also question, why does the function attach_root_port (in usbhub.c) disable and enable of port before initialize connected device ?
Reset & enable of port (if new device is connected) should be enough, because, according to USB specification:
- hub should automatically disable the port if device is disconnected or port is not powered
- ports should be disabled by hub after power-up of hub
But maybe there are some special cases or buggy hubs/devices which needs such behavior (?) - I don't know, so I didn't touch this part of code.
It's the right strategy: if it doesn't bug and unlikely to, leave it alone.
Quoted 26 lines
Best regards
Ales
Quoted 13 lines
Quoted 5 lines
Best regards
Ales
Quoted 8 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 7 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 8 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 6 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 7 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Quoted 5 lines
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko