[PATCH v5] thunderbolt: thunderbolt: add vendor's NVM formats

[PATCH v5] thunderbolt: thunderbolt: add vendor's NVM formats

[Szuying Chen]

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

The patch add tb_switch_nvm_alloc() contain an array that has functions
pointers to vendor_ops that vendor to define.
And add tb_switch_nvm_upgradable() to enable nvm upgrade for vendors
that in switch_nvm_upgrade_vendors[].

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
v4 -> v5 : Take Mika's suggestion and use nvm->vops to point the vendor
specific operations. Moved vendor:intel part of the code to make all
the vendors (includes Intel) support it in nvm.c. And add nvm specific
operations for ASMedia.

 drivers/thunderbolt/nvm.c    | 204 +++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/switch.c | 100 ++++-------------
 drivers/thunderbolt/tb.h     |  19 +++-
 3 files changed, 242 insertions(+), 81 deletions(-)

diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
index b3f310389378..6ee048565616 100644
--- a/drivers/thunderbolt/nvm.c
+++ b/drivers/thunderbolt/nvm.c
@@ -12,8 +12,212 @@

 #include "tb.h"

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

+/* Vendor provides NVM firmware upgrade function */
+static const u32 switch_nvm_upgrade_vendors[] = {
+	0x174c,
+};
+
+void tb_switch_nvm_upgradable(struct tb_switch *sw)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(switch_nvm_upgrade_vendors); i++) {
+		if (switch_nvm_upgrade_vendors[i] == sw->config.vendor_id)
+			sw->no_nvm_upgrade = false;
+	}
+}
+
+static inline int nvm_read(struct tb_switch *sw, unsigned int address,
+			   void *buf, size_t size)
+{
+	if (tb_switch_is_usb4(sw))
+		return usb4_switch_nvm_read(sw, address, buf, size);
+	return dma_port_flash_read(sw->dma_port, address, buf, size);
+}
+
+static int  asmedia_nvm_version(struct tb_switch *sw)
+{
+	struct tb_nvm *nvm = sw->nvm;
+	u64 val;
+	int ret;
+
+	ret = nvm_read(sw, 0x1c, &val, sizeof(val));
+	if (ret)
+		return ret;
+
+	nvm->major = (((u8)val >> 0x18) << 0x10 | ((u8)(val >> 0x20)) << 0x8 | (u8)(val >> 0x28));
+	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
+	nvm->nvm_size = SZ_512K;
+
+	return 0;
+}
+
+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;
+	const 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 */
+		buf += hdr_size;
+		image_size -= hdr_size;
+	}
+
+	return image_size;
+}
+
+struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
+	.version = asmedia_nvm_version,
+};
+
+struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
+	.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[] = {
+	{ 0x8086, &intel_switch_nvm_ops },
+	{ 0x8087, &intel_switch_nvm_ops },
+	{ 0x174c, &asmedia_switch_nvm_ops },
+
+};
+
+/**
+ * tb_switch_nvm_alloc() - alloc nvm and set nvm->vops to point
+ * the vendor specific operations.
+ * @sw: thunderbolt switch
+ *
+ */
+struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw)
+{
+	const struct tb_nvm_vendor_ops *vops = NULL;
+	struct tb_nvm *nvm;
+	int i;
+	int ret;
+
+	/**
+	 * 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->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..392586641828 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -102,10 +102,10 @@ 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;
+	unsigned int image_size;
 	const u8 *buf = sw->nvm->buf;
-	u16 ds_size;
 	int ret;
+	const struct tb_nvm_vendor_ops *vops = sw->nvm->vops;

 	if (!buf)
 		return -EINVAL;
@@ -114,49 +114,13 @@ static int nvm_validate_and_write(struct tb_switch *sw)
 	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;
-		}
+	/*Vendors to validate before write to router NVM*/
+	if (vops->validate) {
+		ret = vops->validate(sw);
+		if (ret < 0)
+			return ret;

-		/* Skip headers in the image */
-		buf += hdr_size;
-		image_size -= hdr_size;
+		image_size = ret;
 	}

 	if (tb_switch_is_usb4(sw))
@@ -384,28 +348,21 @@ 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_nvm_alloc(&sw->dev);
-	if (IS_ERR(nvm))
+	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;
+		}
 		return PTR_ERR(nvm);
