Re: [PATCH 07/10] ehci-dbgp,ehci: Allow early or late use of the dbgp device

[Jason Wessel <jason.wessel@windriver.com>]

Alan Stern wrote:
> On Fri, 31 Jul 2009, Jason Wessel wrote:
>
>> If the EHCI debug port is initialized and in use, the EHCI host
>> controller driver must follow two rules.
>>
>> 1) If the EHCI host driver issues a controller reset, the debug
>>    controller driver re-initialization must get called after the reset
>>    is completed.
>>
>> 2) The EHCI host driver should ignore any requests to the physical
>>    EHCI debug port when the EHCI debug port is in use.
>>
>> The code to check for the debug port was moved from ehci_pci_reinit()
>> to ehci_pci_setup because it must get called prior to ehci_reset()
>> which will clear the debug port registers.
>> --- a/drivers/usb/host/ehci-hub.c
>> +++ b/drivers/usb/host/ehci-hub.c
>> @@ -816,6 +816,15 @@ static int ehci_hub_control (
>>      case SetPortFeature:
>>          selector = wIndex >> 8;
>>          wIndex &= 0xff;
>> +        if (unlikely(ehci->debug)) {
>
> Don't you need a similar test for ClearPortFeature?  Or does that not
> matter?

I guess it depends if the user space tries to poke it, and then
perhaps yes.  Do you know if that would be the case?

The rejection of the SetPortFeature at this level prevents the generic
hub code from activating the port, in so far as all my tests cases
show.

Given that it won't hurt anything to have the same code in the
ClearPortFeature I can put it in.

>
>> +            /* If the debug port is active any port
>> +             * feature requests should get denied */
>> +            if ((readl(&ehci->debug->control) & DBGP_ENABLED) &&
>> +                wIndex == HCS_DEBUG_PORT(ehci->hcs_params)) {
>
> The order of the two tests here should be reversed, to avoid an
> unnecessary I/O access in the common case.
>

Yes.  I agree here.


>> +                retval = -ENODEV;
>> +                goto error_exit;
>
> This is the wrong return value.  You should return -EPIPE, i.e., goto
> error instead of error_exit.


Do you have some insight as to how to account for the not using the
port another way?  If I return -EPIPE, vs -ENODEV.  The kernel code
does very different things.  I chose -ENODEV based on the results
below.

With -ENODEV the kernel emits the following when trying to probe the
usb port with the dbgp device.


hub 2-0:1.0: cannot reset port 1 (err = -19)
hub 2-0:1.0: cannot reset port 1 (err = -19)
hub 2-0:1.0: cannot reset port 1 (err = -19)
hub 2-0:1.0: cannot reset port 1 (err = -19)
hub 2-0:1.0: unable to enumerate USB device on port 1


With returning -EPIPE you get this result.

hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
USB Serial support registered for generic
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: Cannot enable port 1.  Maybe the USB cable is bad?
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: Cannot enable port 1.  Maybe the USB cable is bad?
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: Cannot enable port 1.  Maybe the USB cable is bad?
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: cannot reset port 1 (err = -32)
hub 2-0:1.0: Cannot enable port 1.  Maybe the USB cable is bad?
hub 2-0:1.0: unable to enumerate USB device on port 1


I was looking for the easiest maintainable approach to disabling the
physical port from getting reset by the HCD hub driver, which does
different things depending on the error code that is passed back.

Knowing what happens with -EPIPE, would you still prefer it to be
changed that way?  Another possibility here is to implement the
restriction on the port reset another way, if you have any
suggestions.

In general folks may not need to use earlyprintk=dbpg,keep too often,
but it sure is nice when it works because you can use it as a full
console monitoring device after the /sbin/init handoff, including
debugging USB printk's because the usb debug controller driver is
completely separate.


Jason.