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@xxxxxxxxxxxxxx>
>
> The patch add ASMedia NVM formats.
>
> Signed-off-by: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
> ---
> 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