Date:	Mon, 23 May 2016 12:23:04 +0300
From:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Oliver Neukum <oneukum@...e.com>,
	Rajaram R <rajaram.officemail@...il.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCHv2] usb: USB Type-C Connector Class

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@...ux.intel.com>
> > > > ---
> > > >  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

Powered by blists - more mailing lists