[PATCH] thunderbolt: Clamp DMA tunnel credits to what a hop register can hold

[PATCH] thunderbolt: Clamp DMA tunnel credits to what a hop register can hold

[Fan Ye via B4 Relay]

From: Fan Ye <fy15309206903@gmail.com>

struct tb_regs_hop::initial_credits is 7 bits wide, so a hop can be
programmed with 127 credits at most. tb_tunnel_alloc_dma() picks the
credit count for a DMA tunnel out of two values that are not bounded by
that: the dma_credits module parameter, which has no upper limit at all,
and the host router's baMaxHI, which usb4_switch_credits_init() reads
out of a 16-bit field and only checks for presence, never for magnitude.

What comes out is handed to tb_dma_init_rx_path() and
tb_dma_init_tx_path(), which lower it to what the adapter has room for
and store the result in struct tb_path_hop::initial_credits. That field
is an unsigned int, so an oversized number survives until
tb_path_activate() copies it into the 7-bit register field and keeps
only the low bits. The path then runs on a credit count that neither
the driver nor the user chose, while port->dma_credits goes on
accounting for the number that was asked for. Nothing reports the
difference.

The adapter has to have that many buffers for the truncation to be
reachable, and ADP_CS_4_TOTAL_BUFFERS_MASK leaves room for it: the lane
adapters of an ASMedia ASM4242 host router report 174. Asking such a
router for 172 credits leaves 172 & 0x7f == 44 in the hop.

Clamp the count where it is chosen. tb_tunnel_alloc_dma() is the only
entry point for DMA tunnels, the clamp sits ahead of every hop
assignment and every credit accounting update, and each step below it
can only lower the value further, so one bound covers the whole tunnel
and the driver's bookkeeping stays in step with the hardware. Before the
module parameter existed this line read min_not_zero(TB_DMA_CREDITS,
nhi->sw->max_dma_credits) and could not yield more than 14.

While at it, carry the count in an unsigned int. Both min_not_zero()
operands and every function it is passed to are unsigned already, so the
int only added a signed detour in the middle of an otherwise unsigned
path.

Fixes: 7ee20d0afb69 ("thunderbolt: Allow specifying custom credits for DMA tunnels")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Fan Ye <fy15309206903@gmail.com>
---
The truncation is not reachable with the defaults. dma_credits is 14 and
the ASM4242 in front of me reports baMaxHI 32, so 127 is never
approached; I had to drop the baMaxHI cap with a local debug patch and
ask for 172 to see it. Reading the hop straight back after
tb_path_activate() had written it then showed 44 in the register. With
this patch it shows 127.

That is not an argument for 127 being a good number to run on. On this
hardware anything above what the router asks for costs packets - the
receiving adapter starts dropping around 35 credits, and further up the
control channel stops answering altogether - so the clamp is a statement
about what the field can hold, not a recommendation.

The same 7-bit field is assigned from three other places: USB3
(sw->max_usb3_credits, with no min() at all), DP AUX
(sw->min_dp_aux_credits) and PCIe (min(sw->max_pcie_credits,
available)). usb4_switch_credits_init() only checks that those buffer
allocation values are present, never how large they are, so nothing in
the driver stops them from passing 127 either. I left them alone because
I have no evidence any router advertises that much there - the one I can
read out reports 64/2/64 for USB3/DP AUX/PCIe, and the bad advertisement
already handled in tree, quirk_dp_credit_allocation(), is 56. Should
those get the same treatment, or does the spec bound them in a way that
makes it unnecessary?

One thing I deliberately left alone: tb_available_credits() computes
spare = min_not_zero(sw->max_dma_credits, dma_credits) without this
bound, so with dma_credits set above 127 it now reserves more than a DMA
tunnel can take, at the expense of the DP stream count. Clamping there
too is one more line, but it changes how many DP streams fit, which is
more than a truncation fix should do on its own.

No Cc: stable, since reaching this needs either dma_credits set by hand
or a router advertising baMaxHI above 127, and I have not seen the
latter. Happy to add it if you disagree.
---
 drivers/thunderbolt/tunnel.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index b7f32305f14a..b2a8e4950200 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -48,6 +48,15 @@
 #define TB_DP_AUX_PRIORITY		2
 #define TB_DP_AUX_WEIGHT		1
 
