[4/5] usb: typec: ucsi: Preliminary support for alternate modes

Hi,

On Mon, Feb 11, 2019 at 11:43:17PM +0000, Michael Hsu wrote:

Hi Heikki,

quoted

-----Original Message-----
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Sent: Friday, February 8, 2019 5:48 AM
To: Michael Hsu <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta
[off-list ref]; linux-usb@vger.kernel.org
Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate
modes

Hi Michael,

On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote:

quoted

Hi Heikki,

quoted

-----Original Message-----
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Sent: Thursday, February 7, 2019 5:16 AM
To: Michael Hsu <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta
[off-list ref]; linux-usb@vger.kernel.org
Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for
alternate modes

Hi Michael,

On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote:

quoted

Your existing patch matches the SVID and then requires one-for-one
ordering in case either alt mode table has multiple mode entries
for a

particular SVID.

And we'll fix that, but that does not solve the issue that I'm
talking about. There are two separate issues here. That problem is a
bug in the driver, but the major problem still is that you have
separate alternate modes for the two DP pin assignments, and fixing the

index handling does not help with that.

quoted

quoted

According to the DP alt mode standard you do not have a separate
mode for every supported DṔ pin assignment. When you change the DP
alt mode pin assignment, the current mode is not exited and a new one

entered.

quoted

quoted

You stay in the DisplayPort mode, and simply re-configure using the
DisplayPort Configure command. In your implementation however, the
mode is existed, and a new one entered. So your implementation is
not made according to the DisplayPort Alt Mode standard.

I guess I have been able to explain just how big of a problem that
is, and also how exactly we will have to handle it... Let's look at
this from user space perspective.

The user space should not need to be aware of the method used to
interface with the USB Type-C connectors on the system. The kernel
needs to hide that and supply unified interface to the user space
which looks and behaves the same, _always_.

In case of the DP alt mode, the user space should be allowed to
expect the behaviour to follow the DP alt mode spec, so when the
user space selects another pin assignment for the DP alt mode
adapter, it should be a pass-fail operation without any side
effects, just like explained in the VDM flows in the DP alt mode spec.

But now with your PPM, there is a major side effect. Every time the
user space selects a new DP pin assignment, the current mode is
actually exited and a new mode entered. That is not standard
behaviour. The user space will not just ignore the mode being exited.

Not exactly true... If you look on a USB PD analyzer at the type c
connector's CC lines, you would *not* see and "Exit Mode" VDMs - just "DP

Configure" VDMs.

So your PPM does not actually reflect the behaviour of the hardware and
firmware. You are not actually changing the mode when you change the pin
assignment, yet you still want the operating system to think it has to
change the mode every time it changes the pin assignment.

That means the two modes you have are for completely customised software
(firmware) representation of the DisplayPort alt mode support that does not
follow the standard, and as we can now see, reflect even the actual
behaviour.

If your connector could act as the sink, how many DP alt modes would return
to the partner as a response to Discover Modes with DP SVID? You would
return only one regardless of how many pin assignments you support, right?
That's what you need to return to the OS as well, as source or sink.

quoted

The "active" sysfs node with the value of "yes" would change paths
(since another UCSI CAM index is active), but that's because there was
a UCSI connector status change.

quoted

It has to react to it, most likely by assuming there was a fatal error

somewhere.

quoted

quoted

So in practice the user space is now forced to handle your connector
as a special case.

The expected user behavior is to read every sysfs node of the form...
   /sys/class/typec/.../svid
   /sys/class/typec/.../vdo
And if it decides that's the alt mode it wants to activate, it writes 1 to
   /sys/class/typec/.../active
And optionally, reads above sysfs file to confirm that it did indeed
change to 'yes'.
It would treat it as an failure to enter alt mode if it was read as
still 'no'.

Do you understand that those steps you can take _only_ on your hardware in
order to change the DP pin assignment? They work only with your PPM. The
approach is completely custom.

I disagree, most user-mode applications would read the contents of the 'svid'
and 'vdo' sysfs nodes and then decide if they want to enter that alternate
mode - by writing to the corresponding 'active' node.

Now the user can read the vdo from DP alt mode, and always know that
it contains the DisplayPort capabilities, just like it should. If we
were to add support for your PPM as is, the user would loose guarantee
for that. The vdo would then mean something completely different with
your PPM in case of the connectors.

The user would then have to be able to, firstly identify the hardware,
and then of course know how that handle it.

Basically the user space would have to deal with your hardware
completely separately. And only with your hardware.

<snip>

quoted

The other UCSI PPMs handle this part correctly. There is always only one
DisplayPort alt mode for the connector, regardless of how many DP pin
assignment the connector support. Your PPM is the only exception.

