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

2016-07-05 Thread Heikki Krogerus 
On Mon, Jul 04, 2016 at 10:45:02AM -0700, Guenter Roeck wrote:
> On 07/04/2016 10:11 AM, Heikki Krogerus wrote:
> [ ... ]
> 
> > > > 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.
> > > > 
> > > On the other side, not being able to rely on a well defined ABI makes the
> > > ABI much less useful.
> > 
> > What do we do about this?
> > 
> I'll have to think about it. Unfortunately, I'll have to put this on the
> back-burner for the time being. My primary problem is that I need
> a functional  driver _now_ (ie by the end of this week), to meet an
> upcoming deadline. Due to to the locking problem, I can not rely on the
> typec infrastructure ... meaning I have to take two steps back and get
> my code working with the Android infrastructure (which, coincidentally,
> does not require locking and thus doesn't have all the resulting
> limitations and complexities).

That is unfortunate.

I added a commit where I remove the lock completely (at least for
now) to my github.


Cheers,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-04 Thread Guenter Roeck 

On 07/04/2016 10:11 AM, Heikki Krogerus wrote:
[ ... ]


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.


On the other side, not being able to rely on a well defined ABI makes the
ABI much less useful.


What do we do about this?


I'll have to think about it. Unfortunately, I'll have to put this on the
back-burner for the time being. My primary problem is that I need
a functional  driver _now_ (ie by the end of this week), to meet an
upcoming deadline. Due to to the locking problem, I can not rely on the
typec infrastructure ... meaning I have to take two steps back and get
my code working with the Android infrastructure (which, coincidentally,
does not require locking and thus doesn't have all the resulting
limitations and complexities).

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-04 Thread Heikki Krogerus 
Hi Guenter,

On Sun, Jul 03, 2016 at 02:28:44PM -0700, Guenter Roeck wrote:
> On 07/03/2016 12:38 PM, Heikki Krogerus wrote:
> > 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.
> > 
> 
> No, that is not what I said or asked for. The problems I am seeing are due
> to locking in the class code. "Asynchronous" above referred to the state
> machine vs. role change requests by the class code, which operates independent
> of each other in my code. Set requests from the class code still wait for
> the state machine to complete.
> 
> The problem is that the state machine now needs to know if the class code
> has a set role request pending, because in that case it can no longer report
> role changes directly to the class code. This includes role changes unrelated
> to the actual set role request. That code is now much more complicated, 
> especially
> since each callback into the typec code is handled differently. For example,
> typec_disconnect() does not require a lock (unless I missed it), but many
> other functions do.

It seems that I have misunderstood your whole point. I'm really sorry.
I was in a hurry to start my vacation.

So let's just fix the locking.



> > > > 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.
> > 
> On the other side, not being able to rely on a well defined ABI makes the
> ABI much less useful.

What do we do about this?


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-03 Thread Guenter Roeck 

On 07/03/2016 12:38 PM, Heikki Krogerus wrote:

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.



No, that is not what I said or asked for. The problems I am seeing are due
to locking in the class code. "Asynchronous" above referred to the state
machine vs. role change requests by the class code, which operates independent
of each other in my code. Set requests from the class code still wait for
the state machine to complete.

The problem is that the state machine now needs to know if the class code
has a set role request pending, because in that case it can no longer report
role changes directly to the class code. This includes role changes unrelated
to the actual set role request. That code is now much more complicated, 
especially
since each callback into the typec code is handled differently. For example,
typec_disconnect() does not require a lock (unless I missed it), but many
other functions do.


Would you even be able to make it work? How would you report errors
for example?



It worked just great when you had no locks. All I had to do is handle
role changes in the driver, and to implement the necessary locks there.
Actually, before you introduced the locks, you had suggested that this
was the way to go, which is why I implemented it that way.


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



No, that is not correct. Again, all I had to do was to implement a lock
in the driver which waited until the state machine was idle, and _then_
check if the set command needed to do anything. No queuing necessary,
no race conditions.

Keep in mind there are no role _change_ requests, the requests are to set
a specific role - and I can do that once I have the state machine lock.
The second request would again be a request to set a specific role. It waits
for the state machine lock, and after it is acquired decides if a role
change is necessary or not.

Sure, I can switch to your method, and I guess I'll have to.
I just think that the resulting -EBUSY responses are unnecessary.

I will reiterate, though, that this all worked just fine with the previous
version of your code, where sysfs role change requests did not require
or implement a lock in the 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.


Why?


Example:

Class code requests data role change, remote partner requests power role
change. Both are handled in parallel by the state machine. State machine
tries to report power role change to class code and gets stuck because the
class

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

