[PATCH 0/6] thunderbolt / ACPI: Add support for USB4 _OSC

[PATCH 0/6] thunderbolt / ACPI: Add support for USB4 _OSC

[Mika Westerberg]

Hi all,

The just released ACPI 6.4 spec [1] adds a new _OSC method that is used to
negotiate OS support for native USB4 features such as PCIe tunneling. This
patch series adds Linux support for the new _OSC and modifies the
Thunderbolt/USB4 driver accordingly to enable/disable tunneling of
different protocols.

There is an additional setting in the firmware connection manager that
allows the BIOS to disable PCIe tunneling, so we add support for this and
also make the software connection manager to switch to this "nopcie"
security level when the _OSC does not allow PCIe tunneling.

This applies on top of thunderbolt.git/next.

[1] https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf

Mario Limonciello (2):
  thunderbolt: Fix possible NULL pointer dereference in tb_acpi_add_link()
  ACPI: Execute platform _OSC also with query bit clear

Mika Westerberg (4):
  thunderbolt: Add support for PCIe tunneling disabled (SL5)
  thunderbolt: Allow disabling XDomain protocol
  ACPI: Add support for native USB4 control _OSC
  thunderbolt: Add support for native USB4 _OSC

 .../ABI/testing/sysfs-bus-thunderbolt         |   2 +
 Documentation/admin-guide/thunderbolt.rst     |   7 ++
 drivers/acpi/bus.c                            | 119 ++++++++++++++++--
 drivers/thunderbolt/acpi.c                    |  67 +++++++++-
 drivers/thunderbolt/domain.c                  |  16 ++-
 drivers/thunderbolt/icm.c                     |   6 +-
 drivers/thunderbolt/nhi.c                     |  27 +++-
 drivers/thunderbolt/switch.c                  |   6 +-
 drivers/thunderbolt/tb.c                      |  22 +++-
 drivers/thunderbolt/tb.h                      |  13 ++
 drivers/thunderbolt/tunnel.c                  |  10 +-
 drivers/thunderbolt/usb4.c                    |  11 +-
 drivers/thunderbolt/xdomain.c                 |   9 ++
 include/linux/acpi.h                          |  10 ++
 include/linux/thunderbolt.h                   |   3 +
 15 files changed, 299 insertions(+), 29 deletions(-)

-- 
2.29.2

[PATCH 1/6] thunderbolt: Fix possible NULL pointer dereference in tb_acpi_add_link()

[Mika Westerberg]

From: Mario Limonciello <mario.limonciello@dell.com>

When we walk up the device hierarchy in tb_acpi_add_link() make sure we
break the loop if the device has no parent. Otherwise we may crash the
kernel by dereferencing a NULL pointer.

Fixes: b2be2b05cf3b ("thunderbolt: Create device links from ACPI description")
Cc: stable@vger.kernel.org
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
index a5f988a9f948..b5442f979b4d 100644
--- a/drivers/thunderbolt/acpi.c
+++ b/drivers/thunderbolt/acpi.c
@@ -56,7 +56,7 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
 	 * managed with the xHCI and the SuperSpeed hub so we create the
 	 * link from xHCI instead.
 	 */
-	while (!dev_is_pci(dev))
+	while (dev && !dev_is_pci(dev))
 		dev = dev->parent;
 
 	if (!dev)
-- 
2.29.2

[PATCH 2/6] thunderbolt: Add support for PCIe tunneling disabled (SL5)

[Mika Westerberg]

Recent Intel Thunderbolt firmware connection manager has support for
another security level, SL5, that disables PCIe tunneling. This option
can be turned on from the BIOS.

When this is set the driver exposes a new security level "nopcie" to the
userspace and hides the authorized attribute under connected devices.

While there we also hide it when "dponly" security level is enabled
since it is not really usable in that case anyway.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  2 ++
 Documentation/admin-guide/thunderbolt.rst       |  7 +++++++
 drivers/thunderbolt/domain.c                    | 12 +++++++++++-
 drivers/thunderbolt/switch.c                    |  6 +++++-
 include/linux/thunderbolt.h                     |  3 +++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index 581dea95245b..d7f09d011b6d 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -85,6 +85,8 @@ Description:	This attribute holds current Thunderbolt security level
 		usbonly  Automatically tunnel USB controller of the
 			 connected Thunderbolt dock (and Display Port). All
 			 PCIe links downstream of the dock are removed.
+		nopcie   USB4 system where PCIe tunneling is disabled from
+			 the BIOS.
 		=======  ==================================================
 
 What: /sys/bus/thunderbolt/devices/.../authorized
diff --git a/Documentation/admin-guide/thunderbolt.rst b/Documentation/admin-guide/thunderbolt.rst
index 0d4348445f91..f18e881373c4 100644
--- a/Documentation/admin-guide/thunderbolt.rst
+++ b/Documentation/admin-guide/thunderbolt.rst
@@ -47,6 +47,9 @@ be DMA masters and thus read contents of the host memory without CPU and OS
 knowing about it. There are ways to prevent this by setting up an IOMMU but
 it is not always available for various reasons.
 
+Some USB4 systems have a BIOS setting to disable PCIe tunneling. This is
+treated as another security level (nopcie).
+
 The security levels are as follows:
 
   none
@@ -77,6 +80,10 @@ The security levels are as follows:
     Display Port in a dock. All PCIe links downstream of the dock are
     removed.
 
+  nopcie
+    PCIe tunneling is disabled/forbidden from the BIOS. Available in some
+    USB4 systems.
+
 The current security level can be read from
 ``/sys/bus/thunderbolt/devices/domainX/security`` where ``domainX`` is
 the Thunderbolt domain the host controller manages. There is typically
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 9ba2181464cc..a1c79c9c4f66 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -118,6 +118,7 @@ static const char * const tb_security_names[] = {
 	[TB_SECURITY_SECURE] = "secure",
 	[TB_SECURITY_DPONLY] = "dponly",
 	[TB_SECURITY_USBONLY] = "usbonly",
+	[TB_SECURITY_NOPCIE] = "nopcie",
 };
 
 static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,
@@ -243,8 +244,14 @@ static ssize_t deauthorization_show(struct device *dev,
 				    char *buf)
 {
 	const struct tb *tb = container_of(dev, struct tb, dev);
+	bool deauthorization = false;
 
-	return sprintf(buf, "%d\n", !!tb->cm_ops->disapprove_switch);
+	/* Only meaningful if authorization is supported */
+	if (tb->security_level == TB_SECURITY_USER ||
+	    tb->security_level == TB_SECURITY_SECURE)
+		deauthorization = !!tb->cm_ops->disapprove_switch;
+
+	return sprintf(buf, "%d\n", deauthorization);
 }
 static DEVICE_ATTR_RO(deauthorization);
 
@@ -452,6 +459,9 @@ int tb_domain_add(struct tb *tb)
 			goto err_ctl_stop;
 	}
 
+	tb_dbg(tb, "security level set to %s\n",
+	       tb_security_names[tb->security_level]);
+
 	ret = device_add(&tb->dev);
 	if (ret)
 		goto err_ctl_stop;
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index cdba05e72486..60fd92113740 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1768,7 +1768,11 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct tb_switch *sw = tb_to_switch(dev);
 
