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

[Heikki Krogerus]

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.

> > +	if (ret < 0) {
> > +		port->prefer_role = -1;
> > +		ret = size;
> > +		goto out;
> > +	}
> > +
> > +	role = ret;
> > +
> > +	ret = port->cap->try_role(port->cap, role);
> > +	if (ret)
> > +		goto out;
> > +
> > +	port->prefer_role = role;
> > +	ret = size;
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}
> > +
> > +static ssize_t
> > +preferred_role_show(struct device *dev, struct device_attribute *attr,
> > +		    char *buf)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	ssize_t ret = 0;
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	if (port->prefer_role < 0)
> > +		goto out;
> > +
> > +	ret = sprintf(buf, "%s\n", typec_roles[port->prefer_role]);
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(preferred_role);
> > +
> > +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.

> > +	ret = size;
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}


Thanks,

-- 
heikki