Re: [PATCH 07/24] thunderbolt: Convert switch to a device

[Lukas Wunner]

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?

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.


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


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


> > > +
> > > +	/*
> > > +	 * 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.


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


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


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

Thanks,

Lukas