Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Guenter Roeck]

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

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.

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.

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 ?

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.

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.

Thanks,
Guenter