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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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!