2016-07-03 Thread Heikki Krogerus 
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 >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.

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

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

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.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-01 Thread Guenter Roeck 
On Fri, Jul 01, 2016 at 10:38:03AM +0300, Heikki Krogerus wrote:
> On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> > On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > > The purpose of USB Type-C connector class is to provide
> > > unified interface for the user space to get the status and
> > > basic information about USB Type-C connectors on a system,
> > > control over data role swapping, and when the port supports
> > > USB Power Delivery, also control over power role swapping
> > > and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus 
> > > ---
> > [ ... ]
> > 
> > > +
> > > +What:
> > > /sys/class/typec/-partner/supports_usb_power_delivery
> > > +Date:June 2016
> > > +Contact: Heikki Krogerus 
> > > +Description:
> > > + Shows if the partner supports USB Power Delivery.
> > > + - 1 if USB Power Delivery is supported
> > > + - 0 when it's not
> > > +
> > > +
> > > +What:/sys/class/typec/-partner/id_header_vdo
> > > +Date:June 2016
> > > +Contact: Heikki Krogerus 
> > > +Description:
> > > + If the partner supports USB Power Deliver, shows the VDO
> > > + returned from Discover Identity USB Power Delivery command.
> > > +
> > > + If the partner does not support USB Power Delivery, the
> > > + attribute is hidden.
> > > +
> > On second thought, and after merging the code (and realizing that I don't 
> > get
> > the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> > is that useful here. It also doesn't provide all information either.
> 
> Yeah, I don't think it's available with UCSI either..
> 
> > Would it make sense to split it into multiple decoded attributes ?
> > 
> > - vendor-id
> >   [bit 0..15 of ID header VDO]
> > - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
> > passive/active for cable plugs)
> >   Might map into typec_partner_type, but I don't see a 1:1 match.
> >   [bit 27..29 of ID header VDO]
> > - alternate-mode-supported
> >   [bit 26 of ID header VDO]
> > - capabilities (ufp, dfp, drp, none (?))
> >   [bit 30/31 of ID header VDO]
> > - product-id
> >   [bit 16..31 of Product VDO]
> > 
> > Does this make any sense ?
> 
> I feel a bit uncomfortable exposing so many attribute like that which
> will give details that we can only know when both ends support USB
> PD...
> 
> Here's my proposal for this:
> There has to be a special group for these devices, partners and
> cables, a directory named "pd" or "power_deliver" (or something like
> that), which exposes those. That group will not be responsibility of
> this class, but instead the PD framework that you are working on
> (right?).
> 
> So basically, in this class we will not expose those attributes, and
> we'll also get rid of the "supports_usb_power_delivery" attribute (the
> "pd" groups should only exist when USB PD is actually supported).
> 
> Initially that group needs to be assigned to the "groups" member of
> struct device of the partners in the drivers before they are
> registered. That member is meant for optional attribute groups, but we
> can later think of something better, a way perhaps to bind that group
> the device types "groups" member for partners, cables, etc. or
> something similar in case using the "groups" member of struct device
> is not ideal. But initially we can use that.
> 
> How would that sound to you?
> 
Quite frankly I don't know. At first glance it sounds like a bad idea - it means
that PD specific atttributes would not be standardized, which would reduce the
valus of the class code substantially. I'll have to think about this.

