Messages in this thread | | | Date | Thu, 16 Feb 2023 00:04:39 -0600 | Subject | Re: [PATCH v2 1/2] thunderbolt: Adjust how NVM reading works | From | Mario Limonciello <> |
| |
On 2/15/23 23:49, Mika Westerberg wrote: > Hi Mario, > > On Wed, Feb 15, 2023 at 11:25:19AM -0600, Mario Limonciello wrote: >> Some TBT3 devices have a hard time reliably responding to bit banging >> requests correctly when connected to AMD USB4 hosts running Linux. >> >> These problems are not reported in any other CM supported on AMD platforms, >> and comparing the Windows and Pre-OS implementations the Linux CM is the >> only one that utilizes bit banging to access the DROM. >> Other CM implementations access the DROM directly from the NVM instead of >> bit banging. >> >> Adjust the flow to use this method to fetch the NVM when the downstream >> device is Thunderbolt 3 and only use bit banging to access TBT 2 or TBT 1 >> devices. As the flow is modified, also remove the retry sequence that was >> introduced from commit f022ff7bf377 ("thunderbolt: Retry DROM read once >> if parsing fails") as it will not be necessary if the NVM is fetched this >> way. >> >> Cc: stable@vger.kernel.org >> Fixes: f022ff7bf377 ("thunderbolt: Retry DROM read once if parsing fails") > I don't think it fixes a regression of above commit and I don't think > this is stable material because it is quite a big change. I would rather > let it sit in -rcX for a while to make sure no user visible changes are > accidentally introduced. Is this OK for you?
No worry on dropping these two tags. I'll do that for a v3. The original change in v1 was more stable material than this is now.
While adopting your feedback below I'll keep in mind patch ordering for refactoring as I do want to get the core of the solution back to stable eventually when we're collectively confident on it.
> Did you check that the UUID of these (and other possible) devices stay > the same before and after the patch? Yeah I did across a few hotplug cycles and suspend cycles. > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v1->v2: >> * Update commit message to indicate which CMs are tested >> * Adjust flow to only fetch DROM from NVM on TBT3 and bit bang on TBT1/2 >> --- >> drivers/thunderbolt/eeprom.c | 145 +++++++++++++++++++---------------- >> 1 file changed, 80 insertions(+), 65 deletions(-) >> >> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c >> index c90d22f56d4e..d1be72b6afdb 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) { >> @@ -544,7 +544,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; >> + >> + res = tb_drom_read_n(sw, 14, (u8 *) size, 2); >> + if (res) >> + return res; >> + *size &= 0x3ff; >> + *size += TB_DROM_DATA_START; >> + 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"); >> + 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; >> +err: >> + kfree(sw->drom); >> + sw->drom = NULL; >> + return res; >> +} > Can you split the refactoring part into a separate patch?
Sure.
>> + >> +static int tb_drom_parse_v1(struct tb_switch *sw) >> { >> const struct tb_drom_header *header = >> (const struct tb_drom_header *)sw->drom; >> @@ -555,7 +585,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; >> @@ -589,6 +619,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; >> + >> + if (header->data_len + TB_DROM_DATA_START != *size) { >> + tb_sw_warn(sw, "drom size mismatch\n"); >> + 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; >> +err: >> + kfree(sw->drom); >> + sw->drom = NULL; >> + return -EIO; >> +} >> + >> /** >> * tb_drom_read() - Copy DROM to sw->drom and parse it >> * @sw: Router whose DROM to read and parse >> @@ -602,8 +669,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; >> @@ -614,11 +680,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 >> @@ -627,7 +693,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,64 +706,13 @@ int tb_drom_read(struct tb_switch *sw) >> return 0; >> } >> >> - res = tb_drom_read_n(sw, 14, (u8 *) &size, 2); >> + /* TBT3 devices have the DROM as part of NVM */ >> + if (sw->generation < 3) > This is true for TBT2 devices too. I think you want to check for the > sw->cap_lc here instead. If it is set the device has LC and therefore we > can use the LC UUID registers to figure out the UUID in later stages. > Otherwise we need to read it through bitbanging. OK thanks, will adjust. >> + res = tb_drom_bit_bang(sw, &size); >> + else >> + res = tb_drom_copy_nvm(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
| |