Message-ID: <e49679b9-7b5a-5d97-c63f-a6004af0aaaa@amd.com>
Date:   Mon, 15 Aug 2022 12:28:13 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Szuying Chen <chensiying21@...il.com>, gregkh@...uxfoundation.org,
        mika.westerberg@...ux.intel.com, andreas.noever@...il.com,
        michael.jamet@...el.com, YehezkelShB@...il.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Yd_Tseng@...edia.com.tw, Chloe_Chen@...edia.com.tw,
        Richard_Hsu@...edia.com.tw, Richard Hughes <rhughes@...hat.com>
Subject: Re: [PATCH] thunderbolt: thunderbolt: add vendor's NVM formats

+hughsie for additional comments

Various inline comments below.

On 8/14/2022 23:11, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@...edia.com.tw>
> 
> The patch add tb_nvm_validate() contain an array that has functions
> pointers to asmedia_nvm_validate().
> And asmedia_nvm_validate() that recognize supported vendor works in one
> of the following cases:
> Case nvm_upgrade: enable nvm's attribute by setting no_nvm_upgrade
> flag to create nvm_authenticate file node.
> Case nvm_add:add active/non-active NVM devices.
> Case nvm_write:update firmware to non-ative NVM device.
> 
> Our patches were through checkpatch.pl. But the file(switch.c.)
> have existed 13 warning before we patch it.

Please feel free to add other patches to the series to clean up 
warnings.  Just because you didn't introduce them doesn't mean you can't 
fix them =)

> 
> Signed-off-by: Szuying Chen <Chloe_Chen@...edia.com.tw>
> ---
>   drivers/thunderbolt/nvm.c    | 147 +++++++++++++++++++++++++++++++++++
>   drivers/thunderbolt/switch.c |  17 ++++
>   drivers/thunderbolt/tb.h     |  18 +++++
>   3 files changed, 182 insertions(+)
> 
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index b3f310389378..6db2034ec8e5 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -9,11 +9,158 @@
>   #include <linux/idr.h>
>   #include <linux/slab.h>
>   #include <linux/vmalloc.h>
> +#include <linux/pm_runtime.h>
> 
>   #include "tb.h"
> 
>   static DEFINE_IDA(nvm_ida);
> 
> +static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val,
> +			      size_t bytes)
> +{
> +	struct tb_nvm *nvm = priv;
> +	struct tb_switch *sw = tb_to_switch(nvm->dev);
> +	int ret;
> +
> +	pm_runtime_get_sync(&sw->dev);
> +	if (!mutex_trylock(&sw->tb->lock)) {
> +		ret = restart_syscall();
> +		goto out;
> +	}
> +	ret = usb4_switch_nvm_read(sw, offset, val, bytes);
> +	mutex_unlock(&sw->tb->lock);
> +
> +out:
> +	pm_runtime_mark_last_busy(&sw->dev);
> +	pm_runtime_put_autosuspend(&sw->dev);
> +
> +	return ret;
> +}
> +
> +static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
> +			       size_t bytes)
> +{
> +	struct tb_nvm *nvm = priv;
> +	struct tb_switch *sw = tb_to_switch(nvm->dev);
> +	int ret;
> +
> +	if (!mutex_trylock(&sw->tb->lock))
> +		return restart_syscall();
> +
> +	/*
> +	 * Since writing the NVM image might require some special steps,
> +	 * for example when CSS headers are written, we cache the image
> +	 * locally here and handle the special cases when the user asks
> +	 * us to authenticate the image.
> +	 */
> +	ret = tb_nvm_write_buf(nvm, offset, val, bytes);
> +	mutex_unlock(&sw->tb->lock);
> +
> +	return ret;
> +}
> +
> +static int asmedia_nvm_validate(struct tb_switch *sw, unsigned int mode)
> +{
> +	struct tb_nvm *nvm;
> +	u32 val;
> +	u32 nvm_size;
> +	int ret = 0;
> +	unsigned int image_size;
> +	const u8 *buf = sw->nvm->buf;
> +
> +	switch (mode) {
> +	case nvm_upgrade:
> +		if (sw->no_nvm_upgrade)
> +			sw->no_nvm_upgrade = false;
> +
> +		break;
> +
> +	case nvm_add:
> +		nvm = tb_nvm_alloc(&sw->dev);
> +		if (IS_ERR(nvm)) {
> +			ret = PTR_ERR(nvm);
> +			break;
> +		}
> +
> +		ret = usb4_switch_nvm_read(sw, NVM_Date, &val, sizeof(val));
> +		if (ret)
> +			break;
> +
> +		nvm->nvm_asm.date = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
> +		ret = usb4_switch_nvm_read(sw, NVM_CUSTOMER_ID, &val, sizeof(val));
> +		if (ret)
> +			break;
> +
> +		nvm->nvm_asm.customerID = (((u8)val) << 0x8 | ((u8)(val >> 0x8)));
> +		nvm->nvm_asm.version = (u8)(val >> 0x10);
> +		nvm_size = SZ_512K;
> +		ret = tb_nvm_add_active(nvm, nvm_size, tb_switch_nvm_read);
> +		if (ret)
> +			break;
> +
> +		ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, tb_switch_nvm_write);
> +		if (ret)
> +			break;
> +
> +		sw->nvm = nvm;
> +		break;
> +
> +	case nvm_write:
> +		if (!buf) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		image_size = sw->nvm->buf_data_size;
> +		if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
> +		if (!ret)
> +			sw->nvm->flushed = true;
> +
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	if ((mode == nvm_add) && (ret != 0))
> +		tb_nvm_free(sw->nvm);
> +
> +	return ret;
> +}
> +
> +struct tb_nvm_id {
> +	u16 hw_vendor_id;
> +	int (*validate)(struct tb_switch *sw, unsigned int handle);
> +};
> +
> +static const struct tb_nvm_id tb_nvm_vendors[] = {
> +	/* ASMedia software CM firmware upgrade */
> +	{ 0x174c, asmedia_nvm_validate },
> +};
> +
> +/**
> + * tb_nvm_vendor_handle() - support vendor's NVM format
> + * @sw: Thunderbolt switch
> + * @handle: 0:NvmUpgradeSuppport, 1:NvmAdd, 2:NvmWrite
> + */
> +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode)
> +{
> +	int res, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tb_nvm_vendors); i++) {
> +		const struct tb_nvm_id *id = &tb_nvm_vendors[i];
> +
> +		if (id->hw_vendor_id && id->hw_vendor_id != sw->config.vendor_id)
> +			continue;
> +
> +		 res = id->validate(sw, mode);
> +	}
> +	return res;
> +}
> +
>   /**
>    * 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..352e64f3dc92 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -114,6 +114,14 @@ static int nvm_validate_and_write(struct tb_switch *sw)
>   	if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE)
>   		return -EINVAL;
> 
> +	/*
> +	 * Vendor's nvm write. If the image has been flushed to the
> +	 * storage are, nvm write is complete.
> +	 */
> +	ret = tb_nvm_validate(sw, nvm_write);
> +	if (sw->nvm->flushed)
> +		return ret;
> +
>   	/*
>   	 * FARB pointer must point inside the image and must at least
>   	 * contain parts of the digital section we will be reading here.
> @@ -390,6 +398,11 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
>   	if (!nvm_readable(sw))
>   		return 0;
> 
> +	/* Vendor's NVM formats add */
> +	ret = tb_nvm_validate(sw, nvm_add);
> +	if (ret)
> +		return ret;
> +
>   	/*
>   	 * The NVM format of non-Intel hardware is not known so
>   	 * currently restrict NVM upgrade for Intel hardware. We may

This comment should be adjusted as after your patch lands both Intel and 
ASMedia formats are known and included.

> @@ -1953,6 +1966,9 @@ static ssize_t nvm_version_show(struct device *dev,
>   		ret = -ENODATA;
>   	else if (!sw->nvm)
>   		ret = -EAGAIN;
> +	/*ASMedia NVM version show format xxxxxx_xxxx_xx */
> +	else if (sw->config.vendor_id == 0x174C)
> +		ret = sprintf(buf, "%06x_%04x_%02x\n", sw->nvm->nvm_asm.date, sw->nvm->nvm_asm.customerID, sw->nvm->nvm_asm.version);

