All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] thunderbolt: Start lane initialization after sleep
@ 2021-01-05  9:28 Mika Westerberg
  2021-01-05  9:28 ` [PATCH 2/2] thunderbolt: Add support for de-authorizing devices Mika Westerberg
  2021-01-05 13:35 ` [PATCH 1/2] thunderbolt: Start lane initialization after sleep Yehezkel Bernat
  0 siblings, 2 replies; 9+ messages in thread
From: Mika Westerberg @ 2021-01-05  9:28 UTC (permalink / raw)
  To: linux-usb
  Cc: Michael Jamet, Yehezkel Bernat, Lukas Wunner, Andreas Noever,
	Christian Kellner, Mario Limonciello, Mika Westerberg

USB4 spec says that for TBT3 compatible device routers the connection
manager needs to set SLI (Start Lane Initialization) to get the lanes
that were not connected back to functional state after sleep. Same needs
to be done if the link was XDomain.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/lc.c      | 35 +++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/switch.c  | 27 ++++++++++++++++++++++++++-
 drivers/thunderbolt/tb.h      |  1 +
 drivers/thunderbolt/tb_regs.h |  1 +
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c
index 41e6c738f6c8..bc671730a11f 100644
--- a/drivers/thunderbolt/lc.c
+++ b/drivers/thunderbolt/lc.c
@@ -158,6 +158,41 @@ void tb_lc_unconfigure_xdomain(struct tb_port *port)
 	tb_lc_set_xdomain_configured(port, false);
 }
 
+/**
+ * tb_lc_start_lane_initialization() - Start lane initialization
+ * @port: Device router lane 0 adapter
+ *
+ * Starts lane initialization for @port after the router resumed from
+ * sleep. Should be called for those downstream lane adapters that were
+ * not connected (tb_lc_configure_port() was not called) before sleep.
+ *
+ * Returns %0 in success and negative errno in case of failure.
+ */
+int tb_lc_start_lane_initialization(struct tb_port *port)
+{
+	struct tb_switch *sw = port->sw;
+	int ret, cap;
+	u32 ctrl;
+
+	if (!tb_route(sw))
+		return 0;
+
+	if (sw->generation < 2)
+		return 0;
+
+	cap = find_port_lc_cap(port);
+	if (cap < 0)
+		return cap;
+
+	ret = tb_sw_read(sw, &ctrl, TB_CFG_SWITCH, cap + TB_LC_SX_CTRL, 1);
+	if (ret)
+		return ret;
+
+	ctrl |= TB_LC_SX_CTRL_SLI;
+
+	return tb_sw_write(sw, &ctrl, TB_CFG_SWITCH, cap + TB_LC_SX_CTRL, 1);
+}
+
 static int tb_lc_set_wake_one(struct tb_switch *sw, unsigned int offset,
 			      unsigned int flags)
 {
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index a8572f49d3ad..e7e6726ff5d1 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1065,6 +1065,17 @@ void tb_port_lane_bonding_disable(struct tb_port *port)
 	tb_port_set_link_width(port, 1);
 }
 
