[PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates

[PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates

[Mario Limonciello]

Currently updates to Thunderbolt and USB4 controllers are fully atomic
actions. When writing into the non-active NVM nothing gets flushed to
the hardware until authenticate is sent.

There has been some desire to improve the perceived performance of these
updates, particularly for userland that may perform the update upon
a performance sensitive time like logging out.

So allow userland to flush the image to hardware at runtime, and then
allow authenticating the image at another time.

For the Dell WD19TB some specific hardware capability exists that allows
extending this to automatically complete the update when unplugged.
Export that functionality to userspace as well.

This patch series is done relative thunderbolt.git/next.

Mario Limonciello (2):
  thunderbolt: Add support for separating the flush to SPI and
    authenticate
  thunderbolt: Add support for authenticate on disconnect

 .../ABI/testing/sysfs-bus-thunderbolt         | 24 +++++-
 drivers/thunderbolt/Makefile                  |  1 +
 drivers/thunderbolt/eeprom.c                  |  2 +
 drivers/thunderbolt/lc.c                      | 14 ++++
 drivers/thunderbolt/quirks.c                  | 38 +++++++++
 drivers/thunderbolt/switch.c                  | 81 +++++++++++++++----
 drivers/thunderbolt/tb-quirks.h               | 16 ++++
 drivers/thunderbolt/tb.h                      |  4 +
 drivers/thunderbolt/tb_regs.h                 |  1 +
 9 files changed, 162 insertions(+), 19 deletions(-)
 create mode 100644 drivers/thunderbolt/quirks.c
 create mode 100644 drivers/thunderbolt/tb-quirks.h

-- 
2.25.1

[PATCH 1/2] thunderbolt: Add support for separating the flush to SPI and authenticate

[Mario Limonciello]

This allows userspace to have a shorter period of time that the device
is unusable and to call it at a more convenient time.

For example flushing the image may happen while the user is using the
machine and authenticating/rebooting may happen while logging out.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 .../ABI/testing/sysfs-bus-thunderbolt         | 11 ++++-
 drivers/thunderbolt/switch.c                  | 43 ++++++++++++-------
 drivers/thunderbolt/tb.h                      |  1 +
 3 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index 82e80de78dd0..26b15cbc9881 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -178,11 +178,18 @@ KernelVersion:	4.13
 Contact:	thunderbolt-software@lists.01.org
 Description:	When new NVM image is written to the non-active NVM
 		area (through non_activeX NVMem device), the
-		authentication procedure is started by writing 1 to
-		this file. If everything goes well, the device is
+		authentication procedure is started by writing to
+		this file.
+		If everything goes well, the device is
 		restarted with the new NVM firmware. If the image
 		verification fails an error code is returned instead.
 
+		This file will accept writing values "1" or "2"
+		- Writing "1" will flush the image to the storage
+		area and authenticate the image in one action.
+		- Writing "2" will only flush the image to the storage
+		area.
+
 		When read holds status of the last authentication
 		operation if an error occurred during the process. This
 		is directly the status value from the DMA configuration
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index d7d60cd9226f..4c476a58db38 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -27,6 +27,11 @@
 #define NVM_MIN_SIZE		SZ_32K
 #define NVM_MAX_SIZE		SZ_512K
 
+enum nvm_write_ops {
+	WRITE_AND_AUTHENTICATE = 1,
+	WRITE_ONLY = 2,
+};
+
 static DEFINE_IDA(nvm_ida);
 
 struct nvm_auth_status {
@@ -164,8 +169,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
 	}
 
 	if (tb_switch_is_usb4(sw))
-		return usb4_switch_nvm_write(sw, 0, buf, image_size);
-	return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
+		ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
+	else
+		ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
+	if (!ret)
+		sw->nvm->flushed = true;
+	return ret;
 }
 
 static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
@@ -371,6 +380,7 @@ static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
 		}
 	}
 
+	sw->nvm->flushed = false;
 	sw->nvm->buf_data_size = offset + bytes;
 	memcpy(sw->nvm->buf + offset, val, bytes);
 
@@ -1536,7 +1546,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct tb_switch *sw = tb_to_switch(dev);
-	bool val;
+	int val;
 	int ret;
 
 	pm_runtime_get_sync(&sw->dev);
@@ -1552,25 +1562,28 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 		goto exit_unlock;
 	}
 
-	ret = kstrtobool(buf, &val);
+	ret = kstrtoint(buf, 10, &val);
 	if (ret)
 		goto exit_unlock;
 
 	/* Always clear the authentication status */
 	nvm_clear_auth_status(sw);
 
