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@xxxxxxx>
> ---
> 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
