[PATCH v8 0/3] thunderbolt: add vendor's NVM formats

[PATCH v8 0/3] thunderbolt: add vendor's NVM formats

[Szuying Chen]

From: Szuying Chen <Chloe_Chen@asmedia.com.tw>

The patch series for vendors to extend their NVM format.

v7->v8: The nvm_read() defined in tb.h. Modify
tb_switch_nvm_validate() return value and no_nvm_upgrade bit setting.

Szuying Chen (3):
  thunderbolt: Add vendor's specific operations of NVM
  thunderbolt: Modify tb_nvm major and minor size.
  thunderbolt: To extend ASMedia NVM formats.

 drivers/thunderbolt/nvm.c    | 234 +++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/switch.c | 104 +++-------------
 drivers/thunderbolt/tb.c     |   2 +-
 drivers/thunderbolt/tb.h     |  12 +-
 4 files changed, 263 insertions(+), 89 deletions(-)

--
2.34.1

[PATCH v8 1/3] thunderbolt: Add vendor's specific operations of NVM

[Szuying Chen]

From: Szuying Chen <Chloe_Chen@asmedia.com.tw>

The current code only supports the Intel NVM format and we hope vendors
can easily add their NVM formats. We provide an array to find vendors
that support NVM, it will set nvm->vops to point the vendor specific
operations.

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
v7->v8: As suggestion by Mika, the nvm_read() defined in tb.h
and modify the return value of tb_switch_nvm_validate().

 drivers/thunderbolt/nvm.c    | 194 +++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/switch.c | 104 ++++---------------
 drivers/thunderbolt/tb.h     |   8 ++
 3 files changed, 220 insertions(+), 86 deletions(-)

diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
index b3f310389378..878d705bd0cb 100644
--- a/drivers/thunderbolt/nvm.c
+++ b/drivers/thunderbolt/nvm.c
@@ -12,8 +12,202 @@

 #include "tb.h"

+/* Switch NVM support */
+#define NVM_CSS		0x10
+
 static DEFINE_IDA(nvm_ida);