-	if (val) {
-		if (!sw->nvm->buf) {
-			ret = -EINVAL;
-			goto exit_unlock;
-		}
-
-		ret = nvm_validate_and_write(sw);
-		if (ret)
-			goto exit_unlock;
+	if (val > 0) {
+		if (!sw->nvm->flushed) {
+			if (!sw->nvm->buf) {
+				ret = -EINVAL;
+				goto exit_unlock;
+			}
 
-		sw->nvm->authenticating = true;
-		ret = nvm_authenticate(sw);
+			ret = nvm_validate_and_write(sw);
+			if (ret || val == WRITE_ONLY)
+				goto exit_unlock;
+		}
+		if (val == WRITE_AND_AUTHENTICATE) {
+			sw->nvm->authenticating = true;
+			ret = nvm_authenticate(sw);
+		}
 	}
 
 exit_unlock:
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 2eb2bcd3cca3..222ec19737fa 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -40,6 +40,7 @@ struct tb_switch_nvm {
 	void *buf;
 	size_t buf_data_size;
 	bool authenticating;
+	bool flushed;
 };
 
 #define TB_SWITCH_KEY_SIZE		32
-- 
2.25.1

[PATCH 2/2] thunderbolt: Add support for authenticate on disconnect

[Mario Limonciello]

Some external devices can support completing thunderbolt authentication
when they are unplugged. For this to work though, the link controller must
remain operational.

The only device known to support this right now is the Dell WD19TB, so add
a quirk for this.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 .../ABI/testing/sysfs-bus-thunderbolt         | 13 ++++++
 drivers/thunderbolt/Makefile                  |  1 +
 drivers/thunderbolt/eeprom.c                  |  2 +
 drivers/thunderbolt/lc.c                      | 14 +++++++
 drivers/thunderbolt/quirks.c                  | 38 +++++++++++++++++
 drivers/thunderbolt/switch.c                  | 42 +++++++++++++++++--
 drivers/thunderbolt/tb-quirks.h               | 16 +++++++
 drivers/thunderbolt/tb.h                      |  3 ++
 drivers/thunderbolt/tb_regs.h                 |  1 +
 9 files changed, 126 insertions(+), 4 deletions(-)
 create mode 100644 drivers/thunderbolt/quirks.c
 create mode 100644 drivers/thunderbolt/tb-quirks.h

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index 26b15cbc9881..30798f9a8f59 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -243,3 +243,16 @@ KernelVersion:	4.15
 Contact:	thunderbolt-software@lists.01.org
 Description:	This contains XDomain service specific settings as
 		bitmask. Format: %x
+
+What:		/sys/bus/thunderbolt/devices/.../nvm_authenticate_on_disconnect
+Date:		August 2020
+KernelVersion:	5.9
+Contact:	thunderbolt-software@lists.01.org
+Description:	For supported devices, automatically authenticate the new Thunderbolt
+		image when the device is disconnected from the host system.
+
+		This file will accept writing values "1" or "2"
+		- Writing "1" will flush the image to the storage
+		area and prepare the device for authentication on disconnect.
+		- Writing "2" will only flush the image to the storage
+		area.
diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
index eae28dd45250..013c5564565a 100644
--- a/drivers/thunderbolt/Makefile
+++ b/drivers/thunderbolt/Makefile
@@ -2,3 +2,4 @@
 obj-${CONFIG_USB4} := thunderbolt.o
 thunderbolt-objs := nhi.o nhi_ops.o ctl.o tb.o switch.o cap.o path.o tunnel.o eeprom.o
 thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o tmu.o usb4.o
+thunderbolt-objs += quirks.o
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index b451a5aa90b5..32838c016c4f 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -10,6 +10,7 @@
 #include <linux/property.h>
 #include <linux/slab.h>
 #include "tb.h"
+#include "tb-quirks.h"
 
 /**
  * tb_eeprom_ctl_write() - write control word
@@ -599,6 +600,7 @@ int tb_drom_read(struct tb_switch *sw)
 		sw->uid = header->uid;
 	sw->vendor = header->vendor_id;
 	sw->device = header->model_id;
+	check_tbt_quirks(sw);
 
 	crc = tb_crc32(sw->drom + TB_DROM_DATA_START, header->data_len);
 	if (crc != header->data_crc32) {
diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c
index bd44d50246d2..45d2a1de2069 100644
--- a/drivers/thunderbolt/lc.c
+++ b/drivers/thunderbolt/lc.c
@@ -366,3 +366,17 @@ int tb_lc_dp_sink_dealloc(struct tb_switch *sw, struct tb_port *in)
 	tb_port_dbg(in, "sink %d de-allocated\n", sink);
 	return 0;
 }
+
+/**
+ * lc_force_power() - Force powers a ridge
+ * @sw: thunderbolt switch
+ *
+ * This is useful to let authentication cycle pass even without
+ * a Thunderbolt link present
+ */
+int lc_force_power(struct tb_switch *sw)
+{
+	u32 in = 0xFFFF;
+
+	return tb_sw_write(sw, &in, TB_CFG_SWITCH, TB_LC_POWER, 1);
+}
diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
new file mode 100644
index 000000000000..b9c5cfb97645
--- /dev/null
+++ b/drivers/thunderbolt/quirks.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Thunderbolt driver - quirks
+ *
+ * Copyright (c) 2020 Mario Limonciello <mario.limonciello@dell.com>
+ */
+
+#include "tb.h"
+#include "tb-quirks.h"
+
+static void quirk_force_power_link(struct tb_switch *sw)
+{
+	sw->quirks |= QUIRK_FORCE_POWER_LINK_CONTROLLER;
+}
+
+struct tbt_quirk {
+	u16 vendor;
+	u16 device;
+	void (*hook)(struct tb_switch *sw);
+};
+
+static struct tbt_quirk tbt_quirks[] = {
+	/* Dell WD19TB supports self-authentication on unplug */
+	{ 0x00d4, 0xb070, quirk_force_power_link },
+};
+
+void check_tbt_quirks(struct tb_switch *sw)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tbt_quirks); i++) {
+		struct tbt_quirk *q = &tbt_quirks[i];
+
+		if (sw->device == q->device &&
+		    sw->vendor == q->vendor)
+			q->hook(sw);
+	}
+}
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 4c476a58db38..0576fe7c0054 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 
 #include "tb.h"
