Date:   Wed, 24 May 2017 14:43:22 +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 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

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

> I don't know if this makes sense, but a discussion about the pros
> and cons is probably warranted.  IIUC once the patches go in, the
> layout is set in stone because it's a user-space API.

Yes, that's right. Also that is one of the reasons why this series tries
to keep the attribute amout minimum and their contents simple.

> > +What:		/sys/bus/thunderbolt/devices/.../device
> > +Date:		Sep 2017
> > +KernelVersion:	4.13
> > +Contact:	thunderbolt-software@...ts.01.org
> > +Description:	This attribute contains id of this device extracted from
> > +		the device DROM.
> 
> Hm, why did you choose to expose the DROM vendor + device ID instead of
> the one in switch config space?

That is the actual vendor/device who made the thing and can be used to
identify the actual device as well.

> The vendor on my MacBookPro9,1 is just 0x0001 and the device is 0x000a.
> Is this valid?  Does Intel assign the vendor IDs, i.e. 0x0001 = Apple?

Apple vendor ID in Thunderbolt realm is 0x0001. Yes, Intel assigns
those.

> > +	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL)
> > +		tb_sw_warn(sw, "unknown switch vendor id %#x\n",
> > +			   sw->config.vendor_id);
> > +
> > +	if (sw->config.device_id != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_PORT_RIDGE &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE)
> > +		tb_sw_warn(sw, "unsupported switch device id %#x\n",
> > +			   sw->config.device_id);
> 
> I'm wondering if this makes sense anymore, as said we should try to avoid
> having to amend device lists in multiple places in the driver for each
> newly introduced controller.

I agree. In the next version I'm moving this to be part of the code
where we identify the generation. So at least it will be removed from
this place.

> > +static void tb_switch_set_uuid(struct tb_switch *sw)
> > +{
> > +	u32 uuid[4];
> > +	int cap;
> > +
> > +	if (sw->uuid)
> > +		return;
> 
> When can this be nonzero?

On Macs with older controller than AR (no ICM running).

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

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

> 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 have here Mac Mini (Light Ridge), Macbook (Cactus Ridge) and newer
Macbook with Alpine Ridge and I've tried to make sure it works 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.
0-1/authorized:1
0-1/device:0x5
0-1/device_name:34UC97
0-1/uevent:DEVTYPE=thunderbolt_device
0-1/unique_id:00000000-0000-0018-0022-c32682b35855
0-1/vendor:0x1e
0-1/vendor_name:LG Electronics
0-1030301/authorized:1
0-1030301/device:0x301
0-1030301/device_name:AKiTiO Thunder3 Duo Pro
0-1030301/nvm_authenticate:0x0
0-1030301/nvm_version:16.0
0-1030301/uevent:DEVTYPE=thunderbolt_device
0-1030301/unique_id:d3010000-0000-9518-a29c-9bc5cca35116
0-1030301/vendor:0x41
0-1030301/vendor_name:inXtron
0-301/authorized:1
0-301/device:0x3
0-301/device_name:eSata Hub
0-301/uevent:DEVTYPE=thunderbolt_device
0-301/unique_id:102f9d1e-0000-0300-ffff-ffffffffffff
0-301/vendor:0x3
0-301/vendor_name:LaCie
0-30301/authorized:1
0-30301/device:0x305
0-30301/device_name:AKiTiO Thunder3 PCIe Box
0-30301/nvm_authenticate:0x0
0-30301/nvm_version:19.0
0-30301/uevent:DEVTYPE=thunderbolt_device
0-30301/unique_id:dc010000-0000-8508-a22d-32ca6421cb16
0-30301/vendor:0x41
0-30301/vendor_name:inXtron
domain0/security:none
domain0/uevent:DEVTYPE=thunderbolt_domain

> BTW, why is there a uid for each switch and a UUID on top?

I think the UUID came with Redwood Ridge or so so before that there was
only UID.

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

> Actually just using the 64-bit uid would still suffice for that use case.
> That is something I've never really comprehended.

Well they decided to go with full UUID and I can't blame them ;-)

Powered by blists - more mailing lists