In the absence of any express or implied prohibition on this implementation
(in ucsi, type c, or usb PD specifications), it would be better driver design
to support this (be flexible in what you accept as input, be strict in what
you output).

quoted

quoted

The unexpected behavior which you are talking about is if the user
uses the displayport pin assignment sysfs node to switch from pin C to pin
D.

Note that the sysfs attribute file for the DP pin assignment is the
_standard_ interface. That sysfs file is the interface the user space will
use for changing the DisplayPort pin assignment.

No problem... With the use of '&' masking between the UCSI connector alt mode
vdo with the UCSI partner alt mode vdo, they DP sysfs pin assignment nodes
would work.

What is a problem is requiring one for one mode index matching between the
UCSI connector alt mode table with the UCSI partner alt mode table.

According to DP over Type C spec, 
   "Future versions of this Standard may describe other modes associated with
   DP_SID. Such modes will be identified by having a non-zero value in bits
   31:24 of the VDO. The DFP_U shall examine the list of modes returned until
   it finds 0s in bits 31:24 of the VDO and a non-zero value in bits 23:0 of
   the VDO (i.e., DisplayPort capabilities). The DFP_U and UFP_U shall use the
   corresponding offset (indexed from 1) as the Object Position in the Enter
   Mode, DisplayPort Configure, DisplayPort Status Update, Attention, and Exit
   Mode commands."

Yes, we may have multiple modes defined in future versions of the DP
alt mode spec, but the connector, just like the partners attached to
it, will still have only one mode per definition.

Right now there is only one mode defined, but you still return two for
the connector.

Above requirement means that for SVID 0xff01, it is possible for multiple Mode
VDOs to be associated with it. Hence, the latest patch's algorithm (of
matching mode index will not work).

To illustrate:
    Alt mode adapter responds to Discover Modes VDM (for SVID 0xff01) with
    following:
    (a) 0x00000c05
             ==> this has svid 0xff01, partner mode index 1
    (b) 0x01xxxxxxx --> non-zero value in bits 31 to 24 indicate future DP alt
    mode
            ==> this has svid 0xff01, partner mode index 2
But GPU PPM supports only (b) hence its UCSI GET_ALTERNATE_MODES (recipient =
connector) returns only one connecter alt mode:
     (1) 0x01xxxxxx --> GPU supports only extended DP alt mode
               ==> this has svid 0xff01, connector mode index 1

The patch's algorithm of '==' comparison with mode index would yield the wrong
correlation between connector and partner alt modes.

Note how using '&' mask between the UCSI's connector mode VDO and UCSI's
partner mode VDO would work in all cases.

The current patch does have a bug, and it needs to be fixed, but that
does not fix the issue you have with the two DP modes for the
connector. We are talking about two separate issues here.

quoted

quoted

Then, a reviously active mode would turn to 'no' and another active
sysfs node would change to 'yes'.

But, user-mode application has to be able to expect this anyways. (This =
some active sysfs node changing even though the application did not
cause it.) For example, a TypeC-to-DP dongle is attached.  This dongle
is also a hub so a downstream USB superspeed device can be connected
to it.  When there is no downstream USB device attached, DisplayPort
Pin C (4 lanes of video) are supported.  But, user plugs in a USB
device to the dongle - then the dongle will switch from pin C to pin D
automatically to allow the USB superspeed lanes to be connected.  In
this case, I'd expect your displayport ABI to reflect the automatic
pin assignment change from C to D caused not by any sysfs activity,
but simply plugging in USB device to the typec alt mode adapter.  In
our particular case, in addition to the "pin" sysfs node changing from
"[C] D E" to "C [D] E", the "active" sysfs node with the "yes" value
would change from one path to another.  So, applications must expect
values of sysfs nodes under /sys/class/typec to change by itself.

We are not talking about just some sysfs files here. Every alternate mode
will have a kernel object representing them. When a change happens on those
objects, for example if a mode is entered or exited (regardless of was it as
a result of user action or not), or the DP pin assignment gets changed, the
kernel generates events for those objects. Those events are the primary
source of initial information in user space.

The problem is that changing the DP pin assignment has different meaning
than exiting and entering the modes. Again, changing the DP pin assignment
does not cause the mode to be exited and another to be entered. That is how
the standard defines it (and that is the reason why we have the separate
sysfs attribute file for the DP pin assignment in the first place), and
that's how the user space will expect the interface to behave.

With your PPM however the pin assignment is completely coupled with the
connector mode. If the pin assignment is changed, the mode gets changed, and
if the mode is changed, the pin assignment gets changed.
That is simply not the standard behaviour.