+/**
+ * struct tb_nvm_vendor_ops - Vendor NVM specific operations
+ * @read_version: Used NVM read get Firmware version.
+ * @validate: Vendors have their validate method before NVM write.
+ */
+struct tb_nvm_vendor_ops {
+	int (*read_version)(struct tb_switch *sw);
+	int (*validate)(struct tb_switch *sw);
+};
+
+static int intel_nvm_version(struct tb_switch *sw)
+{
+	struct tb_nvm *nvm = sw->nvm;
+	u32 val;
+	int ret;
+
+	/*
+	 * If the switch is in safe-mode the only accessible portion of
+	 * the NVM is the non-active one where userspace is expected to
+	 * write new functional NVM.
+	 */
+	if (!sw->safe_mode) {
+		u32 nvm_size, hdr_size;
+
+		ret = nvm_read(sw, NVM_FLASH_SIZE, &val, sizeof(val));
+		if (ret)
+			return ret;
+
+		hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
+		nvm_size = (SZ_1M << (val & 7)) / 8;
+		nvm_size = (nvm_size - hdr_size) / 2;
+
+		ret = nvm_read(sw, NVM_VERSION, &val, sizeof(val));
+		if (ret)
+			return ret;
+
+		nvm->major = val >> 16;
+		nvm->minor = val >> 8;
+		nvm->nvm_size = nvm_size;
+	}
+
+	return 0;
+}
+
+static int intel_nvm_validate(struct tb_switch *sw)
+{
+	unsigned int image_size, hdr_size;
+	u8 *buf = sw->nvm->buf;
+	u16 ds_size;
+	int ret;
+
+	image_size = sw->nvm->buf_data_size;
+	if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE)
+		return -EINVAL;
+
+	/*
+	 * FARB pointer must point inside the image and must at least
+	 * contain parts of the digital section we will be reading here.
+	 */
+	hdr_size = (*(u32 *)buf) & 0xffffff;
+	if (hdr_size + NVM_DEVID + 2 >= image_size)
+		return -EINVAL;
+
+	/* Digital section start should be aligned to 4k page */
+	if (!IS_ALIGNED(hdr_size, SZ_4K))
+		return -EINVAL;
+
+	/*
+	 * Read digital section size and check that it also fits inside
+	 * the image.
+	 */
+	ds_size = *(u16 *)(buf + hdr_size);
+	if (ds_size >= image_size)
+		return -EINVAL;
+
+	if (!sw->safe_mode) {
+		u16 device_id;
+
+		/*
+		 * Make sure the device ID in the image matches the one
+		 * we read from the switch config space.
+		 */
+		device_id = *(u16 *)(buf + hdr_size + NVM_DEVID);
+		if (device_id != sw->config.device_id)
+			return -EINVAL;
+
+		if (sw->generation < 3) {
+			/* Write CSS headers first */
+			ret = dma_port_flash_write(sw->dma_port,
+				DMA_PORT_CSS_ADDRESS, buf + NVM_CSS,
+				DMA_PORT_CSS_MAX_SIZE);
+			if (ret)
+				return ret;
+		}
+
+		/* Skip headers in the image */
+		sw->nvm->buf = buf + hdr_size;
+		sw->nvm->buf_data_size = image_size - hdr_size;
+	}
+
+	return 0;
+}
+
+static const struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
+	.read_version = intel_nvm_version,
+	.validate = intel_nvm_validate,
+};
+
+struct switch_nvm_vendor {
+	u16 vendor;
+	const struct tb_nvm_vendor_ops *vops;
+};
+
+static const struct switch_nvm_vendor switch_nvm_vendors[] = {
+	{ PCI_VENDOR_ID_INTEL, &intel_switch_nvm_ops },
+	{ 0x8087, &intel_switch_nvm_ops },
+};
+
+/**
+ * tb_switch_nvm_validate() - Validate NVM image
+ * @switch: Switch to NVM write
+ *
+ * The function include vendor's validate before writes data to actual NVM
+ * flash device. Return %0 in success and error otherwise.
+ */
+int tb_switch_nvm_validate(struct tb_switch *sw)
+{
+	const struct tb_nvm_vendor_ops *vops = sw->nvm->vops;
+	const u8 *buf = sw->nvm->buf;
+	unsigned int image_size;
+
+	if (!buf)
+		return -EINVAL;
+
+	image_size = sw->nvm->buf_data_size;
+	if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE)
+		return -EINVAL;
+
+	if (!vops)
+		return 0;
+
+	return vops->validate?vops->validate(sw):0;
+}
+
+/**
+ * tb_switch_nvm_alloc() - Allocate new NVM structure.
+ * @sw: Switch to allocate NVM
+ *
+ * Allocates new NVM structure and returns it. In case of error returns
+ * ERR_PTR().
+ */
+struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw)
+{
+	const struct tb_nvm_vendor_ops *vops = NULL;
+	struct tb_nvm *nvm;
+	int ret;
+	int i;
+
+	/*
+	 * If the vendor matches on the array then set nvm->vops to
+	 * point the vendor specific operations.
+	 */
+	for (i = 0; i < ARRAY_SIZE(switch_nvm_vendors); i++) {
+		const struct switch_nvm_vendor *v = &switch_nvm_vendors[i];
+
+		if (v->vendor == sw->config.vendor_id) {
+			vops = v->vops;
+			break;
+		}
+	}
+
+	if (!vops)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	nvm = tb_nvm_alloc(&sw->dev);
+	if (IS_ERR(nvm))
+		return nvm;
+
+	nvm->vops = vops;
+	sw->nvm = nvm;
+	ret = vops->read_version(sw);
+	if (ret)
+		goto err_nvm;
+
+	return nvm;
+
+err_nvm:
+	tb_nvm_free(nvm);
+	return ERR_PTR(ret);
+}
+
 /**
  * tb_nvm_alloc() - Allocate new NVM structure
  * @dev: Device owning the NVM
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 244f8cd38b25..6df6553da4c7 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -102,62 +102,17 @@ static void nvm_clear_auth_status(const struct tb_switch *sw)

 static int nvm_validate_and_write(struct tb_switch *sw)
 {
-	unsigned int image_size, hdr_size;
-	const u8 *buf = sw->nvm->buf;
-	u16 ds_size;
+	unsigned int image_size;
+	const u8 *buf;
 	int ret;

-	if (!buf)
-		return -EINVAL;
+	/* validate NVM image before NVM write */
+	ret = tb_switch_nvm_validate(sw);
+	if (ret)
+		return ret;

