RE: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver.

[Pawel Laszczak]

H Greg
>
>On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
>> to driver/usb/common/debug.c file. These moved functions include:
>>     dwc3_decode_get_status
>>     dwc3_decode_set_clear_feature
>>     dwc3_decode_set_address
>>     dwc3_decode_get_set_descriptor
>>     dwc3_decode_get_configuration
>>     dwc3_decode_set_configuration
>>     dwc3_decode_get_intf
>>     dwc3_decode_set_intf
>>     dwc3_decode_synch_frame
>>     dwc3_decode_set_sel
>>     dwc3_decode_set_isoch_delay
>>     dwc3_decode_ctrl
>>
>> These functions are used also in inroduced cdns3 driver.
>>
>> All functions prefixes were changed from dwc3 to usb.
>
>Ick, why?

Because CDNS3 driver in one of the previous version had implemented very similar function as dwc3 and Felipe suggested that we should 
make common file with these functions. He also suggested that this function also could be used on host side. 
It was the reason why I've took function from DWC driver and put it in separate file in usb/common directory.
I change only the prefix to make this function more common.  

>
>> Also, function's parameters has been extended according to the name
>> of fields in standard SETUP packet.
>> Additionally, patch adds usb_decode_ctrl function to
>> include/linux/usb/ch9.h file.
>
>Why ch9.h?  It's not something that is specified in the spec, it's a
>usb-specific thing :)
Why not ?

Similar as usb_state_string function from include/linux/usb/ch9.h which 
" Returns human readable name for the state", the usb_decode_ctrl function 
make the same but for standard USB request. 

>
>Also, the api for that function is not ok.  If you are going to make
>this something that the whole kernel can call, you have to fix it up:
>
>> +/**
>> + * usb_decode_ctrl - Returns human readable representation of control request.
>> + * @str: buffer to return a human-readable representation of control request.
>> + *       This buffer should have about 200 bytes.
>
>"about 200 bytes" is not very specific.
>
>Pass in the length so we know we don't overflow it.

I didn't want to change to much this code, because it's not my code. 
I'm not sure if I should ?

>
>> + * @bRequestType: matches the USB bmRequestType field
>> + * @bRequest: matches the USB bRequest field
>> + * @wValue: matches the USB wValue field (CPU byte order)
>> + * @wIndex: matches the USB wIndex field (CPU byte order)
>> + * @wLength: matches the USB wLength field (CPU byte order)
>> + *
>> + * Function returns decoded, formatted and human-readable description of
>> + * control request packet.
>> + *
>> + * Important: wValue, wIndex, wLength parameters before invoking this function
>> + * should be processed by le16_to_cpu macro.
>> + */
>> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
>> +			    __u16 wValue,  __u16 wIndex, __u16 wLength);
>
>Why are you returning a value, isn't the data stored in str?  Why not
>just return an int saying if the call succeeded or not?

Currently this function is called only from trace point, and probably it will be use only
In this way. 
If function prototype looks like above, we can simply call it in following way:
	TP_printk("%s", usb_decode_ctrl(__get_str(str), __entry->bRequestType,
					__entry->bRequest, __entry->wValue,
					__entry->wIndex, __entry->wLength)


Thanks 
Pawel laszczak