Re: [PATCH 01/11] net/hns3: fix uninitialized RTC time

[Dongdong Liu <liudongdong3@huawei.com>]

Hi Ferruh

Many thanks for your review.

On 2023/6/2 17:00, Ferruh Yigit wrote:
> On 5/29/2023 2:09 PM, Dongdong Liu wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Driver doesn't initialize RTC time during probe phase, which
>> lead to an inaccurate time.
>>
>> Fixes: 38b539d96eb6 ("net/hns3: support IEEE 1588 PTP")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>>  drivers/net/hns3/hns3_ptp.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/hns3/hns3_ptp.c b/drivers/net/hns3/hns3_ptp.c
>> index db3c007b12..fb834bb180 100644
>> --- a/drivers/net/hns3/hns3_ptp.c
>> +++ b/drivers/net/hns3/hns3_ptp.c
>> @@ -59,6 +59,8 @@ hns3_ptp_int_en(struct hns3_hw *hw, bool en)
>>  int
>>  hns3_ptp_init(struct hns3_hw *hw)
>>  {
>> +	struct timespec sys_time;
>> +	struct rte_eth_dev *dev;
>>  	int ret;
>>
>>  	if (!hns3_dev_get_support(hw, PTP))
>> @@ -71,6 +73,11 @@ hns3_ptp_init(struct hns3_hw *hw)
>>  	/* Start PTP timer */
>>  	hns3_write_dev(hw, HNS3_CFG_TIME_CYC_EN, 1);
>>
>> +	/* Initializing the RTC. */
>> +	dev = &rte_eth_devices[hw->data->port_id];
>
>
> Better to not access 'rte_eth_devices[]' global array directly from the
> driver, driver should keep reference to the eth_dev internally.
>
> 'hns3_timesync_write_time()' already gets 'hw' from 'eth_dev' and uses
> it. Perhaps 'hns3_timesync_write_time()' should get 'hw' as paramter.
>
> Since 'hns3_timesync_write_time()' used for dev_ops, it is possible to
> get internal version of it, like:
>
> ```
> hns3_timesync_write_time_(struct hns3_hw *hw, timespec *ts) {
> }
>
> hns3_timesync_write_time(struct rte_eth_dev *dev, timespec *ts) {
> 	struct hns3_hw *hw = <dev to hw>;
>
> 	hns3_timesync_write_time_(hw, ts);
> }
> ```
>
> And this function can directly use internal version.
Good point.
Will fix in v2.

Thanks,
Dongdong
> 	
>
>> +	clock_gettime(CLOCK_REALTIME, &sys_time);
>> +	(void)hns3_timesync_write_time(dev, &sys_time);
>> +
>>  	return 0;
>>  }
>>
>
> .
>