[PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The GEN4 TxFFE register is not part of the USB4 v1.0 specification, so
understandably some pre-USB4v2 retimers (like the Parade PS8830) don't
seem to implement it.

The immediate idea to counter this would be to introduce a version
check for that specific register, but on a second thought, the current
flow only returns a quiet -EIO if there's _any_ failures, without
hinting at what the actual problem is.

To take care of both of these issues, simply print an error line for
each SB register read that fails and go on with attempting to read the
others.

Note that this is not quite in-spec behavior ("The SB Register Space
registers shall have the structure and fields described in Table 4-17.
Registers not listed in Table 4-20 are undefined and shall not be
used."), but it's the easiest fix that shouldn't (TM) have real-world
bad side effects.

Fixes: 6d241fa00159 ("thunderbolt: Add sideband register access to debugfs")
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/thunderbolt/debugfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
index 042f6a0d0f7f..8237e1ea6d09 100644
--- a/drivers/thunderbolt/debugfs.c
+++ b/drivers/thunderbolt/debugfs.c
@@ -2361,8 +2361,10 @@ static int sb_regs_show(struct tb_port *port, const struct sb_reg *sb_regs,
 		memset(data, 0, sizeof(data));
 		ret = usb4_port_sb_read(port, target, index, regs->reg, data,
 					regs->size);
-		if (ret)
-			return ret;
+		if (ret) {
+			seq_printf(s, "0x%02x Error reading register: %d\n", regs->reg, ret);
+			continue;
+		}
 
 		seq_printf(s, "0x%02x", regs->reg);
 		for (j = 0; j < regs->size; j++)

---
base-commit: db7efce4ae23ad5e42f5f55428f529ff62b86fab
change-id: 20260409-topic-tbt_sb_debugfs-2e500fee9706

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Mika Westerberg]

Hi,

On Thu, Apr 09, 2026 at 01:22:01PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The GEN4 TxFFE register is not part of the USB4 v1.0 specification, so
> understandably some pre-USB4v2 retimers (like the Parade PS8830) don't
> seem to implement it.
>
> The immediate idea to counter this would be to introduce a version
> check for that specific register, but on a second thought, the current
> flow only returns a quiet -EIO if there's _any_ failures, without
> hinting at what the actual problem is.

Please don't use _any_ emphasis in the commit messages here or in the
future.

> To take care of both of these issues, simply print an error line for
> each SB register read that fails and go on with attempting to read the
> others.
> 
> Note that this is not quite in-spec behavior ("The SB Register Space
> registers shall have the structure and fields described in Table 4-17.
> Registers not listed in Table 4-20 are undefined and shall not be
> used."), but it's the easiest fix that shouldn't (TM) have real-world
> bad side effects.

Also drop the "(TM)" thing.

I assume you have tested this on a hardware that supports this too, right?

> Fixes: 6d241fa00159 ("thunderbolt: Add sideband register access to debugfs")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  drivers/thunderbolt/debugfs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
> index 042f6a0d0f7f..8237e1ea6d09 100644
> --- a/drivers/thunderbolt/debugfs.c
> +++ b/drivers/thunderbolt/debugfs.c
> @@ -2361,8 +2361,10 @@ static int sb_regs_show(struct tb_port *port, const struct sb_reg *sb_regs,
>  		memset(data, 0, sizeof(data));
>  		ret = usb4_port_sb_read(port, target, index, regs->reg, data,
>  					regs->size);
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			seq_printf(s, "0x%02x Error reading register: %d\n", regs->reg, ret);

Why not tb_port_dgb/warn()() here instead so it goes into dmesg, not to the
output.

> +			continue;
> +		}
>  
>  		seq_printf(s, "0x%02x", regs->reg);
>  		for (j = 0; j < regs->size; j++)
> 
> ---
> base-commit: db7efce4ae23ad5e42f5f55428f529ff62b86fab
> change-id: 20260409-topic-tbt_sb_debugfs-2e500fee9706
> 
> Best regards,
> -- 
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Konrad Dybcio]

On 4/9/26 2:04 PM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Apr 09, 2026 at 01:22:01PM +0200, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> The GEN4 TxFFE register is not part of the USB4 v1.0 specification, so
>> understandably some pre-USB4v2 retimers (like the Parade PS8830) don't
>> seem to implement it.
>>
>> The immediate idea to counter this would be to introduce a version
>> check for that specific register, but on a second thought, the current
>> flow only returns a quiet -EIO if there's _any_ failures, without
>> hinting at what the actual problem is.
> 
> Please don't use _any_ emphasis in the commit messages here or in the
> future.

If I must, I shall.. other maintainers don't mind.

>> To take care of both of these issues, simply print an error line for
>> each SB register read that fails and go on with attempting to read the
>> others.
>>
>> Note that this is not quite in-spec behavior ("The SB Register Space
>> registers shall have the structure and fields described in Table 4-17.
>> Registers not listed in Table 4-20 are undefined and shall not be
>> used."), but it's the easiest fix that shouldn't (TM) have real-world
>> bad side effects.
> 
> Also drop the "(TM)" thing.
>
> I assume you have tested this on a hardware that supports this too, right?

Hardware that exposes that register this does not exercise the altered
code path.

>> Fixes: 6d241fa00159 ("thunderbolt: Add sideband register access to debugfs")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> ---
>>  drivers/thunderbolt/debugfs.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
>> index 042f6a0d0f7f..8237e1ea6d09 100644
>> --- a/drivers/thunderbolt/debugfs.c
>> +++ b/drivers/thunderbolt/debugfs.c
>> @@ -2361,8 +2361,10 @@ static int sb_regs_show(struct tb_port *port, const struct sb_reg *sb_regs,
>>  		memset(data, 0, sizeof(data));
>>  		ret = usb4_port_sb_read(port, target, index, regs->reg, data,
>>  					regs->size);
>> -		if (ret)
>> -			return ret;
>> +		if (ret) {
>> +			seq_printf(s, "0x%02x Error reading register: %d\n", regs->reg, ret);
> 
> Why not tb_port_dgb/warn()() here instead so it goes into dmesg, not to the
> output.

Because when one reads out sys/debugfs, it's generally expected that the
related output is provided there.

If we don't want to print the retval, I can copy the message that's printed
when switch/port capabilities readout fails, i.e.

-- drivers/thunderbolt/debugfs.c : cap_show_by_dw()
if (port)
	ret = tb_port_read(port, &data, TB_CFG_PORT, cap + offset + i, 1);
else
	ret = tb_sw_read(sw, &data, TB_CFG_SWITCH, cap + offset + i, 1);
if (ret) {
	seq_printf(s, "0x%04x <not accessible>\n", cap + offset + i);
	continue;
}

Konrad

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Mika Westerberg]

On Thu, Apr 09, 2026 at 02:59:22PM +0200, Konrad Dybcio wrote:
> On 4/9/26 2:04 PM, Mika Westerberg wrote:
> > Hi,
> > 
> > On Thu, Apr 09, 2026 at 01:22:01PM +0200, Konrad Dybcio wrote:
> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>
> >> The GEN4 TxFFE register is not part of the USB4 v1.0 specification, so
> >> understandably some pre-USB4v2 retimers (like the Parade PS8830) don't
> >> seem to implement it.
> >>
> >> The immediate idea to counter this would be to introduce a version
> >> check for that specific register, but on a second thought, the current
> >> flow only returns a quiet -EIO if there's _any_ failures, without
> >> hinting at what the actual problem is.
> > 
> > Please don't use _any_ emphasis in the commit messages here or in the
> > future.
> 
> If I must, I shall.. other maintainers don't mind.

I do. We are professionals, let's keep the commit logs as such, not rants.

> >> To take care of both of these issues, simply print an error line for
> >> each SB register read that fails and go on with attempting to read the
> >> others.
> >>
> >> Note that this is not quite in-spec behavior ("The SB Register Space
> >> registers shall have the structure and fields described in Table 4-17.
> >> Registers not listed in Table 4-20 are undefined and shall not be
> >> used."), but it's the easiest fix that shouldn't (TM) have real-world
> >> bad side effects.
> > 
> > Also drop the "(TM)" thing.
> >
> > I assume you have tested this on a hardware that supports this too, right?
> 
> Hardware that exposes that register this does not exercise the altered
> code path.

Well it may happen now that previously we got -EIO from some other register
and we stopped there, now this changes and we actually continue reading so
this definitely should be tested.

> >> Fixes: 6d241fa00159 ("thunderbolt: Add sideband register access to debugfs")
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >> ---
> >>  drivers/thunderbolt/debugfs.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
> >> index 042f6a0d0f7f..8237e1ea6d09 100644
> >> --- a/drivers/thunderbolt/debugfs.c
> >> +++ b/drivers/thunderbolt/debugfs.c
> >> @@ -2361,8 +2361,10 @@ static int sb_regs_show(struct tb_port *port, const struct sb_reg *sb_regs,
> >>  		memset(data, 0, sizeof(data));
> >>  		ret = usb4_port_sb_read(port, target, index, regs->reg, data,
> >>  					regs->size);
> >> -		if (ret)
> >> -			return ret;
> >> +		if (ret) {
> >> +			seq_printf(s, "0x%02x Error reading register: %d\n", regs->reg, ret);
> > 
> > Why not tb_port_dgb/warn()() here instead so it goes into dmesg, not to the
> > output.
> 
> Because when one reads out sys/debugfs, it's generally expected that the
> related output is provided there.
> 
> If we don't want to print the retval, I can copy the message that's printed
> when switch/port capabilities readout fails, i.e.
> 
> -- drivers/thunderbolt/debugfs.c : cap_show_by_dw()
> if (port)
> 	ret = tb_port_read(port, &data, TB_CFG_PORT, cap + offset + i, 1);
> else
> 	ret = tb_sw_read(sw, &data, TB_CFG_SWITCH, cap + offset + i, 1);
> if (ret) {
> 	seq_printf(s, "0x%04x <not accessible>\n", cap + offset + i);
> 	continue;

Yes this is better.

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Konrad Dybcio]

On 4/9/26 4:32 PM, Mika Westerberg wrote:
> On Thu, Apr 09, 2026 at 02:59:22PM +0200, Konrad Dybcio wrote:
>> On 4/9/26 2:04 PM, Mika Westerberg wrote:

[...]

>>> I assume you have tested this on a hardware that supports this too, right?
>>
>> Hardware that exposes that register this does not exercise the altered
>> code path.
> 
> Well it may happen now that previously we got -EIO from some other register
> and we stopped there, now this changes and we actually continue reading so
> this definitely should be tested.

The only register before USB4_SB_GEN4_TXFFE that isn't in-spec for
both retimers in v1.0 and v2.0 is USB4_SB_LRD_TUNING (0x07). The PS8830
interestingly reports all zeroes (not a bounce).

The registers following USB4_SB_GEN4_TXFFE in the array are
USB4_SB_VERSION and USB4_SB_DATA. The former is not accessed anywhere
else in the code, at first glance. The latter is, during NVM r/w and
in margining ops, which have definitely been in use for a long time.

Plus both of them are the v1.0 spec. The USB4_SB_GEN4_TXFFE specifically
isn't (the retimer supplement pdf lists it as Rsvd, the main spec pdf
omits it in the SB register table), as it wasn't previously useful (since
Gen4 came about in v2.0).


I don't think there's an easy way to limit the reading of this register
since the bit indicating Gen4 capability is in USB4_SB_LINK_CONF (0x0c),
which is Rsvd on retimers regardless of the spec revision. A connected
port could easily have higher/lower capabilities, too.


So all in all, my understanding is that any bugs caused by this patch
(which would have to be in the form of "reading a register causes a
wrongful change in behavior") would really surface spec non-compliance
from a retimer, which should be quirked out explicitly if that's the
case.


I only have hardware with various Parade retimers, none of which claim
Gen4 support.

Konrad

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Konrad Dybcio]

On 4/10/26 4:29 PM, Konrad Dybcio wrote:
> On 4/9/26 4:32 PM, Mika Westerberg wrote:
>> On Thu, Apr 09, 2026 at 02:59:22PM +0200, Konrad Dybcio wrote:
>>> On 4/9/26 2:04 PM, Mika Westerberg wrote:
> 
> [...]
> 
>>>> I assume you have tested this on a hardware that supports this too, right?
>>>
>>> Hardware that exposes that register this does not exercise the altered
>>> code path.
>>
>> Well it may happen now that previously we got -EIO from some other register
>> and we stopped there, now this changes and we actually continue reading so
>> this definitely should be tested.
> 
> The only register before USB4_SB_GEN4_TXFFE that isn't in-spec for
> both retimers in v1.0 and v2.0 is USB4_SB_LRD_TUNING (0x07). The PS8830
> interestingly reports all zeroes (not a bounce).
> 
> The registers following USB4_SB_GEN4_TXFFE in the array are
> USB4_SB_VERSION and USB4_SB_DATA. The former is not accessed anywhere
> else in the code, at first glance. The latter is, during NVM r/w and
> in margining ops, which have definitely been in use for a long time.
> 
> Plus both of them are the v1.0 spec. The USB4_SB_GEN4_TXFFE specifically
> isn't (the retimer supplement pdf lists it as Rsvd, the main spec pdf
> omits it in the SB register table), as it wasn't previously useful (since
> Gen4 came about in v2.0).
> 
> 
> I don't think there's an easy way to limit the reading of this register
> since the bit indicating Gen4 capability is in USB4_SB_LINK_CONF (0x0c),
> which is Rsvd on retimers regardless of the spec revision. A connected
> port could easily have higher/lower capabilities, too.

Checked again, the USB4_SB_FW_VERSION (0x02) register's lowest 8 bytes
are 0/1 for retimers implementing USB4v1 and 2 for v2, so we may go this
path too

Konrad

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Mika Westerberg]

On Fri, Apr 10, 2026 at 04:43:54PM +0200, Konrad Dybcio wrote:
> On 4/10/26 4:29 PM, Konrad Dybcio wrote:
> > On 4/9/26 4:32 PM, Mika Westerberg wrote:
> >> On Thu, Apr 09, 2026 at 02:59:22PM +0200, Konrad Dybcio wrote:
> >>> On 4/9/26 2:04 PM, Mika Westerberg wrote:
> > 
> > [...]
> > 
> >>>> I assume you have tested this on a hardware that supports this too, right?
> >>>
> >>> Hardware that exposes that register this does not exercise the altered
> >>> code path.
> >>
> >> Well it may happen now that previously we got -EIO from some other register
> >> and we stopped there, now this changes and we actually continue reading so
> >> this definitely should be tested.
> > 
> > The only register before USB4_SB_GEN4_TXFFE that isn't in-spec for
> > both retimers in v1.0 and v2.0 is USB4_SB_LRD_TUNING (0x07). The PS8830
> > interestingly reports all zeroes (not a bounce).
> > 
> > The registers following USB4_SB_GEN4_TXFFE in the array are
> > USB4_SB_VERSION and USB4_SB_DATA. The former is not accessed anywhere
> > else in the code, at first glance. The latter is, during NVM r/w and
> > in margining ops, which have definitely been in use for a long time.
> > 
> > Plus both of them are the v1.0 spec. The USB4_SB_GEN4_TXFFE specifically
> > isn't (the retimer supplement pdf lists it as Rsvd, the main spec pdf
> > omits it in the SB register table), as it wasn't previously useful (since
> > Gen4 came about in v2.0).
> > 
> > 
> > I don't think there's an easy way to limit the reading of this register
> > since the bit indicating Gen4 capability is in USB4_SB_LINK_CONF (0x0c),
> > which is Rsvd on retimers regardless of the spec revision. A connected
> > port could easily have higher/lower capabilities, too.
> 
> Checked again, the USB4_SB_FW_VERSION (0x02) register's lowest 8 bytes
> are 0/1 for retimers implementing USB4v1 and 2 for v2, so we may go this
> path too

