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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160701071348.GA21901@kuha.fi.intel.com>
Date:	Fri, 1 Jul 2016 10:13:48 +0300
From:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Oliver Neukum <oneukum@...e.com>,
	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Roger Quadros <rogerq@...com>,
	Rajaram R <rajaram.officemail@...il.com>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCHv4 1/2] usb: USB Type-C connector class

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