On Fri, May 20, 2016 at 10:02:28AM -0700, Guenter Roeck wrote: > On Fri, May 20, 2016 at 01:47:03PM +0300, Heikki Krogerus wrote: > > 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. > > > Ok. > > > > 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. > > > Not really. I was trying to understand where you would expect the policy engine > to reside, which you answered above. Ah OK, got it. Sorry. > > 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. > > > Makes sense. > > > 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". > > Ok. > > > 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. > > > I still think that the lack of synchronization is inherently racy. > For example, on a power role swap request, port->pwr_role can change > (via the currently missing notification) after it was evaluated in > current_power_role_store(), but before port->cap->pr_swap() is called. > The lower level code has at this point no means to know which power role > change was requested, which may result in the power role being swapped > again even though it already is in the requested state. You are correct. We need to pass the requested role to the drivers. > > > 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? > > > Roles are set differently on port registration vs. disconnect. > This means that roles (can) differ between "initial disconnect state" > and "disconnect state after connect". On port registration, usb_role, > pwr_role, and vconn_role are all set based on the current pwr_opmode. > During port registration, they are all initialized with 0. Got it. I'll fix that. > > > 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. > > > Sounds good to me, as long as the lower level code can inform the class > about state/role changes. > > > 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". > > > Since you mention UCSI - in UCSI, the two API functions available to set > the power role (Set Power Direction Mode, Set Power Direction Mode) > both provide the desired role to the connector driver. How does this map > to the API in the class code, where a power swap is requested without > telling the low level code about the desired role ? > > Personally I prefer the approach used in UCSI, or let's say what I perceive > the approach to be: Maybe the class code should just send a request to the > connector driver to change the role to X, independent of the current role. > This way, most if not all synchronization problems could be handled by the > lower level driver. That sounds good to me. And it fits to the plan that the class does not make any decisions. > On a side note, I don't see how to request a vconn swap with UCSI. > Is that supported ? No, with UCSI we can not support vconn swap. Originally I was not even proposing an attribute for vconn swap in v1, but there was demand for it. 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
