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

[Heikki Krogerus]

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?

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.

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