+	buf = sw->nvm->buf;
 	image_size = sw->nvm->buf_data_size;
-	if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE)
-		return -EINVAL;
-
-	/*
-	 * FARB pointer must point inside the image and must at least
-	 * contain parts of the digital section we will be reading here.
-	 */
-	hdr_size = (*(u32 *)buf) & 0xffffff;
-	if (hdr_size + NVM_DEVID + 2 >= image_size)
-		return -EINVAL;
-
-	/* Digital section start should be aligned to 4k page */
-	if (!IS_ALIGNED(hdr_size, SZ_4K))
-		return -EINVAL;
-
-	/*
-	 * Read digital section size and check that it also fits inside
-	 * the image.
-	 */
-	ds_size = *(u16 *)(buf + hdr_size);
-	if (ds_size >= image_size)
-		return -EINVAL;
-
-	if (!sw->safe_mode) {
-		u16 device_id;
-
-		/*
-		 * Make sure the device ID in the image matches the one
-		 * we read from the switch config space.
-		 */
-		device_id = *(u16 *)(buf + hdr_size + NVM_DEVID);
-		if (device_id != sw->config.device_id)
-			return -EINVAL;
-
-		if (sw->generation < 3) {
-			/* Write CSS headers first */
-			ret = dma_port_flash_write(sw->dma_port,
-				DMA_PORT_CSS_ADDRESS, buf + NVM_CSS,
-				DMA_PORT_CSS_MAX_SIZE);
-			if (ret)
-				return ret;
-		}
-
-		/* Skip headers in the image */
-		buf += hdr_size;
-		image_size -= hdr_size;
-	}

 	if (tb_switch_is_usb4(sw))
 		ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
@@ -300,7 +255,7 @@ static inline bool nvm_upgradeable(struct tb_switch *sw)
 	return nvm_readable(sw);
 }

