Message-ID: <20160520104703.GA12663@kuha.fi.intel.com>
Date:	Fri, 20 May 2016 13:47:03 +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 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.

> 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

Powered by blists - more mailing lists