-	if (attr == &dev_attr_device.attr) {
+	if (attr == &dev_attr_authorized.attr) {
+		if (sw->tb->security_level == TB_SECURITY_NOPCIE ||
+		    sw->tb->security_level == TB_SECURITY_DPONLY)
+			return 0;
+	} else if (attr == &dev_attr_device.attr) {
 		if (!sw->device)
 			return 0;
 	} else if (attr == &dev_attr_device_name.attr) {
diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
index 034dccf93955..659a0a810fa1 100644
--- a/include/linux/thunderbolt.h
+++ b/include/linux/thunderbolt.h
@@ -45,6 +45,8 @@ enum tb_cfg_pkg_type {
  * @TB_SECURITY_USBONLY: Only tunnel USB controller of the connected
  *			 Thunderbolt dock (and Display Port). All PCIe
  *			 links downstream of the dock are removed.
+ * @TB_SECURITY_NOPCIE: For USB4 systems this level is used when the
+ *			PCIe tunneling is disabled from the BIOS.
  */
 enum tb_security_level {
 	TB_SECURITY_NONE,
@@ -52,6 +54,7 @@ enum tb_security_level {
 	TB_SECURITY_SECURE,
 	TB_SECURITY_DPONLY,
 	TB_SECURITY_USBONLY,
+	TB_SECURITY_NOPCIE,
 };
 
 /**
-- 
2.29.2

[PATCH 3/6] thunderbolt: Allow disabling XDomain protocol

[Mika Westerberg]

This allows disabling XDomain protocol completely if the user does not
plan to use the USB4/Thunderbolt peer-to-peer functionality, or for
security reasons.

XDomain protocol is enabled by default but with this commit it is
possible to disable it by passing "xdomain=0" as module parameter (or
through the kernel command line).

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/domain.c  | 4 +++-
 drivers/thunderbolt/icm.c     | 6 ++++--
 drivers/thunderbolt/tb.c      | 3 +++
 drivers/thunderbolt/tb.h      | 1 +
 drivers/thunderbolt/xdomain.c | 9 +++++++++
 5 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index a1c79c9c4f66..89ae614eaba2 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -412,7 +412,9 @@ static bool tb_domain_event_cb(void *data, enum tb_cfg_pkg_type type,
 	switch (type) {
 	case TB_CFG_PKG_XDOMAIN_REQ:
 	case TB_CFG_PKG_XDOMAIN_RESP:
-		return tb_xdomain_handle_request(tb, type, buf, size);
+		if (tb_is_xdomain_enabled())
+			return tb_xdomain_handle_request(tb, type, buf, size);
+		break;
 
 	default:
 		tb->cm_ops->handle_event(tb, type, buf, size);
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 8b7f941a9bb7..6b853c59aa34 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -1701,10 +1701,12 @@ static void icm_handle_notification(struct work_struct *work)
 			icm->device_disconnected(tb, n->pkg);
 			break;
 		case ICM_EVENT_XDOMAIN_CONNECTED:
-			icm->xdomain_connected(tb, n->pkg);
+			if (tb_is_xdomain_enabled())
+				icm->xdomain_connected(tb, n->pkg);
 			break;
 		case ICM_EVENT_XDOMAIN_DISCONNECTED:
-			icm->xdomain_disconnected(tb, n->pkg);
+			if (tb_is_xdomain_enabled())
+				icm->xdomain_disconnected(tb, n->pkg);
 			break;
 		case ICM_EVENT_RTD3_VETO:
 			icm->rtd3_veto(tb, n->pkg);
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index d08879849abe..4a99cc3913c9 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -179,6 +179,9 @@ static void tb_scan_xdomain(struct tb_port *port)
 	struct tb_xdomain *xd;
 	u64 route;
 
+	if (!tb_is_xdomain_enabled())
+		return;
+
 	route = tb_downstream_route(port);
 	xd = tb_xdomain_find_by_route(tb, route);
 	if (xd) {
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 31468de658e4..01fe849e9d4c 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -953,6 +953,7 @@ static inline u64 tb_downstream_route(struct tb_port *port)
 	       | ((u64) port->port << (port->sw->config.depth * 8));
 }
 
+bool tb_is_xdomain_enabled(void);
 bool tb_xdomain_handle_request(struct tb *tb, enum tb_cfg_pkg_type type,
 			       const void *buf, size_t size);
 struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent,
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index f2d4db1cd84d..655ae4fefd69 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -30,6 +30,10 @@ struct xdomain_request_work {
 	struct tb *tb;
 };
 
+static bool tb_xdomain_enabled = true;
+module_param_named(xdomain, tb_xdomain_enabled, bool, 0444);
+MODULE_PARM_DESC(xdomain, "allow XDomain protocol (default: true)");
+
 /* Serializes access to the properties and protocol handlers below */
 static DEFINE_MUTEX(xdomain_lock);
 
@@ -47,6 +51,11 @@ static const uuid_t tb_xdp_uuid =
 	UUID_INIT(0xb638d70e, 0x42ff, 0x40bb,
 		  0x97, 0xc2, 0x90, 0xe2, 0xc0, 0xb2, 0xff, 0x07);
 
+bool tb_is_xdomain_enabled(void)
+{
+	return tb_xdomain_enabled;
+}
+
 static bool tb_xdomain_match(const struct tb_cfg_request *req,
 			     const struct ctl_pkg *pkg)
 {
-- 
2.29.2

[PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Mika Westerberg]

From: Mario Limonciello <mario.limonciello@dell.com>

The platform _OSC can change the hardware state when query bit is not
set. According to ACPI spec it is recommended that the OS runs _OSC with
query bit set until the platform does not mask any of the capabilities.
Then it should run it with query bit clear in order to actually commit
the changes. At the moment Linux only runs the _OSC with query bit set
and this is going to cause problems with the USB4 CM (Connection
Manager) switch that is going to commit the switch only when the OS
requests control over the feature.

For this reason modify the _OSC support so that we first execute it with
query bit set, then use the returned valu as base of the features we
want to control and run the _OSC again with query bit clear.

Also rename the function to better match what it does.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/bus.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 1682f8b454a2..ca7c7b2bf56e 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -282,9 +282,9 @@ bool osc_pc_lpi_support_confirmed;
 EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
 
 static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
-static void acpi_bus_osc_support(void)
+static void acpi_bus_osc_negotiate_platform_control(void)
 {
-	u32 capbuf[2];
+	u32 capbuf[2], *capbuf_ret;
 	struct acpi_osc_context context = {
 		.uuid_str = sb_uuid_str,
 		.rev = 1,
@@ -321,17 +321,36 @@ static void acpi_bus_osc_support(void)
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
 		return;
-	if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
-		u32 *capbuf_ret = context.ret.pointer;
-		if (context.ret.length > OSC_SUPPORT_DWORD) {
-			osc_sb_apei_support_acked =
-				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
-			osc_pc_lpi_support_confirmed =
-				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
-		}
+
+	if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
+		return;
+
+	capbuf_ret = context.ret.pointer;
+	if (context.ret.length <= OSC_SUPPORT_DWORD) {
 		kfree(context.ret.pointer);
+		return;
 	}
-	/* do we need to check other returned cap? Sounds no */
+
+	/*
+	 * Now run _OSC again with query flag clean and with the caps
+	 * both platform and OS supports.
+	 */
+	capbuf[OSC_QUERY_DWORD] = 0;
+	capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
+	kfree(context.ret.pointer);
+
+	if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
+		return;
+
+	capbuf_ret = context.ret.pointer;
+	if (context.ret.length > OSC_SUPPORT_DWORD) {
+		osc_sb_apei_support_acked =
+			capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
+		osc_pc_lpi_support_confirmed =
+			capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
+	}
+
+	kfree(context.ret.pointer);
 }
 
 /* --------------------------------------------------------------------------
@@ -1168,7 +1187,7 @@ static int __init acpi_bus_init(void)
 	 * _OSC method may exist in module level code,
 	 * so it must be run after ACPI_FULL_INITIALIZATION
 	 */
-	acpi_bus_osc_support();
+	acpi_bus_osc_negotiate_platform_control();
 
 	/*
 	 * _PDC control method may load dynamic SSDT tables,
-- 
2.29.2

[PATCH 5/6] ACPI: Add support for native USB4 control _OSC

[Mika Westerberg]

ACPI 6.4 introduced a new _OSC capability that is used negotiate native
connection manager support. Connection manager is the entity that is
responsible for tunneling over the USB4 fabric. If the platform rejects
the native access then firmware based connection manager is used.

The new _OSC also includes a set of bits that can be used to disable
certain tunnel types such as PCIe for security reasons for instance.

This implements the new USB4 _OSC so that we try to negotiate native
USB4 support if the Thunderbolt/USB4 (CONFIG_USB4) driver is enabled.
Drivers can determine what was negotiated by checking two new variables
exposed in this patch.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/bus.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h | 10 ++++++
 2 files changed, 86 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index ca7c7b2bf56e..f7ad2d283e51 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -281,6 +281,12 @@ bool osc_sb_apei_support_acked;
 bool osc_pc_lpi_support_confirmed;
 EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
 
+/*
+ * ACPI 6.4 Operating System Capabilities for USB.
+ */
+bool osc_sb_native_usb4_support_confirmed;
+EXPORT_SYMBOL_GPL(osc_sb_native_usb4_support_confirmed);
+
 static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
 static void acpi_bus_osc_negotiate_platform_control(void)
 {
@@ -317,6 +323,9 @@ static void acpi_bus_osc_negotiate_platform_control(void)
 	if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_DIVERSE_HIGH_SUPPORT;
 
+	if (IS_ENABLED(CONFIG_USB4))
+		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_NATIVE_USB4_SUPPORT;
+
 	if (!ghes_disable)
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
@@ -348,8 +357,74 @@ static void acpi_bus_osc_negotiate_platform_control(void)
 			capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
 		osc_pc_lpi_support_confirmed =
 			capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
+		osc_sb_native_usb4_support_confirmed =
+			capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
+	}
+
+	kfree(context.ret.pointer);
+}
+
+/*
+ * Native control of USB4 capabilities. If any of the tunneling bits is
+ * set it means OS is in control and we use software based connection
+ * manager.
+ */
+u32 osc_sb_native_usb4_control;
+EXPORT_SYMBOL_GPL(osc_sb_native_usb4_control);
+
+static void acpi_bus_decode_usb_osc(const char *msg, u32 bits)
+{
+	printk(KERN_INFO PREFIX "%s USB3%c DisplayPort%c PCIe%c XDomain%c\n", msg,
+	       (bits & OSC_USB_USB3_TUNNELING) ? '+' : '-',
+	       (bits & OSC_USB_DP_TUNNELING) ? '+' : '-',
+	       (bits & OSC_USB_PCIE_TUNNELING) ? '+' : '-',
+	       (bits & OSC_USB_XDOMAIN) ? '+' : '-');
+}
+
+static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
+static void acpi_bus_osc_negotiate_usb_control(void)
+{
+	u32 capbuf[3];
+	struct acpi_osc_context context = {
+		.uuid_str = sb_usb_uuid_str,
+		.rev = 1,
+		.cap.length = sizeof(capbuf),
+		.cap.pointer = capbuf,
+	};
+	acpi_handle handle;
+	acpi_status status;
+	u32 control;
+
+	if (!osc_sb_native_usb4_support_confirmed)
+		return;
+
+	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
+		return;
+
+	control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING |
+		  OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN;
+
+	capbuf[OSC_QUERY_DWORD] = 0;
+	capbuf[OSC_SUPPORT_DWORD] = 0;
+	capbuf[OSC_CONTROL_DWORD] = control;
+
+	status = acpi_run_osc(handle, &context);
+	if (ACPI_FAILURE(status))
+		return;
+
+	if (context.ret.length != sizeof(capbuf)) {
+		printk(KERN_INFO PREFIX "USB4 _OSC: returned invalid length buffer\n");
+		goto out_free;
 	}
 
+	osc_sb_native_usb4_control =
+		control & ((u32 *)context.ret.pointer)[OSC_CONTROL_DWORD];
+
+	acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control);
+	acpi_bus_decode_usb_osc("USB4 _OSC: OS controls",
+				osc_sb_native_usb4_control);
+
+out_free:
 	kfree(context.ret.pointer);
 }
 
@@ -1188,6 +1263,7 @@ static int __init acpi_bus_init(void)
 	 * so it must be run after ACPI_FULL_INITIALIZATION
 	 */
 	acpi_bus_osc_negotiate_platform_control();
+	acpi_bus_osc_negotiate_usb_control();
 
 	/*
 	 * _PDC control method may load dynamic SSDT tables,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 2630c2e953f7..ac68c2d4e393 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -546,9 +546,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
 #define OSC_SB_OSLPI_SUPPORT			0x00000100
 #define OSC_SB_CPC_DIVERSE_HIGH_SUPPORT		0x00001000
 #define OSC_SB_GENERIC_INITIATOR_SUPPORT	0x00002000
+#define OSC_SB_NATIVE_USB4_SUPPORT		0x00040000
 
 extern bool osc_sb_apei_support_acked;
 extern bool osc_pc_lpi_support_confirmed;
+extern bool osc_sb_native_usb4_support_confirmed;
+
+/* USB4 Capabilities */
+#define OSC_USB_USB3_TUNNELING			0x00000001
+#define OSC_USB_DP_TUNNELING			0x00000002
+#define OSC_USB_PCIE_TUNNELING			0x00000004
+#define OSC_USB_XDOMAIN				0x00000008
+
+extern u32 osc_sb_native_usb4_control;
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
 #define OSC_PCI_EXT_CONFIG_SUPPORT		0x00000001
-- 
2.29.2

[PATCH 6/6] thunderbolt: Add support for native USB4 _OSC

[Mika Westerberg]

ACPI 6.4 introduced a new _OSC capability used to negotiate whether the
OS is supposed to use Software (native) or Firmware based Connection
Manager. If the native support is granted then there are set of bits
that enable/disable different tunnel types that the Software Connection
Manager is allowed to tunnel.

This adds support for this new USB4 _OSC accordingly. When PCIe
tunneling is disabled then the driver switches security level to be
"nopcie" following the security level 5 used in Firmware based
Connection Manager.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/acpi.c    | 65 +++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.c     | 27 +++++++++++++--
 drivers/thunderbolt/tb.c      | 19 +++++++++-
 drivers/thunderbolt/tb.h      | 12 +++++++
 drivers/thunderbolt/tunnel.c  | 10 +++---
 drivers/thunderbolt/usb4.c    | 11 ++++--
 drivers/thunderbolt/xdomain.c |  2 +-
 7 files changed, 134 insertions(+), 12 deletions(-)

diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
index b5442f979b4d..35fa17f7e599 100644
--- a/drivers/thunderbolt/acpi.c
+++ b/drivers/thunderbolt/acpi.c
@@ -115,3 +115,68 @@ void tb_acpi_add_links(struct tb_nhi *nhi)
 	if (ACPI_FAILURE(status))
 		dev_warn(&nhi->pdev->dev, "failed to enumerate tunneled ports\n");
 }
+
+/**
+ * tb_acpi_is_native() - Did the platform grant native TBT/USB4 control
+ *
+ * Returns %true if the platform granted OS native control over
+ * TBT/USB4. In this case software based connection manager can be used,
+ * otherwise there is firmware based connection manager running.
+ */
+bool tb_acpi_is_native(void)
+{
+	return osc_sb_native_usb4_support_confirmed &&
+	       osc_sb_native_usb4_control;
+}
+
+/**
+ * tb_acpi_may_tunnel_usb3() - Is USB3 tunneling allowed by the platform
+ *
+ * When software based connection manager is used, this function
+ * returns %true if platform allows native USB3 tunneling.
+ */
+bool tb_acpi_may_tunnel_usb3(void)
+{
+	if (tb_acpi_is_native())
+		return osc_sb_native_usb4_control & OSC_USB_USB3_TUNNELING;
+	return true;
+}
+
+/**
+ * tb_acpi_may_tunnel_dp() - Is DisplayPort tunneling allowed by the platform
+ *
+ * When software based connection manager is used, this function
+ * returns %true if platform allows native DP tunneling.
+ */
+bool tb_acpi_may_tunnel_dp(void)
+{
+	if (tb_acpi_is_native())
+		return osc_sb_native_usb4_control & OSC_USB_DP_TUNNELING;
+	return true;
+}
+
+/**
+ * tb_acpi_may_tunnel_pcie() - Is PCIe tunneling allowed by the platform
+ *
+ * When software based connection manager is used, this function
+ * returns %true if platform allows native PCIe tunneling.
+ */
+bool tb_acpi_may_tunnel_pcie(void)
+{
+	if (tb_acpi_is_native())
+		return osc_sb_native_usb4_control & OSC_USB_PCIE_TUNNELING;
+	return true;
+}
+
+/**
+ * tb_acpi_is_xdomain_allowed() - Are XDomain connections allowed
+ *
+ * When software based connection manager is used, this function
+ * returns %true if platform allows XDomain connections.
+ */
+bool tb_acpi_is_xdomain_allowed(void)
+{
+	if (tb_acpi_is_native())
+		return osc_sb_native_usb4_control & OSC_USB_XDOMAIN;
+	return true;
+}
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index cfc622da4f83..2ea953c853bd 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1188,6 +1188,29 @@ static void tb_apple_add_links(struct tb_nhi *nhi)
 	}
 }
 
+static struct tb *nhi_select_cm(struct tb_nhi *nhi)
+{
+	struct tb *tb;
+
+	/*
+	 * USB4 case is simple. If we got control of any of the
+	 * capabilities, we use software CM.
+	 */
+	if (tb_acpi_is_native())
+		return tb_probe(nhi);
+
+	/*
+	 * Either firmware based CM is running (we did not get control
+	 * from the firmware) or this is pre-USB4 PC so try first
+	 * firmware CM and then fallback to software CM.
+	 */
+	tb = icm_probe(nhi);
+	if (!tb)
+		tb = tb_probe(nhi);
+
+	return tb;
+}
+
 static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct tb_nhi *nhi;
@@ -1256,9 +1279,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	tb_apple_add_links(nhi);
 	tb_acpi_add_links(nhi);
 
-	tb = icm_probe(nhi);
-	if (!tb)
-		tb = tb_probe(nhi);
+	tb = nhi_select_cm(nhi);
 	if (!tb) {
 		dev_err(&nhi->pdev->dev,
 			"failed to determine connection manager, aborting\n");
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 4a99cc3913c9..122b32640552 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -437,6 +437,11 @@ static int tb_tunnel_usb3(struct tb *tb, struct tb_switch *sw)
 	struct tb_cm *tcm = tb_priv(tb);
 	struct tb_tunnel *tunnel;
 
+	if (!tb_acpi_may_tunnel_usb3()) {
+		tb_dbg(tb, "USB3 tunneling disabled, not creating tunnel\n");
+		return 0;
+	}
+
 	up = tb_switch_find_port(sw, TB_TYPE_USB3_UP);
 	if (!up)
 		return 0;
@@ -512,6 +517,9 @@ static int tb_create_usb3_tunnels(struct tb_switch *sw)
 	struct tb_port *port;
 	int ret;
 
+	if (!tb_acpi_may_tunnel_usb3())
+		return 0;
+
 	if (tb_route(sw)) {
 		ret = tb_tunnel_usb3(sw->tb, sw);
 		if (ret)
@@ -841,6 +849,11 @@ static void tb_tunnel_dp(struct tb *tb)
 	struct tb_port *port, *in, *out;
 	struct tb_tunnel *tunnel;
 
+	if (!tb_acpi_may_tunnel_dp()) {
+		tb_dbg(tb, "DP tunneling disabled, not creating tunnel\n");
+		return;
+	}
+
 	/*
 	 * Find pair of inactive DP IN and DP OUT adapters and then
 	 * establish a DP tunnel between them.
@@ -1549,7 +1562,11 @@ struct tb *tb_probe(struct tb_nhi *nhi)
 	if (!tb)
 		return NULL;
 
-	tb->security_level = TB_SECURITY_USER;
+	if (tb_acpi_may_tunnel_pcie())
+		tb->security_level = TB_SECURITY_USER;
+	else
+		tb->security_level = TB_SECURITY_NOPCIE;
+
 	tb->cm_ops = &tb_cm_ops;
 
 	tcm = tb_priv(tb);
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 01fe849e9d4c..4dd4ff79dede 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1039,8 +1039,20 @@ void tb_check_quirks(struct tb_switch *sw);
 
 #ifdef CONFIG_ACPI
 void tb_acpi_add_links(struct tb_nhi *nhi);
+
+bool tb_acpi_is_native(void);
+bool tb_acpi_may_tunnel_usb3(void);
+bool tb_acpi_may_tunnel_dp(void);
+bool tb_acpi_may_tunnel_pcie(void);
+bool tb_acpi_is_xdomain_allowed(void);
 #else
 static inline void tb_acpi_add_links(struct tb_nhi *nhi) { }
+
+static inline bool tb_acpi_is_native(void) { return true; }
+static inline bool tb_acpi_may_tunnel_usb3(void) { return true; }
+static inline bool tb_acpi_may_tunnel_dp(void) { return true; }
+static inline bool tb_acpi_may_tunnel_pcie(void) { return true; }
+static inline bool tb_acpi_is_xdomain_allowed(void) { return true; }
 #endif
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index dcdf9c7a9cae..5d3110e604bc 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -932,12 +932,14 @@ static int tb_usb3_activate(struct tb_tunnel *tunnel, bool activate)
 static int tb_usb3_consumed_bandwidth(struct tb_tunnel *tunnel,
 		int *consumed_up, int *consumed_down)
 {
+	int pcie_enabled = tb_acpi_may_tunnel_pcie();
+
 	/*
-	 * PCIe tunneling affects the USB3 bandwidth so take that it
-	 * into account here.
+	 * PCIe tunneling, if enabled, affects the USB3 bandwidth so
+	 * take that it into account here.
 	 */
-	*consumed_up = tunnel->allocated_up * (3 + 1) / 3;
-	*consumed_down = tunnel->allocated_down * (3 + 1) / 3;
+	*consumed_up = tunnel->allocated_up * (3 + pcie_enabled) / 3;
+	*consumed_down = tunnel->allocated_down * (3 + pcie_enabled) / 3;
 	return 0;
 }
 
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index 67a2867382ed..680bc738dd66 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -331,13 +331,18 @@ int usb4_switch_setup(struct tb_switch *sw)
 	if (ret)
 		return ret;
 
-	if (sw->link_usb4 && tb_switch_find_port(parent, TB_TYPE_USB3_DOWN)) {
+	if (tb_acpi_may_tunnel_usb3() && sw->link_usb4 &&
+	    tb_switch_find_port(parent, TB_TYPE_USB3_DOWN)) {
 		val |= ROUTER_CS_5_UTO;
 		xhci = false;
 	}
 
-	/* Only enable PCIe tunneling if the parent router supports it */
-	if (tb_switch_find_port(parent, TB_TYPE_PCIE_DOWN)) {
+	/*
+	 * Only enable PCIe tunneling if the parent router supports it
+	 * and it is not disabled.
+	 */
+	if (tb_acpi_may_tunnel_pcie() &&
+	    tb_switch_find_port(parent, TB_TYPE_PCIE_DOWN)) {
 		val |= ROUTER_CS_5_PTO;
 		/*
 		 * xHCI can be enabled if PCIe tunneling is supported
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 655ae4fefd69..a7af869e234b 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -53,7 +53,7 @@ static const uuid_t tb_xdp_uuid =
 
 bool tb_is_xdomain_enabled(void)
 {
-	return tb_xdomain_enabled;
+	return tb_xdomain_enabled && tb_acpi_is_xdomain_allowed();
 }
 
 static bool tb_xdomain_match(const struct tb_cfg_request *req,
-- 
2.29.2

Re: [PATCH 2/6] thunderbolt: Add support for PCIe tunneling disabled (SL5)

[Yehezkel Bernat]

On Tue, Jan 26, 2021 at 5:57 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Recent Intel Thunderbolt firmware connection manager has support for
> another security level, SL5, that disables PCIe tunneling. This option
> can be turned on from the BIOS.
>
> When this is set the driver exposes a new security level "nopcie" to the
> userspace and hides the authorized attribute under connected devices.
>
> While there we also hide it when "dponly" security level is enabled
> since it is not really usable in that case anyway.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

Looks good to me, I'm just not sure I understand how this is different from
dponly mode. Is this just because it comes from the new _OSC?

Re: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Yehezkel Bernat]

On Tue, Jan 26, 2021 at 5:57 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> From: Mario Limonciello <mario.limonciello@dell.com>
>
> The platform _OSC can change the hardware state when query bit is not
> set. According to ACPI spec it is recommended that the OS runs _OSC with
> query bit set until the platform does not mask any of the capabilities.
> Then it should run it with query bit clear in order to actually commit
> the changes. At the moment Linux only runs the _OSC with query bit set
> and this is going to cause problems with the USB4 CM (Connection
> Manager) switch that is going to commit the switch only when the OS
> requests control over the feature.
>
> For this reason modify the _OSC support so that we first execute it with
> query bit set, then use the returned valu as base of the features we

Totally out of my depth here, but just noticed the typo (valu => value).

> want to control and run the _OSC again with query bit clear.
>
> Also rename the function to better match what it does.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

Re: [PATCH 2/6] thunderbolt: Add support for PCIe tunneling disabled (SL5)

[Mika Westerberg]

On Tue, Jan 26, 2021 at 06:18:47PM +0200, Yehezkel Bernat wrote:
> On Tue, Jan 26, 2021 at 5:57 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Recent Intel Thunderbolt firmware connection manager has support for
> > another security level, SL5, that disables PCIe tunneling. This option
> > can be turned on from the BIOS.
> >
> > When this is set the driver exposes a new security level "nopcie" to the
> > userspace and hides the authorized attribute under connected devices.
> >
> > While there we also hide it when "dponly" security level is enabled
> > since it is not really usable in that case anyway.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> 
> Looks good to me, I'm just not sure I understand how this is different from
> dponly mode. Is this just because it comes from the new _OSC?

The firmware connection manager reports this new security level instead
of dponly so we reflect that to the userspace, and while at it take
advantage of the nopcie when USB4 _OSC disables PCIe tunneling so they
both look the same from userspace perspective.

Re: [PATCH 2/6] thunderbolt: Add support for PCIe tunneling disabled (SL5)

[Yehezkel Bernat]

On Tue, Jan 26, 2021 at 6:26 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Jan 26, 2021 at 06:18:47PM +0200, Yehezkel Bernat wrote:
> > On Tue, Jan 26, 2021 at 5:57 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > Recent Intel Thunderbolt firmware connection manager has support for
> > > another security level, SL5, that disables PCIe tunneling. This option
> > > can be turned on from the BIOS.
> > >
> > > When this is set the driver exposes a new security level "nopcie" to the
> > > userspace and hides the authorized attribute under connected devices.
> > >
> > > While there we also hide it when "dponly" security level is enabled
> > > since it is not really usable in that case anyway.
> > >
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> >
> > Looks good to me, I'm just not sure I understand how this is different from
> > dponly mode. Is this just because it comes from the new _OSC?
>
> The firmware connection manager reports this new security level instead
> of dponly so we reflect that to the userspace, and while at it take
> advantage of the nopcie when USB4 _OSC disables PCIe tunneling so they
> both look the same from userspace perspective.

Makes sense. Thanks for the clarification!

Re: [PATCH 0/6] thunderbolt / ACPI: Add support for USB4 _OSC

[Yehezkel Bernat]

On Tue, Jan 26, 2021 at 5:57 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi all,
>
> The just released ACPI 6.4 spec [1] adds a new _OSC method that is used to
> negotiate OS support for native USB4 features such as PCIe tunneling. This
> patch series adds Linux support for the new _OSC and modifies the
> Thunderbolt/USB4 driver accordingly to enable/disable tunneling of
> different protocols.
>
> There is an additional setting in the firmware connection manager that
> allows the BIOS to disable PCIe tunneling, so we add support for this and
> also make the software connection manager to switch to this "nopcie"
> security level when the _OSC does not allow PCIe tunneling.
>
> This applies on top of thunderbolt.git/next.
>
> [1] https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
>
> Mario Limonciello (2):
>   thunderbolt: Fix possible NULL pointer dereference in tb_acpi_add_link()
>   ACPI: Execute platform _OSC also with query bit clear
>
> Mika Westerberg (4):
>   thunderbolt: Add support for PCIe tunneling disabled (SL5)
>   thunderbolt: Allow disabling XDomain protocol
>   ACPI: Add support for native USB4 control _OSC
>   thunderbolt: Add support for native USB4 _OSC
>

For Thunderbolt parts,

    Acked-by: Yehezkel Bernat <YehezkelShB@gmail.com>

Re: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Rafael J. Wysocki]

On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> From: Mario Limonciello <mario.limonciello@dell.com>
>
> The platform _OSC can change the hardware state when query bit is not
> set. According to ACPI spec it is recommended that the OS runs _OSC with
> query bit set until the platform does not mask any of the capabilities.
> Then it should run it with query bit clear in order to actually commit
> the changes. At the moment Linux only runs the _OSC with query bit set

And that's because there was nothing it could ask to control using the
_SB scope _OSC.

Today it is just reporting what features are supported by it.

However, with the upcoming USB4 CM support it needs to ask for the
control of that feature and that's why the _SB scope _OSC support
needs to be extended.  So it is not a fix for a bug or missing spec
coverage, which this part of the changelog kind of implies, it's just
enabling a new feature.

> and this is going to cause problems with the USB4 CM (Connection
> Manager) switch that is going to commit the switch only when the OS
> requests control over the feature.
>
> For this reason modify the _OSC support so that we first execute it with
> query bit set, then use the returned valu as base of the features we

s/valu/value/

> want to control and run the _OSC again with query bit clear.
>
> Also rename the function to better match what it does.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> ---
>  drivers/acpi/bus.c | 43 +++++++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 1682f8b454a2..ca7c7b2bf56e 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -282,9 +282,9 @@ bool osc_pc_lpi_support_confirmed;
>  EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
>
>  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> -static void acpi_bus_osc_support(void)
> +static void acpi_bus_osc_negotiate_platform_control(void)
>  {
> -       u32 capbuf[2];
> +       u32 capbuf[2], *capbuf_ret;
>         struct acpi_osc_context context = {
>                 .uuid_str = sb_uuid_str,
>                 .rev = 1,
> @@ -321,17 +321,36 @@ static void acpi_bus_osc_support(void)
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
>         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
>                 return;
> -       if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
> -               u32 *capbuf_ret = context.ret.pointer;
> -               if (context.ret.length > OSC_SUPPORT_DWORD) {
> -                       osc_sb_apei_support_acked =
> -                               capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> -                       osc_pc_lpi_support_confirmed =
> -                               capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
> -               }
> +
> +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> +               return;
> +
> +       capbuf_ret = context.ret.pointer;
> +       if (context.ret.length <= OSC_SUPPORT_DWORD) {
>                 kfree(context.ret.pointer);
> +               return;
>         }
> -       /* do we need to check other returned cap? Sounds no */
> +
> +       /*
> +        * Now run _OSC again with query flag clean and with the caps

s/clean/clear/

> +        * both platform and OS supports.

s/both platform and OS supports/supported by both the OS and the platform/

> +        */
> +       capbuf[OSC_QUERY_DWORD] = 0;
> +       capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> +       kfree(context.ret.pointer);
> +
> +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> +               return;
> +
> +       capbuf_ret = context.ret.pointer;
> +       if (context.ret.length > OSC_SUPPORT_DWORD) {
> +               osc_sb_apei_support_acked =
> +                       capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> +               osc_pc_lpi_support_confirmed =
> +                       capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
> +       }
> +
> +       kfree(context.ret.pointer);
>  }
>
>  /* --------------------------------------------------------------------------
> @@ -1168,7 +1187,7 @@ static int __init acpi_bus_init(void)
>          * _OSC method may exist in module level code,
>          * so it must be run after ACPI_FULL_INITIALIZATION
>          */
> -       acpi_bus_osc_support();
> +       acpi_bus_osc_negotiate_platform_control();
>
>         /*
>          * _PDC control method may load dynamic SSDT tables,
> --
> 2.29.2
>

Re: [PATCH 5/6] ACPI: Add support for native USB4 control _OSC

[Rafael J. Wysocki]

On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> ACPI 6.4 introduced a new _OSC capability that is used negotiate native
> connection manager support. Connection manager is the entity that is
> responsible for tunneling over the USB4 fabric. If the platform rejects
> the native access then firmware based connection manager is used.
>
> The new _OSC also includes a set of bits that can be used to disable
> certain tunnel types such as PCIe for security reasons for instance.
>
> This implements the new USB4 _OSC so that we try to negotiate native
> USB4 support if the Thunderbolt/USB4 (CONFIG_USB4) driver is enabled.
> Drivers can determine what was negotiated by checking two new variables
> exposed in this patch.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/bus.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h | 10 ++++++
>  2 files changed, 86 insertions(+)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index ca7c7b2bf56e..f7ad2d283e51 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -281,6 +281,12 @@ bool osc_sb_apei_support_acked;
>  bool osc_pc_lpi_support_confirmed;
>  EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
>
> +/*
> + * ACPI 6.4 Operating System Capabilities for USB.
> + */
> +bool osc_sb_native_usb4_support_confirmed;
> +EXPORT_SYMBOL_GPL(osc_sb_native_usb4_support_confirmed);
> +
>  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
>  static void acpi_bus_osc_negotiate_platform_control(void)
>  {
> @@ -317,6 +323,9 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>         if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_DIVERSE_HIGH_SUPPORT;
>
> +       if (IS_ENABLED(CONFIG_USB4))
> +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_NATIVE_USB4_SUPPORT;
> +
>         if (!ghes_disable)
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
>         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> @@ -348,8 +357,74 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>                         capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
>                 osc_pc_lpi_support_confirmed =
>                         capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
> +               osc_sb_native_usb4_support_confirmed =
> +                       capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
> +       }
> +
> +       kfree(context.ret.pointer);
> +}
> +
> +/*
> + * Native control of USB4 capabilities. If any of the tunneling bits is
> + * set it means OS is in control and we use software based connection
> + * manager.
> + */
> +u32 osc_sb_native_usb4_control;
> +EXPORT_SYMBOL_GPL(osc_sb_native_usb4_control);
> +
> +static void acpi_bus_decode_usb_osc(const char *msg, u32 bits)
> +{
> +       printk(KERN_INFO PREFIX "%s USB3%c DisplayPort%c PCIe%c XDomain%c\n", msg,
> +              (bits & OSC_USB_USB3_TUNNELING) ? '+' : '-',
> +              (bits & OSC_USB_DP_TUNNELING) ? '+' : '-',
> +              (bits & OSC_USB_PCIE_TUNNELING) ? '+' : '-',
> +              (bits & OSC_USB_XDOMAIN) ? '+' : '-');
> +}
> +
> +static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
> +static void acpi_bus_osc_negotiate_usb_control(void)
> +{
> +       u32 capbuf[3];
> +       struct acpi_osc_context context = {
> +               .uuid_str = sb_usb_uuid_str,
> +               .rev = 1,
> +               .cap.length = sizeof(capbuf),
> +               .cap.pointer = capbuf,
> +       };
> +       acpi_handle handle;
> +       acpi_status status;
> +       u32 control;
> +
> +       if (!osc_sb_native_usb4_support_confirmed)
> +               return;
> +
> +       if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> +               return;
> +
> +       control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING |
> +                 OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN;
> +
> +       capbuf[OSC_QUERY_DWORD] = 0;
> +       capbuf[OSC_SUPPORT_DWORD] = 0;
> +       capbuf[OSC_CONTROL_DWORD] = control;
> +
> +       status = acpi_run_osc(handle, &context);

This is the same _OSC that is evaluated in
acpi_bus_osc_negotiate_platform_control(), right?

So shouldn't the capbuf[OSC_SUPPORT_DWORD] be whatever is negotiated
through acpi_bus_osc_negotiate_platform_control()?  At least that
would be consistent with acpi_pci_osc_control_set().

And if this was done I'm not sure if the change in the previous patch
would be needed then?

> +       if (ACPI_FAILURE(status))
> +               return;
> +
> +       if (context.ret.length != sizeof(capbuf)) {
> +               printk(KERN_INFO PREFIX "USB4 _OSC: returned invalid length buffer\n");
> +               goto out_free;
>         }
>
> +       osc_sb_native_usb4_control =
> +               control & ((u32 *)context.ret.pointer)[OSC_CONTROL_DWORD];
> +
> +       acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control);
> +       acpi_bus_decode_usb_osc("USB4 _OSC: OS controls",
> +                               osc_sb_native_usb4_control);
> +
> +out_free:
>         kfree(context.ret.pointer);
>  }
>
> @@ -1188,6 +1263,7 @@ static int __init acpi_bus_init(void)
>          * so it must be run after ACPI_FULL_INITIALIZATION
>          */
>         acpi_bus_osc_negotiate_platform_control();
> +       acpi_bus_osc_negotiate_usb_control();
>
>         /*
>          * _PDC control method may load dynamic SSDT tables,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 2630c2e953f7..ac68c2d4e393 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -546,9 +546,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
>  #define OSC_SB_OSLPI_SUPPORT                   0x00000100
>  #define OSC_SB_CPC_DIVERSE_HIGH_SUPPORT                0x00001000
>  #define OSC_SB_GENERIC_INITIATOR_SUPPORT       0x00002000
> +#define OSC_SB_NATIVE_USB4_SUPPORT             0x00040000
>
>  extern bool osc_sb_apei_support_acked;
>  extern bool osc_pc_lpi_support_confirmed;
> +extern bool osc_sb_native_usb4_support_confirmed;
> +
> +/* USB4 Capabilities */
> +#define OSC_USB_USB3_TUNNELING                 0x00000001
> +#define OSC_USB_DP_TUNNELING                   0x00000002
> +#define OSC_USB_PCIE_TUNNELING                 0x00000004
> +#define OSC_USB_XDOMAIN                                0x00000008
> +
> +extern u32 osc_sb_native_usb4_control;
>
>  /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
>  #define OSC_PCI_EXT_CONFIG_SUPPORT             0x00000001
> --
> 2.29.2
>

RE: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Limonciello, Mario]

> On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > From: Mario Limonciello <mario.limonciello@dell.com>
> >
> > The platform _OSC can change the hardware state when query bit is not
> > set. According to ACPI spec it is recommended that the OS runs _OSC with
> > query bit set until the platform does not mask any of the capabilities.
> > Then it should run it with query bit clear in order to actually commit
> > the changes. At the moment Linux only runs the _OSC with query bit set
> 
> And that's because there was nothing it could ask to control using the
> _SB scope _OSC.
> 
> Today it is just reporting what features are supported by it.
> 
> However, with the upcoming USB4 CM support it needs to ask for the
> control of that feature and that's why the _SB scope _OSC support
> needs to be extended.  So it is not a fix for a bug or missing spec
> coverage, which this part of the changelog kind of implies, it's just
> enabling a new feature.

Other operating systems behave as described in the ACPI spec long before USB4 CM
support was added.  Admittedly this is semantics of whether to call it
a "bug", but specifically the lack of this in the existing Linux kernel code
*can* actually cause you to get into a situation where you have no functional
USB4.  This will happen if you boot between two different kernels or potentially
two different operating systems.  This is due to how the selection of FW or SW
CM is made.  If this patch "alone" was brought further backward the older kernels
FW CM mode would be activated in those situations.

> 
> > and this is going to cause problems with the USB4 CM (Connection
> > Manager) switch that is going to commit the switch only when the OS
> > requests control over the feature.
> >
> > For this reason modify the _OSC support so that we first execute it with
> > query bit set, then use the returned valu as base of the features we
> 
> s/valu/value/
> 
> > want to control and run the _OSC again with query bit clear.
> >
> > Also rename the function to better match what it does.
> >
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > ---
> >  drivers/acpi/bus.c | 43 +++++++++++++++++++++++++++++++------------
> >  1 file changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 1682f8b454a2..ca7c7b2bf56e 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -282,9 +282,9 @@ bool osc_pc_lpi_support_confirmed;
> >  EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
> >
> >  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> > -static void acpi_bus_osc_support(void)
> > +static void acpi_bus_osc_negotiate_platform_control(void)
> >  {
> > -       u32 capbuf[2];
> > +       u32 capbuf[2], *capbuf_ret;
> >         struct acpi_osc_context context = {
> >                 .uuid_str = sb_uuid_str,
> >                 .rev = 1,
> > @@ -321,17 +321,36 @@ static void acpi_bus_osc_support(void)
> >                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> >         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> >                 return;
> > -       if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
> > -               u32 *capbuf_ret = context.ret.pointer;
> > -               if (context.ret.length > OSC_SUPPORT_DWORD) {
> > -                       osc_sb_apei_support_acked =
> > -                               capbuf_ret[OSC_SUPPORT_DWORD] &
> OSC_SB_APEI_SUPPORT;
> > -                       osc_pc_lpi_support_confirmed =
> > -                               capbuf_ret[OSC_SUPPORT_DWORD] &
> OSC_SB_PCLPI_SUPPORT;
> > -               }
> > +
> > +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > +               return;
> > +
> > +       capbuf_ret = context.ret.pointer;
> > +       if (context.ret.length <= OSC_SUPPORT_DWORD) {
> >                 kfree(context.ret.pointer);
> > +               return;
> >         }
> > -       /* do we need to check other returned cap? Sounds no */
> > +
> > +       /*
> > +        * Now run _OSC again with query flag clean and with the caps
> 
> s/clean/clear/
> 
> > +        * both platform and OS supports.
> 
> s/both platform and OS supports/supported by both the OS and the platform/
> 
> > +        */
> > +       capbuf[OSC_QUERY_DWORD] = 0;
> > +       capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> > +       kfree(context.ret.pointer);
> > +
> > +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > +               return;
> > +
> > +       capbuf_ret = context.ret.pointer;
> > +       if (context.ret.length > OSC_SUPPORT_DWORD) {
> > +               osc_sb_apei_support_acked =
> > +                       capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> > +               osc_pc_lpi_support_confirmed =
> > +                       capbuf_ret[OSC_SUPPORT_DWORD] &
> OSC_SB_PCLPI_SUPPORT;
> > +       }
> > +
> > +       kfree(context.ret.pointer);
> >  }
> >
> >  /* ------------------------------------------------------------------------
> --
> > @@ -1168,7 +1187,7 @@ static int __init acpi_bus_init(void)
> >          * _OSC method may exist in module level code,
> >          * so it must be run after ACPI_FULL_INITIALIZATION
> >          */
> > -       acpi_bus_osc_support();
> > +       acpi_bus_osc_negotiate_platform_control();
> >
> >         /*
> >          * _PDC control method may load dynamic SSDT tables,
> > --
> > 2.29.2
> >

Re: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Rafael J. Wysocki]

On Tue, Jan 26, 2021 at 6:37 PM Limonciello, Mario
<Mario.Limonciello@dell.com> wrote:
>
> > On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > From: Mario Limonciello <mario.limonciello@dell.com>
> > >
> > > The platform _OSC can change the hardware state when query bit is not
> > > set. According to ACPI spec it is recommended that the OS runs _OSC with
> > > query bit set until the platform does not mask any of the capabilities.
> > > Then it should run it with query bit clear in order to actually commit
> > > the changes. At the moment Linux only runs the _OSC with query bit set
> >
> > And that's because there was nothing it could ask to control using the
> > _SB scope _OSC.
> >
> > Today it is just reporting what features are supported by it.
> >
> > However, with the upcoming USB4 CM support it needs to ask for the
> > control of that feature and that's why the _SB scope _OSC support
> > needs to be extended.  So it is not a fix for a bug or missing spec
> > coverage, which this part of the changelog kind of implies, it's just
> > enabling a new feature.
>
> Other operating systems behave as described in the ACPI spec long before USB4 CM
> support was added.  Admittedly this is semantics of whether to call it
> a "bug", but specifically the lack of this in the existing Linux kernel code
> *can* actually cause you to get into a situation where you have no functional
> USB4.  This will happen if you boot between two different kernels or potentially
> two different operating systems.  This is due to how the selection of FW or SW
> CM is made.  If this patch "alone" was brought further backward the older kernels
> FW CM mode would be activated in those situations.

I would put that information into the changelog.

Moreover, have you looked at acpi_pci_osc_control_set()?

What it does is analogous to what you are proposing, but a bit
different, and I would like to preserve consistency between _OSC use
cases.

So would it be possible to adjust the _SB _OSC evaluation flow to
follow the PCI _OSC one?  That is, if any control bits are there, pass
them along with the last evaluation of _OSC with the query flag clear.
Or is the latter defective and if so then why?

>
> >
> > > and this is going to cause problems with the USB4 CM (Connection
> > > Manager) switch that is going to commit the switch only when the OS
> > > requests control over the feature.
> > >
> > > For this reason modify the _OSC support so that we first execute it with
> > > query bit set, then use the returned valu as base of the features we
> >
> > s/valu/value/
> >
> > > want to control and run the _OSC again with query bit clear.
> > >
> > > Also rename the function to better match what it does.
> > >
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > >
> > > ---
> > >  drivers/acpi/bus.c | 43 +++++++++++++++++++++++++++++++------------
> > >  1 file changed, 31 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index 1682f8b454a2..ca7c7b2bf56e 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -282,9 +282,9 @@ bool osc_pc_lpi_support_confirmed;
> > >  EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
> > >
> > >  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> > > -static void acpi_bus_osc_support(void)
> > > +static void acpi_bus_osc_negotiate_platform_control(void)
> > >  {
> > > -       u32 capbuf[2];
> > > +       u32 capbuf[2], *capbuf_ret;
> > >         struct acpi_osc_context context = {
> > >                 .uuid_str = sb_uuid_str,
> > >                 .rev = 1,
> > > @@ -321,17 +321,36 @@ static void acpi_bus_osc_support(void)
> > >                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> > >         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> > >                 return;
> > > -       if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
> > > -               u32 *capbuf_ret = context.ret.pointer;
> > > -               if (context.ret.length > OSC_SUPPORT_DWORD) {
> > > -                       osc_sb_apei_support_acked =
> > > -                               capbuf_ret[OSC_SUPPORT_DWORD] &
> > OSC_SB_APEI_SUPPORT;
> > > -                       osc_pc_lpi_support_confirmed =
> > > -                               capbuf_ret[OSC_SUPPORT_DWORD] &
> > OSC_SB_PCLPI_SUPPORT;
> > > -               }
> > > +
> > > +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > +               return;
> > > +
> > > +       capbuf_ret = context.ret.pointer;
> > > +       if (context.ret.length <= OSC_SUPPORT_DWORD) {
> > >                 kfree(context.ret.pointer);
> > > +               return;
> > >         }
> > > -       /* do we need to check other returned cap? Sounds no */
> > > +
> > > +       /*
> > > +        * Now run _OSC again with query flag clean and with the caps
> >
> > s/clean/clear/
> >
> > > +        * both platform and OS supports.
> >
> > s/both platform and OS supports/supported by both the OS and the platform/
> >
> > > +        */
> > > +       capbuf[OSC_QUERY_DWORD] = 0;
> > > +       capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> > > +       kfree(context.ret.pointer);
> > > +
> > > +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > +               return;
> > > +
> > > +       capbuf_ret = context.ret.pointer;
> > > +       if (context.ret.length > OSC_SUPPORT_DWORD) {
> > > +               osc_sb_apei_support_acked =
> > > +                       capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> > > +               osc_pc_lpi_support_confirmed =
> > > +                       capbuf_ret[OSC_SUPPORT_DWORD] &
> > OSC_SB_PCLPI_SUPPORT;
> > > +       }
> > > +
> > > +       kfree(context.ret.pointer);
> > >  }
> > >
> > >  /* ------------------------------------------------------------------------
> > --
> > > @@ -1168,7 +1187,7 @@ static int __init acpi_bus_init(void)
> > >          * _OSC method may exist in module level code,
> > >          * so it must be run after ACPI_FULL_INITIALIZATION
> > >          */
> > > -       acpi_bus_osc_support();
> > > +       acpi_bus_osc_negotiate_platform_control();
> > >
> > >         /*
> > >          * _PDC control method may load dynamic SSDT tables,
> > > --
> > > 2.29.2
> > >

Re: [PATCH 5/6] ACPI: Add support for native USB4 control _OSC

[Mika Westerberg]

Hi,

On Tue, Jan 26, 2021 at 06:35:20PM +0100, Rafael J. Wysocki wrote:
> > +       capbuf[OSC_QUERY_DWORD] = 0;
> > +       capbuf[OSC_SUPPORT_DWORD] = 0;
> > +       capbuf[OSC_CONTROL_DWORD] = control;
> > +
> > +       status = acpi_run_osc(handle, &context);
> 
> This is the same _OSC that is evaluated in
> acpi_bus_osc_negotiate_platform_control(), right?

Yes, but different UUID.

> So shouldn't the capbuf[OSC_SUPPORT_DWORD] be whatever is negotiated
> through acpi_bus_osc_negotiate_platform_control()?  At least that
> would be consistent with acpi_pci_osc_control_set().

The ACPI 6.4 spec says that the support field for this _OSC is reserved
(table 6.14 in the spec). So as far as I can tell this is not the same
as what we pass for the platform _OSC.

Re: [PATCH 5/6] ACPI: Add support for native USB4 control _OSC

[Rafael J. Wysocki]

On Tue, Jan 26, 2021 at 6:46 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Tue, Jan 26, 2021 at 06:35:20PM +0100, Rafael J. Wysocki wrote:
> > > +       capbuf[OSC_QUERY_DWORD] = 0;
> > > +       capbuf[OSC_SUPPORT_DWORD] = 0;
> > > +       capbuf[OSC_CONTROL_DWORD] = control;
> > > +
> > > +       status = acpi_run_osc(handle, &context);
> >
> > This is the same _OSC that is evaluated in
> > acpi_bus_osc_negotiate_platform_control(), right?
>
> Yes, but different UUID.

I've missed that, sorry for the noise.

Re: [PATCH 5/6] ACPI: Add support for native USB4 control _OSC

[Rafael J. Wysocki]

On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> ACPI 6.4 introduced a new _OSC capability that is used negotiate native
> connection manager support. Connection manager is the entity that is
> responsible for tunneling over the USB4 fabric. If the platform rejects
> the native access then firmware based connection manager is used.
>
> The new _OSC also includes a set of bits that can be used to disable
> certain tunnel types such as PCIe for security reasons for instance.
>
> This implements the new USB4 _OSC so that we try to negotiate native
> USB4 support if the Thunderbolt/USB4 (CONFIG_USB4) driver is enabled.
> Drivers can determine what was negotiated by checking two new variables
> exposed in this patch.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/bus.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h | 10 ++++++
>  2 files changed, 86 insertions(+)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index ca7c7b2bf56e..f7ad2d283e51 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -281,6 +281,12 @@ bool osc_sb_apei_support_acked;
>  bool osc_pc_lpi_support_confirmed;
>  EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
>
> +/*
> + * ACPI 6.4 Operating System Capabilities for USB.
> + */
> +bool osc_sb_native_usb4_support_confirmed;
> +EXPORT_SYMBOL_GPL(osc_sb_native_usb4_support_confirmed);
> +
>  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
>  static void acpi_bus_osc_negotiate_platform_control(void)
>  {
> @@ -317,6 +323,9 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>         if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_DIVERSE_HIGH_SUPPORT;
>
> +       if (IS_ENABLED(CONFIG_USB4))
> +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_NATIVE_USB4_SUPPORT;
> +
>         if (!ghes_disable)
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
>         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> @@ -348,8 +357,74 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>                         capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
>                 osc_pc_lpi_support_confirmed =
>                         capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
> +               osc_sb_native_usb4_support_confirmed =
> +                       capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
> +       }
> +
> +       kfree(context.ret.pointer);
> +}
> +
> +/*
> + * Native control of USB4 capabilities. If any of the tunneling bits is
> + * set it means OS is in control and we use software based connection
> + * manager.
> + */
> +u32 osc_sb_native_usb4_control;
> +EXPORT_SYMBOL_GPL(osc_sb_native_usb4_control);
> +
> +static void acpi_bus_decode_usb_osc(const char *msg, u32 bits)
> +{
> +       printk(KERN_INFO PREFIX "%s USB3%c DisplayPort%c PCIe%c XDomain%c\n", msg,
> +              (bits & OSC_USB_USB3_TUNNELING) ? '+' : '-',
> +              (bits & OSC_USB_DP_TUNNELING) ? '+' : '-',
> +              (bits & OSC_USB_PCIE_TUNNELING) ? '+' : '-',
> +              (bits & OSC_USB_XDOMAIN) ? '+' : '-');
> +}
> +
> +static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
> +static void acpi_bus_osc_negotiate_usb_control(void)
> +{
> +       u32 capbuf[3];
> +       struct acpi_osc_context context = {
> +               .uuid_str = sb_usb_uuid_str,
> +               .rev = 1,
> +               .cap.length = sizeof(capbuf),
> +               .cap.pointer = capbuf,
> +       };
> +       acpi_handle handle;
> +       acpi_status status;
> +       u32 control;
> +
> +       if (!osc_sb_native_usb4_support_confirmed)
> +               return;
> +
> +       if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> +               return;
> +
> +       control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING |
> +                 OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN;
> +
> +       capbuf[OSC_QUERY_DWORD] = 0;
> +       capbuf[OSC_SUPPORT_DWORD] = 0;
> +       capbuf[OSC_CONTROL_DWORD] = control;
> +
> +       status = acpi_run_osc(handle, &context);
> +       if (ACPI_FAILURE(status))
> +               return;
> +
> +       if (context.ret.length != sizeof(capbuf)) {
> +               printk(KERN_INFO PREFIX "USB4 _OSC: returned invalid length buffer\n");
> +               goto out_free;
>         }
>
> +       osc_sb_native_usb4_control =
> +               control & ((u32 *)context.ret.pointer)[OSC_CONTROL_DWORD];
> +
> +       acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control);
> +       acpi_bus_decode_usb_osc("USB4 _OSC: OS controls",
> +                               osc_sb_native_usb4_control);
> +
> +out_free:
>         kfree(context.ret.pointer);
>  }
>
> @@ -1188,6 +1263,7 @@ static int __init acpi_bus_init(void)
>          * so it must be run after ACPI_FULL_INITIALIZATION
>          */
>         acpi_bus_osc_negotiate_platform_control();
> +       acpi_bus_osc_negotiate_usb_control();
>
>         /*
>          * _PDC control method may load dynamic SSDT tables,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 2630c2e953f7..ac68c2d4e393 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -546,9 +546,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
>  #define OSC_SB_OSLPI_SUPPORT                   0x00000100
>  #define OSC_SB_CPC_DIVERSE_HIGH_SUPPORT                0x00001000
>  #define OSC_SB_GENERIC_INITIATOR_SUPPORT       0x00002000
> +#define OSC_SB_NATIVE_USB4_SUPPORT             0x00040000
>
>  extern bool osc_sb_apei_support_acked;
>  extern bool osc_pc_lpi_support_confirmed;
> +extern bool osc_sb_native_usb4_support_confirmed;
> +
> +/* USB4 Capabilities */
> +#define OSC_USB_USB3_TUNNELING                 0x00000001
> +#define OSC_USB_DP_TUNNELING                   0x00000002
> +#define OSC_USB_PCIE_TUNNELING                 0x00000004
> +#define OSC_USB_XDOMAIN                                0x00000008
> +
> +extern u32 osc_sb_native_usb4_control;
>
>  /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
>  #define OSC_PCI_EXT_CONFIG_SUPPORT             0x00000001
> --
> 2.29.2
>

