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

Hi Heikki,

-----Original Message-----
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Sent: Tuesday, February 12, 2019 7:22 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,

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

quoted

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.

quoted

quoted

quoted

quoted

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?

quoted

quoted

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

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.

quoted

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

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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

We can't change our PPM firmware since we have too many devices out their already.

But, we'll be submitting future patches on top of yours which address the objections you've mentioned already in this thread.

quoted

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.

quoted

quoted

quoted

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.

quoted

quoted

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.

Agreed.  Please update your patch for the mode index issue (being able to
reliably match partner mode index with the connector mode index returned by
UCSI).

quoted

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.

OK, but this means the "active" sysfs node for a partner mode (which is not
supported by GET_SUPPORTED_CAM) will always show "no".  And writing "yes"
to it will silently fail...

Did this intentional?  Having some partner sysfs "active" nodes which do nothing without any warning to user that they don't work?

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.

OK, we can come up with some method of interfacing our PPM firmware's mode
table with your patch's expectation (of only one alt mode index per DP alt mode).

Let's treat that independently from your set of patches.

quoted

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

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.

quoted

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,

--
heikki

Thanks,
Michael

nvpublic