Date:   Fri, 17 Feb 2023 15:36:34 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>, Sanju.Mehta@amd.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] thunderbolt: Refactor DROM reading
Message-ID: <Y++C4gWAfbw2rQUt@black.fi.intel.com>
References: <20230216201910.12370-1-mario.limonciello@amd.com>
 <20230216201910.12370-4-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230216201910.12370-4-mario.limonciello@amd.com>
Precedence: bulk

Hi Mario,

On Thu, Feb 16, 2023 at 02:19:10PM -0600, Mario Limonciello wrote:
> The NVM reading code has a series of gotos that potentially introduce
> unexpected behaviors with retries if something unexpected has failed
> to parse.
> 
> Additionally the retry logic introduced in commit f022ff7bf377
> ("thunderbolt: Retry DROM read once if parsing fails") was added from
> failures in bit banging, which aren't expected to be present when the
> DROM is fetched directly from the NVM.

Okay that's why you remove the EILSEQ returns below, right?

> Refactor the code to remove the gotos and drop the retry logic.

Thanks for doing this. Few minor stylistic comments below. I can also
fix these myself when applying if you prefer.

Note I will be on vacation next week but will be picking up patches once
v6.3-rc1 is released.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2->v3:
>     * Split out refactor
> ---
>  drivers/thunderbolt/eeprom.c | 147 +++++++++++++++++++----------------
>  1 file changed, 79 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index a326cf16ca3d..2a078c69f0d2 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -416,7 +416,7 @@ static int tb_drom_parse_entries(struct tb_switch *sw, size_t header_size)
>  		if (pos + 1 == drom_size || pos + entry->len > drom_size
>  				|| !entry->len) {
>  			tb_sw_warn(sw, "DROM buffer overrun\n");
> -			return -EILSEQ;
> +			return -EIO;
>  		}
>  
>  		switch (entry->type) {
> @@ -543,7 +543,37 @@ static int tb_drom_read_n(struct tb_switch *sw, u16 offset, u8 *val,
>  	return tb_eeprom_read_n(sw, offset, val, count);
>  }
>  
> -static int tb_drom_parse(struct tb_switch *sw)
> +static int tb_drom_bit_bang(struct tb_switch *sw, u16 *size)
> +{
> +	int res;

ret

> +
> +	res = tb_drom_read_n(sw, 14, (u8 *) size, 2);
> +	if (res)
> +		return res;

empty line here.

> +	*size &= 0x3ff;
> +	*size += TB_DROM_DATA_START;

here too.

> +	tb_sw_dbg(sw, "reading drom (length: %#x)\n", *size);
> +	if (*size < sizeof(struct tb_drom_header)) {
> +		tb_sw_warn(sw, "drom too small, aborting\n");

DROM

> +		return -EIO;
> +	}
> +
> +	sw->drom = kzalloc(*size, GFP_KERNEL);
> +	if (!sw->drom)
> +		return -ENOMEM;
> +
> +	res = tb_drom_read_n(sw, 0, sw->drom, *size);
> +	if (res)
> +		goto err;
> +
> +	return 0;

empty line

> +err:
> +	kfree(sw->drom);
> +	sw->drom = NULL;

empty line

> +	return res;
> +}
> +
> +static int tb_drom_parse_v1(struct tb_switch *sw)
>  {
>  	const struct tb_drom_header *header =
>  		(const struct tb_drom_header *)sw->drom;
> @@ -554,7 +584,7 @@ static int tb_drom_parse(struct tb_switch *sw)
>  		tb_sw_warn(sw,
>  			"DROM UID CRC8 mismatch (expected: %#x, got: %#x)\n",
>  			header->uid_crc8, crc);
> -		return -EILSEQ;
> +		return -EIO;
>  	}
>  	if (!sw->uid)
>  		sw->uid = header->uid;
> @@ -588,6 +618,43 @@ static int usb4_drom_parse(struct tb_switch *sw)
>  	return tb_drom_parse_entries(sw, USB4_DROM_HEADER_SIZE);
>  }
>  
> +static int tb_drom_parse(struct tb_switch *sw, u16 *size)
> +{
> +	struct tb_drom_header *header = (void *) sw->drom;
> +	int res;

ret

> +
> +	if (header->data_len + TB_DROM_DATA_START != *size) {
> +		tb_sw_warn(sw, "drom size mismatch\n");

DROM

> +		goto err;
> +	}
> +
> +	tb_sw_dbg(sw, "DROM version: %d\n", header->device_rom_revision);
> +
> +	switch (header->device_rom_revision) {
> +	case 3:
> +		res = usb4_drom_parse(sw);
> +		break;
> +	default:
> +		tb_sw_warn(sw, "DROM device_rom_revision %#x unknown\n",
> +			   header->device_rom_revision);
> +		fallthrough;
> +	case 1:
> +		res = tb_drom_parse_v1(sw);
> +		break;
> +	}
> +
> +	if (res) {
> +		tb_sw_warn(sw, "parsing DROM failed\n");
> +		goto err;
> +	}
> +
> +	return 0;

empty line

> +err:
> +	kfree(sw->drom);
> +	sw->drom = NULL;

empty line

> +	return -EIO;
> +}
> +
>  /**
>   * tb_drom_read() - Copy DROM to sw->drom and parse it
>   * @sw: Router whose DROM to read and parse
> @@ -601,8 +668,7 @@ static int usb4_drom_parse(struct tb_switch *sw)
>  int tb_drom_read(struct tb_switch *sw)
>  {
>  	u16 size;
> -	struct tb_drom_header *header;
> -	int res, retries = 1;
> +	int res;
>  
>  	if (sw->drom)
>  		return 0;
> @@ -613,11 +679,11 @@ int tb_drom_read(struct tb_switch *sw)
>  		 * in a device property. Use it if available.
>  		 */
>  		if (tb_drom_copy_efi(sw, &size) == 0)
> -			goto parse;
> +			return tb_drom_parse(sw, &size);
>  
>  		/* Non-Apple hardware has the DROM as part of NVM */
>  		if (tb_drom_copy_nvm(sw, &size) == 0)
> -			goto parse;
> +			return tb_drom_parse(sw, &size);
>  
>  		/*
>  		 * USB4 hosts may support reading DROM through router
> @@ -626,7 +692,7 @@ int tb_drom_read(struct tb_switch *sw)
>  		if (tb_switch_is_usb4(sw)) {
>  			usb4_switch_read_uid(sw, &sw->uid);
>  			if (!usb4_copy_host_drom(sw, &size))
> -				goto parse;
> +				return tb_drom_parse(sw, &size);
>  		} else {
>  			/*
>  			 * The root switch contains only a dummy drom
> @@ -640,67 +706,12 @@ int tb_drom_read(struct tb_switch *sw)
>  	}
>  
>  	/* We can use LC to get UUID later */
> -	if (sw->cap_lc && tb_drom_copy_nvm(sw, &size) == 0)
> -		goto parse;
> -
> -	res = tb_drom_read_n(sw, 14, (u8 *) &size, 2);
> +	if (sw->cap_lc)
> +		res = tb_drom_copy_nvm(sw, &size);
> +	else
> +		res = tb_drom_bit_bang(sw, &size);
>  	if (res)
>  		return res;
> -	size &= 0x3ff;
> -	size += TB_DROM_DATA_START;
> -	tb_sw_dbg(sw, "reading drom (length: %#x)\n", size);
> -	if (size < sizeof(*header)) {
> -		tb_sw_warn(sw, "drom too small, aborting\n");
> -		return -EIO;
> -	}
> -
> -	sw->drom = kzalloc(size, GFP_KERNEL);
> -	if (!sw->drom)
> -		return -ENOMEM;
> -read:
> -	res = tb_drom_read_n(sw, 0, sw->drom, size);
> -	if (res)
> -		goto err;
> -
> -parse:
> -	header = (void *) sw->drom;
> -
> -	if (header->data_len + TB_DROM_DATA_START != size) {
> -		tb_sw_warn(sw, "drom size mismatch\n");
> -		if (retries--) {
> -			msleep(100);
> -			goto read;
> -		}
> -		goto err;
> -	}
> -
> -	tb_sw_dbg(sw, "DROM version: %d\n", header->device_rom_revision);
> -
> -	switch (header->device_rom_revision) {
> -	case 3:
> -		res = usb4_drom_parse(sw);
> -		break;
> -	default:
> -		tb_sw_warn(sw, "DROM device_rom_revision %#x unknown\n",
> -			   header->device_rom_revision);
> -		fallthrough;
> -	case 1:
> -		res = tb_drom_parse(sw);
> -		break;
> -	}
> -
> -	/* If the DROM parsing fails, wait a moment and retry once */
> -	if (res == -EILSEQ && retries--) {
> -		tb_sw_warn(sw, "parsing DROM failed\n");
> -		msleep(100);
> -		goto read;
> -	}
>  
> -	if (!res)
> -		return 0;
> -
> -err:
> -	kfree(sw->drom);
> -	sw->drom = NULL;
> -	return -EIO;
> +	return tb_drom_parse(sw, &size);
>  }
> -- 
> 2.34.1