-static inline int nvm_read(struct tb_switch *sw, unsigned int address,
+inline int nvm_read(struct tb_switch *sw, unsigned int address,
 			   void *buf, size_t size)
 {
 	if (tb_switch_is_usb4(sw))
@@ -384,28 +339,22 @@ static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
 static int tb_switch_nvm_add(struct tb_switch *sw)
 {
 	struct tb_nvm *nvm;
-	u32 val;
 	int ret;

 	if (!nvm_readable(sw))
 		return 0;

-	/*
-	 * The NVM format of non-Intel hardware is not known so
-	 * currently restrict NVM upgrade for Intel hardware. We may
-	 * relax this in the future when we learn other NVM formats.
-	 */
-	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL &&
-	    sw->config.vendor_id != 0x8087) {
-		dev_info(&sw->dev,
-			 "NVM format of vendor %#x is not known, disabling NVM upgrade\n",
-			 sw->config.vendor_id);
-		return 0;
-	}
+	nvm = tb_switch_nvm_alloc(sw);
+	if (IS_ERR(nvm)) {
+		if (PTR_ERR(nvm) == -EOPNOTSUPP) {
+			dev_info(&sw->dev,
+				"NVM format of vendor %#x is not known, disabling NVM upgrade\n",
+				sw->config.vendor_id);
+			return 0;
+		}

-	nvm = tb_nvm_alloc(&sw->dev);
-	if (IS_ERR(nvm))
 		return PTR_ERR(nvm);
+	}

 	/*
 	 * If the switch is in safe-mode the only accessible portion of
@@ -413,24 +362,7 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
 	 * write new functional NVM.
 	 */
 	if (!sw->safe_mode) {
-		u32 nvm_size, hdr_size;
-
-		ret = nvm_read(sw, NVM_FLASH_SIZE, &val, sizeof(val));
-		if (ret)
-			goto err_nvm;
-
-		hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
-		nvm_size = (SZ_1M << (val & 7)) / 8;
-		nvm_size = (nvm_size - hdr_size) / 2;
-
-		ret = nvm_read(sw, NVM_VERSION, &val, sizeof(val));
-		if (ret)
-			goto err_nvm;
-
-		nvm->major = val >> 16;
-		nvm->minor = val >> 8;
-
-		ret = tb_nvm_add_active(nvm, nvm_size, tb_switch_nvm_read);
+		ret = tb_nvm_add_active(nvm, nvm->nvm_size, tb_switch_nvm_read);
 		if (ret)
 			goto err_nvm;
 	}
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 5db76de40cc1..7ef571f88606 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -42,6 +42,8 @@
  *		   image
  * @authenticating: The device is authenticating the new NVM
  * @flushed: The image has been flushed to the storage area
+ * @nvm_size: Size in bytes of the activate NVM
+ * @vops: Vendor NVM specific operations
  *
  * The user of this structure needs to handle serialization of possible
  * concurrent access.
@@ -57,6 +59,8 @@ struct tb_nvm {
 	size_t buf_data_size;
 	bool authenticating;
 	bool flushed;
+	u32 nvm_size;
+	const struct tb_nvm_vendor_ops *vops;
 };

 enum tb_nvm_write_ops {
@@ -736,6 +740,8 @@ static inline void tb_domain_put(struct tb *tb)
 	put_device(&tb->dev);
 }

+inline int nvm_read(struct tb_switch *sw, unsigned int address,
+			   void *buf, size_t size);
 struct tb_nvm *tb_nvm_alloc(struct device *dev);
 int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, nvmem_reg_read_t reg_read);
 int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void *val,
@@ -759,6 +765,7 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
 				  u64 route);
 struct tb_switch *tb_switch_alloc_safe_mode(struct tb *tb,
 			struct device *parent, u64 route);
+struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw);
 int tb_switch_configure(struct tb_switch *sw);
 int tb_switch_add(struct tb_switch *sw);
 void tb_switch_remove(struct tb_switch *sw);
@@ -767,6 +774,7 @@ int tb_switch_resume(struct tb_switch *sw);
 int tb_switch_reset(struct tb_switch *sw);
 int tb_switch_wait_for_bit(struct tb_switch *sw, u32 offset, u32 bit,
 			   u32 value, int timeout_msec);
+int tb_switch_nvm_validate(struct tb_switch *sw);
 void tb_sw_set_unplugged(struct tb_switch *sw);
 struct tb_port *tb_switch_find_port(struct tb_switch *sw,
 				    enum tb_port_type type);
--
2.34.1

Re: [PATCH v8 1/3] thunderbolt: Add vendor's specific operations of NVM

[kernel test robot]

Hi Szuying,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Szuying-Chen/thunderbolt-add-vendor-s-NVM-formats/20220902-174246
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 42e66b1cc3a070671001f8a1e933a80818a192bf
config: loongarch-randconfig-s051-20220901 (https://download.01.org/0day-ci/archive/20220904/202209040723.T3GBSVUx-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/02a1339d2c5a67367909bfcb11e307d3cfa44f74
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Szuying-Chen/thunderbolt-add-vendor-s-NVM-formats/20220902-174246
        git checkout 02a1339d2c5a67367909bfcb11e307d3cfa44f74
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
   drivers/thunderbolt/nvm.c: note: in included file:
>> drivers/thunderbolt/tb.h:743:20: sparse: sparse: marked inline, but without a definition
>> drivers/thunderbolt/tb.h:743:20: sparse: sparse: marked inline, but without a definition

vim +743 drivers/thunderbolt/tb.h

   742	
 > 743	inline int nvm_read(struct tb_switch *sw, unsigned int address,
   744				   void *buf, size_t size);
   745	struct tb_nvm *tb_nvm_alloc(struct device *dev);
   746	int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, nvmem_reg_read_t reg_read);
   747	int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void *val,
   748			     size_t bytes);
   749	int tb_nvm_add_non_active(struct tb_nvm *nvm, size_t size,
   750				  nvmem_reg_write_t reg_write);
   751	void tb_nvm_free(struct tb_nvm *nvm);
   752	void tb_nvm_exit(void);
   753	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v8 2/3] thunderbolt: Modify tb_nvm major and minor size.

