Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531185 > unrolled thread

Re: [PATCHv12 2/3] usb: USB Type-C connector class

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv12 2/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2016-11-28 11:30 +0100
    Re: [PATCHv12 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-11-28 15:30 +0100
      Re: [PATCHv12 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-11-28 21:20 +0100
        Re: [PATCHv12 2/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2016-11-29 09:00 +0100
        Re: [PATCHv12 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-11-29 14:10 +0100
          Re: [PATCHv12 2/3] usb: USB Type-C connector class Greg KH <gregkh@linuxfoundation.org> - 2016-11-29 14:30 +0100
            Re: [PATCHv12 2/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2016-11-29 15:00 +0100
              Re: [PATCHv12 2/3] usb: USB Type-C connector class Greg KH <gregkh@linuxfoundation.org> - 2016-11-29 15:10 +0100

#1531185 — Re: [PATCHv12 2/3] usb: USB Type-C connector class

On Thu, 2016-11-24 at 11:57 +0200, Heikki Krogerus wrote:
> On Wed, Nov 23, 2016 at 09:12:04PM -0800, Guenter Roeck wrote:

> > In our implementation, the default preferred role is determined by the
> > low level driver (as, in my understanding, is suggested by the standard).
> > This means that the ABI will report "no preferred role", unless user space
> > overwrites it, even though there _is_ in fact a preferred role, and the
> > low level driver will execute try.src or try.snk based on that role.
> 
> I'm not sure which standard are you referring? Try.SNK and Try.SRC are
> optional mechanisms for *policy-based* role preference according to
> the USB Type-C spec. The policy really should always come from the

Not all that obvious. If you are looking at it from a distro view
point if you know that you are booting on basically a gadget, you'll
be happy to take the hint. And if the hardware knows it is better
as a sink or source, we should take the hint.

> user space in our case, but I don't think that rules out for example
> initial role preferences coming from the lower level drivers.

Indeed. That should not be a hindrance to submission and inclusion.

> We will need a way the OS can set the initial preference for every
> port. Note that once we can support that, what ever the lower level
> drivers request will be overridden by it. So if for example the
> platform has preference for an initial role, we will simply ignore it
> if the policy says otherwise.

Again, not obvious in a distro. I would actually prefer a module
parameter that would allow us to prefer try.src, as we know how
to be a master.

None of that should hinder submission and inclusion.

	Regards
		Oliver

[toc] | [next] | [standalone]

#1531353

On Mon, Nov 28, 2016 at 11:19:32AM +0100, Oliver Neukum wrote:
> On Thu, 2016-11-24 at 11:57 +0200, Heikki Krogerus wrote:
> > On Wed, Nov 23, 2016 at 09:12:04PM -0800, Guenter Roeck wrote:
> 
> > > In our implementation, the default preferred role is determined by the
> > > low level driver (as, in my understanding, is suggested by the standard).
> > > This means that the ABI will report "no preferred role", unless user space
> > > overwrites it, even though there _is_ in fact a preferred role, and the
> > > low level driver will execute try.src or try.snk based on that role.
> > 
> > I'm not sure which standard are you referring? Try.SNK and Try.SRC are
> > optional mechanisms for *policy-based* role preference according to
> > the USB Type-C spec. The policy really should always come from the
> 
> Not all that obvious. If you are looking at it from a distro view
> point if you know that you are booting on basically a gadget, you'll
> be happy to take the hint. And if the hardware knows it is better
> as a sink or source, we should take the hint.
> 
> > user space in our case, but I don't think that rules out for example
> > initial role preferences coming from the lower level drivers.
> 
> Indeed. That should not be a hindrance to submission and inclusion.
> 
> > We will need a way the OS can set the initial preference for every
> > port. Note that once we can support that, what ever the lower level
> > drivers request will be overridden by it. So if for example the
> > platform has preference for an initial role, we will simply ignore it
> > if the policy says otherwise.
> 
> Again, not obvious in a distro. I would actually prefer a module
> parameter that would allow us to prefer try.src, as we know how
> to be a master.

I would be happy with module parameter.

> None of that should hinder submission and inclusion.

It's already there in v13.

I better ping Greg already after rc1 this time, just so we don't start
the review again when we are already at rc5 (and I have forgotten
about this whole thing). I'll ask about the module parameter idea
then, though I'm guessing he won't like it. But maybe he has some
suggestions.


Thanks Oliver,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1531645

On Mon, Nov 28, 2016 at 04:23:23PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 28, 2016 at 11:19:32AM +0100, Oliver Neukum wrote:
> > On Thu, 2016-11-24 at 11:57 +0200, Heikki Krogerus wrote:
> > > On Wed, Nov 23, 2016 at 09:12:04PM -0800, Guenter Roeck wrote:
> > 
> > > > In our implementation, the default preferred role is determined by the
> > > > low level driver (as, in my understanding, is suggested by the standard).
> > > > This means that the ABI will report "no preferred role", unless user space
> > > > overwrites it, even though there _is_ in fact a preferred role, and the
> > > > low level driver will execute try.src or try.snk based on that role.
> > > 
> > > I'm not sure which standard are you referring? Try.SNK and Try.SRC are
> > > optional mechanisms for *policy-based* role preference according to
> > > the USB Type-C spec. The policy really should always come from the
> > 

The Type-C specification states (in 4.5.2.2.11):

"Note: if both Try.SRC and Try.SNK mechanisms are implemented, only one shall be
enabled by the port at any given time. Deciding which of these two mechanisms
is enabled is product design-specific."

I read into this:
- The class code can not assume that either of those mechanisms are implemented.
- "product-design specific" means that the product designer determines which of
  the two mechanisms (if any) is enabled. While not explicitly stated, I would
  assume this to be set either by hardware or via devicetree / ACPI, and not
  from user space.

I don't mind to have user space control; all I am asking for is to have the
means for lower level code (which is the most likely entity to know about
product design) to be able to inform higher layers about its initial
preferences. We have this now, so I am happy.

> > Not all that obvious. If you are looking at it from a distro view
> > point if you know that you are booting on basically a gadget, you'll
> > be happy to take the hint. And if the hardware knows it is better
> > as a sink or source, we should take the hint.
> > 
> > > user space in our case, but I don't think that rules out for example
> > > initial role preferences coming from the lower level drivers.
> > 
> > Indeed. That should not be a hindrance to submission and inclusion.
> > 
> > > We will need a way the OS can set the initial preference for every
> > > port. Note that once we can support that, what ever the lower level
> > > drivers request will be overridden by it. So if for example the
> > > platform has preference for an initial role, we will simply ignore it
> > > if the policy says otherwise.
> > 
> > Again, not obvious in a distro. I would actually prefer a module
> > parameter that would allow us to prefer try.src, as we know how
> > to be a master.
> 
> I would be happy with module parameter.
> 
Personally I don't really care about a module parameter; as mentioned above,
I would expect the preference, if it needs to be selectable, to be configured
with devicetree or ACPI properties (or by a platform driver which sets a device
property).

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1531978

On Mon, 2016-11-28 at 12:11 -0800, Guenter Roeck wrote:
> On Mon, Nov 28, 2016 at 04:23:23PM +0200, Heikki Krogerus wrote:
> > On Mon, Nov 28, 2016 at 11:19:32AM +0100, Oliver Neukum wrote:

> The Type-C specification states (in 4.5.2.2.11):
> 
> "Note: if both Try.SRC and Try.SNK mechanisms are implemented, only one shall be
> enabled by the port at any given time. Deciding which of these two mechanisms
> is enabled is product design-specific."
> 
> I read into this:
> - The class code can not assume that either of those mechanisms are implemented.

Total agreement

> - "product-design specific" means that the product designer determines which of
>   the two mechanisms (if any) is enabled. While not explicitly stated, I would
>   assume this to be set either by hardware or via devicetree / ACPI, and not
>   from user space.

I read this as spec-speak for "not our problem"

> I don't mind to have user space control; all I am asking for is to have the
> means for lower level code (which is the most likely entity to know about
> product design) to be able to inform higher layers about its initial
> preferences. We have this now, so I am happy.

So am I.

> Personally I don't really care about a module parameter; as mentioned above,
> I would expect the preference, if it needs to be selectable, to be configured
> with devicetree or ACPI properties (or by a platform driver which sets a device
> property).

Well, as a distro for generic desktops and servers you will face an XHCI
on PCI and UCSI telling you that your ports can express preferences. Now
deal with it. And it must be said that such distros have over a decade
of experience in acting as a master. The slave capability is less well
developed. We'd like to be master. And if possible we'd like to avoid
a later switch of roles, so the choice should be easily made in initrd.

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1532265

Hi Guenter,

On Mon, Nov 28, 2016 at 12:11:43PM -0800, Guenter Roeck wrote:
> Personally I don't really care about a module parameter; as mentioned above,
> I would expect the preference, if it needs to be selectable, to be configured
> with devicetree or ACPI properties (or by a platform driver which sets a device
> property).

Unfortunately we can not assume the firmware to be always correct.
Companies love to recycle the firmware. We are going to see products
from a company X that should prefer source role, a desktop for
example, but still give the OS a device property that says otherwise.
The reason for that is most likely because the previous product from
that company was some kind of mobile device.

So IMHO we need some way for the OS to override this thing eventually.
If not module parameters, then something else. The other option is
board specific quirks, and I would really prefer to avoid those if we
can.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1532276

On Tue, Nov 29, 2016 at 02:59:58PM +0200, Heikki Krogerus wrote:
> Hi Guenter,
> 
> On Mon, Nov 28, 2016 at 12:11:43PM -0800, Guenter Roeck wrote:
> > Personally I don't really care about a module parameter; as mentioned above,
> > I would expect the preference, if it needs to be selectable, to be configured
> > with devicetree or ACPI properties (or by a platform driver which sets a device
> > property).
> 
> Unfortunately we can not assume the firmware to be always correct.
> Companies love to recycle the firmware. We are going to see products
> from a company X that should prefer source role, a desktop for
> example, but still give the OS a device property that says otherwise.
> The reason for that is most likely because the previous product from
> that company was some kind of mobile device.
> 
> So IMHO we need some way for the OS to override this thing eventually.
> If not module parameters, then something else. The other option is
> board specific quirks, and I would really prefer to avoid those if we
> can.

Whatever it is, it is NOT going to be a module parameter, sorry, that
ship has long sailed and will not be coming back.  This isn't the 1990's
anymore...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1532312

On Tue, 2016-11-29 at 14:20 +0100, Greg KH wrote:
> On Tue, Nov 29, 2016 at 02:59:58PM +0200, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Mon, Nov 28, 2016 at 12:11:43PM -0800, Guenter Roeck wrote:
> > > Personally I don't really care about a module parameter; as mentioned above,
> > > I would expect the preference, if it needs to be selectable, to be configured
> > > with devicetree or ACPI properties (or by a platform driver which sets a device
> > > property).
> > 
> > Unfortunately we can not assume the firmware to be always correct.
> > Companies love to recycle the firmware. We are going to see products
> > from a company X that should prefer source role, a desktop for
> > example, but still give the OS a device property that says otherwise.
> > The reason for that is most likely because the previous product from
> > that company was some kind of mobile device.
> > 
> > So IMHO we need some way for the OS to override this thing eventually.
> > If not module parameters, then something else. The other option is
> > board specific quirks, and I would really prefer to avoid those if we
> > can.
> 
> Whatever it is, it is NOT going to be a module parameter, sorry, that
> ship has long sailed and will not be coming back.  This isn't the 1990's
> anymore...

Do you have a sensible alternative that works at boot time?

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1532319

On Tue, Nov 29, 2016 at 02:48:46PM +0100, Oliver Neukum wrote:
> On Tue, 2016-11-29 at 14:20 +0100, Greg KH wrote:
> > On Tue, Nov 29, 2016 at 02:59:58PM +0200, Heikki Krogerus wrote:
> > > Hi Guenter,
> > > 
> > > On Mon, Nov 28, 2016 at 12:11:43PM -0800, Guenter Roeck wrote:
> > > > Personally I don't really care about a module parameter; as mentioned above,
> > > > I would expect the preference, if it needs to be selectable, to be configured
> > > > with devicetree or ACPI properties (or by a platform driver which sets a device
> > > > property).
> > > 
> > > Unfortunately we can not assume the firmware to be always correct.
> > > Companies love to recycle the firmware. We are going to see products
> > > from a company X that should prefer source role, a desktop for
> > > example, but still give the OS a device property that says otherwise.
> > > The reason for that is most likely because the previous product from
> > > that company was some kind of mobile device.
> > > 
> > > So IMHO we need some way for the OS to override this thing eventually.
> > > If not module parameters, then something else. The other option is
> > > board specific quirks, and I would really prefer to avoid those if we
> > > can.
> > 
> > Whatever it is, it is NOT going to be a module parameter, sorry, that
> > ship has long sailed and will not be coming back.  This isn't the 1990's
> > anymore...
> 
> Do you have a sensible alternative that works at boot time?

I'm for a device tree or acpi setting, as this really is a platform
thing, it is the only place that should "know" this type of problem, so
that is where it can be 'specified'.

Yes, firmware is "hard", but it gets really really tiring constantly
having to paper over firmware and hardware bugs in the kernel just
because we seem to be the ones that are willing to actually fix problems
that others cause.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web