Message-ID: <20170525065703.GG8541@lahna.fi.intel.com>
Date:   Thu, 25 May 2017 09:57:03 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <yehezkel.bernat@...el.com>,
        Amir Levy <amir.jer.levy@...el.com>,
        Andy Lutomirski <luto@...nel.org>, Mario.Limonciello@...l.com,
        Jared.Dominguez@...l.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/24] thunderbolt: Convert switch to a device

On Wed, May 24, 2017 at 03:53:59PM +0200, Lukas Wunner wrote:
> On Wed, May 24, 2017 at 02:43:22PM +0300, Mika Westerberg wrote:
> > On Wed, May 24, 2017 at 01:09:08PM +0200, Lukas Wunner wrote:
> > > On Thu, May 18, 2017 at 05:38:57PM +0300, Mika Westerberg wrote:
> > > > Thunderbolt domain consists of switches that are connected to each
> > > > other, forming a bus. This will convert each switch into a real Linux
> > > > device structure and adds them to the domain. The advantage here is
> > > > that we get all the goodies from the driver core, like reference
> > > > counting and sysfs hierarchy for free.
> > > 
> > > I'm wondering, would it make sense to also model each port of a switch
> > > as a device?
> > > 
> > > With your patches, the hierarchy looks something like this:
> > > nhi_pci_dev/domain0/switch0/switch1/switch2
> > > 
> > > If each port is modeled as a device, we'd get something like this:
> > > nhi_pci_dev/domain0/switch0/port1/switch1/port3/switch2
> > > 
> > > I think with the controllers that shipped, there can be up to two
> > > child switches below a switch.  If ports are not modeled as devices,
> > > you can't tell which port a switch is connected to.
> > 
> > Yes you can - it is part of the route string that we use here as bus
> > address. Each "hop" there is the port number through the switch is
> > accessed. For example I have here 4 devices connected:
> > 
> > $ ls -1 /sys/bus/thunderbolt/devices/
> > 0-0
> > 0-1
> > 0-1040301
> > 0-301
> > 0-40301
> > domain0
> 
> I see.
> 
> Is it possible to determine for a given port of type PCIe to which
> downstream bridge it belongs?
> 
> E.g. on my Light Ridge, the PCIe ports are numbered 7, 8, 9, 10.
> The downstream bridges on the controller are numbered 03, 04, 05, 06.
> But the ordering seems to be mixed up, e.g. port 7 corresponds to
> downstream bridge 0000:06:04.0.  Not to 03, as one would expect.
>                           ^^
> 
> Can this perhaps be determined from config space?

I don't think it is in config space. I'm not sure if there is a way to
determine that actually. It may be part of the DROM entry for the PCIe
adapter.

> If we knew the correlation between Thunderbolt PCIe port and downstream
> bridge, we could provide a symlink in sysfs from the Thunderbolt bus
> to the PCI bus.  E.g. in the switch directory for a plugged in device,
> there would be a symlink to the corresponding upstream bridge.  For the
> root switch, this would be the upstream bridge of the host controller.

Yes.

> > > Also, if ports are modeled as devices we'd be able to store attributes
> > > such as port type in sysfs.  Of course we could also store those in
> > > each switch directory as "port0", "port1", ... files.
> > 
> > Is there any reason adding these? It will not help the user to identify
> > the connected device and I don't see how that information could be
> > useful.
> >
> > That kind of information could go to debugfs, though.
> 
> Well, currently we print that stuff to syslog and it's valuable to
> understand what's going on.  Being able to check e.g. the type of
> a port on a running system would be even better.  I'm not saying
> we should model ports as devices, I just want to have a discussion
> about the pros and cons.

Sure.