I also checked from Retimer 1.0 spec and there it is still "Reserved. May
have non-zero value". Probably not good to rely on that.

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Konrad Dybcio]

On 4/10/26 5:10 PM, Mika Westerberg wrote:
> On Fri, Apr 10, 2026 at 04:43:54PM +0200, Konrad Dybcio wrote:
>> On 4/10/26 4:29 PM, Konrad Dybcio wrote:
>>> On 4/9/26 4:32 PM, Mika Westerberg wrote:
>>>> On Thu, Apr 09, 2026 at 02:59:22PM +0200, Konrad Dybcio wrote:
>>>>> On 4/9/26 2:04 PM, Mika Westerberg wrote:
>>>
>>> [...]
>>>
>>>>>> I assume you have tested this on a hardware that supports this too, right?
>>>>>
>>>>> Hardware that exposes that register this does not exercise the altered
>>>>> code path.
>>>>
>>>> Well it may happen now that previously we got -EIO from some other register
>>>> and we stopped there, now this changes and we actually continue reading so
>>>> this definitely should be tested.
>>>
>>> The only register before USB4_SB_GEN4_TXFFE that isn't in-spec for
>>> both retimers in v1.0 and v2.0 is USB4_SB_LRD_TUNING (0x07). The PS8830
>>> interestingly reports all zeroes (not a bounce).
>>>
>>> The registers following USB4_SB_GEN4_TXFFE in the array are
>>> USB4_SB_VERSION and USB4_SB_DATA. The former is not accessed anywhere
>>> else in the code, at first glance. The latter is, during NVM r/w and
>>> in margining ops, which have definitely been in use for a long time.
>>>
>>> Plus both of them are the v1.0 spec. The USB4_SB_GEN4_TXFFE specifically
>>> isn't (the retimer supplement pdf lists it as Rsvd, the main spec pdf
>>> omits it in the SB register table), as it wasn't previously useful (since
>>> Gen4 came about in v2.0).
>>>
>>>
>>> I don't think there's an easy way to limit the reading of this register
>>> since the bit indicating Gen4 capability is in USB4_SB_LINK_CONF (0x0c),
>>> which is Rsvd on retimers regardless of the spec revision. A connected
>>> port could easily have higher/lower capabilities, too.
>>
>> Checked again, the USB4_SB_FW_VERSION (0x02) register's lowest 8 bytes
>> are 0/1 for retimers implementing USB4v1 and 2 for v2, so we may go this
>> path too
> 
> I also checked from Retimer 1.0 spec and there it is still "Reserved. May
> have non-zero value". Probably not good to rely on that.