At the same time I'll have to find a somewhat working solution for the locking
issues. I'll have to focus on that for the time being, because it affects
the core architecture of our code. So I don't really have the time to look
into attribute support right now.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-01 Thread 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(>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.
> 
> 
> 
> > > > +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(>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

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

2016-07-01 Thread 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(>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.



> > > +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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-01 Thread Heikki Krogerus 
On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > The purpose of USB Type-C connector class is to provide
> > unified interface for the user space to get the status and
> > basic information about USB Type-C connectors on a system,
> > control over data role swapping, and when the port supports
> > USB Power Delivery, also control over power role swapping
> > and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus 
> > ---
> [ ... ]
> 
> > +
> > +What:  
> > /sys/class/typec/-partner/supports_usb_power_delivery
> > +Date:  June 2016
> > +Contact:   Heikki Krogerus 
> > +Description:
> > +   Shows if the partner supports USB Power Delivery.
> > +   - 1 if USB Power Delivery is supported
> > +   - 0 when it's not
> > +
> > +
> > +What:  /sys/class/typec/-partner/id_header_vdo
> > +Date:  June 2016
> > +Contact:   Heikki Krogerus 
> > +Description:
> > +   If the partner supports USB Power Deliver, shows the VDO
> > +   returned from Discover Identity USB Power Delivery command.
> > +
> > +   If the partner does not support USB Power Delivery, the
> > +   attribute is hidden.
> > +
> On second thought, and after merging the code (and realizing that I don't get
> the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> is that useful here. It also doesn't provide all information either.

Yeah, I don't think it's available with UCSI either..

> Would it make sense to split it into multiple decoded attributes ?
> 
> - vendor-id
>   [bit 0..15 of ID header VDO]
> - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
>   passive/active for cable plugs)
>   Might map into typec_partner_type, but I don't see a 1:1 match.
>   [bit 27..29 of ID header VDO]
> - alternate-mode-supported
>   [bit 26 of ID header VDO]
> - capabilities (ufp, dfp, drp, none (?))
>   [bit 30/31 of ID header VDO]
> - product-id
>   [bit 16..31 of Product VDO]
> 
> Does this make any sense ?

I feel a bit uncomfortable exposing so many attribute like that which
will give details that we can only know when both ends support USB
PD...

Here's my proposal for this:
There has to be a special group for these devices, partners and
cables, a directory named "pd" or "power_deliver" (or something like
that), which exposes those. That group will not be responsibility of
this class, but instead the PD framework that you are working on
(right?).

So basically, in this class we will not expose those attributes, and
we'll also get rid of the "supports_usb_power_delivery" attribute (the
"pd" groups should only exist when USB PD is actually supported).

Initially that group needs to be assigned to the "groups" member of
struct device of the partners in the drivers before they are
registered. That member is meant for optional attribute groups, but we
can later think of something better, a way perhaps to bind that group
the device types "groups" member for partners, cables, etc. or
something similar in case using the "groups" member of struct device
is not ideal. But initially we can use that.

How would that sound to you?


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-07-01 Thread 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(>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(>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(>lock);
> > +
> > +   if (port->prefer_role < 0)
> > +   goto out;
> > +
> > +   ret = sprintf(buf, "%s\n", typec_roles[port->prefer_role]);
> > +out:
> > +   mutex_unlock(>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(>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(>lock);
> > +   return ret;
> > +}


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-06-30 Thread Guenter Roeck 
On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus 
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  236 +
>  Documentation/usb/typec.txt |  103 +++
>  MAINTAINERS |9 +
>  drivers/usb/Kconfig |2 +
>  drivers/usb/Makefile|2 +
>  drivers/usb/typec/Kconfig   |7 +
>  drivers/usb/typec/Makefile  |1 +
>  drivers/usb/typec/typec.c   | 1277 
> +++
>  include/linux/usb/typec.h   |  260 ++
>  9 files changed, 1897 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>  create mode 100644 Documentation/usb/typec.txt
>  create mode 100644 drivers/usb/typec/Kconfig
>  create mode 100644 drivers/usb/typec/Makefile
>  create mode 100644 drivers/usb/typec/typec.c
>  create mode 100644 include/linux/usb/typec.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 000..7cd40e5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,236 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What:/sys/class/typec//current_data_role
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + The current USB data role the port is operating in. This
> + attribute can be used for requesting data role swapping on the
> + port.
> +
> + Valid values:
> + - host
> + - device
> +
> +What:/sys/class/typec//current_power_role
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + The current power role of the port. This attribute can be used
> + to request power role swap on the port when the port supports
> + USB Power Delivery.
> +
> + Valid values:
> + - source
> + - sink
> +
> +What:/sys/class/typec//current_vconn_role
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + Shows the current VCONN role of the port. This attribute can be
> + used to request VCONN role swap on the port when the port
> + supports USB Power Delivery.
> +
> + Valid values are:
> + - source
> + - sink
> +
> +What:/sys/class/typec//power_operation_mode
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + Shows the current power operational mode the port is in.
> +
> + Valid values:
> + - USB - Normal power levels defined in USB specifications
> + - BC1.2 - Power levels defined in Battery Charging Specification
> +   v1.2
> + - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
> + specification.
> + - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
> + specification.
> +- USB Power Delivery - The voltages and currents defined in 
> USB
> +Power Delivery specification
> +
> +What:/sys/class/typec//preferred_role
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + The user space can notify the driver about the preferred role.
> + It should be handled as enabling of Try.SRC or Try.SNK, as
> + defined in USB Type-C specification, in the port drivers. By
> + default there is no preferred role.
> +
> + Valid values:
> + - host
> + - device
> + - For example "none" to remove preference (anything else except
> +   "host" or "device")
> +
> +What:/sys/class/typec//supported_accessory_modes
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + Lists the Accessory Modes, defined in the USB Type-C
> + specification, the port supports.
> +
> +What:/sys/class/typec//supported_data_roles
> +Date:June 2016
> +Contact: Heikki

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

2016-06-30 Thread Guenter Roeck 
On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus 
> ---
[ ... ]

> +
> +What:
> /sys/class/typec/-partner/supports_usb_power_delivery
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + Shows if the partner supports USB Power Delivery.
> + - 1 if USB Power Delivery is supported
> + - 0 when it's not
> +
> +
> +What:/sys/class/typec/-partner/id_header_vdo
> +Date:June 2016
> +Contact: Heikki Krogerus 
> +Description:
> + If the partner supports USB Power Deliver, shows the VDO
> + returned from Discover Identity USB Power Delivery command.
> +
> + If the partner does not support USB Power Delivery, the
> + attribute is hidden.
> +
On second thought, and after merging the code (and realizing that I don't get
the raw data from the Type-C Port Manager), I am not sure if a raw attribute
is that useful here. It also doesn't provide all information either.

Would it make sense to split it into multiple decoded attributes ?

- vendor-id
  [bit 0..15 of ID header VDO]
- product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
passive/active for cable plugs)
  Might map into typec_partner_type, but I don't see a 1:1 match.
  [bit 27..29 of ID header VDO]
- alternate-mode-supported
  [bit 26 of ID header VDO]
- capabilities (ufp, dfp, drp, none (?))
  [bit 30/31 of ID header VDO]
- product-id
  [bit 16..31 of Product VDO]

Does this make any sense ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html