[Szuying Chen]

From: Szuying Chen <Chloe_Chen@asmedia.com.tw>

Intel's version can be stored in 2 bytes, but ASMedia's version
requires 8 bytes. Extend the 'major' and 'minor' members of the tb_nvm
structure to support both vendors.

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
v7->v8: Fix patch message based on suggestion by Mario

 drivers/thunderbolt/tb.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 7ef571f88606..f3f834511983 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -50,8 +50,8 @@
  */
 struct tb_nvm {
 	struct device *dev;
-	u8 major;
-	u8 minor;
+	u32 major;
+	u32 minor;
 	int id;
 	struct nvmem_device *active;
 	struct nvmem_device *non_active;
--
2.34.1

[PATCH v8 3/3] thunderbolt: To extend ASMedia NVM formats.

[Szuying Chen]

From: Szuying Chen <Chloe_Chen@asmedia.com.tw>

The patch add ASMedia NVM formats.

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
v7->v8: Fix the no_nvm_upgrade bit setting on suggestion by Mika.

 drivers/thunderbolt/nvm.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/tb.c  |  2 +-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
index 878d705bd0cb..8393d82dd108 100644
--- a/drivers/thunderbolt/nvm.c
+++ b/drivers/thunderbolt/nvm.c
@@ -12,9 +12,16 @@

 #include "tb.h"

+/* ID of Router */
+#define ROUTER_VENDOR_ID_ASMEDIA 0x174c
+
 /* Switch NVM support */
 #define NVM_CSS		0x10

+/* ASMedia specific NVM offsets */
+#define ASMEDIA_NVM_DATE	0x1c
+#define ASMEDIA_NVM_VERSION	0x28
+
 static DEFINE_IDA(nvm_ida);

 /**
@@ -120,11 +127,43 @@ static int intel_nvm_validate(struct tb_switch *sw)
 	return 0;
 }

+static int asmedia_nvm_version(struct tb_switch *sw)
+{
+	struct tb_nvm *nvm = sw->nvm;
+	u32 val;
+	int ret;
+
+	/* ASMedia get version and date format is xxxxxx.xxxxxx */
+	ret = nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
+	if (ret)
+		return ret;
+
+	nvm->major = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
+
+	ret = nvm_read(sw, ASMEDIA_NVM_DATE, &val, sizeof(val));
+	if (ret)
+		return ret;
+
+	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
+
+	/*
+	 * Asmedia NVM size fixed on 512K. We currently have no plan
+	 * to increase size in the future.
+	 */
+	nvm->nvm_size = SZ_512K;
+
+	return 0;
+}
+
 static const struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
 	.read_version = intel_nvm_version,
 	.validate = intel_nvm_validate,
 };