+#include "tb-quirks.h"
 
 /* Switch NVM support */
 
@@ -1542,8 +1543,8 @@ static ssize_t nvm_authenticate_show(struct device *dev,
 	return sprintf(buf, "%#x\n", status);
 }
 
-static ssize_t nvm_authenticate_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
+static ssize_t nvm_authenticate_sysfs(struct device *dev, const char *buf,
+				      bool disconnect)
 {
 	struct tb_switch *sw = tb_to_switch(dev);
 	int val;
@@ -1581,8 +1582,12 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 				goto exit_unlock;
 		}
 		if (val == WRITE_AND_AUTHENTICATE) {
-			sw->nvm->authenticating = true;
-			ret = nvm_authenticate(sw);
+			if (disconnect) {
+				ret = lc_force_power(sw);
+			} else {
+				sw->nvm->authenticating = true;
+				ret = nvm_authenticate(sw);
+			}
 		}
 	}
 
@@ -1592,12 +1597,36 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	pm_runtime_mark_last_busy(&sw->dev);
 	pm_runtime_put_autosuspend(&sw->dev);
 
+	return ret;
+}
+
+static ssize_t nvm_authenticate_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	int ret = nvm_authenticate_sysfs(dev, buf, false);
 	if (ret)
 		return ret;
 	return count;
 }
 static DEVICE_ATTR_RW(nvm_authenticate);
 
+static ssize_t nvm_authenticate_on_disconnect_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	return nvm_authenticate_show(dev, attr, buf);
+}
+
+static ssize_t nvm_authenticate_on_disconnect_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	int ret = nvm_authenticate_sysfs(dev, buf, true);
+
+	if (ret)
+		return ret;
+	return count;
+}
+static DEVICE_ATTR_RW(nvm_authenticate_on_disconnect);
+
 static ssize_t nvm_version_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -1655,6 +1684,7 @@ static struct attribute *switch_attrs[] = {
 	&dev_attr_generation.attr,
 	&dev_attr_key.attr,
 	&dev_attr_nvm_authenticate.attr,
+	&dev_attr_nvm_authenticate_on_disconnect.attr,
 	&dev_attr_nvm_version.attr,
 	&dev_attr_rx_speed.attr,
 	&dev_attr_rx_lanes.attr,
@@ -1709,6 +1739,10 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
 		if (tb_route(sw))
 			return attr->mode;
 		return 0;
+	} else if (attr == &dev_attr_nvm_authenticate_on_disconnect.attr) {
+		if (sw->quirks & QUIRK_FORCE_POWER_LINK_CONTROLLER)
+			return attr->mode;
+		return 0;
 	}
 
 	return sw->safe_mode ? 0 : attr->mode;
