Hello Heikki, On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote: > The purpose of this class is to provide unified interface for user > space to get the status and basic information about USB Type-C > Connectors in the system, control data role swapping, and when USB PD > is available, also power role swapping and Alternate Modes. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > --- > drivers/usb/Kconfig | 2 + > drivers/usb/Makefile | 2 + > drivers/usb/type-c/Kconfig | 7 + > drivers/usb/type-c/Makefile | 1 + > drivers/usb/type-c/typec.c | 957 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb/typec.h | 230 +++++++++++ > 6 files changed, 1199 insertions(+) > create mode 100644 drivers/usb/type-c/Kconfig > create mode 100644 drivers/usb/type-c/Makefile > create mode 100644 drivers/usb/type-c/typec.c > create mode 100644 include/linux/usb/typec.h > > Hi, > > Like I've told some of you guys, I'm trying to implement a bus for > the Alternate Modes, but I'm still nowhere near finished with that > one, so let's just get the class ready now. The altmode bus should in > any case not affect the userspace interface proposed in this patch. > > As you can see, the Alternate Modes are handled completely differently > compared to the original proposal. Every Alternate Mode will have > their own device instance (which will be then later bound to an > Alternate Mode specific driver once we have the bus), but also every > partner, cable and cable plug will have their own device instances > representing them. > > An other change is that the data role is now handled in two ways. > The current_data_role file will represent static mode of the port, and > it will use the names for the roles as they are defined in the spec: > DFP, UFP and DRP. This file should be used if the port needs to be > fixed to one specific role with DRP ports. So this approach will > replace the suggestions for "preferred" data role we had. The > current_usb_data_role will use values "host" and "device" and it will > be used for data role swapping when already connected. > What I am missing completely is a means to handle role and alternate mode changes triggered by the partner. The need for those should be obvious, unless I am really missing something (just consider two devices supporting this code connected to each other). Also, I am not sure where the policy engine is supposed to reside. I understand that some policy changes (eg unsolicited requests to switch roles) can be triggered from user space. However, role change requests triggered from the partner need to be evaluated quickly (typically within 15 ms), so user space can not get involved. Maybe it would help to have some text describing where the policy engine is expected to reside and how it is involved in the decision making process. This includes the initial decision making process, when it needs to be decided if role changes should be requested or if one or multiple alternate modes should be entered after the initial connection has been established. On top of that, I am concerned about synchronization problems with role changes triggered from user space. The driver is not told about the desired role, only that it shall perform a role change. If a role change triggered by the partner is ongoing at the time a role change request is made from user space, it may well happen that the dual role change results in a revert to the original role. Some synchronization primitives as well as an API change might resolve that, but essentially it would mean that drivers have to implement at least part of the policy engine. It might make more sense to have that code in the infrastructure. On disconnect, a port reverts to the default role. However, on port registration, the port roles are left at 0, meaning they always default to device/sink independent of the port's capabilities. Is this on purpose ? current_data_role_store() lets user space set a fixed port role. However, the port role variables are not updated. How is this supposed to be handled ? The same is true for other role change attributes - I don't see any code to update the role variables. Presumably this would have to be done in the class code, since the port data structure is private. Overall, I am quite concerned by the lack of synchronization primitives between the class code and port drivers, but also in the class code itself. For example, nothing prevents multiple user space processes from writing into the same (or different) attribute(s) repeatedly. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