+static int tb_port_start_lane_initialization(struct tb_port *port)
+{
+	int ret;
+
+	if (tb_switch_is_usb4(port->sw))
+		return 0;
+
+	ret = tb_lc_start_lane_initialization(port);
+	return ret == -EINVAL ? 0 : ret;
+}
+
 /**
  * tb_port_is_enabled() - Is the adapter port enabled
  * @port: Port to check
@@ -2694,8 +2705,22 @@ int tb_switch_resume(struct tb_switch *sw)
 
 	/* check for surviving downstream switches */
 	tb_switch_for_each_port(sw, port) {
-		if (!tb_port_has_remote(port) && !port->xdomain)
+		if (!tb_port_has_remote(port) && !port->xdomain) {
+			/*
+			 * For disconnected downstream lane adapters
+			 * start lane initialization now so we detect
+			 * future connects.
+			 */
+			if (!tb_is_upstream_port(port) && tb_port_is_null(port))
+				tb_port_start_lane_initialization(port);
 			continue;
+		} else if (port->xdomain) {
+			/*
+			 * Start lane initialization for XDomain so the
+			 * link gets re-established.
+			 */
+			tb_port_start_lane_initialization(port);
+		}
 
 		if (tb_wait_for_port(port, true) <= 0) {
 			tb_port_warn(port,
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 554feda1e359..34ae83b9e52a 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -923,6 +923,7 @@ int tb_lc_configure_port(struct tb_port *port);
 void tb_lc_unconfigure_port(struct tb_port *port);
 int tb_lc_configure_xdomain(struct tb_port *port);
 void tb_lc_unconfigure_xdomain(struct tb_port *port);
+int tb_lc_start_lane_initialization(struct tb_port *port);
 int tb_lc_set_wake(struct tb_switch *sw, unsigned int flags);
 int tb_lc_set_sleep(struct tb_switch *sw);
 bool tb_lc_lane_bonding_possible(struct tb_switch *sw);
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index ae427a953489..626751e06292 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -464,6 +464,7 @@ struct tb_regs_hop {
 #define TB_LC_SX_CTRL_L1D		BIT(17)
 #define TB_LC_SX_CTRL_L2C		BIT(20)
 #define TB_LC_SX_CTRL_L2D		BIT(21)
+#define TB_LC_SX_CTRL_SLI		BIT(29)
 #define TB_LC_SX_CTRL_UPSTREAM		BIT(30)
 #define TB_LC_SX_CTRL_SLP		BIT(31)
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
  2021-01-05  9:28 [PATCH 1/2] thunderbolt: Start lane initialization after sleep Mika Westerberg
@ 2021-01-05  9:28 ` Mika Westerberg
  2021-01-05 13:53   ` Yehezkel Bernat
  2021-01-05 13:35 ` [PATCH 1/2] thunderbolt: Start lane initialization after sleep Yehezkel Bernat
  1 sibling, 1 reply; 9+ messages in thread
From: Mika Westerberg @ 2021-01-05  9:28 UTC (permalink / raw)
  To: linux-usb
  Cc: Michael Jamet, Yehezkel Bernat, Lukas Wunner, Andreas Noever,
	Christian Kellner, Mario Limonciello, Mika Westerberg

In some cases it is useful to be able de-authorize devices. For example
if user logs out the userspace can have a policy that disconnects PCIe
devices until logged in again. This is only possible for software based
connection manager as it directly controls the tunnels.

For this reason make the authorized attribute accept writing 0 which
makes the software connection manager to tear down the corresponding
PCIe tunnel. Userspace can check if this is supported by reading a new
domain attribute deauthorization, that holds 1 in that case.

While there correct tb_domain_approve_switch() kernel-doc and
description of authorized attribute to mention that it is only about
PCIe tunnels.

Cc: Christian Kellner <christian@kellner.me>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 .../ABI/testing/sysfs-bus-thunderbolt         | 18 +++++++---
 drivers/thunderbolt/domain.c                  | 32 +++++++++++++++--
 drivers/thunderbolt/switch.c                  | 34 ++++++++++++++++++-
 drivers/thunderbolt/tb.c                      | 20 +++++++++++
 drivers/thunderbolt/tb.h                      |  3 ++
 5 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index a91b4b24496e..81aa090137b4 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -49,6 +49,15 @@ Description:	Holds a comma separated list of device unique_ids that
 		If a device is authorized automatically during boot its
 		boot attribute is set to 1.
 
+What: /sys/bus/thunderbolt/devices/.../domainX/deauthorization
+Date:		May 2021
+KernelVersion:	5.12
+Contact:	Mika Westerberg <mika.westerberg@linux.intel.com>
+Description:	This attribute tells whether the system supports
+		de-authorization of devices. Value of 1 means user can
+		de-authorize PCIe tunnel by writing 0 to authorized
+		attribute under each device.
+
 What: /sys/bus/thunderbolt/devices/.../domainX/iommu_dma_protection
 Date:		Mar 2019
 KernelVersion:	4.21
@@ -84,17 +93,18 @@ KernelVersion:	4.13
 Contact:	thunderbolt-software@lists.01.org
 Description:	This attribute is used to authorize Thunderbolt devices
 		after they have been connected. If the device is not
-		authorized, no devices such as PCIe and Display port are
-		available to the system.
+		authorized, no PCIe devices are available to the system.
 
 		Contents of this attribute will be 0 when the device is not
 		yet authorized.
 
 		Possible values are supported:
 
-		==  ===========================================
+		==  ===================================================
+		0   The device will be de-authorized (only supported if
+		    deauthorization attribute under domain contains 1)
 		1   The device will be authorized and connected
-		==  ===========================================
+		==  ===================================================
 
 		When key attribute contains 32 byte hex string the possible
 		values are:
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index f0de94f7acbf..5826f7a29742 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -238,6 +238,16 @@ static ssize_t boot_acl_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(boot_acl);
 
+static ssize_t deauthorization_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	const struct tb *tb = container_of(dev, struct tb, dev);
+
+	return sprintf(buf, "%d\n", !!tb->cm_ops->disapprove_switch);
+}
+static DEVICE_ATTR_RO(deauthorization);
+
 static ssize_t iommu_dma_protection_show(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
@@ -267,6 +277,7 @@ static DEVICE_ATTR_RO(security);
 
 static struct attribute *domain_attrs[] = {
 	&dev_attr_boot_acl.attr,
+	&dev_attr_deauthorization.attr,
 	&dev_attr_iommu_dma_protection.attr,
 	&dev_attr_security.attr,
 	NULL,
@@ -601,14 +612,31 @@ int tb_domain_runtime_resume(struct tb *tb)
 	return 0;
 }
 
+/**
+ * tb_domain_disapprove_switch() - Disapprove switch
+ * @tb: Domain the switch belongs to
+ * @sw: Switch to disapprove
+ *
+ * This will disconnect PCIe tunnel from parent to this @sw.
+ *
+ * Return: %0 on success and negative errno in case of failure.
+ */
+int tb_domain_disapprove_switch(struct tb *tb, struct tb_switch *sw)
+{
+	if (!tb->cm_ops->disapprove_switch)
+		return -EPERM;
+
+	return tb->cm_ops->disapprove_switch(tb, sw);
+}
+
 /**
  * tb_domain_approve_switch() - Approve switch
  * @tb: Domain the switch belongs to
  * @sw: Switch to approve
  *
  * This will approve switch by connection manager specific means. In
- * case of success the connection manager will create tunnels for all
- * supported protocols.
+ * case of success the connection manager will create PCIe tunnel from
+ * parent to @sw.
  */
 int tb_domain_approve_switch(struct tb *tb, struct tb_switch *sw)
 {
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index e7e6726ff5d1..83e17631884c 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1387,6 +1387,30 @@ static ssize_t authorized_show(struct device *dev,
 	return sprintf(buf, "%u\n", sw->authorized);
 }
 
+static int disapprove_switch(struct device *dev, void *data)
+{
+	struct tb_switch *sw;
+
+	sw = tb_to_switch(dev);
+	if (sw && sw->authorized) {
+		int ret;
+
+		/* First children */
+		ret = device_for_each_child_reverse(&sw->dev, NULL, disapprove_switch);
+		if (ret)
+			return ret;
+
+		ret = tb_domain_disapprove_switch(sw->tb, sw);
+		if (ret)
+			return ret;
+
+		sw->authorized = 0;
+		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
+	}
+
+	return 0;
+}
+
 static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
 {
 	int ret = -EINVAL;
@@ -1394,10 +1418,18 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
 	if (!mutex_trylock(&sw->tb->lock))
 		return restart_syscall();
 
-	if (sw->authorized)
+	if (!!sw->authorized == !!val)
 		goto unlock;
 
 	switch (val) {
+	/* Disapprove switch */
+	case 0:
+		if (tb_route(sw)) {
+			ret = disapprove_switch(&sw->dev, NULL);
+			goto unlock;
+		}
+		break;
+
 	/* Approve switch */
 	case 1:
 		if (sw->key)
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 51d5b031cada..d08879849abe 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1002,6 +1002,25 @@ static void tb_disconnect_and_release_dp(struct tb *tb)
 	}
 }
 
+static int tb_disconnect_pci(struct tb *tb, struct tb_switch *sw)
+{
+	struct tb_tunnel *tunnel;
+	struct tb_port *up;
+
+	up = tb_switch_find_port(sw, TB_TYPE_PCIE_UP);
+	if (WARN_ON(!up))
+		return -ENODEV;
+
+	tunnel = tb_find_tunnel(tb, TB_TUNNEL_PCI, NULL, up);
+	if (WARN_ON(!tunnel))
+		return -ENODEV;
+
+	tb_tunnel_deactivate(tunnel);
+	list_del(&tunnel->list);
+	tb_tunnel_free(tunnel);
+	return 0;
+}
+
 static int tb_tunnel_pci(struct tb *tb, struct tb_switch *sw)
 {
 	struct tb_port *up, *down, *port;
@@ -1512,6 +1531,7 @@ static const struct tb_cm_ops tb_cm_ops = {
 	.runtime_suspend = tb_runtime_suspend,
 	.runtime_resume = tb_runtime_resume,
 	.handle_event = tb_handle_event,
+	.disapprove_switch = tb_disconnect_pci,
 	.approve_switch = tb_tunnel_pci,
 	.approve_xdomain_paths = tb_approve_xdomain_paths,
 	.disconnect_xdomain_paths = tb_disconnect_xdomain_paths,
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 34ae83b9e52a..31468de658e4 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -361,6 +361,7 @@ struct tb_path {
  * @handle_event: Handle thunderbolt event
  * @get_boot_acl: Get boot ACL list
  * @set_boot_acl: Set boot ACL list
+ * @disapprove_switch: Disapprove switch (disconnect PCIe tunnel)
  * @approve_switch: Approve switch
  * @add_switch_key: Add key to switch
  * @challenge_switch_key: Challenge switch using key
@@ -394,6 +395,7 @@ struct tb_cm_ops {
 			     const void *buf, size_t size);
 	int (*get_boot_acl)(struct tb *tb, uuid_t *uuids, size_t nuuids);
 	int (*set_boot_acl)(struct tb *tb, const uuid_t *uuids, size_t nuuids);
+	int (*disapprove_switch)(struct tb *tb, struct tb_switch *sw);
 	int (*approve_switch)(struct tb *tb, struct tb_switch *sw);
 	int (*add_switch_key)(struct tb *tb, struct tb_switch *sw);
 	int (*challenge_switch_key)(struct tb *tb, struct tb_switch *sw,
@@ -629,6 +631,7 @@ int tb_domain_thaw_noirq(struct tb *tb);
 void tb_domain_complete(struct tb *tb);
 int tb_domain_runtime_suspend(struct tb *tb);
 int tb_domain_runtime_resume(struct tb *tb);
+int tb_domain_disapprove_switch(struct tb *tb, struct tb_switch *sw);
 int tb_domain_approve_switch(struct tb *tb, struct tb_switch *sw);
 int tb_domain_approve_switch_key(struct tb *tb, struct tb_switch *sw);
 int tb_domain_challenge_switch_key(struct tb *tb, struct tb_switch *sw);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] thunderbolt: Start lane initialization after sleep
  2021-01-05  9:28 [PATCH 1/2] thunderbolt: Start lane initialization after sleep Mika Westerberg
  2021-01-05  9:28 ` [PATCH 2/2] thunderbolt: Add support for de-authorizing devices Mika Westerberg
@ 2021-01-05 13:35 ` Yehezkel Bernat
  2021-01-11 14:22   ` Mika Westerberg
  1 sibling, 1 reply; 9+ messages in thread
From: Yehezkel Bernat @ 2021-01-05 13:35 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-usb, Michael Jamet, Lukas Wunner, Andreas Noever,
	Christian Kellner, Mario Limonciello

On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> USB4 spec says that for TBT3 compatible device routers the connection
> manager needs to set SLI (Start Lane Initialization) to get the lanes
> that were not connected back to functional state after sleep. Same needs
> to be done if the link was XDomain.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/thunderbolt/lc.c      | 35 +++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/switch.c  | 27 ++++++++++++++++++++++++++-
>  drivers/thunderbolt/tb.h      |  1 +
>  drivers/thunderbolt/tb_regs.h |  1 +
>  4 files changed, 63 insertions(+), 1 deletion(-)
>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
  2021-01-05  9:28 ` [PATCH 2/2] thunderbolt: Add support for de-authorizing devices Mika Westerberg
@ 2021-01-05 13:53   ` Yehezkel Bernat
  2021-01-05 16:36     ` Mika Westerberg
  0 siblings, 1 reply; 9+ messages in thread
From: Yehezkel Bernat @ 2021-01-05 13:53 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-usb, Michael Jamet, Lukas Wunner, Andreas Noever,
	Christian Kellner, Mario Limonciello

On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> In some cases it is useful to be able de-authorize devices. For example
> if user logs out the userspace can have a policy that disconnects PCIe
> devices until logged in again. This is only possible for software based
> connection manager as it directly controls the tunnels.
>
> For this reason make the authorized attribute accept writing 0 which
> makes the software connection manager to tear down the corresponding
> PCIe tunnel. Userspace can check if this is supported by reading a new
> domain attribute deauthorization, that holds 1 in that case.

What a great feature! Thanks for implementing it.

BTW, is there any general way to disable the device operations before such a
disconnection? The user has a way to stop removable disks, for example, but
maybe other devices need additional precaution from the user (eGPU?).


>                 Possible values are supported:
>
> -               ==  ===========================================
> +               ==  ===================================================
> +               0   The device will be de-authorized (only supported if
> +                   deauthorization attribute under domain contains 1)
>                 1   The device will be authorized and connected
> -               ==  ===========================================
> +               ==  ===================================================
>
>                 When key attribute contains 32 byte hex string the possible
>                 values are:

As 0 is available for 'secure' security level too, you may want to reflect it in
the documentation here somehow.


> +static int disapprove_switch(struct device *dev, void *data)

Maybe it's better to mark `data` as `__maybe_unused`?

> +{
> +       struct tb_switch *sw;
> +
> +       sw = tb_to_switch(dev);
> +       if (sw && sw->authorized) {
> +               int ret;
> +
> +               /* First children */
> +               ret = device_for_each_child_reverse(&sw->dev, NULL, disapprove_switch);
> +               if (ret)
> +                       return ret;
> +
> +               ret = tb_domain_disapprove_switch(sw->tb, sw);
> +               if (ret)
> +                       return ret;
> +
> +               sw->authorized = 0;
> +               kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
> +       }
> +
> +       return 0;
> +}
> +

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
  2021-01-05 13:53   ` Yehezkel Bernat
@ 2021-01-05 16:36     ` Mika Westerberg
  2021-01-05 16:48       ` Limonciello, Mario
  0 siblings, 1 reply; 9+ messages in thread
From: Mika Westerberg @ 2021-01-05 16:36 UTC (permalink / raw)
  To: Yehezkel Bernat
  Cc: linux-usb, Michael Jamet, Lukas Wunner, Andreas Noever,
	Christian Kellner, Mario Limonciello

Hi,

On Tue, Jan 05, 2021 at 03:53:33PM +0200, Yehezkel Bernat wrote:
> On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > In some cases it is useful to be able de-authorize devices. For example
> > if user logs out the userspace can have a policy that disconnects PCIe
> > devices until logged in again. This is only possible for software based
> > connection manager as it directly controls the tunnels.
> >
> > For this reason make the authorized attribute accept writing 0 which
> > makes the software connection manager to tear down the corresponding
> > PCIe tunnel. Userspace can check if this is supported by reading a new
> > domain attribute deauthorization, that holds 1 in that case.
> 
> What a great feature! Thanks for implementing it.
> 
> BTW, is there any general way to disable the device operations before such a
> disconnection? The user has a way to stop removable disks, for example, but
> maybe other devices need additional precaution from the user (eGPU?).

There are ways but it depends on the driver, I guess. For instance eGPUS
at the moment (the ones I've tested) simply crash as their driver does
not support hot-remove ;-)

What ends up happening is essentially PCIe hot-remove so drivers that
are prepared for that should be fine. Of course if you had mounted
filesystem behind the PCIe link that was torn down you end up losing
your data, so the user interface should make sure the user is aware of
that.

> >                 Possible values are supported:
> >
> > -               ==  ===========================================
> > +               ==  ===================================================
> > +               0   The device will be de-authorized (only supported if
> > +                   deauthorization attribute under domain contains 1)
> >                 1   The device will be authorized and connected
> > -               ==  ===========================================
> > +               ==  ===================================================
> >
> >                 When key attribute contains 32 byte hex string the possible
> >                 values are:
> 
> As 0 is available for 'secure' security level too, you may want to reflect it in
> the documentation here somehow.

OK.

> > +static int disapprove_switch(struct device *dev, void *data)
> 
> Maybe it's better to mark `data` as `__maybe_unused`?

Or rename `data` as `unused`? I think in ACPI side we are doing that
but sure, I'll change it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
  2021-01-05 16:36     ` Mika Westerberg
@ 2021-01-05 16:48       ` Limonciello, Mario
  2021-01-05 17:06         ` Mika Westerberg
  0 siblings, 1 reply; 9+ messages in thread
From: Limonciello, Mario @ 2021-01-05 16:48 UTC (permalink / raw)
  To: Mika Westerberg, Yehezkel Bernat
  Cc: linux-usb, Michael Jamet, Lukas Wunner, Andreas Noever,
	Christian Kellner

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Tuesday, January 5, 2021 10:36
> To: Yehezkel Bernat
> Cc: linux-usb@vger.kernel.org; Michael Jamet; Lukas Wunner; Andreas Noever;
> Christian Kellner; Limonciello, Mario
> Subject: Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On Tue, Jan 05, 2021 at 03:53:33PM +0200, Yehezkel Bernat wrote:
> > On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > In some cases it is useful to be able de-authorize devices. For example
> > > if user logs out the userspace can have a policy that disconnects PCIe
> > > devices until logged in again. This is only possible for software based
> > > connection manager as it directly controls the tunnels.
> > >
> > > For this reason make the authorized attribute accept writing 0 which
> > > makes the software connection manager to tear down the corresponding
> > > PCIe tunnel. Userspace can check if this is supported by reading a new
> > > domain attribute deauthorization, that holds 1 in that case.

Just another idea, rather than the value of a new "deauthorize" attribute indicating
whether this is supported how about instead a "connection_manager" attribute?

My thought being userspace can potentially make a judgement call from the information
on how tunnels are going to behave (particularly in long chains from the suspend/resume
cycle coming back differently).

> >
> > What a great feature! Thanks for implementing it.

I agree, this sounds like a great idea.

> >
> > BTW, is there any general way to disable the device operations before such a
> > disconnection? The user has a way to stop removable disks, for example, but
> > maybe other devices need additional precaution from the user (eGPU?).
> 
> There are ways but it depends on the driver, I guess. For instance eGPUS
> at the moment (the ones I've tested) simply crash as their driver does
> not support hot-remove ;-)
> 
> What ends up happening is essentially PCIe hot-remove so drivers that
> are prepared for that should be fine. Of course if you had mounted
> filesystem behind the PCIe link that was torn down you end up losing
> your data, so the user interface should make sure the user is aware of
> that.

I think it's also worth mentioning this risk in the thunderbolt.rst documentation
directly.

> 
> > >                 Possible values are supported:
> > >
> > > -               ==  ===========================================
> > > +               ==  ===================================================
> > > +               0   The device will be de-authorized (only supported if
> > > +                   deauthorization attribute under domain contains 1)
> > >                 1   The device will be authorized and connected
> > > -               ==  ===========================================
> > > +               ==  ===================================================
> > >
> > >                 When key attribute contains 32 byte hex string the
> possible
> > >                 values are:
> >
> > As 0 is available for 'secure' security level too, you may want to reflect
> it in
> > the documentation here somehow.
> 
> OK.
> 
> > > +static int disapprove_switch(struct device *dev, void *data)
> >
> > Maybe it's better to mark `data` as `__maybe_unused`?
> 
> Or rename `data` as `unused`? I think in ACPI side we are doing that
> but sure, I'll change it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
  2021-01-05 16:48       ` Limonciello, Mario
@ 2021-01-05 17:06         ` Mika Westerberg
  2021-01-05 17:08           ` Limonciello, Mario
  0 siblings, 1 reply; 9+ messages in thread
From: Mika Westerberg @ 2021-01-05 17:06 UTC (permalink / raw)
  To: Limonciello, Mario
  Cc: Yehezkel Bernat, linux-usb, Michael Jamet, Lukas Wunner,
	Andreas Noever, Christian Kellner

On Tue, Jan 05, 2021 at 04:48:23PM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Sent: Tuesday, January 5, 2021 10:36
> > To: Yehezkel Bernat
> > Cc: linux-usb@vger.kernel.org; Michael Jamet; Lukas Wunner; Andreas Noever;
> > Christian Kellner; Limonciello, Mario
> > Subject: Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > Hi,
> > 
> > On Tue, Jan 05, 2021 at 03:53:33PM +0200, Yehezkel Bernat wrote:
> > > On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > >
> > > > In some cases it is useful to be able de-authorize devices. For example
> > > > if user logs out the userspace can have a policy that disconnects PCIe
> > > > devices until logged in again. This is only possible for software based
> > > > connection manager as it directly controls the tunnels.
> > > >
> > > > For this reason make the authorized attribute accept writing 0 which
> > > > makes the software connection manager to tear down the corresponding
> > > > PCIe tunnel. Userspace can check if this is supported by reading a new
> > > > domain attribute deauthorization, that holds 1 in that case.
> 
> Just another idea, rather than the value of a new "deauthorize" attribute indicating
> whether this is supported how about instead a "connection_manager" attribute?
> 
> My thought being userspace can potentially make a judgement call from the information
> on how tunnels are going to behave (particularly in long chains from the suspend/resume
> cycle coming back differently).

I went for "deauthorization" because that kind of allows this to work on
systems with firmware based connection manager too (yes, Intel Maple
Ridge is using FW CM even if it is USB4 :( ). However, at the moment the
FW CM does not support any if this but nobody knows if something like
this will be implemented there.

That said, I'm fine to change this to whatever you guys think is the
best :) If "connection_manager=sw/fw" or so is better then no problem
changing that.

> > > What a great feature! Thanks for implementing it.
> 
> I agree, this sounds like a great idea.
> 
> > >
> > > BTW, is there any general way to disable the device operations before such a
> > > disconnection? The user has a way to stop removable disks, for example, but
> > > maybe other devices need additional precaution from the user (eGPU?).
> > 
> > There are ways but it depends on the driver, I guess. For instance eGPUS
> > at the moment (the ones I've tested) simply crash as their driver does
> > not support hot-remove ;-)
> > 
> > What ends up happening is essentially PCIe hot-remove so drivers that
> > are prepared for that should be fine. Of course if you had mounted
> > filesystem behind the PCIe link that was torn down you end up losing
> > your data, so the user interface should make sure the user is aware of
> > that.
> 
> I think it's also worth mentioning this risk in the thunderbolt.rst documentation
> directly.

Sure, I'll add there mention about this.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
  2021-01-05 17:06         ` Mika Westerberg
@ 2021-01-05 17:08           ` Limonciello, Mario
  0 siblings, 0 replies; 9+ messages in thread
From: Limonciello, Mario @ 2021-01-05 17:08 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Yehezkel Bernat, linux-usb, Michael Jamet, Lukas Wunner,
	Andreas Noever, Christian Kellner



> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Tuesday, January 5, 2021 11:06
> To: Limonciello, Mario
> Cc: Yehezkel Bernat; linux-usb@vger.kernel.org; Michael Jamet; Lukas Wunner;
> Andreas Noever; Christian Kellner
> Subject: Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing devices
> 
> 
> [EXTERNAL EMAIL]
> 
> On Tue, Jan 05, 2021 at 04:48:23PM +0000, Limonciello, Mario wrote:
> > > -----Original Message-----
> > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Sent: Tuesday, January 5, 2021 10:36
> > > To: Yehezkel Bernat
> > > Cc: linux-usb@vger.kernel.org; Michael Jamet; Lukas Wunner; Andreas
> Noever;
> > > Christian Kellner; Limonciello, Mario
> > > Subject: Re: [PATCH 2/2] thunderbolt: Add support for de-authorizing
> devices
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > > Hi,
> > >
> > > On Tue, Jan 05, 2021 at 03:53:33PM +0200, Yehezkel Bernat wrote:
> > > > On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > >
> > > > > In some cases it is useful to be able de-authorize devices. For
> example
> > > > > if user logs out the userspace can have a policy that disconnects PCIe
> > > > > devices until logged in again. This is only possible for software
> based
> > > > > connection manager as it directly controls the tunnels.
> > > > >
> > > > > For this reason make the authorized attribute accept writing 0 which
> > > > > makes the software connection manager to tear down the corresponding
> > > > > PCIe tunnel. Userspace can check if this is supported by reading a new
> > > > > domain attribute deauthorization, that holds 1 in that case.
> >
> > Just another idea, rather than the value of a new "deauthorize" attribute
> indicating
> > whether this is supported how about instead a "connection_manager"
> attribute?
> >
> > My thought being userspace can potentially make a judgement call from the
> information
> > on how tunnels are going to behave (particularly in long chains from the
> suspend/resume
> > cycle coming back differently).
> 
> I went for "deauthorization" because that kind of allows this to work on
> systems with firmware based connection manager too (yes, Intel Maple
> Ridge is using FW CM even if it is USB4 :( ). However, at the moment the
> FW CM does not support any if this but nobody knows if something like
> this will be implemented there.
> 
> That said, I'm fine to change this to whatever you guys think is the
> best :) If "connection_manager=sw/fw" or so is better then no problem
> changing that.

In that case I think you're right to leave it as deauthorization.  If there
is a strong enough use case to detect connection manager at userspace (IE it
will actually do something differently from that information) that attribute
can always be exported later.

> 
> > > > What a great feature! Thanks for implementing it.
> >
> > I agree, this sounds like a great idea.
> >
> > > >
> > > > BTW, is there any general way to disable the device operations before
> such a
> > > > disconnection? The user has a way to stop removable disks, for example,
> but
> > > > maybe other devices need additional precaution from the user (eGPU?).
> > >
> > > There are ways but it depends on the driver, I guess. For instance eGPUS
> > > at the moment (the ones I've tested) simply crash as their driver does
> > > not support hot-remove ;-)
> > >
> > > What ends up happening is essentially PCIe hot-remove so drivers that
> > > are prepared for that should be fine. Of course if you had mounted
> > > filesystem behind the PCIe link that was torn down you end up losing
> > > your data, so the user interface should make sure the user is aware of
> > > that.
> >
> > I think it's also worth mentioning this risk in the thunderbolt.rst
> documentation
> > directly.
> 
> Sure, I'll add there mention about this.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] thunderbolt: Start lane initialization after sleep
  2021-01-05 13:35 ` [PATCH 1/2] thunderbolt: Start lane initialization after sleep Yehezkel Bernat
@ 2021-01-11 14:22   ` Mika Westerberg
  0 siblings, 0 replies; 9+ messages in thread
From: Mika Westerberg @ 2021-01-11 14:22 UTC (permalink / raw)
  To: Yehezkel Bernat
  Cc: linux-usb, Michael Jamet, Lukas Wunner, Andreas Noever,
	Christian Kellner, Mario Limonciello

On Tue, Jan 05, 2021 at 03:35:27PM +0200, Yehezkel Bernat wrote:
> On Tue, Jan 5, 2021 at 11:28 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > USB4 spec says that for TBT3 compatible device routers the connection
> > manager needs to set SLI (Start Lane Initialization) to get the lanes
> > that were not connected back to functional state after sleep. Same needs
> > to be done if the link was XDomain.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/thunderbolt/lc.c      | 35 +++++++++++++++++++++++++++++++++++
> >  drivers/thunderbolt/switch.c  | 27 ++++++++++++++++++++++++++-
> >  drivers/thunderbolt/tb.h      |  1 +
> >  drivers/thunderbolt/tb_regs.h |  1 +
> >  4 files changed, 63 insertions(+), 1 deletion(-)
> >
> 
> Acked-by: Yehezkel Bernat <YehezkelShB@gmail.com>

Applied to thunderbolt.git/next.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-01-11 14:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05  9:28 [PATCH 1/2] thunderbolt: Start lane initialization after sleep Mika Westerberg
2021-01-05  9:28 ` [PATCH 2/2] thunderbolt: Add support for de-authorizing devices Mika Westerberg
2021-01-05 13:53   ` Yehezkel Bernat
2021-01-05 16:36     ` Mika Westerberg
2021-01-05 16:48       ` Limonciello, Mario
2021-01-05 17:06         ` Mika Westerberg
2021-01-05 17:08           ` Limonciello, Mario
2021-01-05 13:35 ` [PATCH 1/2] thunderbolt: Start lane initialization after sleep Yehezkel Bernat
2021-01-11 14:22   ` Mika Westerberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.