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

[Pantelis Antoniou <panto@antoniou-consulting.com>]

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@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. 

More users == less bugs.

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

> 
>> +		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