Message-ID: <20160520170228.GA12632@roeck-us.net>
Date:	Fri, 20 May 2016 10:02:28 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
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 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.

> 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.

> > 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.

> > 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.

On a side note, I don't see how to request a vconn swap with UCSI.
Is that supported ?

Thanks,
Guenter

Powered by blists - more mailing lists