Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 07:43, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@xxxxxxxxxxxxxxxxx> writes:
>>>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>>>  
>>>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>>>> bmAttributes parameter");
>>>>>>>> +
>>>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>>>> Descriptor's bMaxPower parameter");
>>>>>>>
>>>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>>>> to
>>>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>>>
>>>>>> Yes I always run checkpatch :)
>>>>>
>>>>> do you really ? Why do you have a 98-character line, then ?
> 
> btw, you didn't reply this ^^
> 
>>>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>>>  	.label		= "MIDI Gadget",
>>>>>>>>  	.bConfigurationValue = 1,
>>>>>>>>  	/* .iConfiguration = DYNAMIC */
>>>>>>>> -	.bmAttributes	= USB_CONFIG_ATT_ONE,
>>>>>>>
>>>>>>> nack, nackety nack nack nack :-)
>>>>>>>
>>>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>>>> give users the oportunity to violate USB spec.
>>>>>>
>>>>>> You are right. It needs to check the value before it assigns to
>>>>>> bmAttributes.
>>>>>
>>>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>>>> case, I'm not convinced this is necessary at all.
>>>>
>>>> Right.
>>>>
>>>> This is necessary because this driver is actually wrong in which is
>>>> asking for the host to power itself. This is not specified on USB-MIDI
>>>> specification, neither makes any sense since this configuration is
>>>> device specific.
>>>>
>>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>>> I really don't know what is the best solution if this is not something
>>>> you like it.
>>>
>>> well, you could use our configfs-based gadget interface. You don't
>>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>>> static modules and rely only on configfs. If configfs doesn't let you
>>> change what you want/need, then we can talk about adding support for
>>> those.
>>>
>>> bMaxPower and bmAttributes sound like good things to have configurable
>>> over configfs but beware of what the USB specification says about them,
>>> we cannot let users violate the spec by passing bogus values on these
>>> fields.
>>
>> I agree that we should move to configfs, but the truth is that these
>> legacy devices are still useful. They just do one thing, mostly, but
> 
> yes, they are useful as they are. They don't need to be changed to be
> useful. Plus, you can have a gadget built with configfs that does only
> one thing. And you can do that with a simple shell script.
> 
>> its easy and simple to setup and use. So I think before we have some
> 
> so is configfs.
> 
>> sort of preset library of configfs-based gadget drivers, we still need
>> these modules.
> 
> there is already a library called libusbg.

By preset library I meant scripts or little programs that implement the
legacy drivers we have.

> 
>> Any suggestions on that?
>>
>> Do you want to support what I am proposing for gmidi.ko or just ignore
>> it and move on to configfs?
> 
> I prefer to not touch these gadget drivers if at all necessary. If you
> fixing a bug, then sure we must fix bugs. But you're not fixing a bug
> and, on top of that, you're adding regressions and violating the USB
> spec. This shows that you're writing these patches without knowing
> (and/or even caring about) the specification at all.

Yes, I see your point. My mistake was to not to enforce the first bit to
be set enabling the user to break the USB spec. I didn't think of that
scenario. And that's why it's always useful to have kernel maintainers
and others to provide such insights. :)

Anyway, I see that this patch is not useful even if corrected.

> 
> Here's some enlightening presentation you probably wanna watch:
> 
> https://www.youtube.com/watch?v=fMeH7wqOwXA
> 
> TL;DR : this project is large and you need to convince me we need your
> code/patch.
> 

Felipe

Attachment: 0x92698E6A.asc
Description: application/pgp-keys