thunderbolt: Confirm the necessity to configure asym link first

[PATCH] thunderbolt: Confirm the necessity to configure asym link first

Posted by zhangjianrong 7 months, 2 weeks ago

Current implementation can cause allocation failures in
tb_alloc_dp_bandwidth() in some cases. For example:
allocated_down(30Gbps), allocated_up(50Gbps),
requested_down(10Gbps).

Signed-off-by: zhangjianrong <zhangjianrong5@huawei.com>
---
 drivers/thunderbolt/tb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index a7c6919fbf97..558455d9716b 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1039,6 +1039,9 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
 			break;
 
 		if (downstream) {
+			/* Does consumed + requested exceed the threshold */
+			if (consumed_down + requested_down < asym_threshold)
+				continue;
 			/*
 			 * Downstream so make sure upstream is within the 36G
 			 * (40G - guard band 10%), and the requested is above
@@ -1048,20 +1051,17 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
 				ret = -ENOBUFS;
 				break;
 			}
-			/* Does consumed + requested exceed the threshold */
-			if (consumed_down + requested_down < asym_threshold)
-				continue;
 
 			width_up = TB_LINK_WIDTH_ASYM_RX;
 			width_down = TB_LINK_WIDTH_ASYM_TX;
 		} else {
 			/* Upstream, the opposite of above */
+			if (consumed_up + requested_up < asym_threshold)
+				continue;
 			if (consumed_down + requested_down >= TB_ASYM_MIN) {
 				ret = -ENOBUFS;
 				break;
 			}
-			if (consumed_up + requested_up < asym_threshold)
-				continue;
 
 			width_up = TB_LINK_WIDTH_ASYM_TX;
 			width_down = TB_LINK_WIDTH_ASYM_RX;
-- 
2.34.1

Re: [PATCH] thunderbolt: Confirm the necessity to configure asym link first

Posted by Mika Westerberg 7 months, 2 weeks ago

Hi,

On Thu, Jun 26, 2025 at 04:41:07PM +0800, zhangjianrong wrote:
> Current implementation can cause allocation failures in
> tb_alloc_dp_bandwidth() in some cases. For example:
> allocated_down(30Gbps), allocated_up(50Gbps),
> requested_down(10Gbps).

I'm not sure I understand the above.

Can you describe in which real life situation this can happen?

> 
> Signed-off-by: zhangjianrong <zhangjianrong5@huawei.com>
> ---
>  drivers/thunderbolt/tb.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index a7c6919fbf97..558455d9716b 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -1039,6 +1039,9 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
>  			break;
>  
>  		if (downstream) {
> +			/* Does consumed + requested exceed the threshold */
> +			if (consumed_down + requested_down < asym_threshold)
> +				continue;
>  			/*
>  			 * Downstream so make sure upstream is within the 36G
>  			 * (40G - guard band 10%), and the requested is above
> @@ -1048,20 +1051,17 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
>  				ret = -ENOBUFS;
>  				break;
>  			}
> -			/* Does consumed + requested exceed the threshold */
> -			if (consumed_down + requested_down < asym_threshold)
> -				continue;
>  
>  			width_up = TB_LINK_WIDTH_ASYM_RX;
>  			width_down = TB_LINK_WIDTH_ASYM_TX;
>  		} else {
>  			/* Upstream, the opposite of above */
> +			if (consumed_up + requested_up < asym_threshold)
> +				continue;
>  			if (consumed_down + requested_down >= TB_ASYM_MIN) {
>  				ret = -ENOBUFS;
>  				break;
>  			}
> -			if (consumed_up + requested_up < asym_threshold)
> -				continue;
>  
>  			width_up = TB_LINK_WIDTH_ASYM_TX;
>  			width_down = TB_LINK_WIDTH_ASYM_RX;
> -- 
> 2.34.1

Re: [PATCH] thunderbolt: Confirm the necessity to configure asym link first

Posted by Charalampos Mitrodimas 7 months, 2 weeks ago

Mika Westerberg <mika.westerberg@linux.intel.com> writes:

> Hi,
>
> On Thu, Jun 26, 2025 at 04:41:07PM +0800, zhangjianrong wrote:
>> Current implementation can cause allocation failures in
>> tb_alloc_dp_bandwidth() in some cases. For example:
>> allocated_down(30Gbps), allocated_up(50Gbps),
>> requested_down(10Gbps).
>
> I'm not sure I understand the above.
>
> Can you describe in which real life situation this can happen?

I suppose this can happen when reducing bandwidth while total upstream
bandwidth usage on the link exceeds TB_ASYM_MIN (36 Gbps). The
allocation fails at the asymmetric limit check before checking whether
the downstream request actually needs asymmetric mode.

>
>> 
>> Signed-off-by: zhangjianrong <zhangjianrong5@huawei.com>
>> ---
>>  drivers/thunderbolt/tb.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>> index a7c6919fbf97..558455d9716b 100644
>> --- a/drivers/thunderbolt/tb.c
>> +++ b/drivers/thunderbolt/tb.c
>> @@ -1039,6 +1039,9 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
>>  			break;
>>  
>>  		if (downstream) {
>> +			/* Does consumed + requested exceed the threshold */
>> +			if (consumed_down + requested_down < asym_threshold)
>> +				continue;
>>  			/*
>>  			 * Downstream so make sure upstream is within the 36G
>>  			 * (40G - guard band 10%), and the requested is above
>> @@ -1048,20 +1051,17 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
>>  				ret = -ENOBUFS;
>>  				break;
>>  			}
>> -			/* Does consumed + requested exceed the threshold */
>> -			if (consumed_down + requested_down < asym_threshold)
>> -				continue;
>>  
>>  			width_up = TB_LINK_WIDTH_ASYM_RX;
>>  			width_down = TB_LINK_WIDTH_ASYM_TX;
>>  		} else {
>>  			/* Upstream, the opposite of above */
>> +			if (consumed_up + requested_up < asym_threshold)
>> +				continue;
>>  			if (consumed_down + requested_down >= TB_ASYM_MIN) {
>>  				ret = -ENOBUFS;
>>  				break;
>>  			}
>> -			if (consumed_up + requested_up < asym_threshold)
>> -				continue;
>>  
>>  			width_up = TB_LINK_WIDTH_ASYM_TX;
>>  			width_down = TB_LINK_WIDTH_ASYM_RX;
>> -- 
>> 2.34.1

Re: [PATCH] thunderbolt: Confirm the necessity to configure asym link first

Posted by Mika Westerberg 7 months, 2 weeks ago

Hi,

On Thu, Jun 26, 2025 at 11:50:00AM +0000, Charalampos Mitrodimas wrote:
> Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> 
> > Hi,
> >
> > On Thu, Jun 26, 2025 at 04:41:07PM +0800, zhangjianrong wrote:
> >> Current implementation can cause allocation failures in
> >> tb_alloc_dp_bandwidth() in some cases. For example:
> >> allocated_down(30Gbps), allocated_up(50Gbps),
> >> requested_down(10Gbps).
> >
> > I'm not sure I understand the above.
> >
> > Can you describe in which real life situation this can happen?
> 
> I suppose this can happen when reducing bandwidth while total upstream
> bandwidth usage on the link exceeds TB_ASYM_MIN (36 Gbps). The
> allocation fails at the asymmetric limit check before checking whether
> the downstream request actually needs asymmetric mode.

Right, but I would like to see here in the changelog explanation of the
situation and preferably parts of the dmesg showing the error as well.

@zhangjianrong, can you do that and resend?

Note, I will be on vacation after today so expect delay from my side.

> >> 
> >> Signed-off-by: zhangjianrong <zhangjianrong5@huawei.com>
> >> ---
> >>  drivers/thunderbolt/tb.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> >> index a7c6919fbf97..558455d9716b 100644
> >> --- a/drivers/thunderbolt/tb.c
> >> +++ b/drivers/thunderbolt/tb.c
> >> @@ -1039,6 +1039,9 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
> >>  			break;
> >>  
> >>  		if (downstream) {
> >> +			/* Does consumed + requested exceed the threshold */
> >> +			if (consumed_down + requested_down < asym_threshold)
> >> +				continue;
> >>  			/*
> >>  			 * Downstream so make sure upstream is within the 36G
> >>  			 * (40G - guard band 10%), and the requested is above
> >> @@ -1048,20 +1051,17 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
> >>  				ret = -ENOBUFS;
> >>  				break;
> >>  			}
> >> -			/* Does consumed + requested exceed the threshold */
> >> -			if (consumed_down + requested_down < asym_threshold)
> >> -				continue;
> >>  
> >>  			width_up = TB_LINK_WIDTH_ASYM_RX;
> >>  			width_down = TB_LINK_WIDTH_ASYM_TX;
> >>  		} else {
> >>  			/* Upstream, the opposite of above */
> >> +			if (consumed_up + requested_up < asym_threshold)
> >> +				continue;
> >>  			if (consumed_down + requested_down >= TB_ASYM_MIN) {
> >>  				ret = -ENOBUFS;
> >>  				break;
> >>  			}
> >> -			if (consumed_up + requested_up < asym_threshold)
> >> -				continue;
> >>  
> >>  			width_up = TB_LINK_WIDTH_ASYM_TX;
> >>  			width_down = TB_LINK_WIDTH_ASYM_RX;
> >> -- 
> >> 2.34.1

Re: [PATCH] thunderbolt: Confirm the necessity to configure asym link first

Posted by zhangjianrong (E) 7 months, 2 weeks ago


On 6/27/2025 12:46 PM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Jun 26, 2025 at 11:50:00AM +0000, Charalampos Mitrodimas wrote:
>> Mika Westerberg <mika.westerberg@linux.intel.com> writes:
>>
>>> Hi,
>>>
>>> On Thu, Jun 26, 2025 at 04:41:07PM +0800, zhangjianrong wrote:
>>>> Current implementation can cause allocation failures in
>>>> tb_alloc_dp_bandwidth() in some cases. For example:
>>>> allocated_down(30Gbps), allocated_up(50Gbps),
>>>> requested_down(10Gbps).
>>>
>>> I'm not sure I understand the above.
>>>
>>> Can you describe in which real life situation this can happen?
>>
>> I suppose this can happen when reducing bandwidth while total upstream
>> bandwidth usage on the link exceeds TB_ASYM_MIN (36 Gbps). The
>> allocation fails at the asymmetric limit check before checking whether
>> the downstream request actually needs asymmetric mode.
> 
> Right, but I would like to see here in the changelog explanation of the
> situation and preferably parts of the dmesg showing the error as well.

OK, I will update the commit message, but I don't have the dmesg because 
I can't find a host router that contains two dp adapters.

> 
> @zhangjianrong, can you do that and resend?
> 
> Note, I will be on vacation after today so expect delay from my side.
> 
>>>>
>>>> Signed-off-by: zhangjianrong <zhangjianrong5@huawei.com>
>>>> ---
>>>>   drivers/thunderbolt/tb.c | 10 +++++-----
>>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>>>> index a7c6919fbf97..558455d9716b 100644
>>>> --- a/drivers/thunderbolt/tb.c
>>>> +++ b/drivers/thunderbolt/tb.c
>>>> @@ -1039,6 +1039,9 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
>>>>   			break;
>>>>   
>>>>   		if (downstream) {
>>>> +			/* Does consumed + requested exceed the threshold */
>>>> +			if (consumed_down + requested_down < asym_threshold)
>>>> +				continue;
>>>>   			/*
>>>>   			 * Downstream so make sure upstream is within the 36G
>>>>   			 * (40G - guard band 10%), and the requested is above
>>>> @@ -1048,20 +1051,17 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
>>>>   				ret = -ENOBUFS;
>>>>   				break;
>>>>   			}
>>>> -			/* Does consumed + requested exceed the threshold */
>>>> -			if (consumed_down + requested_down < asym_threshold)
>>>> -				continue;
>>>>   
>>>>   			width_up = TB_LINK_WIDTH_ASYM_RX;
>>>>   			width_down = TB_LINK_WIDTH_ASYM_TX;
>>>>   		} else {
>>>>   			/* Upstream, the opposite of above */
>>>> +			if (consumed_up + requested_up < asym_threshold)
>>>> +				continue;
>>>>   			if (consumed_down + requested_down >= TB_ASYM_MIN) {
>>>>   				ret = -ENOBUFS;
>>>>   				break;
>>>>   			}
>>>> -			if (consumed_up + requested_up < asym_threshold)
>>>> -				continue;
>>>>   
>>>>   			width_up = TB_LINK_WIDTH_ASYM_TX;
>>>>   			width_down = TB_LINK_WIDTH_ASYM_RX;
>>>> -- 
>>>> 2.34.1
>