Thanks for the responses everyone

> IMHO, considering the amount of Android users, we can merge this into
> composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
> something along the lines of

I suspect that once you see what this entails you won't be so willing :)

The code for handling accessory control requests is heavily tied into
f_accessory itself.
See acc_ctrlrequest in
https://android.googlesource.com/kernel/common/+/android-4.14/drivers/usb/gadget/function/f_accessory.c
We have configfs handle the requests in android_setup
https://android.googlesource.com/kernel/common/+/android-4.14/drivers/usb/gadget/configfs.c
The three requests we care about are ACCESSORY_START, which sends a uevent
signal, ACCESSORY_SEND_STRING, which stores strings that an ioctl later
retrieves, and ACCESSORY_GET_PROTOCOL, where the device must respond with
its protocol version. We'd probably want to change these communication
methods into configfs attrs under accessory, which means taking all the
other pieces of f_accessory as well. This is kind of weird since the
function itself minus control requests is quite easy to change to
functionfs. It may take more effort to tidy this up than to implement it
generically.

> If I understand correctly the purpose of creating a dedicated
configuration
> is to allow linking the functions to it and this way marking these
functions
> "special" (not assigned to a configuration proper).

yep

> While I understand that engineers should discuss facts rather than
feelings,
> the idea of creating a special configuration doesn't feel right to me,
> because this way you are creating a configuration which is not meant to be
> ever selected by the host. I can be convinced with arguments, though.

The configuration is not just never meant to be selected, the host doesn't
even see it in descriptors since it is not in cdev. Using a configuration
is convenient because the config group already exists and bind() already
takes in a configuration. Otherwise I might store a list of the functions
directly in gadget_info instead.

> I would like to draw your attention to one fact: a functionfs instance
> directory in configfs is intentionally empty, since the functionfs
delegates
> actual implementation, including descriptors and strings, to userspace.
> I would advise to consider (or at least prove wrong) the idea of adding
some
> attribute (file) to functionfs's configfs directory and using it for
marking
> this functionfs instance as special. This would require implementing some
> exclusion mechanism (if this ffs instance is linked to a configuration,
> it is impossible to mark it as special and vice versa).

I think the reason functionfs config attrs are empty is because we already
have several ways to pass in arguments to functionfs (mount args,
descriptor flags) that are already being used.

I'm not sure that it's feasible (without changing an interface somewhere)
to enable this functionality directly within functionfs. The issue is in
order to handle setups a function has to be alloced and bound to a
configuration with an underlying cdev. It's theoretically possible to just
bind to a cdev, but then you'd need to create new methods in struct
usb_function. Also, I think having a list of functions could be useful -- a
vendor could have a kernel function handling some control requests, and a
functionfs handling all the rest. In that case, they'd just link the
functions to control config in order.

> Let me just mention that if we had a generic interface we could cover
> not only android requirements but also do a great job for people who are
> fuzzing USB hosts. Such an interface would allow to easily craft some
> random requests without all this noise related to using GadgetFS...

My main intention is to handle vendor specific requests (compliant but not
standard). This is kind of interesting though and it would not be that hard
to allow -- have a bool flag attr control_config_priority and if that is
set, give setups to functionfs *before* composite.

I can provide patches in either case. I've already started experimenting

Thanks,
Jerry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to