diff mbox series

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

Message ID 20230529130940.1501-2-liudongdong3@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/hns3: add some bugfixes for hns3 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing warning apply patch failure

Commit Message

Dongdong Liu May 29, 2023, 1:09 p.m. UTC 
  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(+)

Comments

Ferruh Yigit June 2, 2023, 9 a.m. UTC | #1 
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.
	

> +	clock_gettime(CLOCK_REALTIME, &sys_time);
> +	(void)hns3_timesync_write_time(dev, &sys_time);
> +
>  	return 0;
>  }
>
Dongdong Liu June 2, 2023, 10:55 a.m. UTC | #2 
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;
>>  }
>>
>
> .
>
diff mbox series

Patch

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];
+	clock_gettime(CLOCK_REALTIME, &sys_time);
+	(void)hns3_timesync_write_time(dev, &sys_time);
+
 	return 0;
 }