RE: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Limonciello, Mario]

> I would put that information into the changelog.

Thanks, @Mika Westerberg can you collapse that in when you re-spin the
series?

> 
> Moreover, have you looked at acpi_pci_osc_control_set()?
> 
> What it does is analogous to what you are proposing, but a bit
> different, and I would like to preserve consistency between _OSC use
> cases.
> 
> So would it be possible to adjust the _SB _OSC evaluation flow to
> follow the PCI _OSC one?  That is, if any control bits are there, pass
> them along with the last evaluation of _OSC with the query flag clear.
> Or is the latter defective and if so then why?

Basically the only difference is another line cloning OSC_CONTROL_DWORD from
capbuf_ret to capbuf?

Yes, this actually sounds like it better adheres to the spec to me.

Quoting spec:
" If the OS is granted control of a feature in the Control Field in one call to
_OSC, then it must preserve the set state of that bit (requesting that feature)
in all subsequent calls."


> 
> >
> > >
> > > > and this is going to cause problems with the USB4 CM (Connection
> > > > Manager) switch that is going to commit the switch only when the OS
> > > > requests control over the feature.
> > > >
> > > > For this reason modify the _OSC support so that we first execute it with
> > > > query bit set, then use the returned valu as base of the features we
> > >
> > > s/valu/value/
> > >
> > > > want to control and run the _OSC again with query bit clear.
> > > >
> > > > Also rename the function to better match what it does.
> > > >
> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > >
> > > > ---
> > > >  drivers/acpi/bus.c | 43 +++++++++++++++++++++++++++++++------------
> > > >  1 file changed, 31 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > > index 1682f8b454a2..ca7c7b2bf56e 100644
> > > > --- a/drivers/acpi/bus.c
> > > > +++ b/drivers/acpi/bus.c
> > > > @@ -282,9 +282,9 @@ bool osc_pc_lpi_support_confirmed;
> > > >  EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
> > > >
> > > >  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> > > > -static void acpi_bus_osc_support(void)
> > > > +static void acpi_bus_osc_negotiate_platform_control(void)
> > > >  {
> > > > -       u32 capbuf[2];
> > > > +       u32 capbuf[2], *capbuf_ret;
> > > >         struct acpi_osc_context context = {
> > > >                 .uuid_str = sb_uuid_str,
> > > >                 .rev = 1,
> > > > @@ -321,17 +321,36 @@ static void acpi_bus_osc_support(void)
> > > >                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> > > >         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> > > >                 return;
> > > > -       if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
> > > > -               u32 *capbuf_ret = context.ret.pointer;
> > > > -               if (context.ret.length > OSC_SUPPORT_DWORD) {
> > > > -                       osc_sb_apei_support_acked =
> > > > -                               capbuf_ret[OSC_SUPPORT_DWORD] &
> > > OSC_SB_APEI_SUPPORT;
> > > > -                       osc_pc_lpi_support_confirmed =
> > > > -                               capbuf_ret[OSC_SUPPORT_DWORD] &
> > > OSC_SB_PCLPI_SUPPORT;
> > > > -               }
> > > > +
> > > > +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > > +               return;
> > > > +
> > > > +       capbuf_ret = context.ret.pointer;
> > > > +       if (context.ret.length <= OSC_SUPPORT_DWORD) {
> > > >                 kfree(context.ret.pointer);
> > > > +               return;
> > > >         }
> > > > -       /* do we need to check other returned cap? Sounds no */
> > > > +
> > > > +       /*
> > > > +        * Now run _OSC again with query flag clean and with the caps
> > >
> > > s/clean/clear/
> > >
> > > > +        * both platform and OS supports.
> > >
> > > s/both platform and OS supports/supported by both the OS and the platform/
> > >
> > > > +        */
> > > > +       capbuf[OSC_QUERY_DWORD] = 0;
> > > > +       capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> > > > +       kfree(context.ret.pointer);
> > > > +
> > > > +       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > > +               return;
> > > > +
> > > > +       capbuf_ret = context.ret.pointer;
> > > > +       if (context.ret.length > OSC_SUPPORT_DWORD) {
> > > > +               osc_sb_apei_support_acked =
> > > > +                       capbuf_ret[OSC_SUPPORT_DWORD] &
> OSC_SB_APEI_SUPPORT;
> > > > +               osc_pc_lpi_support_confirmed =
> > > > +                       capbuf_ret[OSC_SUPPORT_DWORD] &
> > > OSC_SB_PCLPI_SUPPORT;
> > > > +       }
> > > > +
> > > > +       kfree(context.ret.pointer);
> > > >  }
> > > >
> > > >  /* --------------------------------------------------------------------
> ----
> > > --
> > > > @@ -1168,7 +1187,7 @@ static int __init acpi_bus_init(void)
> > > >          * _OSC method may exist in module level code,
> > > >          * so it must be run after ACPI_FULL_INITIALIZATION
> > > >          */
> > > > -       acpi_bus_osc_support();
> > > > +       acpi_bus_osc_negotiate_platform_control();
> > > >
> > > >         /*
> > > >          * _PDC control method may load dynamic SSDT tables,
> > > > --
> > > > 2.29.2
> > > >

Re: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Mika Westerberg]

On Tue, Jan 26, 2021 at 10:43:32PM +0000, Limonciello, Mario wrote:
> > I would put that information into the changelog.
> 
> Thanks, @Mika Westerberg can you collapse that in when you re-spin the
> series?

Sure.

> > 
> > Moreover, have you looked at acpi_pci_osc_control_set()?
> > 
> > What it does is analogous to what you are proposing, but a bit
> > different, and I would like to preserve consistency between _OSC use
> > cases.
> > 
> > So would it be possible to adjust the _SB _OSC evaluation flow to
> > follow the PCI _OSC one?  That is, if any control bits are there, pass
> > them along with the last evaluation of _OSC with the query flag clear.
> > Or is the latter defective and if so then why?
> 
> Basically the only difference is another line cloning OSC_CONTROL_DWORD from
> capbuf_ret to capbuf?
> 
> Yes, this actually sounds like it better adheres to the spec to me.
> 
> Quoting spec:
> " If the OS is granted control of a feature in the Control Field in one call to
> _OSC, then it must preserve the set state of that bit (requesting that feature)
> in all subsequent calls."

However, the platform wide _OSC does not actually have this
OSC_CONTROL_DWORD at all ;-)

I think what we do in this patch is already equivalent to what the PCI
_OSC is doing:

  1. Query bit set _OSC
  2. Take the returned OSC_SUPPORT_DWORD buffer and
  3. Pass it to the _OSC with query bit clear.

I may be missing something, though.

Re: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Rafael J. Wysocki]

On Wed, Jan 27, 2021 at 1:49 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Jan 26, 2021 at 10:43:32PM +0000, Limonciello, Mario wrote:
> > > I would put that information into the changelog.
> >
> > Thanks, @Mika Westerberg can you collapse that in when you re-spin the
> > series?
>
> Sure.
>
> > >
> > > Moreover, have you looked at acpi_pci_osc_control_set()?
> > >
> > > What it does is analogous to what you are proposing, but a bit
> > > different, and I would like to preserve consistency between _OSC use
> > > cases.
> > >
> > > So would it be possible to adjust the _SB _OSC evaluation flow to
> > > follow the PCI _OSC one?  That is, if any control bits are there, pass
> > > them along with the last evaluation of _OSC with the query flag clear.
> > > Or is the latter defective and if so then why?
> >
> > Basically the only difference is another line cloning OSC_CONTROL_DWORD from
> > capbuf_ret to capbuf?
> >
> > Yes, this actually sounds like it better adheres to the spec to me.
> >
> > Quoting spec:
> > " If the OS is granted control of a feature in the Control Field in one call to
> > _OSC, then it must preserve the set state of that bit (requesting that feature)
> > in all subsequent calls."
>
> However, the platform wide _OSC does not actually have this
> OSC_CONTROL_DWORD at all ;-)

Right.

> I think what we do in this patch is already equivalent to what the PCI
> _OSC is doing:
>
>   1. Query bit set _OSC
>   2. Take the returned OSC_SUPPORT_DWORD buffer and
>   3. Pass it to the _OSC with query bit clear.

Yes, it is.

Given the way the USB4 _OSC protocol is defined (which admittedly
confused me somewhat), the code changes in this patch are fine by me.

Thanks and sorry for the confusion.

Re: [PATCH 1/6] thunderbolt: Fix possible NULL pointer dereference in tb_acpi_add_link()

[Mika Westerberg]

On Tue, Jan 26, 2021 at 06:57:18PM +0300, Mika Westerberg wrote:
> From: Mario Limonciello <mario.limonciello@dell.com>
> 
> When we walk up the device hierarchy in tb_acpi_add_link() make sure we
> break the loop if the device has no parent. Otherwise we may crash the
> kernel by dereferencing a NULL pointer.
> 
> Fixes: b2be2b05cf3b ("thunderbolt: Create device links from ACPI description")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied this one separately to thunderbolt.git/fixes.