+	}

 	/*
 	 * If the switch is in safe-mode the only accessible portion of
@@ -413,24 +370,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;
 	}
@@ -442,7 +382,6 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
 			goto err_nvm;
 	}

-	sw->nvm = nvm;
 	return 0;

 err_nvm:
@@ -2890,6 +2829,9 @@ int tb_switch_add(struct tb_switch *sw)
 			return ret;
 	}

+	/* Enable the NVM firmware upgrade for specific vendors */
+	tb_switch_nvm_upgradable(sw);
+
 	ret = device_add(&sw->dev);
 	if (ret) {
 		dev_err(&sw->dev, "failed to add device: %d\n", ret);
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 5db76de40cc1..24b3db35bd9b 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -48,8 +48,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;
@@ -57,6 +57,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 {
@@ -65,6 +67,17 @@ enum tb_nvm_write_ops {
 	AUTHENTICATE_ONLY = 3,
 };

+/**
+ * struct tb_nvm_vendor_ops - vendor NVM specific operations
+ * @version: Used NVM read get Firmware version.
+ * @validate: Vendors have their validate method before NVM write.
+ *
+ */
+struct tb_nvm_vendor_ops {
+	int (*version)(struct tb_switch *sw);
+	int (*validate)(struct tb_switch *sw);
+};
+
 #define TB_SWITCH_KEY_SIZE		32
 #define TB_SWITCH_MAX_DEPTH		6
 #define USB4_SWITCH_MAX_DEPTH		5
@@ -736,6 +749,8 @@ static inline void tb_domain_put(struct tb *tb)
 	put_device(&tb->dev);
 }

+void tb_switch_nvm_upgradable(struct tb_switch *sw);
+struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw);
 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,
--
2.34.1

Re: [PATCH v5] thunderbolt: thunderbolt: add vendor's NVM formats

[Greg KH]

On Mon, Aug 22, 2022 at 03:31:05PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> The patch add tb_switch_nvm_alloc() contain an array that has functions
> pointers to vendor_ops that vendor to define.
> And add tb_switch_nvm_upgradable() to enable nvm upgrade for vendors
> that in switch_nvm_upgrade_vendors[].
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---
> v4 -> v5 : Take Mika's suggestion and use nvm->vops to point the vendor
> specific operations. Moved vendor:intel part of the code to make all
> the vendors (includes Intel) support it in nvm.c. And add nvm specific
> operations for ASMedia.
> 
>  drivers/thunderbolt/nvm.c    | 204 +++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/switch.c | 100 ++++-------------
>  drivers/thunderbolt/tb.h     |  19 +++-
>  3 files changed, 242 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index b3f310389378..6ee048565616 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -12,8 +12,212 @@
> 
>  #include "tb.h"
> 
> +/* Switch NVM support */
> +#define NVM_CSS
> +
>  static DEFINE_IDA(nvm_ida);
> 
> +/* Vendor provides NVM firmware upgrade function */
> +static const u32 switch_nvm_upgrade_vendors[] = {
> +	0x174c,
> +};
> +
> +void tb_switch_nvm_upgradable(struct tb_switch *sw)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(switch_nvm_upgrade_vendors); i++) {
> +		if (switch_nvm_upgrade_vendors[i] == sw->config.vendor_id)
> +			sw->no_nvm_upgrade = false;
> +	}
> +}
> +
> +static inline int nvm_read(struct tb_switch *sw, unsigned int address,
> +			   void *buf, size_t size)
> +{
> +	if (tb_switch_is_usb4(sw))
> +		return usb4_switch_nvm_read(sw, address, buf, size);
> +	return dma_port_flash_read(sw->dma_port, address, buf, size);
> +}
> +
> +static int  asmedia_nvm_version(struct tb_switch *sw)

Odd extra space :(

> +{
> +	struct tb_nvm *nvm = sw->nvm;
> +	u64 val;
> +	int ret;
> +
> +	ret = nvm_read(sw, 0x1c, &val, sizeof(val));
> +	if (ret)
> +		return ret;
> +
> +	nvm->major = (((u8)val >> 0x18) << 0x10 | ((u8)(val >> 0x20)) << 0x8 | (u8)(val >> 0x28));
> +	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));

