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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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