In Table 4-3 below that definition, it says:

"""
Shall be set to 00h or 01h

It is recommended that this field be set to 01h.
"""

But we can revisit limiting those reads another day

Konrad

Re: [PATCH] thunderbolt: debugfs: Don't stop reading SB registers if just one fails

[Mika Westerberg]

On Fri, Apr 10, 2026 at 04:29:43PM +0200, Konrad Dybcio wrote:
> On 4/9/26 4:32 PM, Mika Westerberg wrote:
> > On Thu, Apr 09, 2026 at 02:59:22PM +0200, Konrad Dybcio wrote:
> >> On 4/9/26 2:04 PM, Mika Westerberg wrote:
> 
> [...]
> 
> >>> I assume you have tested this on a hardware that supports this too, right?
> >>
> >> Hardware that exposes that register this does not exercise the altered
> >> code path.
> > 
> > Well it may happen now that previously we got -EIO from some other register
> > and we stopped there, now this changes and we actually continue reading so
> > this definitely should be tested.
> 
> The only register before USB4_SB_GEN4_TXFFE that isn't in-spec for
> both retimers in v1.0 and v2.0 is USB4_SB_LRD_TUNING (0x07). The PS8830
> interestingly reports all zeroes (not a bounce).
> 
> The registers following USB4_SB_GEN4_TXFFE in the array are
> USB4_SB_VERSION and USB4_SB_DATA. The former is not accessed anywhere
> else in the code, at first glance. The latter is, during NVM r/w and
> in margining ops, which have definitely been in use for a long time.
> 
> Plus both of them are the v1.0 spec. The USB4_SB_GEN4_TXFFE specifically
> isn't (the retimer supplement pdf lists it as Rsvd, the main spec pdf
> omits it in the SB register table), as it wasn't previously useful (since
> Gen4 came about in v2.0).
> 
> 
> I don't think there's an easy way to limit the reading of this register
> since the bit indicating Gen4 capability is in USB4_SB_LINK_CONF (0x0c),
> which is Rsvd on retimers regardless of the spec revision. A connected
> port could easily have higher/lower capabilities, too.

Agree.

> So all in all, my understanding is that any bugs caused by this patch
> (which would have to be in the form of "reading a register causes a
> wrongful change in behavior") would really surface spec non-compliance
> from a retimer, which should be quirked out explicitly if that's the
> case.

Right. Also given the fact that this is debugfs access so not anything we
would do during normal operations, likelihood of this causing issues for
regular user should be pretty low.

> I only have hardware with various Parade retimers, none of which claim
> Gen4 support.

Okay I can give the next version a try on my test systems to make sure
there are no surprises. I should have a pretty extensive variation of
retimers (v1, v2 and pre-USB4).