diff --git a/drivers/thunderbolt/tb-quirks.h b/drivers/thunderbolt/tb-quirks.h
new file mode 100644
index 000000000000..6b644eaaac40
--- /dev/null
+++ b/drivers/thunderbolt/tb-quirks.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Thunderbolt driver - quirks
+ *
+ * Copyright (c) 2020 Mario Limonciello <mario.limonciello@dell.com>
+ */
+
+
+#ifndef _TB_QUIRKS
+#define _TB_QUIRKS
+
+#define QUIRK_FORCE_POWER_LINK_CONTROLLER       (1<<1)
+
+void check_tbt_quirks(struct tb_switch *sw);
+
+#endif
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 222ec19737fa..22937be69a1f 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -118,6 +118,7 @@ struct tb_switch_tmu {
  * @depth: Depth in the chain this switch is connected (ICM only)
  * @rpm_complete: Completion used to wait for runtime resume to
  *		  complete (ICM only)
+ * @quirks: Quirks used for this Thunderbolt switch
  *
  * When the switch is being added or removed to the domain (other
  * switches) you need to have domain lock held.
@@ -155,6 +156,7 @@ struct tb_switch {
 	u8 link;
 	u8 depth;
 	struct completion rpm_complete;
+	unsigned long quirks;
 };
 
 /**
@@ -784,6 +786,7 @@ bool tb_lc_lane_bonding_possible(struct tb_switch *sw);
 bool tb_lc_dp_sink_query(struct tb_switch *sw, struct tb_port *in);
 int tb_lc_dp_sink_alloc(struct tb_switch *sw, struct tb_port *in);
 int tb_lc_dp_sink_dealloc(struct tb_switch *sw, struct tb_port *in);
+int lc_force_power(struct tb_switch *sw);
 
 static inline int tb_route_length(u64 route)
 {
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index c29c5075525a..9a2fab9c5346 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -379,6 +379,7 @@ struct tb_regs_hop {
 #define TB_LC_SNK_ALLOCATION_SNK1_SHIFT	4
 #define TB_LC_SNK_ALLOCATION_SNK1_MASK	GENMASK(7, 4)
 #define TB_LC_SNK_ALLOCATION_SNK1_CM	0x1
+#define TB_LC_POWER			0x740
 
 /* Link controller registers */
 #define TB_LC_PORT_ATTR			0x8d
-- 
2.25.1

RE: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates

[Mario.Limonciello]

> -----Original Message-----
> From: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Sent: Monday, June 22, 2020 9:31 AM
> To: Andreas Noever; Michael Jamet; Mika Westerberg; Yehezkel Bernat
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Limonciello,
> Mario
> Subject: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates
> 
> Currently updates to Thunderbolt and USB4 controllers are fully atomic
> actions. When writing into the non-active NVM nothing gets flushed to
> the hardware until authenticate is sent.
> 
> There has been some desire to improve the perceived performance of these
> updates, particularly for userland that may perform the update upon
> a performance sensitive time like logging out.
> 
> So allow userland to flush the image to hardware at runtime, and then
> allow authenticating the image at another time.
> 
> For the Dell WD19TB some specific hardware capability exists that allows
> extending this to automatically complete the update when unplugged.
> Export that functionality to userspace as well.
> 
> This patch series is done relative thunderbolt.git/next.
> 
> Mario Limonciello (2):
>   thunderbolt: Add support for separating the flush to SPI and
>     authenticate
>   thunderbolt: Add support for authenticate on disconnect
> 
>  .../ABI/testing/sysfs-bus-thunderbolt         | 24 +++++-
>  drivers/thunderbolt/Makefile                  |  1 +
>  drivers/thunderbolt/eeprom.c                  |  2 +
>  drivers/thunderbolt/lc.c                      | 14 ++++
>  drivers/thunderbolt/quirks.c                  | 38 +++++++++
>  drivers/thunderbolt/switch.c                  | 81 +++++++++++++++----
>  drivers/thunderbolt/tb-quirks.h               | 16 ++++
>  drivers/thunderbolt/tb.h                      |  4 +
>  drivers/thunderbolt/tb_regs.h                 |  1 +
>  9 files changed, 162 insertions(+), 19 deletions(-)
>  create mode 100644 drivers/thunderbolt/quirks.c
>  create mode 100644 drivers/thunderbolt/tb-quirks.h
> 
> --
> 2.25.1

Just to connect the dots, here is the matching userspace changes for this
change: https://github.com/fwupd/fwupd/pull/2204

Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates

[Mika Westerberg]

Hi Mario,

On Mon, Jun 22, 2020 at 09:30:33AM -0500, Mario Limonciello wrote:
> Currently updates to Thunderbolt and USB4 controllers are fully atomic
> actions. When writing into the non-active NVM nothing gets flushed to
> the hardware until authenticate is sent.
> 
> There has been some desire to improve the perceived performance of these
> updates, particularly for userland that may perform the update upon
> a performance sensitive time like logging out.
> 
> So allow userland to flush the image to hardware at runtime, and then
> allow authenticating the image at another time.
> 
> For the Dell WD19TB some specific hardware capability exists that allows
> extending this to automatically complete the update when unplugged.
> Export that functionality to userspace as well.
> 
> This patch series is done relative thunderbolt.git/next.

Thanks for the patch series. I wonder if you could base this on top of
my "retimer NVM upgrade" series here (you are also Cc'd):

  https://lore.kernel.org/linux-usb/20200616135617.85752-1-mika.westerberg@linux.intel.com/

That series moves some of the common NVM functionality into a separate
file (nvm.c).

> Mario Limonciello (2):
>   thunderbolt: Add support for separating the flush to SPI and
>     authenticate
>   thunderbolt: Add support for authenticate on disconnect
> 
>  .../ABI/testing/sysfs-bus-thunderbolt         | 24 +++++-
>  drivers/thunderbolt/Makefile                  |  1 +
>  drivers/thunderbolt/eeprom.c                  |  2 +
>  drivers/thunderbolt/lc.c                      | 14 ++++
>  drivers/thunderbolt/quirks.c                  | 38 +++++++++
>  drivers/thunderbolt/switch.c                  | 81 +++++++++++++++----
>  drivers/thunderbolt/tb-quirks.h               | 16 ++++
>  drivers/thunderbolt/tb.h                      |  4 +
>  drivers/thunderbolt/tb_regs.h                 |  1 +
>  9 files changed, 162 insertions(+), 19 deletions(-)
>  create mode 100644 drivers/thunderbolt/quirks.c
>  create mode 100644 drivers/thunderbolt/tb-quirks.h
> 
> -- 
> 2.25.1

RE: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates

[Mario.Limonciello]

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Monday, June 22, 2020 11:38 AM
> To: Limonciello, Mario
> Cc: Andreas Noever; Michael Jamet; Yehezkel Bernat; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi Mario,
> 
> On Mon, Jun 22, 2020 at 09:30:33AM -0500, Mario Limonciello wrote:
> > Currently updates to Thunderbolt and USB4 controllers are fully atomic
> > actions. When writing into the non-active NVM nothing gets flushed to
> > the hardware until authenticate is sent.
> >
> > There has been some desire to improve the perceived performance of these
> > updates, particularly for userland that may perform the update upon
> > a performance sensitive time like logging out.
> >
> > So allow userland to flush the image to hardware at runtime, and then
> > allow authenticating the image at another time.
> >
> > For the Dell WD19TB some specific hardware capability exists that allows
> > extending this to automatically complete the update when unplugged.
> > Export that functionality to userspace as well.
> >
> > This patch series is done relative thunderbolt.git/next.
> 
> Thanks for the patch series. I wonder if you could base this on top of
> my "retimer NVM upgrade" series here (you are also Cc'd):
> 
>   https://lore.kernel.org/linux-usb/20200616135617.85752-1-
> mika.westerberg@linux.intel.com/
> 
> That series moves some of the common NVM functionality into a separate
> file (nvm.c).

Sure thing.  Do you by chance already have that on a public branch somewhere
that I can easily rebase it?

> 
> > Mario Limonciello (2):
> >   thunderbolt: Add support for separating the flush to SPI and
> >     authenticate
> >   thunderbolt: Add support for authenticate on disconnect
> >
> >  .../ABI/testing/sysfs-bus-thunderbolt         | 24 +++++-
> >  drivers/thunderbolt/Makefile                  |  1 +
> >  drivers/thunderbolt/eeprom.c                  |  2 +
> >  drivers/thunderbolt/lc.c                      | 14 ++++
> >  drivers/thunderbolt/quirks.c                  | 38 +++++++++
> >  drivers/thunderbolt/switch.c                  | 81 +++++++++++++++----
> >  drivers/thunderbolt/tb-quirks.h               | 16 ++++
> >  drivers/thunderbolt/tb.h                      |  4 +
> >  drivers/thunderbolt/tb_regs.h                 |  1 +
> >  9 files changed, 162 insertions(+), 19 deletions(-)
> >  create mode 100644 drivers/thunderbolt/quirks.c
> >  create mode 100644 drivers/thunderbolt/tb-quirks.h
> >
> > --
> > 2.25.1

Re: [PATCH 1/2] thunderbolt: Add support for separating the flush to SPI and authenticate

[Mika Westerberg]

On Mon, Jun 22, 2020 at 09:30:34AM -0500, Mario Limonciello wrote:
> This allows userspace to have a shorter period of time that the device
> is unusable and to call it at a more convenient time.
> 
> For example flushing the image may happen while the user is using the
> machine and authenticating/rebooting may happen while logging out.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  .../ABI/testing/sysfs-bus-thunderbolt         | 11 ++++-
>  drivers/thunderbolt/switch.c                  | 43 ++++++++++++-------
>  drivers/thunderbolt/tb.h                      |  1 +
>  3 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index 82e80de78dd0..26b15cbc9881 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -178,11 +178,18 @@ KernelVersion:	4.13
>  Contact:	thunderbolt-software@lists.01.org
>  Description:	When new NVM image is written to the non-active NVM
>  		area (through non_activeX NVMem device), the
> -		authentication procedure is started by writing 1 to
> -		this file. If everything goes well, the device is
> +		authentication procedure is started by writing to
> +		this file.
> +		If everything goes well, the device is
>  		restarted with the new NVM firmware. If the image
>  		verification fails an error code is returned instead.
>  
> +		This file will accept writing values "1" or "2"
> +		- Writing "1" will flush the image to the storage
> +		area and authenticate the image in one action.
> +		- Writing "2" will only flush the image to the storage
> +		area.

Does this ("2") also do the basic validation? I think it does so
probably good to mention that here.

> +
>  		When read holds status of the last authentication
>  		operation if an error occurred during the process. This
>  		is directly the status value from the DMA configuration
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index d7d60cd9226f..4c476a58db38 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -27,6 +27,11 @@
>  #define NVM_MIN_SIZE		SZ_32K
>  #define NVM_MAX_SIZE		SZ_512K
>  
> +enum nvm_write_ops {
> +	WRITE_AND_AUTHENTICATE = 1,
> +	WRITE_ONLY = 2,
> +};
> +
>  static DEFINE_IDA(nvm_ida);
>  
>  struct nvm_auth_status {
> @@ -164,8 +169,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
>  	}
>  
>  	if (tb_switch_is_usb4(sw))
> -		return usb4_switch_nvm_write(sw, 0, buf, image_size);
> -	return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> +		ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
> +	else
> +		ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> +	if (!ret)
> +		sw->nvm->flushed = true;
> +	return ret;
>  }
>  
>  static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
> @@ -371,6 +380,7 @@ static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
>  		}
>  	}
>  
> +	sw->nvm->flushed = false;
>  	sw->nvm->buf_data_size = offset + bytes;
>  	memcpy(sw->nvm->buf + offset, val, bytes);
>  
> @@ -1536,7 +1546,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
>  	struct device_attribute *attr, const char *buf, size_t count)
>  {
>  	struct tb_switch *sw = tb_to_switch(dev);
> -	bool val;
> +	int val;
>  	int ret;
>  
>  	pm_runtime_get_sync(&sw->dev);
> @@ -1552,25 +1562,28 @@ static ssize_t nvm_authenticate_store(struct device *dev,
>  		goto exit_unlock;
>  	}
>  
> -	ret = kstrtobool(buf, &val);
> +	ret = kstrtoint(buf, 10, &val);
>  	if (ret)
>  		goto exit_unlock;
>  
>  	/* Always clear the authentication status */
>  	nvm_clear_auth_status(sw);
>  
> -	if (val) {
> -		if (!sw->nvm->buf) {
> -			ret = -EINVAL;
> -			goto exit_unlock;
> -		}
> -
> -		ret = nvm_validate_and_write(sw);
> -		if (ret)
> -			goto exit_unlock;
> +	if (val > 0) {
> +		if (!sw->nvm->flushed) {
> +			if (!sw->nvm->buf) {
> +				ret = -EINVAL;
> +				goto exit_unlock;
> +			}
>  
> -		sw->nvm->authenticating = true;
> -		ret = nvm_authenticate(sw);
> +			ret = nvm_validate_and_write(sw);
> +			if (ret || val == WRITE_ONLY)
> +				goto exit_unlock;
> +		}
> +		if (val == WRITE_AND_AUTHENTICATE) {
> +			sw->nvm->authenticating = true;
> +			ret = nvm_authenticate(sw);
> +		}
>  	}
>  
>  exit_unlock:
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 2eb2bcd3cca3..222ec19737fa 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -40,6 +40,7 @@ struct tb_switch_nvm {
>  	void *buf;
>  	size_t buf_data_size;
>  	bool authenticating;
> +	bool flushed;

Please add kernel-doc about this.

>  };
>  
>  #define TB_SWITCH_KEY_SIZE		32
> -- 
> 2.25.1

Re: [PATCH 2/2] thunderbolt: Add support for authenticate on disconnect

[Mika Westerberg]

On Mon, Jun 22, 2020 at 09:30:35AM -0500, Mario Limonciello wrote:
> Some external devices can support completing thunderbolt authentication
> when they are unplugged. For this to work though, the link controller must
> remain operational.
> 
> The only device known to support this right now is the Dell WD19TB, so add
> a quirk for this.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  .../ABI/testing/sysfs-bus-thunderbolt         | 13 ++++++
>  drivers/thunderbolt/Makefile                  |  1 +
>  drivers/thunderbolt/eeprom.c                  |  2 +
>  drivers/thunderbolt/lc.c                      | 14 +++++++
>  drivers/thunderbolt/quirks.c                  | 38 +++++++++++++++++
>  drivers/thunderbolt/switch.c                  | 42 +++++++++++++++++--
>  drivers/thunderbolt/tb-quirks.h               | 16 +++++++
>  drivers/thunderbolt/tb.h                      |  3 ++
>  drivers/thunderbolt/tb_regs.h                 |  1 +
>  9 files changed, 126 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/thunderbolt/quirks.c
>  create mode 100644 drivers/thunderbolt/tb-quirks.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index 26b15cbc9881..30798f9a8f59 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -243,3 +243,16 @@ KernelVersion:	4.15
>  Contact:	thunderbolt-software@lists.01.org
>  Description:	This contains XDomain service specific settings as
>  		bitmask. Format: %x
> +
> +What:		/sys/bus/thunderbolt/devices/.../nvm_authenticate_on_disconnect
> +Date:		August 2020
> +KernelVersion:	5.9
> +Contact:	thunderbolt-software@lists.01.org

I think you can use your email address here instead.

> +Description:	For supported devices, automatically authenticate the new Thunderbolt
> +		image when the device is disconnected from the host system.
> +
> +		This file will accept writing values "1" or "2"
> +		- Writing "1" will flush the image to the storage
> +		area and prepare the device for authentication on disconnect.
> +		- Writing "2" will only flush the image to the storage
> +		area.

Also here it does the basic image validation so probably good to
mention.

> diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
> index eae28dd45250..013c5564565a 100644
> --- a/drivers/thunderbolt/Makefile
> +++ b/drivers/thunderbolt/Makefile
> @@ -2,3 +2,4 @@
>  obj-${CONFIG_USB4} := thunderbolt.o
>  thunderbolt-objs := nhi.o nhi_ops.o ctl.o tb.o switch.o cap.o path.o tunnel.o eeprom.o
>  thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o tmu.o usb4.o
> +thunderbolt-objs += quirks.o
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index b451a5aa90b5..32838c016c4f 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -10,6 +10,7 @@
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include "tb.h"
> +#include "tb-quirks.h"
>  
>  /**
>   * tb_eeprom_ctl_write() - write control word
> @@ -599,6 +600,7 @@ int tb_drom_read(struct tb_switch *sw)
>  		sw->uid = header->uid;
>  	sw->vendor = header->vendor_id;
>  	sw->device = header->model_id;
> +	check_tbt_quirks(sw);
>  
>  	crc = tb_crc32(sw->drom + TB_DROM_DATA_START, header->data_len);
>  	if (crc != header->data_crc32) {
> diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c
> index bd44d50246d2..45d2a1de2069 100644
> --- a/drivers/thunderbolt/lc.c
> +++ b/drivers/thunderbolt/lc.c
> @@ -366,3 +366,17 @@ int tb_lc_dp_sink_dealloc(struct tb_switch *sw, struct tb_port *in)
>  	tb_port_dbg(in, "sink %d de-allocated\n", sink);
>  	return 0;
>  }
> +
> +/**
> + * lc_force_power() - Force powers a ridge

Maybe "Forces LC to be powered on" or similar?

> + * @sw: thunderbolt switch
> + *
> + * This is useful to let authentication cycle pass even without
> + * a Thunderbolt link present

Add "." at the end of the sentence.

> + */
> +int lc_force_power(struct tb_switch *sw)

Also tb_lc_force_power() or so to follow the conventions used in the
driver.

> +{
> +	u32 in = 0xFFFF;

I prefer 0xffff instead.

> +
> +	return tb_sw_write(sw, &in, TB_CFG_SWITCH, TB_LC_POWER, 1);
> +}
> diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
> new file mode 100644
> index 000000000000..b9c5cfb97645
> --- /dev/null
> +++ b/drivers/thunderbolt/quirks.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thunderbolt driver - quirks
> + *
> + * Copyright (c) 2020 Mario Limonciello <mario.limonciello@dell.com>
> + */
> +
> +#include "tb.h"
> +#include "tb-quirks.h"
> +
> +static void quirk_force_power_link(struct tb_switch *sw)
> +{
> +	sw->quirks |= QUIRK_FORCE_POWER_LINK_CONTROLLER;
> +}
> +
> +struct tbt_quirk {

tb_quirk please.

> +	u16 vendor;
> +	u16 device;
> +	void (*hook)(struct tb_switch *sw);
> +};
> +
> +static struct tbt_quirk tbt_quirks[] = {

const?

> +	/* Dell WD19TB supports self-authentication on unplug */
> +	{ 0x00d4, 0xb070, quirk_force_power_link },
> +};
> +
> +void check_tbt_quirks(struct tb_switch *sw)

Let's call it

tb_check_quirks() or tb_detect_quirks() following USB.

> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tbt_quirks); i++) {
> +		struct tbt_quirk *q = &tbt_quirks[i];
> +
> +		if (sw->device == q->device &&
> +		    sw->vendor == q->vendor)

I think you can write it like this:

		if (sw->device == q->device && sw->vendor == q->vendor)

> +			q->hook(sw);
> +	}
> +}
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 4c476a58db38..0576fe7c0054 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -16,6 +16,7 @@
>  #include <linux/vmalloc.h>
>  
>  #include "tb.h"
> +#include "tb-quirks.h"
>  
>  /* Switch NVM support */
>  
> @@ -1542,8 +1543,8 @@ static ssize_t nvm_authenticate_show(struct device *dev,
>  	return sprintf(buf, "%#x\n", status);
>  }
>  
> -static ssize_t nvm_authenticate_store(struct device *dev,
> -	struct device_attribute *attr, const char *buf, size_t count)
> +static ssize_t nvm_authenticate_sysfs(struct device *dev, const char *buf,
> +				      bool disconnect)
>  {
>  	struct tb_switch *sw = tb_to_switch(dev);
>  	int val;
> @@ -1581,8 +1582,12 @@ static ssize_t nvm_authenticate_store(struct device *dev,
>  				goto exit_unlock;
>  		}
>  		if (val == WRITE_AND_AUTHENTICATE) {
> -			sw->nvm->authenticating = true;
> -			ret = nvm_authenticate(sw);
> +			if (disconnect) {
> +				ret = lc_force_power(sw);
> +			} else {
> +				sw->nvm->authenticating = true;
> +				ret = nvm_authenticate(sw);
> +			}
>  		}
>  	}
>  
> @@ -1592,12 +1597,36 @@ static ssize_t nvm_authenticate_store(struct device *dev,
>  	pm_runtime_mark_last_busy(&sw->dev);
>  	pm_runtime_put_autosuspend(&sw->dev);
>  
> +	return ret;
> +}
> +
> +static ssize_t nvm_authenticate_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	int ret = nvm_authenticate_sysfs(dev, buf, false);
>  	if (ret)
>  		return ret;
>  	return count;
>  }
>  static DEVICE_ATTR_RW(nvm_authenticate);
>  
> +static ssize_t nvm_authenticate_on_disconnect_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	return nvm_authenticate_show(dev, attr, buf);
> +}
> +
> +static ssize_t nvm_authenticate_on_disconnect_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	int ret = nvm_authenticate_sysfs(dev, buf, true);
> +
> +	if (ret)
> +		return ret;
> +	return count;

