| Message ID | 20210105092808.15817-1-mika.westerberg@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] thunderbolt: Start lane initialization after sleep | expand |
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>
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; > +} > +
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.
> -----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.
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.
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.
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)
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(-)