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@xxxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxx>
> >> ---
> >>  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.