> > > On Macs, the UUID is conveyed in an EFI device property.
> > 
> > Yes, but only for the host and UUID is actually coming from the fuses
> > (hardware), not from EFI property.
> 
> Nope, on Macs the UUID is calculated by sha1-hashing a constant, then
> extending that by sha1-hashing the uid, then truncating the result to
> 16 bytes.
> 
> The uid in turn is calculated by combining a 16-bit constant with a
> 24-bit model-specific number and a 24-bit serial number.
> 
> This is done by the EFI NHI driver.  No fuses involved.
> 
> (Okay the serial number is coming from an EEPROM, but not that of
> the Thunderbolt controller).

As far as I can tell they read it from fuse but I haven't looked at the
Apple driver.

> > > > +
> > > > +	/*
> > > > +	 * By default the UUID will be based on UID where upper two
> > > > +	 * dwords are filled with ones.
> > > > +	 */
> > > > +	uuid[0] = sw->uid & 0xffffffff;
> > > > +	uuid[1] = (sw->uid >> 32) & 0xffffffff;
> > > > +	uuid[2] = 0xffffffff;
> > > > +	uuid[3] = 0xffffffff;
> > > > +
> > > > +	/*
> > > > +	 * The newer controllers include fused UUID as part of link
> > > > +	 * controller specific registers
> > > > +	 */
> > > > +	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_LINK_CONTROLLER);
> > > > +	if (cap > 0)
> > > > +		tb_sw_read(sw, uuid, TB_CFG_SWITCH, cap + 3, 4);
> > > > +
> > > > +	sw->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);
> > > 
> > > Hm, so on newer controller the uuid is calculated and the result is
> > > subsequently overwritten?  Meh... :-/
> > 
> > ICM gives the UUID as part of the device connected message. This here to
> > make the UUID working also on systems withouth ICM (older Macs). The
> > special case comes from the older devices where there is no fused UUID
> > so we generate one based on UID instead following what ICM does.
> > 
> > This allows us to show "unique_id" attribute the same way on all
> > systems.
> 
> I'll have to double check if the macOS NHI driver calculates a UUID for
> each switch, and how it does that.  We should try to be compatible.

OS X uses UID instead to show the unique id. Here we should really use
UUID so that we are compatible with Windows as well. Since that UUID is
derived from UID for older switches, we should show pretty much the same
information as on Macs.

> > > How about putting the VSEC code path first, then fall back to calculating
> > > the default UUID?  Apart from being prettier, this would allow me to
> > > easily add the Mac-specific code path.
> > 
> > I can change the ordering but you don't need to add Mac specific path
> > there - this code has been tested on Macs already and it should work
> > exactly the same there.
> 
> I don't doubt that it works, the problem is that the UUID should be
> identical to what macOS uses.

I agree and I think I have checked it on OS X as well but it would be
helpful if you could do the same :)

> > Below is taken from the same devices connected to a Cactus Ridge based
> > Mac.
> > 
> > 0-0/authorized:1
> > 0-0/device:0xa
> > 0-0/device_name:Macintosh
> > 0-0/uevent:DEVTYPE=thunderbolt_device
> > 0-0/unique_id:00000000-0000-0008-83b3-30099bc1194a
> > 0-0/vendor:0x1
> > 0-0/vendor_name:Apple, Inc.
> 
> Interesting, however this means that the device ID is identical across
> all Macs.  (0xa, same as on my MacBookPro9,1 w/ Light Ridge.)

That is fine because we are not authorizing root switches anyway. Only
the devices (switches) that get connected to the root switch.

> > > IIUC, if the switch is the root switch, then the UUID is used to
> > > uniquely identify the domain starting at that switch, is that correct?
> > 
> > Yes, that's correct.
> 
> Why is a UUID calculated for each switch on the chain even if only the
> UUID on the root switch is needed to give the domain a unique ID?

It is different thing. Domain UUID is used to identify another domain if
you have thunderbolt cable connected between two hosts. We will be using
that when XDomain stuff is added.

UUID of non-root switches is used to uniquely identify that particular
device so that userspace knows it is the same user has already
authorized. This way we can automatically authorized the device without
need to ask from user.

Powered by blists - more mailing lists