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

[Lukas Wunner]

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.

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.

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.


> +What:		/sys/bus/thunderbolt/devices/.../device
> +Date:		Sep 2017
> +KernelVersion:	4.13
> +Contact:	thunderbolt-software@xxxxxxxxxxxx
> +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?

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?


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


> +static void tb_switch_set_uuid(struct tb_switch *sw)
> +{
> +	u32 uuid[4];
> +	int cap;
> +
> +	if (sw->uuid)
> +		return;

When can this be nonzero?


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

On Macs, the UUID is conveyed in an EFI device 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.

BTW, why is there a uid for each switch and a UUID on top?
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?
Actually just using the 64-bit uid would still suffice for that use case.
That is something I've never really comprehended.

Thanks,

Lukas