Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> On Fri, Jul 01, 2016 at 10:13:48AM +0300, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > > > +static ssize_t
> > > > +preferred_role_store(struct device *dev, struct device_attribute *attr,
> > > > +		     const char *buf, size_t size)
> > > > +{
> > > > +	struct typec_port *port = to_typec_port(dev);
> > > > +	enum typec_role role;
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&port->lock);
> > > > +
> > > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->cap->try_role) {
> > > > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> > > 
> > > With this, 'echo "sink" > preferred_role' no longer works because
> > > match_string() tries to match the entire string, including the newline
> > > generated by the echo command. One has to use "echo -n" instead.
> > > Is this on purpose ? It is quite unusual.
> > 
> > For some reason I though 'echo -n is expected. I guess I'm still
> > living in the past in the days when the kernel version was still 2.4.
> > It did not use to be so unusual, and there are still plenty of sysfs
> > attributes that do expect 'echo -n..'.
> > 
> > But I'll fix this.
> 
> <snip>
> 
> > > > +static ssize_t
> > > > +current_data_role_store(struct device *dev, struct device_attribute *attr,
> > > > +			const char *buf, size_t size)
> > > > +{
> > > > +	struct typec_port *port = to_typec_port(dev);
> > > > +	int ret = size;
> > > > +
> > > > +	mutex_lock(&port->lock);
> > > > +
> > > With this lock, the code in the driver can no longer call any state updates
> > > during the call to dr_set(), because those (eg typec_set_data_role()) set
> > > the lock as well. This means that the dr_set call and all other similar calls
> > > now have to be asynchronous. As a result, those calls can not return an error
> > > if the role change request fails. Is this on purpose ? I see it as a major
> > > drawback, not only because errors can no longer be returned, but also because
> > > I very much preferred the call to be synchronous.
> > 
> > But the drivers should not call typec_set_data_role() in these
> > cases? That API the drivers should only use if the partners execute
> > the swaps.
> > 
> > So in this case, we need to set the role and notify sysfs here. This
> > function has to return successfully only if the role swap really
> > happened, and dr_set() of course can not return before the driver
> > actually knows the result of, for example, DR_swap.
> > 
> > > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->cap->dr_set) {
> > > > +		dev_dbg(dev, "data role swapping not supported\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->connected)
> > > > +		goto out;
> > > > +
> > > > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > > > +	if (ret < 0)
> > > > +		goto out;
> > > > +
> > > > +	ret = port->cap->dr_set(port->cap, ret);
> > > > +	if (ret)
> > > > +		goto out;
> > 
> > I could have sworn I was setting the port->data_role here, but I guess
> > it was removed at some point...
> > 
> > Need to fix that.
> 
> 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.

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.

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

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

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

Following your logic, one could argue that all alternate mode handling
should be taken out as well. After all, alternate modes are also tied
to PD protocol support, and discovering the partner identity is a
prerequisite of discovering the alternate modes it supports.
That really doesn't make any sense.

Guenter

> The alternate mode VDO is the only that we can for fairly certainly
> always get, and the only things purely tied to USB PD (the plugs are
> also, but the only purpose for them is to expose the alternate modes
> they have).
> 
> Man, I was always against coupling this thing too tightly with USB PD.
> I'm slipping.
> 
> 
> Thanks,
> 
> -- 
> heikki