Hmm,

perhaps like this:

	int ret;

	ret = nvm_authenticate_sysfs(dev, buf, true);
	return ret ? ret : count;

> +}
> +static DEVICE_ATTR_RW(nvm_authenticate_on_disconnect);
> +
>  static ssize_t nvm_version_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> @@ -1655,6 +1684,7 @@ static struct attribute *switch_attrs[] = {
>  	&dev_attr_generation.attr,
>  	&dev_attr_key.attr,
>  	&dev_attr_nvm_authenticate.attr,
> +	&dev_attr_nvm_authenticate_on_disconnect.attr,
>  	&dev_attr_nvm_version.attr,
>  	&dev_attr_rx_speed.attr,
>  	&dev_attr_rx_lanes.attr,
> @@ -1709,6 +1739,10 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
>  		if (tb_route(sw))
>  			return attr->mode;
>  		return 0;
> +	} else if (attr == &dev_attr_nvm_authenticate_on_disconnect.attr) {
> +		if (sw->quirks & QUIRK_FORCE_POWER_LINK_CONTROLLER)
> +			return attr->mode;
> +		return 0;
>  	}
>  
>  	return sw->safe_mode ? 0 : attr->mode;
> diff --git a/drivers/thunderbolt/tb-quirks.h b/drivers/thunderbolt/tb-quirks.h
> new file mode 100644
> index 000000000000..6b644eaaac40
> --- /dev/null
> +++ b/drivers/thunderbolt/tb-quirks.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Thunderbolt driver - quirks
> + *
> + * Copyright (c) 2020 Mario Limonciello <mario.limonciello@dell.com>
> + */
> +
> +
> +#ifndef _TB_QUIRKS
> +#define _TB_QUIRKS
> +
> +#define QUIRK_FORCE_POWER_LINK_CONTROLLER       (1<<1)
> +
> +void check_tbt_quirks(struct tb_switch *sw);