That's tough to read, want to add a comment explaining what you are
doing here?  Like "the major number is in bits X-Y..." and such?

> +	nvm->nvm_size = SZ_512K;
> +
> +	return 0;
> +}
> +
> +static int  intel_nvm_version(struct tb_switch *sw)

extra space.

> +{
> +	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;
> +

extra blank line.  Did you run this patch through checkpatch.pl first?

> +	}
> +
> +	return 0;
> +}
> +
> +static int  intel_nvm_validate(struct tb_switch *sw)
> +{
> +	unsigned int image_size, hdr_size;
> +	const 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 */
> +		buf += hdr_size;
> +		image_size -= hdr_size;
> +	}
> +
> +	return image_size;
> +}
> +
> +struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
> +	.version = asmedia_nvm_version,
> +};
> +
> +struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
> +	.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[] = {
> +	{ 0x8086, &intel_switch_nvm_ops },
> +	{ 0x8087, &intel_switch_nvm_ops },
> +	{ 0x174c, &asmedia_switch_nvm_ops },
> +
> +};
> +
> +/**
> + * tb_switch_nvm_alloc() - alloc nvm and set nvm->vops to point
> + * the vendor specific operations.
> + * @sw: thunderbolt switch
> + *
> + */
> +struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw)
> +{
> +	const struct tb_nvm_vendor_ops *vops = NULL;
> +	struct tb_nvm *nvm;
> +	int i;
> +	int ret;
> +
> +	/**

No need for kernel-doc format here.

> +	 * 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->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..392586641828 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -102,10 +102,10 @@ 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;
> +	unsigned int image_size;
>  	const u8 *buf = sw->nvm->buf;
> -	u16 ds_size;
>  	int ret;
> +	const struct tb_nvm_vendor_ops *vops = sw->nvm->vops;
> 
>  	if (!buf)
>  		return -EINVAL;
> @@ -114,49 +114,13 @@ static int nvm_validate_and_write(struct tb_switch *sw)
>  	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;
> -		}
> +	/*Vendors to validate before write to router NVM*/

Extra ' ' character please.

thanks,

greg k-h

Re: [PATCH v5] thunderbolt: thunderbolt: add vendor's NVM formats

[Mika Westerberg]

Hi,

On Mon, Aug 22, 2022 at 03:31:05PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> The patch add tb_switch_nvm_alloc() contain an array that has functions
> pointers to vendor_ops that vendor to define.
> And add tb_switch_nvm_upgradable() to enable nvm upgrade for vendors
> that in switch_nvm_upgrade_vendors[].
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---
> v4 -> v5 : Take Mika's suggestion and use nvm->vops to point the vendor
> specific operations. Moved vendor:intel part of the code to make all
> the vendors (includes Intel) support it in nvm.c. And add nvm specific
> operations for ASMedia.

In addition to what Greg said, please split this into series. First
patch moves the existing Intel stuff and second patch adds the ASMedia
stuff on top and please run all them through checkpatch.pl and make
sure you fix every single warning.

Also please fix the $subject, it now has "thunderbolt: thunderbolt: ..".

> 
>  drivers/thunderbolt/nvm.c    | 204 +++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/switch.c | 100 ++++-------------
>  drivers/thunderbolt/tb.h     |  19 +++-
>  3 files changed, 242 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index b3f310389378..6ee048565616 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -12,8 +12,212 @@
> 
>  #include "tb.h"
> 
> +/* Switch NVM support */
> +#define NVM_CSS
> +
>  static DEFINE_IDA(nvm_ida);
> 
> +/* Vendor provides NVM firmware upgrade function */
> +static const u32 switch_nvm_upgrade_vendors[] = {
> +	0x174c,
> +};
> +
> +void tb_switch_nvm_upgradable(struct tb_switch *sw)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(switch_nvm_upgrade_vendors); i++) {
> +		if (switch_nvm_upgrade_vendors[i] == sw->config.vendor_id)
> +			sw->no_nvm_upgrade = false;
> +	}
> +}
> +
> +static inline int nvm_read(struct tb_switch *sw, unsigned int address,
> +			   void *buf, size_t size)
> +{
> +	if (tb_switch_is_usb4(sw))
> +		return usb4_switch_nvm_read(sw, address, buf, size);
> +	return dma_port_flash_read(sw->dma_port, address, buf, size);
> +}
> +
> +static int  asmedia_nvm_version(struct tb_switch *sw)
> +{
> +	struct tb_nvm *nvm = sw->nvm;
> +	u64 val;
> +	int ret;
> +
> +	ret = nvm_read(sw, 0x1c, &val, sizeof(val));
> +	if (ret)
> +		return ret;
> +
> +	nvm->major = (((u8)val >> 0x18) << 0x10 | ((u8)(val >> 0x20)) << 0x8 | (u8)(val >> 0x28));
> +	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
> +	nvm->nvm_size = SZ_512K;
> +
> +	return 0;
> +}
> +
> +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;
> +	const 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 */
> +		buf += hdr_size;
> +		image_size -= hdr_size;
> +	}
> +
> +	return image_size;
> +}
> +
> +struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
> +	.version = asmedia_nvm_version,
> +};
> +
> +struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
> +	.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[] = {
> +	{ 0x8086, &intel_switch_nvm_ops },
> +	{ 0x8087, &intel_switch_nvm_ops },
> +	{ 0x174c, &asmedia_switch_nvm_ops },

Only single table is needed, please get rid of
switch_nvm_upgrade_vendors[] so this one is easy to extend with another
vendor. Also order these numerically and you can use the PCI_VENDOR_ if
possible and available.

> +
> +};
> +
> +/**
> + * tb_switch_nvm_alloc() - alloc nvm and set nvm->vops to point
> + * the vendor specific operations.
> + * @sw: thunderbolt switch
> + *

Extra emtpy line, please get rid of all these before submitting another
version.

> + */
> +struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw)
> +{
> +	const struct tb_nvm_vendor_ops *vops = NULL;
> +	struct tb_nvm *nvm;
> +	int i;
> +	int ret;
> +
> +	/**
> +	 * 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->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..392586641828 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -102,10 +102,10 @@ 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;
> +	unsigned int image_size;
>  	const u8 *buf = sw->nvm->buf;
> -	u16 ds_size;
>  	int ret;
> +	const struct tb_nvm_vendor_ops *vops = sw->nvm->vops;
> 
>  	if (!buf)
>  		return -EINVAL;
> @@ -114,49 +114,13 @@ static int nvm_validate_and_write(struct tb_switch *sw)
>  	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;
> -		}
> +	/*Vendors to validate before write to router NVM*/

And fix this one too, again checkpatch.pl helps use it.

> +	if (vops->validate) {
> +		ret = vops->validate(sw);
> +		if (ret < 0)
> +			return ret;
> 
> -		/* Skip headers in the image */
> -		buf += hdr_size;
> -		image_size -= hdr_size;
> +		image_size = ret;
>  	}
> 
>  	if (tb_switch_is_usb4(sw))
> @@ -384,28 +348,21 @@ 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_nvm_alloc(&sw->dev);
> -	if (IS_ERR(nvm))
> +	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;
> +		}
>  		return PTR_ERR(nvm);
> +	}
> 
>  	/*
>  	 * If the switch is in safe-mode the only accessible portion of
> @@ -413,24 +370,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;
>  	}
> @@ -442,7 +382,6 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
>  			goto err_nvm;
>  	}
> 
> -	sw->nvm = nvm;
>  	return 0;
> 
>  err_nvm:
> @@ -2890,6 +2829,9 @@ int tb_switch_add(struct tb_switch *sw)
>  			return ret;
>  	}
> 
> +	/* Enable the NVM firmware upgrade for specific vendors */
> +	tb_switch_nvm_upgradable(sw);
> +
>  	ret = device_add(&sw->dev);
>  	if (ret) {
>  		dev_err(&sw->dev, "failed to add device: %d\n", ret);
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 5db76de40cc1..24b3db35bd9b 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -48,8 +48,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;
> @@ -57,6 +57,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 {
> @@ -65,6 +67,17 @@ enum tb_nvm_write_ops {
>  	AUTHENTICATE_ONLY = 3,
>  };
> 
> +/**
> + * struct tb_nvm_vendor_ops - vendor NVM specific operations
> + * @version: Used NVM read get Firmware version.
> + * @validate: Vendors have their validate method before NVM write.
> + *
> + */
> +struct tb_nvm_vendor_ops {
> +	int (*version)(struct tb_switch *sw);
> +	int (*validate)(struct tb_switch *sw);
> +};
> +
>  #define TB_SWITCH_KEY_SIZE		32
>  #define TB_SWITCH_MAX_DEPTH		6
>  #define USB4_SWITCH_MAX_DEPTH		5
> @@ -736,6 +749,8 @@ static inline void tb_domain_put(struct tb *tb)
>  	put_device(&tb->dev);
>  }
> 
> +void tb_switch_nvm_upgradable(struct tb_switch *sw);
> +struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw);
>  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,
> --
> 2.34.1

