Hi Michael, On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote: > Hi Heikki, > > > -----Original Message----- > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > Sent: Thursday, February 7, 2019 5:16 AM > > To: Michael Hsu <mhsu@xxxxxxxxxx> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Ajay Gupta > > <ajayg@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx > > 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: > > > 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. > > > > 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. > > 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. > 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. > > > It has to react to it, most likely by assuming there was a fatal error somewhere. > > 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. 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. > 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. > 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. > > 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. Why would you need the two DP alt modes for the connector? Br, -- heikki
