On Thu, May 19, 2016 at 10:53:04AM -0700, Guenter Roeck wrote: > 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). We are missing the notifications that are needed in these cases. But I don't see much more we can do about those cases. We can not put any policies in place at this level, because we have to be able to support also things like USB PD and Type-C controllers that take care of all that, leaving us to not be able to do anything else but to pass the information forward. So the framework at this level has to be "stupid", and if more infrastructure is needed, it has to be introduced in an other layer. > 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. Well, yes we need to document these things, but you are now coupling this framework with USB PD and we really should not do that. The policy engine, and the whole USB PD stack, belongs inside the kernel, and it will be completely separated from this framework. This framework can not have any dependencies on the future USB PD stack. This is not only because of the USB PD/Type-C controllers which handle the policy engine on their own and only allow "unsolicited" requests like "swap role" and "enter/exit mode", but also because this framework must work smoothly on systems that don't want to use USB PD and of course also with USB Type-C PHYs that simply don't include USB PD transceiver. The layer that joins these two parts together will be the port drivers themselves, so the USB Type-C/PD PHYs and controllers, at least in the beginning. Any initial decisions about which role or which alternate mode to select belongs to the stack. The userspace will need to be notified, and the userspace can then attempt to request changes after that, but if there is something that blocks the requests, the attempt has to just fail. So we can't provide any knowledge for the userspace about the requirements regarding the high level operations we allow the userspace to request (so in practice swap role/power and enter/exit mode). This information the userspace needs to get from somewhere else just live without it. Nor do we expect the userspace to be aware of the state of the system. So for example if the userspace attempts to activate a mode, the framework will just pass it forward to the port driver, which will then process it with the PD stack, and if for example the state is not PE_SRC_Ready or PE_SNK_Ready or whatever, the operation will just fail probable with -EBUSY in that case. And the knowledge about dependencies related to the alternate modes belong primarily to the alternate mode specific drivers in the end (once we have the bus) like I said. We can't expect the USB PD stack to be aware of those as they are alternate mode specific, and of course we can not trust that the userspace will always do things the right way. But the initial states after connection must be handle by the policy engine of course. > 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. Well, like I said above, this framework can not provide this infrastructure. The framework at this level really has to be "stupid". We simply can not do any decisions or have any expectations at this level. If more infra is needed, it has to be provided in an other layer on top of this bottom layer. So basically the driver have to implement those things for now. > 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 ? On disconnect, the port role is set according to the "fixed_role"? If the port is DFP only, then the port will still be host/source after disconnect. I don't see the problem here? > 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. This is a bug in the code indeed. > 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. We clearly need consensus on what this class will be responsible of. I've tried to explain how I see it above, hopefully with reasonable explanations. So basically, USB PD for this class is just a external feature that the alternate modes and power and vconn swapping depends on, that the class can not take any responsibility of IMHO. The UCSI spec defines an other layer on top of the USB PD stack that basically describes what the userspace interface that I'm trying to achieve with this class is. The "OS Policy". Thanks, -- heikki -- 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