+/*
+ * struct tb_regs_hop::initial_credits is 7 bits wide, so this is the most a
+ * hop can be programmed with. What feeds it is not bounded by that:
+ * ADP_CS_4_TOTAL_BUFFERS_MASK is 10 bits and the dma_credits module
+ * parameter has no upper limit at all. A larger value is stored with its
+ * top bits cut off, leaving the path on a credit count nobody asked for.
+ */
+#define TB_MAX_HOP_CREDITS		127
+
 /* Minimum number of credits needed for PCIe path */
 #define TB_MIN_PCIE_CREDITS		6U
 /*
@@ -1908,7 +1917,7 @@ struct tb_tunnel *tb_tunnel_alloc_dma(struct tb *tb, struct tb_port *nhi,
 	struct tb_tunnel *tunnel;
 	size_t npaths = 0, i = 0;
 	struct tb_path *path;
-	int credits;
+	unsigned int credits;
 
 	/* Ring 0 is reserved for control channel */
 	if (WARN_ON(!receive_ring || !transmit_ring))
@@ -1931,6 +1940,11 @@ struct tb_tunnel *tb_tunnel_alloc_dma(struct tb *tb, struct tb_port *nhi,
 	tunnel->destroy = tb_dma_destroy;
 
 	credits = min_not_zero(dma_credits, nhi->sw->max_dma_credits);
+	if (credits > TB_MAX_HOP_CREDITS) {
+		tb_tunnel_dbg(tunnel, "%u credits do not fit a hop, using %u\n",
+			      credits, TB_MAX_HOP_CREDITS);
+		credits = TB_MAX_HOP_CREDITS;
+	}
 
 	if (receive_ring > 0) {
 		path = tb_path_alloc(tb, dst, receive_path, nhi, receive_ring, 0,

---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
change-id: 20260810-tb-dma-credit-clamp-eb3931e5a588

Best regards,
--  
Fan Ye <fy15309206903@gmail.com>

Re: [PATCH] thunderbolt: Clamp DMA tunnel credits to what a hop register can hold

[Mika Westerberg]

Hi,

On Mon, Aug 10, 2026 at 09:38:30AM +0000, Fan Ye via B4 Relay wrote:
> From: Fan Ye <fy15309206903@gmail.com>
> 
> struct tb_regs_hop::initial_credits is 7 bits wide, so a hop can be
> programmed with 127 credits at most. tb_tunnel_alloc_dma() picks the
> credit count for a DMA tunnel out of two values that are not bounded by
> that: the dma_credits module parameter, which has no upper limit at all,
> and the host router's baMaxHI, which usb4_switch_credits_init() reads
> out of a 16-bit field and only checks for presence, never for magnitude.
> 
> What comes out is handed to tb_dma_init_rx_path() and
> tb_dma_init_tx_path(), which lower it to what the adapter has room for
> and store the result in struct tb_path_hop::initial_credits. That field
> is an unsigned int, so an oversized number survives until
> tb_path_activate() copies it into the 7-bit register field and keeps
> only the low bits. The path then runs on a credit count that neither
> the driver nor the user chose, while port->dma_credits goes on
> accounting for the number that was asked for. Nothing reports the
> difference.
> 
> The adapter has to have that many buffers for the truncation to be
> reachable, and ADP_CS_4_TOTAL_BUFFERS_MASK leaves room for it: the lane
> adapters of an ASMedia ASM4242 host router report 174. Asking such a
> router for 172 credits leaves 172 & 0x7f == 44 in the hop.
> 
> Clamp the count where it is chosen. tb_tunnel_alloc_dma() is the only
> entry point for DMA tunnels, the clamp sits ahead of every hop
> assignment and every credit accounting update, and each step below it
> can only lower the value further, so one bound covers the whole tunnel
> and the driver's bookkeeping stays in step with the hardware. Before the
> module parameter existed this line read min_not_zero(TB_DMA_CREDITS,
> nhi->sw->max_dma_credits) and could not yield more than 14.
> 
> While at it, carry the count in an unsigned int. Both min_not_zero()
> operands and every function it is passed to are unsigned already, so the
> int only added a signed detour in the middle of an otherwise unsigned
> path.

This is huge commit message for a simple thing. Can you trim them to be
of reasonable size?

> Fixes: 7ee20d0afb69 ("thunderbolt: Allow specifying custom credits for DMA tunnels")

I don't think it even needs a fixes tag.

> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Fan Ye <fy15309206903@gmail.com>
> ---
> The truncation is not reachable with the defaults. dma_credits is 14 and
> the ASM4242 in front of me reports baMaxHI 32, so 127 is never
> approached; I had to drop the baMaxHI cap with a local debug patch and
> ask for 172 to see it. Reading the hop straight back after
> tb_path_activate() had written it then showed 44 in the register. With
> this patch it shows 127.
> 
> That is not an argument for 127 being a good number to run on. On this
> hardware anything above what the router asks for costs packets - the
> receiving adapter starts dropping around 35 credits, and further up the
> control channel stops answering altogether - so the clamp is a statement
> about what the field can hold, not a recommendation.
> 
> The same 7-bit field is assigned from three other places: USB3
> (sw->max_usb3_credits, with no min() at all), DP AUX
> (sw->min_dp_aux_credits) and PCIe (min(sw->max_pcie_credits,
> available)). usb4_switch_credits_init() only checks that those buffer
> allocation values are present, never how large they are, so nothing in
> the driver stops them from passing 127 either. I left them alone because
> I have no evidence any router advertises that much there - the one I can
> read out reports 64/2/64 for USB3/DP AUX/PCIe, and the bad advertisement
> already handled in tree, quirk_dp_credit_allocation(), is 56. Should
> those get the same treatment, or does the spec bound them in a way that
> makes it unnecessary?
> 
> One thing I deliberately left alone: tb_available_credits() computes
> spare = min_not_zero(sw->max_dma_credits, dma_credits) without this
> bound, so with dma_credits set above 127 it now reserves more than a DMA
> tunnel can take, at the expense of the DP stream count. Clamping there
> too is one more line, but it changes how many DP streams fit, which is
> more than a truncation fix should do on its own.

Same here regarding the amount of text. This is too much at least to for
me.

> No Cc: stable, since reaching this needs either dma_credits set by hand
> or a router advertising baMaxHI above 127, and I have not seen the
> latter. Happy to add it if you disagree.
> ---
>  drivers/thunderbolt/tunnel.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> index b7f32305f14a..b2a8e4950200 100644
> --- a/drivers/thunderbolt/tunnel.c
> +++ b/drivers/thunderbolt/tunnel.c
> @@ -48,6 +48,15 @@
>  #define TB_DP_AUX_PRIORITY		2
>  #define TB_DP_AUX_WEIGHT		1
>  
> +/*
> + * struct tb_regs_hop::initial_credits is 7 bits wide, so this is the most a
> + * hop can be programmed with. What feeds it is not bounded by that:
> + * ADP_CS_4_TOTAL_BUFFERS_MASK is 10 bits and the dma_credits module
> + * parameter has no upper limit at all. A larger value is stored with its
> + * top bits cut off, leaving the path on a credit count nobody asked for.
> + */

Same applies to this comment - I know LLMs have tendency to over-generate
text so it's your responsibility to cut it down to reasonable size.

> +#define TB_MAX_HOP_CREDITS		127

TB_MAX_CREDITS

> +
>  /* Minimum number of credits needed for PCIe path */
>  #define TB_MIN_PCIE_CREDITS		6U
>  /*
> @@ -1908,7 +1917,7 @@ struct tb_tunnel *tb_tunnel_alloc_dma(struct tb *tb, struct tb_port *nhi,
>  	struct tb_tunnel *tunnel;
>  	size_t npaths = 0, i = 0;
>  	struct tb_path *path;
> -	int credits;
> +	unsigned int credits;
>  
>  	/* Ring 0 is reserved for control channel */
>  	if (WARN_ON(!receive_ring || !transmit_ring))
> @@ -1931,6 +1940,11 @@ struct tb_tunnel *tb_tunnel_alloc_dma(struct tb *tb, struct tb_port *nhi,
>  	tunnel->destroy = tb_dma_destroy;
>  
>  	credits = min_not_zero(dma_credits, nhi->sw->max_dma_credits);
> +	if (credits > TB_MAX_HOP_CREDITS) {
> +		tb_tunnel_dbg(tunnel, "%u credits do not fit a hop, using %u\n",
> +			      credits, TB_MAX_HOP_CREDITS);
> +		credits = TB_MAX_HOP_CREDITS;
> +	}
>  
>  	if (receive_ring > 0) {
>  		path = tb_path_alloc(tb, dst, receive_path, nhi, receive_ring, 0,
> 
> ---
> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
> change-id: 20260810-tb-dma-credit-clamp-eb3931e5a588
> 
> Best regards,
> --  
> Fan Ye <fy15309206903@gmail.com>
>