+static const struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
+	.read_version = asmedia_nvm_version,
+};
+
 struct switch_nvm_vendor {
 	u16 vendor;
 	const struct tb_nvm_vendor_ops *vops;
@@ -133,6 +172,7 @@ struct switch_nvm_vendor {
 static const struct switch_nvm_vendor switch_nvm_vendors[] = {
 	{ PCI_VENDOR_ID_INTEL, &intel_switch_nvm_ops },
 	{ 0x8087, &intel_switch_nvm_ops },
+	{ ROUTER_VENDOR_ID_ASMEDIA, &asmedia_switch_nvm_ops },
 };

 /**
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 9853f6c7e81d..55faa1b5f815 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1417,7 +1417,7 @@ static int tb_start(struct tb *tb)
 	 * mode that is not available so disable firmware upgrade of the
 	 * root switch.
 	 */
-	tb->root_switch->no_nvm_upgrade = true;
+	tb->root_switch->no_nvm_upgrade = !tb_switch_is_usb4(tb->root_switch);
 	/* All USB4 routers support runtime PM */
 	tb->root_switch->rpm = tb_switch_is_usb4(tb->root_switch);

--
2.34.1

Re: [PATCH v8 3/3] thunderbolt: To extend ASMedia NVM formats.

[Limonciello, Mario]

On 9/2/2022 04:40, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> The patch add ASMedia NVM formats.
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---
> v7->v8: Fix the no_nvm_upgrade bit setting on suggestion by Mika.
> 
>   drivers/thunderbolt/nvm.c | 40 +++++++++++++++++++++++++++++++++++++++
>   drivers/thunderbolt/tb.c  |  2 +-
>   2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index 878d705bd0cb..8393d82dd108 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -12,9 +12,16 @@
> 
>   #include "tb.h"
> 
> +/* ID of Router */
> +#define ROUTER_VENDOR_ID_ASMEDIA 0x174c
> +
>   /* Switch NVM support */
>   #define NVM_CSS		0x10
> 
> +/* ASMedia specific NVM offsets */
> +#define ASMEDIA_NVM_DATE	0x1c
> +#define ASMEDIA_NVM_VERSION	0x28
> +
>   static DEFINE_IDA(nvm_ida);
> 
>   /**
> @@ -120,11 +127,43 @@ static int intel_nvm_validate(struct tb_switch *sw)
>   	return 0;
>   }
> 
> +static int asmedia_nvm_version(struct tb_switch *sw)
> +{
> +	struct tb_nvm *nvm = sw->nvm;
> +	u32 val;
> +	int ret;
> +
> +	/* ASMedia get version and date format is xxxxxx.xxxxxx */
> +	ret = nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
> +	if (ret)
> +		return ret;
> +
> +	nvm->major = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
> +
> +	ret = nvm_read(sw, ASMEDIA_NVM_DATE, &val, sizeof(val));
> +	if (ret)
> +		return ret;
> +
> +	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
> +
> +	/*
> +	 * Asmedia NVM size fixed on 512K. We currently have no plan
> +	 * to increase size in the future.
> +	 */
> +	nvm->nvm_size = SZ_512K;

Any chance this can also be gleamed from your NVM?  It would future 
proof the kernel code if you did come up with a need to change it in the 
future some day rather than hardcoding.

> +
> +	return 0;
> +}
> +
>   static const struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
>   	.read_version = intel_nvm_version,
>   	.validate = intel_nvm_validate,
>   };
> 
> +static const struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
> +	.read_version = asmedia_nvm_version,

I recall an earlier version of your patch series was reading the 
customer ID as well.  Would it make sense to have an 
`asmedia_nvm_validate` that checks this matches?

Or any other safety validation that the image is good the kernel might 
want to do?  Checksum or signature or anything?

Even if the hardware does all these verifications it's much easier to 
debug problems if the kernel can do a first line verification to tell 
you what is wrong with the image instead of trying to trace an error 
code from the hardware.

