[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.