Keep in mind that the previous type c ABI did not have this restriction.
This new set of patches for creation of the displayport ABI implicitly added
this.

However, one of my previous suggestions would maintain this invariance (i.e.,
changing pin assignments would not cause alt mode change).
- basically, when PPM returns alt mode table with separate entries for each
mode VDO, the ucsi_ccg.c driver would compress the alt mode table before
presenting it to ucsi/displayport.c.

So, if this is your only objection, we can accept this.  Need to change struct
ucsi_ppm to add function pointers to translate ppm's alt mode table into the
format that ucsi/displayport.c required.

Note that you can't just mix that into the ucsi_ccg.c. You are not the
only one using the Cypress CCGx USB PD controllers. I think even we
use them on some of our platforms, but on our platform the PD
controller is usually attached to the embedded controller, so the
operating systems continues to access the PPM using the ACPI mailbox.

But something like that has to be done. I just realised that the
hardware is already being sold to the customers, so I don't think we
have a choice. The problem is that once the workaround is in, the
firmware will never ever be fixed :-(.

quoted

quoted

quoted

To protect the user space from that, and to keep the interface we
provide to it consistent and predictable, we would have to handle
your PPM completely separately. The user space will see only one
connector DP alt mode no matter what. Even if you are unable change
the PPM, the driver will still have to expose the two connector DP
alternate modes as one connector alternate mode to the user space.

If this is acceptable to you,

It really isn't, but it will be something that we have to do unless you guys
can fix your firmware. This will be a large quirk that we would have to
implement and then maintain, just to workaround yet another firmware issue.

Kernel is already full of workarounds like that, so I'm begging you, please
get the firmware fixed. Surely you now see that it does not exactly comply
with the DP alt mode standard.

This "workaround" was actually required by the following design "flaw" in the
current patch:
- you create partner sysfs nodes using UCSI GET_ALT_MODES (recipient == SOP)
- UCSI SET_NEW_CAM requires *connector* alt mode index
- problem: how to correlate *connector* alt mode index with *partner* alt mode
index in the absence of any device-independent means to do this in the UCSI
spec

I think for clarity's sake I better fix the mode index issue now and
send second version of these patches so we can concentrate on the
bigger problem, the two connector DP alt modes.

Here's a method of avoiding the above dilemma:
- create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with recipient
== connector)
- also query UCSI GET_SUPPORTED_CAM
- create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit for a
particular connector alt mode index

No! We must not hide stuff like that from the user. Even if the
connector does not support a partner mode, we still show it to the
user space.

This rule should apply to everything else. For example, even when the
connector does not support Audio Accessory devices, a partner device
should still pop up.

Nothing is preventing the user from comparing the connector and
partner and determine things from that on its own, for example if
audio accessory or an alternate mode is supported or not, but it means
the interface must follow the rules. No exceptions.

As a shortcut with the alternate modes, the user can determine if a
partner mode is supported by simply checking if it's linked to a
connector mode (there is a symlink pointing to the connector mode), or
alternatively by checking if the mode is bind to a driver.

You might consider it wrong to populate partner sysfs nodes with connector alt
modes, but it makes sense If you ask the question: why report partner alt
modes which are not supported at the connector side?  You'd never be able to
enter that partner alt mode anyways, unless both the connector and partner
mutually supported it.

We can not put user experience aside. The user space must know what
are the capabilities of the platform as well as the capabilities of
the devices attached to the platform, and what is going on in general.

One of the big problems with USB Type-C is that the user has no idea
what does his/her hardware support and not support. That we can not
fix, but we must do everything to make sure that it will at least
always be possible to supply information to the user.

If a connector does not support Audio Accessory, Thunderbolt, or
whatever, the user should always be informed by the operating system
that you just plugged this or that device and, sorry, but it's not
supported on this connector, whenever the user tries to attach one of
those devices.

Otherwise the user will simply assume that everything is working, the
OS did not say anything, and then start scratching his/her head... Why
isn't it working?

The Billboard Device Class does not help here as it only tells the
capabilities of the partner device.

quoted

Why would you need the two DP alt modes for the connector?

As mentioned earlier, this was due to limitation of UCSI which did not allow
querying of current DP pin assignment. We maintained compliance with UCSI, yet
overcame its limitations using this connector alt mode table arrangement.

If you hadn’t objected so strongly to this technique, we would've probably
recommended this technique to all our technical partners.

Maybe even asked Intel to bless this scheme when they publish the next UCSI
spec update.

Unfortunately the other players clearly interpreted the situation
differently.

I think spec update is needed even regardless of this issue. I'll see
if I can start a dialog first inside Intel regarding the limitations
and lack of clear definition in some parts of the current UCSI spec.


thanks,