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;
Date:   Wed, 17 Aug 2022 15:04:22 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Szuying Chen <chensiying21@gmail.com>
Cc:     mario.limonciello@amd.com, 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
Message-ID: <YvzZRsyGR/hakhIo@black.fi.intel.com>
References: <20220817102450.63514-1-chensiying21@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220817102450.63514-1-chensiying21@gmail.com>
Precedence: bulk

Hi,

On Wed, Aug 17, 2022 at 06:24:50PM +0800, Szuying Chen wrote:
> 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.

So instead what you now do, I suggest that we move all the vendor
support out to nvm.c, that includes Intel too. What I mean by this is
that the tb_switch_nvm_add() would then look something like this:

tb_switch_nvm_add(sw)
{
	if (!nvm_readable(sw))
		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;
		}

		return PTR_ERR(nvm);
	}

	ret = tb_nvm_add_active(nvm, nvm->size, tb_switch_nvm_read);
	if (ret)
		...

	if (!sw->no_nvm_upgrade) {
		ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, tb_switch_nvm_write);
		if (ret)
			...
	}

	sw->nvm = nvm;
	...
}

And the tb_switch_nvm_alloc() resides in nvm.c and that one goes over an
array of supported vendors matching sw->config.vendor_id and if it finds
the match it will set nvm->vops to point the vendor specific operations
and in addition it will will populate rest of the nvm fields like this:

static const struct {
	u16 vendor;
	const struct tb_nvm_vendor_ops *vops;
} switch_nvm_vendors[] = {
	{ 0x8086, &intel_switch_nvm_ops },
	{ 0x8087, &intel_switch_nvm_ops },
	{ 0x174c, &asmedia_switch_nvm_ops },
};

tb_switch_nvm_alloc(sw)
{
	struct tb_nvm_vendor_ops *ops = NULL;
	int i;

	for (i = 0; i < ARRAY_SIZE(switch_nvm_vendors); i++) {
		if (switch_nvm_vendors[i].vendor == sw->config.vendor_id)
			vops = &switch_nvm_vendors[i].vops;
			break;
	}

	if (!vops)
		return ERR_PTR(-EOPNOTSUPP);

	nvm = tb_nvm_alloc(&sw->dev);
	if (IS_ERR(nvm))
		...

	nvm->vops = vops;
	ret = vops->populate(nvm);
	if (ret)
		...

	...
}

Then we would have all the vendor specific things in
intel_switch_nvm_ops and asmedia_switch_nvm_ops accordingly and the rest
of the code is generic USB4 stuff. We need to do the same for retimers
too at some point.