Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, May 31, 2016 at 10:48:29AM +0200, Oliver Neukum wrote:
> On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> > Hi Oliver,
> > 
> > On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > > Hi guys,
> > > > 
> > > > I'm attaching a diff instead of full v3. I'm not yet adding attributes
> > > > for the reset and cable_reset. I still don't understand what is the
> > > > case where the userspace would need to be able to tricker reset? Why
> > > > isn't it enough for the userspace to be able to enter/exit modes?
> > > > Oliver! Can you please comment?
> > > 
> > > 1. Because we need error handling.
> > >    Devices crash. Cables will crash. We will get out of sync.
> > >    You never put yourself in a place where you cannot handle an
> > >    IO error.
> > > 2. Because it is in the spec. We do not second guess the spec.
> > >    We implement it.
> > 
> > Error conditions and crashes are the responsibility of the USB PD
> > stack, not userspace. In those cases the stack can not wait for a
> 
> Those are not exclusive conditions.
> 
> > command from the userspace. So for example if a timer like
> > NoResponseTimer times out, the stack an its state machines will have
> > to take care of the reset quite independently.
> 
> Yes. But somebody needs to handle high level errors.
> 
> > If you get out of sync with an alternate mode, you reset that specific
> > alternate mode by exiting and re-entering it, and you do not reset the
> > entire PD connection, port, partner or cable.
> 
> That would be the first step. If that doesn't work you will at that
> point either give up or use the next largest hammer.
> In principle you could do that in kernel space, but that implies
> that the kernel can detect all failures. That is unlikely.

Any PD communication failures the kernel has to be able to detect, so
I guess you mean failures with the alternate modes themselves, right?

In that case, surely exiting the mode is enough to "reset" it? When it
is re-entered, it has to be completely re-configured in any case. I
don't see how resetting the whole port or cable would guarantee that a
mode would become any more functional in case of failures? It will
however make also the other active modes to de-activate even if they
are functioning fine.

> > The resets from userspace would be purely unsolicited. What would the
> > cases where we would need to tricker a reset like that?
> > 
> > I want to be careful with exposing reset to userspace. Reset in USB PD
> > is not just an IO related thing. When you tricker a reset with USB PD,
> > even if it's a soft reset, it may lead into hard reset, which may
> > potentially lead into sudden voltage and current drop, which may lead
> > into the entire system crashing. We really need to understand the
> > cases where it would be necessary to tricker a reset from userspace.
> > Right now I don't see any.
> 
> User space can call reboot. Actually that does not help.
> Reset is an operation that is intended for error handling.
> If all else fails, we will need to use it.
> Its bad consequences apply whether you trigger this from kernel or
> user space. In fact, an operation that may potentially crash the system
> should involve user space.
> 
> > > > We also need to decide how the alternate modes a port support are
> > > > exposed to the userspace. Do we just assume the port drivers will
> > > > create them as devices under the port device itself, just like
> > > > alternate modes of partners and cable plugs are exposed under the
> > > > partners and cable plugs? That works for me, but again, the class does
> > > > not have any effect on that, and it will be just a guideline. Maybe
> > > > we can add some kind of helpers and force the port drivers to use
> > > > them.
> > > 
> > > What are the alternatives?
> > 
> > Can we make a group for them under the port device somehow? Like the
> > supported_alternate_modes I proposed. I guess it's not possible to add
> > devices to a specific group in sysfs. And would it even be useful.
> 
> Please explain. It is not clear to me what you are proposing here.

So is it possible to have a folder in sysfs for the alternate modes
a port supports?

So instead of:

        /sysfs/class/type-c/usbc0/svid:xxx1/
        /sysfs/class/type-c/usbc0/svid:xxx2/
        /sysfs/class/type-c/usbc0/svid:xxx3/
        ...

We would have something like:

        /sysfs/class/type-c/usbc0/supported_alternate_modes/svid:xxx1/
        /sysfs/class/type-c/usbc0/supported_alternate_modes/svid:xxx2/
        /sysfs/class/type-c/usbc0/supported_alternate_modes/svid:xxx3/
        ...


Thanks,

-- 
heikki