On 07/06/2010 07:19 PM, Aleš Nesrsta wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko :
>
>> On 07/05/2010 07:12 PM, Aleš Nesrsta wrote:
>>
>>> Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>
>>>
>>>> On 06/26/2010 12:03 AM, Aleš Nesrsta wrote:
>>>>
>>>>
>>>>> Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On 06/20/2010 11:23 AM, Aleš Nesrsta wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>>
>>>> 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.
>>>>
>>>>
>>>>
>>> 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?
>>
> 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...
>
>
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
> 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
>
>
>>>>
>>>>
>>>>> 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.
>>>>
>>>>
>>>>> Best regards
>>>>> Ales
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>> Best regards
>>>>>>> Ales
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Grub-devel mailing list
>>>>>>> Grub-devel@gnu.org
>>>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> _______________________________________________
>>>>>> Grub-devel mailing list
>>>>>> Grub-devel@gnu.org
>>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>>
>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> Grub-devel mailing list
>>>>> Grub-devel@gnu.org
>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>
>>>>>
>>>>>
>>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>>
>>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
> -- Regards Vladimir 'φ-coder/phcoder' Serbinenko
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
