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

From: Guenter Roeck <linux@roeck-us.net>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Oliver Neukum <oneukum@suse.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Rajaram R <rajaram.officemail@gmail.com>,
	Felipe Balbi <felipe.balbi@linux.intel.com>,
	Mathias Nyman <mathias.nyman@linux.intel.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [RFC PATCHv2] usb: USB Type-C Connector Class
Date: Tue, 31 May 2016 10:20:34 -0700	[thread overview]
Message-ID: <20160531172033.GB14007@roeck-us.net> (raw)
In-Reply-To: <20160531124355.GC10084@kuha.fi.intel.com>

On Tue, May 31, 2016 at 03:43:56PM +0300, Heikki Krogerus wrote:
> On Tue, May 31, 2016 at 03:09:01PM +0300, Heikki Krogerus wrote:
> > 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.
> 
> Forget about it, I'll just add the reset attributes. I'm still not
> clear about their usefulness, but instead they will just create a small
> risk, but I can live with that.
> 

Given my experience over the last few weeks, I think the added risk
may not just be small, and I think the added benefit is questionable.
Reset handling is not well implemented in all devices, and manually
triggered resets in an unexpected state may make the situation worse.

Can you make it optional ? I may choose not to support it to avoid
the risk.

Thanks,
Guenter