Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Szuying Chen <chensiying21@gmail.com>
To:     mika.westerberg@linux.intel.com, mario.limonciello@amd.com
Cc:     gregkh@linuxfoundation.org, andreas.noever@gmail.com,
        michael.jamet@intel.com, YehezkelShB@gmail.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Yd_Tseng@asmedia.com.tw, Chloe_Chen@asmedia.com.tw,
        Richard_Hsu@asmedia.com.tw
Subject: RE: RE: [PATCH v4] thunderbolt: thunderbolt: add vendor's NVM formats
Date:   Wed, 17 Aug 2022 18:24:50 +0800
Message-Id: <20220817102450.63514-1-chensiying21@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
Hi,

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

>> +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;
>> +
>> +	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);
>
>This function does not only "validate" but it also creates the NVMem
>devices and whatnot.
>
>Do you have some public description of the ASMedia format that I could
>take a look? Perhaps we can find some simpler way of validating the
>thing that works accross different vendors.
>

ASMedia NVM format include rom file, firmware and security configuration information. And active firmware depend on this information for processing. We don't need to do any validation during firmware upgrade, so we haven't public description of the ASMedia format.

I think I use "validate" is not fit. This function mainly to create the NVMem devices and write. I will rename in the next patch.

>> +			ret = PTR_ERR(nvm);
>> +			break;
>> +		}
>> +
>> +		ret = usb4_switch_nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
>> +		if (ret)
>> +			break;
>> +
>> +		nvm->nvm_asm.major = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
>> +		ret = usb4_switch_nvm_read(sw, ASMEDIA_NVM_DATE, &val, sizeof(val));
>> +		if (ret)
>> +			break;
>> +
>> +		nvm->nvm_asm.minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (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:
>> +		const u8 *buf = sw->nvm->buf;
>> +
>> +		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;
>> +}


>> @@ -1953,6 +1971,9 @@ static ssize_t nvm_version_show(struct device *dev,
>>  		ret = -ENODATA;
>>  	else if (!sw->nvm)
>>  		ret = -EAGAIN;
>> +	/*ASMedia NVM version show format xxxxxx_xxxxxx */
>> +	else if (sw->config.vendor_id == 0x174C)
>> +		ret = sprintf(buf, "%06x.%06x\n", sw->nvm->nvm_asm.major, sw->nvm->nvm_asm.minor);
>
>And yes, we can make the nvm->major/minor to be 32-bit integers too for
>both Intel and ASMedia and continue to use the %x.%x formatting.
>

Thanks to Mika and Mario for the suggestion, I'll fix it in next patch.

>>  	else
>>  		ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor);
>>
>> @@ -2860,6 +2881,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..7f5c8ae731a0 100644
>> --- a/drivers/thunderbolt/tb.h
>> +++ b/drivers/thunderbolt/tb.h
>> @@ -28,6 +28,15 @@
>>  #define NVM_VERSION		0x08
>>  #define NVM_FLASH_SIZE		0x45
>>
>> +/* ASMedia specific NVM offsets */
>> +#define ASMEDIA_NVM_VERSION	0x28
>> +#define ASMEDIA_NVM_DATE	0x1c
>
>Didn't I already commented about these? Are my emails somehow lost or
>they just get ignored?
>

Sorry, I miss it. I've checked and I will fix it in next patch.