[U-Boot] [PATCH v3 6/9] dfu: Send correct DFU response from composite_setup

[Lukasz Majewski]

Pantelis,

> Hi Lukasz,
> 
> On Dec 10, 2012, at 7:11 PM, Lukasz Majewski wrote:
> 
> > Hi Pantelis,
> > 
> >> DFU is a bit peculiar. It needs to hook to composite setup and
> >> return it's function descriptor.
> >> 
> >> So when get-descriptor request comes with a type of DFU_DT_FUNC
> >> we iterate over the configs, and functions, and when we find
> >> the DFU function we call the setup method which is prepared
> >> to return the appropriate function descriptor.
> > 
> > Sorry, but could you be more informative here? Have you had any non
> > standard problems? I wonder why dfu-util on my linux works OK
> > without this patch?
> > 
> 
> I have absolutely no idea why it works at your side.
> 
> At our side it just didn't work at all without the patches.
> 
> If I had to guess maybe your gadget h/w takes care of replying
> properly for the DFU case.
> 
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto at antoniou-consulting.com>
> >> ---
> >> drivers/usb/gadget/composite.c | 27 +++++++++++++++++++++++++++
> >> drivers/usb/gadget/ep0.c       |  1 +
> >> drivers/usb/gadget/f_dfu.c     |  6 ++++--
> >> 3 files changed, 32 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/usb/gadget/composite.c
> >> b/drivers/usb/gadget/composite.c index ebb5131..6496436 100644
> >> --- a/drivers/usb/gadget/composite.c
> >> +++ b/drivers/usb/gadget/composite.c
> >> @@ -773,6 +773,33 @@ composite_setup(struct usb_gadget *gadget,
> >> const struct usb_ctrlrequest *ctrl) if (value >= 0)
> >> 				value = min(w_length, (u16) value);
> >> 			break;
> >> +
> >> +#ifdef CONFIG_DFU_FUNCTION
> >> +		/* DFU is mighty weird */
> > 				^^^^^^ - please explain this
> > 				"wiredness". 
> > 
> > I don't recall such a hacks in linux kernel composite.c (any special
> > #ifdef). Am I missing something important in DFU?
> > 
> > 
> > Does your device have any special requirement, so you need this
> > hack?
> > 
> > I generally don't like the idea to "patch" composite gadget code
> > with #ifdefs for special function. Please convince me.
> 
> It doesn't work otherwise. I have no idea why you think I would be
> hacking around there if the thing worked at all. And trust me on
> that, it just doesn't without those patches, not to mention the way
> it unceremoniously blows up if you transfer anything larger than the
> buffer set aside originally.
> 
> The way I see it, instead of complaining you should be rejoicing
> since now DFU will be used in an actual production environment. 

I'm not complaining. I try to resolve the problem, since this can make
dfu support better at u-boot. 


Moreover I'm aware that USB is tricky, so I want to understand your
problem.

Tomorrow I will prepare output of USB Ellisys analizer on my side, so we
could get clue what is going on. 	

> 
> More users == less bugs.

I'm really happy, that you have posted patches for NAND flashing. 

Without your determination at debugging, problem with buffer overflow
wouldn't be discovered.

We "only" needs to share knowledge and provide solution acceptable for
community and us.

I'm open.

> 
> When I get a few free cycles I will post a tcpdump capture of the DFU
> USB transaction hanging.

Yes, please. 

> 
> > 
> >> +		case DFU_DT_FUNC:
> >> +			w_value &= 0xff;
> >> +			list_for_each_entry(c, &cdev->configs,
> >> list) {
> >> +				if (w_value != 0) {
> >> +					w_value--;
> >> +					continue;
> >> +				}
> >> +
> >> +				list_for_each_entry(f,
> >> &c->functions, list) { +
> >> +					/* DFU function only */
> >> +					if (strcmp(f->name,
> >> "dfu") != 0)
> >> +						continue;
> >> +
> >> +					value = f->setup(f, ctrl);
> >> +					goto dfu_func_done;
> >> +				}
> >> +			}
> >> +dfu_func_done:
> >> +			if (value >= 0)
> >> +				value = min(w_length, (u16)
> >> value);
> >> +			break;
> >> +#endif
> >> +
> >> 		default:
> >> 			goto unknown;
> >> 		}
> >> diff --git a/drivers/usb/gadget/ep0.c b/drivers/usb/gadget/ep0.c
> >> index aa8f916..971d846 100644
> >> --- a/drivers/usb/gadget/ep0.c
> >> +++ b/drivers/usb/gadget/ep0.c
> >> @@ -221,6 +221,7 @@ static int ep0_get_descriptor (struct
> >> usb_device_instance *device, break;
> >> 
> >> 	case USB_DESCRIPTOR_TYPE_CONFIGURATION:
> >> +	case USB_DESCRIPTOR_TYPE_OTHER_SPEED_CONFIGURATION:
> > 				^^^^^^^^^^^^^- why do you need that?
> >> 		{
> >> 			struct usb_configuration_descriptor
> >> 				*configuration_descriptor;
> >> diff --git a/drivers/usb/gadget/f_dfu.c
> >> b/drivers/usb/gadget/f_dfu.c index 10547e3..6494f5e 100644
> >> --- a/drivers/usb/gadget/f_dfu.c
> >> +++ b/drivers/usb/gadget/f_dfu.c
> >> @@ -534,8 +534,10 @@ dfu_handle(struct usb_function *f, const
> >> struct usb_ctrlrequest *ctrl) value = min(len, (u16)
> >> sizeof(dfu_func)); memcpy(req->buf, &dfu_func, value);
> >> 		}
> >> -	} else /* DFU specific request */
> >> -		value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl,
> >> gadget, req);
> >> +		return value;
> >> +	}
> >> +
> >> +	value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl, gadget,
> >> req); 
> > 
> > Why do you change state even after receiving req_type ==
> > USB_TYPE_STANDARD? I would expect to change the dfu state only when
> > DFU specific request appears.
> > 
> > 
> 
> > 
> >> 	if (value >= 0) {
> >> 		req->length = value;
> > 
> > 
> > 
> > -- 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > Samsung Poland R&D Center | Linux Platform Group
> 
> Regards
> 
> -- Pantelis


Regards,
Lukasz Majewski