I think we can put these two lines into tb.h for now.

> +
> +#endif
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 222ec19737fa..22937be69a1f 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -118,6 +118,7 @@ struct tb_switch_tmu {
>   * @depth: Depth in the chain this switch is connected (ICM only)
>   * @rpm_complete: Completion used to wait for runtime resume to
>   *		  complete (ICM only)
> + * @quirks: Quirks used for this Thunderbolt switch
>   *
>   * When the switch is being added or removed to the domain (other
>   * switches) you need to have domain lock held.
> @@ -155,6 +156,7 @@ struct tb_switch {
>  	u8 link;
>  	u8 depth;
>  	struct completion rpm_complete;
> +	unsigned long quirks;
>  };
>  
>  /**
> @@ -784,6 +786,7 @@ bool tb_lc_lane_bonding_possible(struct tb_switch *sw);
>  bool tb_lc_dp_sink_query(struct tb_switch *sw, struct tb_port *in);
>  int tb_lc_dp_sink_alloc(struct tb_switch *sw, struct tb_port *in);
>  int tb_lc_dp_sink_dealloc(struct tb_switch *sw, struct tb_port *in);
> +int lc_force_power(struct tb_switch *sw);
>  
>  static inline int tb_route_length(u64 route)
>  {
> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> index c29c5075525a..9a2fab9c5346 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -379,6 +379,7 @@ struct tb_regs_hop {
>  #define TB_LC_SNK_ALLOCATION_SNK1_SHIFT	4
>  #define TB_LC_SNK_ALLOCATION_SNK1_MASK	GENMASK(7, 4)
>  #define TB_LC_SNK_ALLOCATION_SNK1_CM	0x1
> +#define TB_LC_POWER			0x740
>  
>  /* Link controller registers */
>  #define TB_LC_PORT_ATTR			0x8d
> -- 
> 2.25.1

Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates

[Mika Westerberg]

On Mon, Jun 22, 2020 at 04:41:35PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Sent: Monday, June 22, 2020 11:38 AM
> > To: Limonciello, Mario
> > Cc: Andreas Noever; Michael Jamet; Yehezkel Bernat; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > Hi Mario,
> > 
> > On Mon, Jun 22, 2020 at 09:30:33AM -0500, Mario Limonciello wrote:
> > > Currently updates to Thunderbolt and USB4 controllers are fully atomic
> > > actions. When writing into the non-active NVM nothing gets flushed to
> > > the hardware until authenticate is sent.
> > >
> > > There has been some desire to improve the perceived performance of these
> > > updates, particularly for userland that may perform the update upon
> > > a performance sensitive time like logging out.
> > >
> > > So allow userland to flush the image to hardware at runtime, and then
> > > allow authenticating the image at another time.
> > >
> > > For the Dell WD19TB some specific hardware capability exists that allows
> > > extending this to automatically complete the update when unplugged.
> > > Export that functionality to userspace as well.
> > >
> > > This patch series is done relative thunderbolt.git/next.
> > 
> > Thanks for the patch series. I wonder if you could base this on top of
> > my "retimer NVM upgrade" series here (you are also Cc'd):
> > 
> >   https://lore.kernel.org/linux-usb/20200616135617.85752-1-
> > mika.westerberg@linux.intel.com/
> > 
> > That series moves some of the common NVM functionality into a separate
> > file (nvm.c).
> 
> Sure thing.  Do you by chance already have that on a public branch somewhere
> that I can easily rebase it?

I just pushed "bleeding-edge" branch that you should be able to base
your stuff on top:

   https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git/log/?h=bleeding-edge

It includes a couple of other patches as well (subject to change since
they are also under review).