TBT3 devices when connected to an AMD USB4 router occasionally fail to
properly respond to requests for the DROM via bit banging.
Depending upon which part of the request failed will impact the severity.
A number of workarounds have been put in place to let the driver handle
the failed requests:
commit e87491a9fd4e3 ("thunderbolt: Retry DROM reads for more failure scenarios")
commit a283de3ec646f ("thunderbolt: Do not resume routers if UID is not set")
commit 6915812bbd109 ("thunderbolt: Do not make DROM read success compulsory")
commit f022ff7bf377 ("thunderbolt: Retry DROM read once if parsing fails")
Still even with these changes the failures do make it through. In comparing
other CM implementations utilized on AMD systems, they all access the
DROM directly from the NVM.
To avoid triggering this issue, try to get the DROM directly from the NVM
in Linux as well when devices have an LC.
v4:
* Style fixups
* Fixup for wrong path for USB4 devices
Mario Limonciello (3):
thunderbolt: Adjust how NVM reading works
thunderbolt: use `tb_eeprom_get_drom_offset` to discover DROM offset
thunderbolt: Refactor DROM reading
drivers/thunderbolt/eeprom.c | 219 +++++++++++++++++++----------------
1 file changed, 122 insertions(+), 97 deletions(-)
--
2.34.1
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.
Refactor the code to remove the gotos and drop the retry logic.
Signed-off-by: Mario Limonciello <[email protected]>
---
v3->v4:
* style fixups
* rebase on earlier patch
* split out root switch case
---
drivers/thunderbolt/eeprom.c | 219 +++++++++++++++++++----------------
1 file changed, 120 insertions(+), 99 deletions(-)
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 873fa4300507..1f7f2d8c453d 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) {
@@ -512,7 +512,7 @@ static int tb_drom_copy_nvm(struct tb_switch *sw, u16 *size)
return ret;
}
-static int usb4_copy_host_drom(struct tb_switch *sw, u16 *size)
+static int usb4_copy_drom(struct tb_switch *sw, u16 *size)
{
int ret;
@@ -535,15 +535,40 @@ static int usb4_copy_host_drom(struct tb_switch *sw, u16 *size)
return ret;
}
-static int tb_drom_read_n(struct tb_switch *sw, u16 offset, u8 *val,
- size_t count)
+static int tb_drom_bit_bang(struct tb_switch *sw, u16 *size)
{
- if (tb_switch_is_usb4(sw))
- return usb4_switch_drom_read(sw, offset, val, count);
- return tb_eeprom_read_n(sw, offset, val, count);
+ int ret;
+
+ ret = tb_eeprom_read_n(sw, 14, (u8 *) size, 2);
+ if (ret)
+ return ret;
+
+ *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;
+
+ ret = tb_eeprom_read_n(sw, 0, sw->drom, *size);
+ if (ret)
+ goto err;
+
+ return 0;
+
+err:
+ kfree(sw->drom);
+ sw->drom = NULL;
+ return ret;
}
-static int tb_drom_parse(struct tb_switch *sw)
+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 +579,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,90 +613,14 @@ static int usb4_drom_parse(struct tb_switch *sw)
return tb_drom_parse_entries(sw, USB4_DROM_HEADER_SIZE);
}
-/**
- * tb_drom_read() - Copy DROM to sw->drom and parse it
- * @sw: Router whose DROM to read and parse
- *
- * This function reads router DROM and if successful parses the entries and
- * populates the fields in @sw accordingly. Can be called for any router
- * generation.
- *
- * Returns %0 in case of success and negative errno otherwise.
- */
-int tb_drom_read(struct tb_switch *sw)
+static int tb_drom_parse(struct tb_switch *sw, u16 *size)
{
- u16 size;
- struct tb_drom_header *header;
- int res, retries = 1;
-
- if (sw->drom)
- return 0;
-
- if (tb_route(sw) == 0) {
- /*
- * Apple's NHI EFI driver supplies a DROM for the root switch
- * in a device property. Use it if available.
- */
- if (tb_drom_copy_efi(sw, &size) == 0)
- goto parse;
-
- /* Non-Apple hardware has the DROM as part of NVM */
- if (tb_drom_copy_nvm(sw, &size) == 0)
- goto parse;
-
- /*
- * USB4 hosts may support reading DROM through router
- * operations.
- */
- if (tb_switch_is_usb4(sw)) {
- usb4_switch_read_uid(sw, &sw->uid);
- if (!usb4_copy_host_drom(sw, &size))
- goto parse;
- } else {
- /*
- * The root switch contains only a dummy drom
- * (header only, no entries). Hardcode the
- * configuration here.
- */
- tb_drom_read_uid_only(sw, &sw->uid);
- }
-
- return 0;
- }
-
- /* We can use LC to get UUID later */
- if (sw->cap_lc && !tb_switch_is_usb4(sw) &&
- tb_drom_copy_nvm(sw, &size) == 0)
- goto parse;
-
- 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(*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;
+ struct tb_drom_header *header = (void *) sw->drom;
+ int ret;
- if (header->data_len + TB_DROM_DATA_START != size) {
+ if (header->data_len + TB_DROM_DATA_START != *size) {
tb_sw_warn(sw, "drom size mismatch\n");
- if (retries--) {
- msleep(100);
- goto read;
- }
+ ret = -EIO;
goto err;
}
@@ -679,29 +628,101 @@ int tb_drom_read(struct tb_switch *sw)
switch (header->device_rom_revision) {
case 3:
- res = usb4_drom_parse(sw);
+ ret = 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);
+ ret = tb_drom_parse_v1(sw);
break;
}
- /* If the DROM parsing fails, wait a moment and retry once */
- if (res == -EILSEQ && retries--) {
+ if (ret) {
tb_sw_warn(sw, "parsing DROM failed\n");
- msleep(100);
- goto read;
+ goto err;
}
- if (!res)
- return 0;
+ return 0;
err:
kfree(sw->drom);
sw->drom = NULL;
- return -EIO;
+
+ return ret;
+}
+
+/**
+ * tb_drom_handle_root() - Handle reading DROM from a root switch
+ * @sw: Router whose DROM to read and parse
+ *
+ * First determine if the switch is USB4. If so, then use USB4
+ * router operations.
+ *
+ * If the switch is not USB4, first try to read from EFI, as Apple's
+ * NHI supplies a DROM for the root switch in a device property.
+ *
+ * If this doesn't work, then try to get it from the NVM directly
+ * without using any bit banging operations.
+ *
+ * If none of those pass, then read just the UID as the root
+ * switch contains a dummy DROM with a header but no entries.
+ *
+ * Returns %0 in case of success and negative errno otherwise.
+ */
+static int tb_drom_handle_root(struct tb_switch *sw)
+{
+ u16 size;
+
+ if (tb_switch_is_usb4(sw)) {
+ usb4_switch_read_uid(sw, &sw->uid);
+ if (usb4_copy_drom(sw, &size) == 0)
+ return tb_drom_parse(sw, &size);
+ } else {
+ if (tb_drom_copy_efi(sw, &size) == 0)
+ return tb_drom_parse(sw, &size);
+
+ if (tb_drom_copy_nvm(sw, &size) == 0)
+ return tb_drom_parse(sw, &size);
+
+ tb_drom_read_uid_only(sw, &sw->uid);
+ }
+
+ return 0;
+}
+
+/**
+ * tb_drom_read() - Copy DROM to sw->drom and parse it
+ * @sw: Router whose DROM to read and parse
+ *
+ * This function reads router DROM and if successful parses the entries and
+ * populates the fields in @sw accordingly. Can be called for any router
+ * generation.
+ *
+ * Returns %0 in case of success and negative errno otherwise.
+ */
+int tb_drom_read(struct tb_switch *sw)
+{
+ u16 size;
+ int ret;
+
+ if (sw->drom)
+ return 0;
+
+ if (tb_route(sw) == 0)
+ return tb_drom_handle_root(sw);
+
+ if (tb_switch_is_usb4(sw)) {
+ usb4_switch_read_uid(sw, &sw->uid);
+ ret = usb4_copy_drom(sw, &size);
+ } else if (sw->cap_lc) {
+ /* We can use LC to get UUID later */
+ ret = tb_drom_copy_nvm(sw, &size);
+ } else
+ ret = tb_drom_bit_bang(sw, &size);
+ if (ret)
+ return ret;
+
+ return tb_drom_parse(sw, &size);
}
--
2.34.1
The static function `tb_eeprom_get_drom_offset` has more safety guards
for the DROM offset fetching. Use this instead of just `tb_sw_read`
No intended functional changes.
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/thunderbolt/eeprom.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 8c9e553e2fca..873fa4300507 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -471,14 +471,13 @@ static int tb_drom_copy_efi(struct tb_switch *sw, u16 *size)
static int tb_drom_copy_nvm(struct tb_switch *sw, u16 *size)
{
- u32 drom_offset;
+ u16 drom_offset;
int ret;
if (!sw->dma_port)
return -ENODEV;
- ret = tb_sw_read(sw, &drom_offset, TB_CFG_SWITCH,
- sw->cap_plug_events + 12, 1);
+ ret = tb_eeprom_get_drom_offset(sw, &drom_offset);
if (ret)
return ret;
--
2.34.1
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 has an LC that can be used to fetch the UUID later. The bit banging
method will only be used if this has failed or no LC is present.
Signed-off-by: Mario Limonciello <[email protected]>
---
v3->v4:
* Don't run code for USB4 devices
v2->v3:
* Split out refactor
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 | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index c90d22f56d4e..8c9e553e2fca 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -640,6 +640,11 @@ int tb_drom_read(struct tb_switch *sw)
return 0;
}
+ /* We can use LC to get UUID later */
+ if (sw->cap_lc && !tb_switch_is_usb4(sw) &&
+ tb_drom_copy_nvm(sw, &size) == 0)
+ goto parse;
+
res = tb_drom_read_n(sw, 14, (u8 *) &size, 2);
if (res)
return res;
--
2.34.1
Hi Mario,
On Thu, Feb 23, 2023 at 03:07:40PM -0600, Mario Limonciello wrote:
> TBT3 devices when connected to an AMD USB4 router occasionally fail to
> properly respond to requests for the DROM via bit banging.
>
> Depending upon which part of the request failed will impact the severity.
> A number of workarounds have been put in place to let the driver handle
> the failed requests:
>
> commit e87491a9fd4e3 ("thunderbolt: Retry DROM reads for more failure scenarios")
> commit a283de3ec646f ("thunderbolt: Do not resume routers if UID is not set")
> commit 6915812bbd109 ("thunderbolt: Do not make DROM read success compulsory")
> commit f022ff7bf377 ("thunderbolt: Retry DROM read once if parsing fails")
>
> Still even with these changes the failures do make it through. In comparing
> other CM implementations utilized on AMD systems, they all access the
> DROM directly from the NVM.
>
> To avoid triggering this issue, try to get the DROM directly from the NVM
> in Linux as well when devices have an LC.
>
> v4:
> * Style fixups
> * Fixup for wrong path for USB4 devices
>
> Mario Limonciello (3):
> thunderbolt: Adjust how NVM reading works
> thunderbolt: use `tb_eeprom_get_drom_offset` to discover DROM offset
> thunderbolt: Refactor DROM reading
I split the device side into a separate function too, renamed root
switch to host router (as that's the correct USB4 term), and fixed a
couple style issues and applied to thunderbolt.git/next, thanks!
Please check that I did not mess up anything :)
[Public]
> -----Original Message-----
> From: Mika Westerberg <[email protected]>
> Sent: Monday, March 6, 2023 03:58
> To: Limonciello, Mario <[email protected]>
> Cc: [email protected]; Mehta, Sanju <[email protected]>;
> [email protected]
> Subject: Re: [PATCH v4 0/3] Fix problems fetching TBT3 DROM from AMD
> USB4 routers
>
> Hi Mario,
>
> On Thu, Feb 23, 2023 at 03:07:40PM -0600, Mario Limonciello wrote:
> > TBT3 devices when connected to an AMD USB4 router occasionally fail to
> > properly respond to requests for the DROM via bit banging.
> >
> > Depending upon which part of the request failed will impact the severity.
> > A number of workarounds have been put in place to let the driver handle
> > the failed requests:
> >
> > commit e87491a9fd4e3 ("thunderbolt: Retry DROM reads for more failure
> scenarios")
> > commit a283de3ec646f ("thunderbolt: Do not resume routers if UID is not
> set")
> > commit 6915812bbd109 ("thunderbolt: Do not make DROM read success
> compulsory")
> > commit f022ff7bf377 ("thunderbolt: Retry DROM read once if parsing fails")
> >
> > Still even with these changes the failures do make it through. In comparing
> > other CM implementations utilized on AMD systems, they all access the
> > DROM directly from the NVM.
> >
> > To avoid triggering this issue, try to get the DROM directly from the NVM
> > in Linux as well when devices have an LC.
> >
> > v4:
> > * Style fixups
> > * Fixup for wrong path for USB4 devices
> >
> > Mario Limonciello (3):
> > thunderbolt: Adjust how NVM reading works
> > thunderbolt: use `tb_eeprom_get_drom_offset` to discover DROM offset
> > thunderbolt: Refactor DROM reading
>
> I split the device side into a separate function too, renamed root
> switch to host router (as that's the correct USB4 term), and fixed a
> couple style issues and applied to thunderbolt.git/next, thanks!
>
> Please check that I did not mess up anything :)
They look good, thanks!
Would you consider to take 8d7f459107f74fbbdde3dd5b3874d2e748cb8a21 to
the RC though, or would prefer to let it bake in next?
Hi,
On Mon, Mar 06, 2023 at 03:14:07PM +0000, Limonciello, Mario wrote:
> [Public]
>
>
>
> > -----Original Message-----
> > From: Mika Westerberg <[email protected]>
> > Sent: Monday, March 6, 2023 03:58
> > To: Limonciello, Mario <[email protected]>
> > Cc: [email protected]; Mehta, Sanju <[email protected]>;
> > [email protected]
> > Subject: Re: [PATCH v4 0/3] Fix problems fetching TBT3 DROM from AMD
> > USB4 routers
> >
> > Hi Mario,
> >
> > On Thu, Feb 23, 2023 at 03:07:40PM -0600, Mario Limonciello wrote:
> > > TBT3 devices when connected to an AMD USB4 router occasionally fail to
> > > properly respond to requests for the DROM via bit banging.
> > >
> > > Depending upon which part of the request failed will impact the severity.
> > > A number of workarounds have been put in place to let the driver handle
> > > the failed requests:
> > >
> > > commit e87491a9fd4e3 ("thunderbolt: Retry DROM reads for more failure
> > scenarios")
> > > commit a283de3ec646f ("thunderbolt: Do not resume routers if UID is not
> > set")
> > > commit 6915812bbd109 ("thunderbolt: Do not make DROM read success
> > compulsory")
> > > commit f022ff7bf377 ("thunderbolt: Retry DROM read once if parsing fails")
> > >
> > > Still even with these changes the failures do make it through. In comparing
> > > other CM implementations utilized on AMD systems, they all access the
> > > DROM directly from the NVM.
> > >
> > > To avoid triggering this issue, try to get the DROM directly from the NVM
> > > in Linux as well when devices have an LC.
> > >
> > > v4:
> > > * Style fixups
> > > * Fixup for wrong path for USB4 devices
> > >
> > > Mario Limonciello (3):
> > > thunderbolt: Adjust how NVM reading works
> > > thunderbolt: use `tb_eeprom_get_drom_offset` to discover DROM offset
> > > thunderbolt: Refactor DROM reading
> >
> > I split the device side into a separate function too, renamed root
> > switch to host router (as that's the correct USB4 term), and fixed a
> > couple style issues and applied to thunderbolt.git/next, thanks!
> >
> > Please check that I did not mess up anything :)
>
> They look good, thanks!
Thanks for checking!
> Would you consider to take 8d7f459107f74fbbdde3dd5b3874d2e748cb8a21 to
> the RC though, or would prefer to let it bake in next?
I would like to keep it too in next just to make sure nothing breaks
accidentally.