On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
I've updated my github branch with a commit where both of these issues
should be fixed. Can you give it a try?
This is going to be very difficult. So far, calls such as
typec_set_data_role() were independent (asynchronous) of role change
requests through sysfs, meaning they happened whenever lower level
confirmed that a role change was complete, for whatever reason. Now
I have to check if a role change request through the class code
is pending and not call typec_set_data_role() and friends in that case.
I'm sorry about the misunderstanding.
What you want is basically that we only support non-blocking mode with
this interface, and we can't do that.
Would you even be able to make it work? How would you report errors
for example?
This becomes even more complicated in situations with parallel role
change requests both from the class code and from the partner.
In such cases the class code may have acquired the lock, and before it
calls the driver, a role change complete is reported. The call to
typec_set_XXX_role() will then get stuck. So I'll have to add another
flag before calling typec_set_data_role() and friends, and then reject
the call to dr_set and all other set requests with -EBUSY.
race conditions.
Or we'll just export the port locks somehow, or provide separate API
for handling them.
..typec_get_port_lock(..
{
return &port->lock;
}
or
typec_lock(..
typec_test_and_lock(..
typec_unlock(..
...
Thinking more ... ultimately, this means that I'll have to reject role
change requests from the class code whenever the state machine is busy,
because I never know if the state machine activity would result in a
call into the typec class code.
Yes, why is that a problem? The other option would be that you queue
the requests from users, and I'm pretty sure we want to avoid that. It
would mean you have to consider a lot of conditions to avoid any
races, and you would also have to make a lot of extra decisions. As an
example, what do you do if two role change requests are in the queue
for the same role? If the first requests is successful, you probable
can just report success to the second request also, _maybe_! But if
the first one fails, do you try again for the second request, and in
that case do you wait for the result of the second request before
reporting to the first, or do you just fail both of them? etc.
Example:At the same time, the protocol driver
will have to reject incoming role change requests while a locally
requested role change request is pending, even if that internal
role change request has not yet resulted in a PD message being sent.
Why?
On the other side, not being able to rely on a well defined ABI makes theIs this really necessary ? It creates a lot of interdependencies.
In summary, this means a substantial amount of code and testing, which
is going to take a while.
You might want to mention all this in the API documentation.
OK.
I've also removed the supports_usb_power_delivery and id_header_vdo
attributes from partners and cables. We really have to put all the USB
PD specific attributes to its own group, and that group can't be in
control of this class. We will simply not always have access to the
kind of USB PD specific details you want to show, for example details
that you get from discover identity command, even when USB PD is fully
supported. For example on systems that use UCSI.
I think we should have a single unified ABI, independent of the underlying
driver, that informs the user about the partner device. I really don't
quite understand why the class code should not be able to report what device
it is connected to (while at the same time being able to report the alternate
modes it supports).
OK, let's plan this more then. Maybe we can make for example a layer
that creates the groups for the PD specific details to the class?
The problem is still that we can only provide results from for example
request identity command reliably when the PD protocol layer is
completely inside kernel, and that is not always the case. So we
really need to group those details in its own group, and that group
will basically indicate is the PD stack inside the kernel or not.
We should not forget also that the userspace can never rely on those
details because of the fact that they simply will not always be
available.