Are you hard-pressed to use this specific format for the string?  I feel 
like it's overloading the definition of a version string quite a bit.

I also worry that userspace has come to expect "major.minor" for this 
and making your string use 2 decimals may mean more deviations for 
userspace too.

Perhaps should you just export it instead as:

"%02x.%06x", sw->nvm->nvm_asm.version, sw->nvm->nvm_asm.date

And move the customer ID into another sysfs file?  I would think this 
fits pretty well with the existing "vendor" or "device" sysfs file 
depending upon it's meaning.

If you do end up having a strong reason for deviating the version string 
format, then I think you should document both what the Intel format is 
(major.minor) and your format explicitly in 
Documentation/admin-guide/thunderbolt.rst.

>   	else
>   		ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor);
> 
> @@ -2860,6 +2876,7 @@ int tb_switch_add(struct tb_switch *sw)
>   		tb_sw_dbg(sw, "uid: %#llx\n", sw->uid);
> 
>   		tb_check_quirks(sw);
> +		tb_nvm_validate(sw, nvm_upgrade);
> 
>   		ret = tb_switch_set_uuid(sw);
>   		if (ret) {
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 5db76de40cc1..7f20f10352d9 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -28,6 +28,22 @@
>   #define NVM_VERSION		0x08
>   #define NVM_FLASH_SIZE		0x45
> 
> +/* ASMedia specific NVM offsets */
> +#define NVM_Date	0x1c
> +#define NVM_CUSTOMER_ID	0x28
> +
> +/* ASMedia specific validation mode */
> +#define nvm_upgrade 0
> +#define nvm_add 1
> +#define nvm_write 2

As all of these values are ASMedia specific, I think the #defines should 
have ASMEDIA in the name.  I know Greg mentioned to roll into an enum, 
and I think you still can but make it something like:

#define ASMEDIA_NVM_DATE	0x1c
#define ASMEDIA_NVM_CUSTOMER_ID	0x28
enum asmeda_nvm_ops {
	ASMEDIA_NVM_UPGRADE = 0,
	ASMEDIA_NVM_ADD = 1,
	ASMEDIA_NVM_WRITE = 2,
};

> +
> +struct nvm_asmedia {
> +	u32 date;
> +	u32 customerID:16;
> +	u32 version:8;
> +	u32 reserved:8;
> +};
> +
>   /**
>    * struct tb_nvm - Structure holding NVM information
>    * @dev: Owner of the NVM
> @@ -57,6 +73,7 @@ struct tb_nvm {
>   	size_t buf_data_size;
>   	bool authenticating;
>   	bool flushed;
> +	struct nvm_asmedia nvm_asm;

Furthermore if you follow my suggestion on how to encode the version you 
can re-use the 'major' and 'minor' members from this struct and don't 
need to deviate in any way from it for your data.

* Major would map to your "version".
* Minor would map to "date".

You could instead then store the customer ID into the switches vendor ID 
or device ID member (whichever makes more sense).

>   };
> 
>   enum tb_nvm_write_ops {
> @@ -736,6 +753,7 @@ static inline void tb_domain_put(struct tb *tb)
>   	put_device(&tb->dev);
>   }
> 
> +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode);
>   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
> 
> 

Powered by blists - more mailing lists