netdev - Re: [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8a67398a-48c1-4aad-9df6-d05688d6c951@linux.dev>
Date: Sun, 3 Nov 2024 21:17:05 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jakub Kicinski <kuba@...nel.org>, Vadim Fedorenko <vadfed@...a.com>
Cc: Michael Chan <michael.chan@...adcom.com>,
 Pavan Chebbi <pavan.chebbi@...adcom.com>, Andrew Lunn
 <andrew+netdev@...n.ch>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
 Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next v4 1/2] bnxt_en: cache only 24 bits of hw counter

On 03/11/2024 19:22, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 13:54:52 -0700 Vadim Fedorenko wrote:
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index fa514be87650..820c7e83e586 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -106,7 +106,7 @@ static void bnxt_ptp_get_current_time(struct bnxt *bp)
>>   	if (!ptp)
>>   		return;
>>   	spin_lock_irqsave(&ptp->ptp_lock, flags);
>> -	WRITE_ONCE(ptp->old_time, ptp->current_time);
>> +	WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
> 
> the casts to u32 seem unnecessary since write to u32 will truncate
> the value, anyway, and they make the lines go over 80 columns
> 
>>   	bnxt_refclk_read(bp, NULL, &ptp->current_time);
>>   	spin_unlock_irqrestore(&ptp->ptp_lock, flags);
>>   }
>> @@ -174,7 +174,7 @@ void bnxt_ptp_update_current_time(struct bnxt *bp)
>>   	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>>   
>>   	bnxt_refclk_read(ptp->bp, NULL, &ptp->current_time);
>> -	WRITE_ONCE(ptp->old_time, ptp->current_time);
>> +	WRITE_ONCE(ptp->old_time, (u32)(ptp->current_time >> BNXT_HI_TIMER_SHIFT));
>>   }
>>   
>>   static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
>> @@ -813,7 +813,7 @@ int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
>>   	if (!ptp)
>>   		return -ENODEV;
>>   
>> -	BNXT_READ_TIME64(ptp, time, ptp->old_time);
>> +	time = (u64)(READ_ONCE(ptp->old_time) << BNXT_HI_TIMER_SHIFT);
> 
> And this cast looks misplaced, I presume you want the shift to operate
> on 64b. The way this is written the shift will be truncated to 32b,
> and then we will promote, with top 32b being all 0.

Indeed, this cast is misplaced. I'll do v5 soon, thanks!