Re: [PATCH v5] thunderbolt: thunderbolt: add vendor's NVM formats

[kernel test robot]

Hi Szuying,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.0-rc2 next-20220822]
[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-thunderbolt-add-vendor-s-NVM-formats/20220822-153229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1c23f9e627a7b412978b4e852793c5e3c3efc555
config: openrisc-randconfig-r001-20220821 (https://download.01.org/0day-ci/archive/20220822/202208222014.gb1mZ0S1-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/abf17cfc1bbabb9346b4ca1ab723628389b773c5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Szuying-Chen/thunderbolt-thunderbolt-add-vendor-s-NVM-formats/20220822-153229
        git checkout abf17cfc1bbabb9346b4ca1ab723628389b773c5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/soc/qcom/ drivers/thunderbolt/

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

All errors (new ones prefixed by >>):

   drivers/thunderbolt/nvm.c: In function 'intel_nvm_validate':
>> drivers/thunderbolt/nvm.c:140:68: error: expected expression before ',' token
     140 |                                 DMA_PORT_CSS_ADDRESS, buf + NVM_CSS,
         |                                                                    ^


vim +140 drivers/thunderbolt/nvm.c

    94	
    95	static int  intel_nvm_validate(struct tb_switch *sw)
    96	{
    97		unsigned int image_size, hdr_size;
    98		const u8 *buf = sw->nvm->buf;
    99		u16 ds_size;
   100		int ret;
   101	
   102		image_size = sw->nvm->buf_data_size;
   103		if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE)
   104			return -EINVAL;
   105	
   106		/*
   107		 * FARB pointer must point inside the image and must at least
   108		 * contain parts of the digital section we will be reading here.
   109		 */
   110		hdr_size = (*(u32 *)buf) & 0xffffff;
   111		if (hdr_size + NVM_DEVID + 2 >= image_size)
   112			return -EINVAL;
   113	
   114		/* Digital section start should be aligned to 4k page */
   115		if (!IS_ALIGNED(hdr_size, SZ_4K))
   116			return -EINVAL;
   117	
   118		/*
   119		 * Read digital section size and check that it also fits inside
   120		 * the image.
   121		 */
   122		ds_size = *(u16 *)(buf + hdr_size);
   123		if (ds_size >= image_size)
   124			return -EINVAL;
   125	
   126		if (!sw->safe_mode) {
   127			u16 device_id;
   128	
   129			/*
   130			 * Make sure the device ID in the image matches the one
   131			 * we read from the switch config space.
   132			 */
   133			device_id = *(u16 *)(buf + hdr_size + NVM_DEVID);
   134			if (device_id != sw->config.device_id)
   135				return -EINVAL;
   136	
   137			if (sw->generation < 3) {
   138				/* Write CSS headers first */
   139				ret = dma_port_flash_write(sw->dma_port,
 > 140					DMA_PORT_CSS_ADDRESS, buf + NVM_CSS,
   141					DMA_PORT_CSS_MAX_SIZE);
   142				if (ret)
   143					return ret;
   144			}
   145	
   146			/* Skip headers in the image */
   147			buf += hdr_size;
   148			image_size -= hdr_size;
   149		}
   150	
   151		return image_size;
   152	}
   153	

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