[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7005af42-e546-6a7c-015f-037a5f0e08a9@linutronix.de>
Date: Tue, 11 Jul 2023 10:37:48 +0200
From: Florian Kauer <florian.kauer@...utronix.de>
To: Leon Romanovsky <leon@...nel.org>,
Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, netdev@...r.kernel.org, kurt@...utronix.de,
vinicius.gomes@...el.com, muhammad.husaini.zulkifli@...el.com,
tee.min.tan@...ux.intel.com, aravindhan.gunasekaran@...el.com,
sasha.neftin@...el.com, Naama Meir <naamax.meir@...ux.intel.com>
Subject: Re: [PATCH net 5/6] igc: Fix launchtime before start of cycle
On 11.07.23 09:09, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote:
>> From: Florian Kauer <florian.kauer@...utronix.de>
>>
>> It is possible (verified on a running system) that frames are processed
>> by igc_tx_launchtime with a txtime before the start of the cycle
>> (baset_est).
>>
>> However, the result of txtime - baset_est is written into a u32,
>> leading to a wrap around to a positive number. The following
>> launchtime > 0 check will only branch to executing launchtime = 0
>> if launchtime is already 0.
>>
>> Fix it by using a s32 before checking launchtime > 0.
>>
>> Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
>> Signed-off-by: Florian Kauer <florian.kauer@...utronix.de>
>> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.de>
>> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>> ---
>> drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 5d24930fed8f..4855caa3bae4 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1016,7 +1016,7 @@ static __le32 igc_tx_launchtime(struct igc_ring *ring, ktime_t txtime,
>> ktime_t base_time = adapter->base_time;
>> ktime_t now = ktime_get_clocktai();
>> ktime_t baset_est, end_of_cycle;
>> - u32 launchtime;
>> + s32 launchtime;
>
> The rest of igc_tx_launchtime() function is very questionable,
> as ktime_sub_ns() returns ktime_t which is s64.
>
> 1049 launchtime = ktime_sub_ns(txtime, baset_est);
> 1050 if (launchtime > 0)
> 1051 div_s64_rem(launchtime, cycle_time, &launchtime);
> 1052 else
> 1053 launchtime = 0;
> 1054
> 1055 return cpu_to_le32(launchtime);
>
If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
something larger than s32 max if cycle_time is larger than s32 max and if that
is the case everything will be broken anyway since the corresponding hardware
register only holds 30 bits.
However, I do not see on first inspection where that case is properly handled
(probably just by rejecting the TAPRIO schedule).
Can someone with more experience in that area please jump in?
Thanks,
Florian
>
>> s64 n;
>>
>> n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
>> --
>> 2.38.1
>>
>>
Powered by blists - more mailing lists