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

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

<snip>

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