Message-ID: <20160703193828.GA30987@kuha.fi.intel.com>
Date:	Sun, 3 Jul 2016 22:38:28 +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

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 &port->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

Powered by blists - more mailing lists