> +};
> +
>   struct switch_nvm_vendor {
>   	u16 vendor;
>   	const struct tb_nvm_vendor_ops *vops;
> @@ -133,6 +172,7 @@ struct switch_nvm_vendor {
>   static const struct switch_nvm_vendor switch_nvm_vendors[] = {
>   	{ PCI_VENDOR_ID_INTEL, &intel_switch_nvm_ops },
>   	{ 0x8087, &intel_switch_nvm_ops },
> +	{ ROUTER_VENDOR_ID_ASMEDIA, &asmedia_switch_nvm_ops },
>   };
> 
>   /**
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 9853f6c7e81d..55faa1b5f815 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -1417,7 +1417,7 @@ static int tb_start(struct tb *tb)
>   	 * mode that is not available so disable firmware upgrade of the
>   	 * root switch.
>   	 */
> -	tb->root_switch->no_nvm_upgrade = true;
> +	tb->root_switch->no_nvm_upgrade = !tb_switch_is_usb4(tb->root_switch);
>   	/* All USB4 routers support runtime PM */
>   	tb->root_switch->rpm = tb_switch_is_usb4(tb->root_switch);
> 
> --
> 2.34.1
> 

Re: [PATCH v8 3/3] thunderbolt: To extend ASMedia NVM formats.

[Mika Westerberg]

Hi Mario,

On Fri, Sep 02, 2022 at 08:32:41AM -0500, Limonciello, Mario wrote:
> On 9/2/2022 04:40, Szuying Chen wrote:
> > From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> > 
> > The patch add ASMedia NVM formats.
> > 
> > Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> > ---
> > v7->v8: Fix the no_nvm_upgrade bit setting on suggestion by Mika.
> > 
> >   drivers/thunderbolt/nvm.c | 40 +++++++++++++++++++++++++++++++++++++++
> >   drivers/thunderbolt/tb.c  |  2 +-
> >   2 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> > index 878d705bd0cb..8393d82dd108 100644
> > --- a/drivers/thunderbolt/nvm.c
> > +++ b/drivers/thunderbolt/nvm.c
> > @@ -12,9 +12,16 @@
> > 
> >   #include "tb.h"
> > 
> > +/* ID of Router */
> > +#define ROUTER_VENDOR_ID_ASMEDIA 0x174c
> > +
> >   /* Switch NVM support */
> >   #define NVM_CSS		0x10
> > 
> > +/* ASMedia specific NVM offsets */
> > +#define ASMEDIA_NVM_DATE	0x1c
> > +#define ASMEDIA_NVM_VERSION	0x28
> > +
> >   static DEFINE_IDA(nvm_ida);
> > 
> >   /**
> > @@ -120,11 +127,43 @@ static int intel_nvm_validate(struct tb_switch *sw)
> >   	return 0;
> >   }
> > 
> > +static int asmedia_nvm_version(struct tb_switch *sw)
> > +{
> > +	struct tb_nvm *nvm = sw->nvm;
> > +	u32 val;
> > +	int ret;
> > +
> > +	/* ASMedia get version and date format is xxxxxx.xxxxxx */
> > +	ret = nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	nvm->major = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
> > +
> > +	ret = nvm_read(sw, ASMEDIA_NVM_DATE, &val, sizeof(val));
> > +	if (ret)
> > +		return ret;
> > +
> > +	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
> > +
> > +	/*
> > +	 * Asmedia NVM size fixed on 512K. We currently have no plan
> > +	 * to increase size in the future.
> > +	 */
> > +	nvm->nvm_size = SZ_512K;
> 
> Any chance this can also be gleamed from your NVM?  It would future proof
> the kernel code if you did come up with a need to change it in the future
> some day rather than hardcoding.
> 
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
> >   	.read_version = intel_nvm_version,
> >   	.validate = intel_nvm_validate,
> >   };
> > 
> > +static const struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
> > +	.read_version = asmedia_nvm_version,
> 
> I recall an earlier version of your patch series was reading the customer ID
> as well.  Would it make sense to have an `asmedia_nvm_validate` that checks
> this matches?

It seems the customer ID was the same 0x28 offset than the
ASMEDIA_NVM_VERSION so I guess it was just renamed.

> 
> Or any other safety validation that the image is good the kernel might want
> to do?  Checksum or signature or anything?
> 
> Even if the hardware does all these verifications it's much easier to debug
> problems if the kernel can do a first line verification to tell you what is
> wrong with the image instead of trying to trace an error code from the
> hardware.

I agree.

Re: [PATCH v8 0/3] thunderbolt: add vendor's NVM formats

[Mika Westerberg]

Hi Szuying Chen,

On Fri, Sep 02, 2022 at 05:40:07PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> The patch series for vendors to extend their NVM format.
> 
> v7->v8: The nvm_read() defined in tb.h. Modify
> tb_switch_nvm_validate() return value and no_nvm_upgrade bit setting.
> 
> Szuying Chen (3):
>   thunderbolt: Add vendor's specific operations of NVM
>   thunderbolt: Modify tb_nvm major and minor size.
>   thunderbolt: To extend ASMedia NVM formats.

Thanks for the patches. There are a couple of things I would still like
to change in this series but I can do those myself, and then I need to
run some testing to make sure Intel NVM formats still work.

No need to send a new version. Once I've done my modifications I will
post the updated series for review and perhaps you can then try on your
side that it still works on ASMedia hardware.

Thanks!