| Message ID | 20160822193834.11624-1-john.tobias.ph@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Lukasz Majewski |
| Headers | show |
Adding Marek on Cc On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote: > When the host machine went to suspend mode and then wake it up, it send > a USB_REQ_GET_STATUS. The existing case condition below, never > become true and it causes the host machine to reset the connection. Once, > it reset you will see an error "Disk Not Ejected Properly". > > case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): > > By applying this patch, the device could respond to the USB_REQ_GET_STATUS > command correctly. > --- > drivers/usb/gadget/ci_udc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c > index d36bcf6..fdec613 100644 > --- a/drivers/usb/gadget/ci_udc.c > +++ b/drivers/usb/gadget/ci_udc.c > @@ -703,8 +703,8 @@ static void handle_setup(void) > list_del_init(&ci_req->queue); > ci_ep->req_primed = false; > > - switch (SETUP(r.bRequestType, r.bRequest)) { > - case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE): > + switch (r.bRequest) { > + case USB_REQ_CLEAR_FEATURE: > _num = r.wIndex & 15; > _in = !!(r.wIndex & 0x80); > > @@ -729,7 +729,7 @@ static void handle_setup(void) > } > return; > > - case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS): > + case USB_REQ_SET_ADDRESS: > /* > * write address delayed (will take effect > * after the next IN txn) > @@ -739,7 +739,7 @@ static void handle_setup(void) > usb_ep_queue(controller.gadget.ep0, req, 0); > return; > > - case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): > + case USB_REQ_GET_STATUS: > req->length = 2; > buf = (char *)req->buf; > buf[0] = 1 << USB_DEVICE_SELF_POWERED; > -- > 2.9.3 >
On 08/24/2016 12:49 AM, Fabio Estevam wrote: > Adding Marek on Cc Thanks > On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote: >> When the host machine went to suspend mode and then wake it up, it send >> a USB_REQ_GET_STATUS. Can you expand on this ? It is not clear what the conditions to replicate this are. Please do so in detail. >> The existing case condition below, never >> become true and it causes the host machine to reset the connection. Once, >> it reset you will see an error "Disk Not Ejected Properly". >> >> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >> >> By applying this patch, the device could respond to the USB_REQ_GET_STATUS >> command correctly. >> --- >> drivers/usb/gadget/ci_udc.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c >> index d36bcf6..fdec613 100644 >> --- a/drivers/usb/gadget/ci_udc.c >> +++ b/drivers/usb/gadget/ci_udc.c >> @@ -703,8 +703,8 @@ static void handle_setup(void) >> list_del_init(&ci_req->queue); >> ci_ep->req_primed = false; >> >> - switch (SETUP(r.bRequestType, r.bRequest)) { >> - case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE): >> + switch (r.bRequest) { >> + case USB_REQ_CLEAR_FEATURE: >> _num = r.wIndex & 15; >> _in = !!(r.wIndex & 0x80); >> >> @@ -729,7 +729,7 @@ static void handle_setup(void) >> } >> return; >> >> - case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS): >> + case USB_REQ_SET_ADDRESS: >> /* >> * write address delayed (will take effect >> * after the next IN txn) >> @@ -739,7 +739,7 @@ static void handle_setup(void) >> usb_ep_queue(controller.gadget.ep0, req, 0); >> return; >> >> - case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >> + case USB_REQ_GET_STATUS: >> req->length = 2; >> buf = (char *)req->buf; >> buf[0] = 1 << USB_DEVICE_SELF_POWERED; >> -- >> 2.9.3 >>
Hi Marek, On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote: > On 08/24/2016 12:49 AM, Fabio Estevam wrote: >> Adding Marek on Cc > > Thanks > >> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote: >>> When the host machine went to suspend mode and then wake it up, it send >>> a USB_REQ_GET_STATUS. > > Can you expand on this ? It is not clear what the conditions to > replicate this are. Please do so in detail. For MacOS Yosemite: Connect your board into your host machine and run UMS. Once, the icon pop-up on your host machine (MacOS), put it in sleep mode (by clicking sleep in the menu under apple icon on the upper left corner of the screen). Then, once, you're sure that the host machine is in sleep mode, just wake it up. 1.0 If you load the uboot without the said patch, you should see a message "Disk Not Ejected Properly", the icon disappeared and the drive is no longer available. 1.1 If you load the uboot with the said patch, you should be able to access the drive after the host woke-up. For Windows (I've only tested in Windows 7): Similar procedures, after the icon pop-up on the screen, put the host machine in sleep mode (by clicking sleep under "Shut down" menu). The behavior should similar to 1.0 and 1.1. Btw, if you are testing it in Windows, connect your device directly into the USB port of your host machine, not in the HUB. > >>> The existing case condition below, never >>> become true and it causes the host machine to reset the connection. Once, >>> it reset you will see an error "Disk Not Ejected Properly". >>> >>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >>> >>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS >>> command correctly. >>> --- >>> drivers/usb/gadget/ci_udc.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c >>> index d36bcf6..fdec613 100644 >>> --- a/drivers/usb/gadget/ci_udc.c >>> +++ b/drivers/usb/gadget/ci_udc.c >>> @@ -703,8 +703,8 @@ static void handle_setup(void) >>> list_del_init(&ci_req->queue); >>> ci_ep->req_primed = false; >>> >>> - switch (SETUP(r.bRequestType, r.bRequest)) { >>> - case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE): >>> + switch (r.bRequest) { >>> + case USB_REQ_CLEAR_FEATURE: >>> _num = r.wIndex & 15; >>> _in = !!(r.wIndex & 0x80); >>> >>> @@ -729,7 +729,7 @@ static void handle_setup(void) >>> } >>> return; >>> >>> - case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS): >>> + case USB_REQ_SET_ADDRESS: >>> /* >>> * write address delayed (will take effect >>> * after the next IN txn) >>> @@ -739,7 +739,7 @@ static void handle_setup(void) >>> usb_ep_queue(controller.gadget.ep0, req, 0); >>> return; >>> >>> - case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >>> + case USB_REQ_GET_STATUS: >>> req->length = 2; >>> buf = (char *)req->buf; >>> buf[0] = 1 << USB_DEVICE_SELF_POWERED; >>> -- >>> 2.9.3 >>> > > > -- > Best regards, > Marek Vasut Regards, John Tobias
On 08/24/2016 03:30 AM, John Tobias wrote: > Hi Marek, Hi, > On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote: >> On 08/24/2016 12:49 AM, Fabio Estevam wrote: >>> Adding Marek on Cc >> >> Thanks >> >>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote: >>>> When the host machine went to suspend mode and then wake it up, it send >>>> a USB_REQ_GET_STATUS. >> >> Can you expand on this ? It is not clear what the conditions to >> replicate this are. Please do so in detail. > > > For MacOS Yosemite: > > Connect your board into your host machine and run UMS. Once, the icon > pop-up on your host machine (MacOS), put it in sleep mode (by clicking > sleep in the menu under apple icon on the upper left corner of the > screen). Then, once, you're sure that the host machine is in sleep > mode, just wake it up. > > 1.0 If you load the uboot without the said patch, you should see a > message "Disk Not Ejected Properly", the icon disappeared and the > drive is no longer available. > 1.1 If you load the uboot with the said patch, you should be able > to access the drive after the host woke-up. > > For Windows (I've only tested in Windows 7): > Similar procedures, after the icon pop-up on the screen, put the host > machine in sleep mode (by clicking sleep under "Shut down" menu). The > behavior should similar to 1.0 and 1.1. OK, I see, now it's clearer . This really should be in the commit message. > Btw, if you are testing it in Windows, connect your device directly > into the USB port of your host machine, not in the HUB. I think I'll have to trust you on all this. Do you know which usb request arrives which throws the code off ? I don't think it's correct to remove the bRequestType checking altogether. Thanks >>>> The existing case condition below, never >>>> become true and it causes the host machine to reset the connection. Once, >>>> it reset you will see an error "Disk Not Ejected Properly". >>>> >>>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >>>> >>>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS >>>> command correctly. >>>> --- >>>> drivers/usb/gadget/ci_udc.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c >>>> index d36bcf6..fdec613 100644 >>>> --- a/drivers/usb/gadget/ci_udc.c >>>> +++ b/drivers/usb/gadget/ci_udc.c >>>> @@ -703,8 +703,8 @@ static void handle_setup(void) >>>> list_del_init(&ci_req->queue); >>>> ci_ep->req_primed = false; >>>> >>>> - switch (SETUP(r.bRequestType, r.bRequest)) { >>>> - case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE): >>>> + switch (r.bRequest) { >>>> + case USB_REQ_CLEAR_FEATURE: >>>> _num = r.wIndex & 15; >>>> _in = !!(r.wIndex & 0x80); >>>> >>>> @@ -729,7 +729,7 @@ static void handle_setup(void) >>>> } >>>> return; >>>> >>>> - case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS): >>>> + case USB_REQ_SET_ADDRESS: >>>> /* >>>> * write address delayed (will take effect >>>> * after the next IN txn) >>>> @@ -739,7 +739,7 @@ static void handle_setup(void) >>>> usb_ep_queue(controller.gadget.ep0, req, 0); >>>> return; >>>> >>>> - case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >>>> + case USB_REQ_GET_STATUS: >>>> req->length = 2; >>>> buf = (char *)req->buf; >>>> buf[0] = 1 << USB_DEVICE_SELF_POWERED; >>>> -- >>>> 2.9.3 >>>> >> >> >> -- >> Best regards, >> Marek Vasut > > Regards, > > John Tobias >
Hi Marek, On Tue, Aug 23, 2016 at 8:29 PM, Marek Vasut <marex@denx.de> wrote: > On 08/24/2016 03:30 AM, John Tobias wrote: >> Hi Marek, > > Hi, > >> On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote: >>> On 08/24/2016 12:49 AM, Fabio Estevam wrote: >>>> Adding Marek on Cc >>> >>> Thanks >>> >>>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote: >>>>> When the host machine went to suspend mode and then wake it up, it send >>>>> a USB_REQ_GET_STATUS. >>> >>> Can you expand on this ? It is not clear what the conditions to >>> replicate this are. Please do so in detail. >> >> >> For MacOS Yosemite: >> >> Connect your board into your host machine and run UMS. Once, the icon >> pop-up on your host machine (MacOS), put it in sleep mode (by clicking >> sleep in the menu under apple icon on the upper left corner of the >> screen). Then, once, you're sure that the host machine is in sleep >> mode, just wake it up. >> >> 1.0 If you load the uboot without the said patch, you should see a >> message "Disk Not Ejected Properly", the icon disappeared and the >> drive is no longer available. >> 1.1 If you load the uboot with the said patch, you should be able >> to access the drive after the host woke-up. >> >> For Windows (I've only tested in Windows 7): >> Similar procedures, after the icon pop-up on the screen, put the host >> machine in sleep mode (by clicking sleep under "Shut down" menu). The >> behavior should similar to 1.0 and 1.1. > > OK, I see, now it's clearer . This really should be in the commit message. I'll resend v2 to include it. > >> Btw, if you are testing it in Windows, connect your device directly >> into the USB port of your host machine, not in the HUB. > > I think I'll have to trust you on all this. > > Do you know which usb request arrives which throws the code off ? USB_REQ_GET_STATUS. I tried to following combinations below but, the return SETUP(r.bRequestType, r.bRequest) doesn't match at all. SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS) SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS) SETUP(USB_DIR_IN, USB_REQ_GET_STATUS) > I don't think it's correct to remove the bRequestType checking altogether. Looking at the fsl_udc_core.c http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405 looks identical. > > Thanks > >>>>> The existing case condition below, never >>>>> become true and it causes the host machine to reset the connection. Once, >>>>> it reset you will see an error "Disk Not Ejected Properly". >>>>> >>>>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >>>>> >>>>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS >>>>> command correctly. >>>>> --- >>>>> drivers/usb/gadget/ci_udc.c | 8 ++++---- >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c >>>>> index d36bcf6..fdec613 100644 >>>>> --- a/drivers/usb/gadget/ci_udc.c >>>>> +++ b/drivers/usb/gadget/ci_udc.c >>>>> @@ -703,8 +703,8 @@ static void handle_setup(void) >>>>> list_del_init(&ci_req->queue); >>>>> ci_ep->req_primed = false; >>>>> >>>>> - switch (SETUP(r.bRequestType, r.bRequest)) { >>>>> - case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE): >>>>> + switch (r.bRequest) { >>>>> + case USB_REQ_CLEAR_FEATURE: >>>>> _num = r.wIndex & 15; >>>>> _in = !!(r.wIndex & 0x80); >>>>> >>>>> @@ -729,7 +729,7 @@ static void handle_setup(void) >>>>> } >>>>> return; >>>>> >>>>> - case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS): >>>>> + case USB_REQ_SET_ADDRESS: >>>>> /* >>>>> * write address delayed (will take effect >>>>> * after the next IN txn) >>>>> @@ -739,7 +739,7 @@ static void handle_setup(void) >>>>> usb_ep_queue(controller.gadget.ep0, req, 0); >>>>> return; >>>>> >>>>> - case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): >>>>> + case USB_REQ_GET_STATUS: >>>>> req->length = 2; >>>>> buf = (char *)req->buf; >>>>> buf[0] = 1 << USB_DEVICE_SELF_POWERED; >>>>> -- >>>>> 2.9.3 >>>>> >>> >>> >>> -- >>> Best regards, >>> Marek Vasut >> >> Regards, >> >> John Tobias >> > > > -- > Best regards, > Marek Vasut Regards, John Tobias
On 08/24/2016 07:25 AM, John Tobias wrote: > Hi Marek, Hi! > On Tue, Aug 23, 2016 at 8:29 PM, Marek Vasut <marex@denx.de> wrote: >> On 08/24/2016 03:30 AM, John Tobias wrote: >>> Hi Marek, >> >> Hi, >> >>> On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote: >>>> On 08/24/2016 12:49 AM, Fabio Estevam wrote: >>>>> Adding Marek on Cc >>>> >>>> Thanks >>>> >>>>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote: >>>>>> When the host machine went to suspend mode and then wake it up, it send >>>>>> a USB_REQ_GET_STATUS. >>>> >>>> Can you expand on this ? It is not clear what the conditions to >>>> replicate this are. Please do so in detail. >>> >>> >>> For MacOS Yosemite: >>> >>> Connect your board into your host machine and run UMS. Once, the icon >>> pop-up on your host machine (MacOS), put it in sleep mode (by clicking >>> sleep in the menu under apple icon on the upper left corner of the >>> screen). Then, once, you're sure that the host machine is in sleep >>> mode, just wake it up. >>> >>> 1.0 If you load the uboot without the said patch, you should see a >>> message "Disk Not Ejected Properly", the icon disappeared and the >>> drive is no longer available. >>> 1.1 If you load the uboot with the said patch, you should be able >>> to access the drive after the host woke-up. >>> >>> For Windows (I've only tested in Windows 7): >>> Similar procedures, after the icon pop-up on the screen, put the host >>> machine in sleep mode (by clicking sleep under "Shut down" menu). The >>> behavior should similar to 1.0 and 1.1. >> >> OK, I see, now it's clearer . This really should be in the commit message. > > I'll resend v2 to include it. Thanks, but please hold on a bit before we finish this discussion. >>> Btw, if you are testing it in Windows, connect your device directly >>> into the USB port of your host machine, not in the HUB. >> >> I think I'll have to trust you on all this. >> >> Do you know which usb request arrives which throws the code off ? > > USB_REQ_GET_STATUS. > > I tried to following combinations below but, the return > SETUP(r.bRequestType, r.bRequest) doesn't match at all. > > SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS) > SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS) > SETUP(USB_DIR_IN, USB_REQ_GET_STATUS) So you get bRequest = USB_REQ_GET_STATUS request with bRequestType set to what exactly to trigger this bug ? >> I don't think it's correct to remove the bRequestType checking altogether. > > Looking at the fsl_udc_core.c > http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405 > looks identical. That doesn't imply it's correct . [...]
Hi John, On Wed, Aug 24, 2016 at 2:25 AM, John Tobias <john.tobias.ph@gmail.com> wrote: > I tried to following combinations below but, the return > SETUP(r.bRequestType, r.bRequest) doesn't match at all. > > SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS) > SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS) > SETUP(USB_DIR_IN, USB_REQ_GET_STATUS) > >> I don't think it's correct to remove the bRequestType checking altogether. > > Looking at the fsl_udc_core.c > http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405 > looks identical. This is the old udc driver for the non-dt imx. Currently we use drivers/usb/chipidea/udc.c, which also seems to match what you do in your patch. Regards, Fabio Estevam
Hi Fabio, On Thu, Sep 8, 2016 at 4:05 AM, Fabio Estevam <festevam@gmail.com> wrote: > Hi John, > > On Wed, Aug 24, 2016 at 2:25 AM, John Tobias <john.tobias.ph@gmail.com> wrote: > >> I tried to following combinations below but, the return >> SETUP(r.bRequestType, r.bRequest) doesn't match at all. >> >> SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS) >> SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS) >> SETUP(USB_DIR_IN, USB_REQ_GET_STATUS) >> >>> I don't think it's correct to remove the bRequestType checking altogether. >> >> Looking at the fsl_udc_core.c >> http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405 >> looks identical. > > This is the old udc driver for the non-dt imx. > > Currently we use drivers/usb/chipidea/udc.c, which also seems to match > what you do in your patch. > > Regards, > > Fabio Estevam Thanks for the info and clarification. Btw, any updates with this patch?. Regards, John
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index d36bcf6..fdec613 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -703,8 +703,8 @@ static void handle_setup(void) list_del_init(&ci_req->queue); ci_ep->req_primed = false; - switch (SETUP(r.bRequestType, r.bRequest)) { - case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE): + switch (r.bRequest) { + case USB_REQ_CLEAR_FEATURE: _num = r.wIndex & 15; _in = !!(r.wIndex & 0x80); @@ -729,7 +729,7 @@ static void handle_setup(void) } return; - case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS): + case USB_REQ_SET_ADDRESS: /* * write address delayed (will take effect * after the next IN txn) @@ -739,7 +739,7 @@ static void handle_setup(void) usb_ep_queue(controller.gadget.ep0, req, 0); return; - case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS): + case USB_REQ_GET_STATUS: req->length = 2; buf = (char *)req->buf; buf[0] = 1 << USB_DEVICE_SELF_POWERED;