Re: [dpdk-dev] [PATCH v2 08/11] net/hinic: add hinic PMD build and doc files

["Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions)" <xuanziyang2@huawei.com>]

Hi Stephen,

Thank you for your comments. We will fix your comments at the next patches.

Thanks,
Ziyang Xuan

-----邮件原件-----
发件人: Stephen Hemminger [mailto:stephen@networkplumber.org] 
发送时间: 2019年5月29日 12:59
收件人: Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions) <xuanziyang2@huawei.com>
抄送: dev@dpdk.org; ferruh.yigit@intel.com; Wangxiaoyun (Cloud, Network Chip Application Development Dept) <cloud.wangxiaoyun@huawei.com>; zhouguoyang <zhouguoyang@huawei.com>; Shahar Belkar <shahar.belkar@huawei.com>; Luoxianjun <luoxianjun@huawei.com>
主题: Re: [dpdk-dev] [PATCH v2 08/11] net/hinic: add hinic PMD build and doc files

On Wed, 29 May 2019 11:50:36 +0800
Ziyang Xuan <xuanziyang2@huawei.com> wrote:

> +Huawei hinic
> +M: Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>
> +M: Ziyang Xuan <xuanziyang2@huawei.com>
> +M: Guoyang Zhou <zhouguoyang@huawei.com>
> +M: Rami Rosen <rami.rosen@huawei.com>

You should remove Rami, he will be missed.

> diff --git a/drivers/net/hinic/base/hinic_logs.c 
> b/drivers/net/hinic/base/hinic_logs.c
> new file mode 100644
> index 000000000..1e9b5ad92
> --- /dev/null
> +++ b/drivers/net/hinic/base/hinic_logs.c
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017 Huawei Technologies Co., Ltd  */
> +
> +
> +#include "hinic_logs.h"
> +
> +int hinic_logtype;
> +
> +RTE_INIT(hinic_init_log)
> +{
> +	hinic_logtype = rte_log_register("pmd.net.hinic");
> +	if (hinic_logtype >= 0)
> +		rte_log_set_level(hinic_logtype, RTE_LOG_INFO); }
> +

You could just put this in some other file


> diff --git a/drivers/net/hinic/base/hinic_logs.h 
> b/drivers/net/hinic/base/hinic_logs.h
> new file mode 100644
> index 000000000..ed84a37a6
> --- /dev/null
> +++ b/drivers/net/hinic/base/hinic_logs.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017 Huawei Technologies Co., Ltd  */
> +
> +#ifndef _HINIC_LOGS_H_
> +#define _HINIC_LOGS_H_
> +
> +#ifdef __cplusplus
> +#if __cplusplus
> +extern "C"{
> +#endif
> +#endif /* __cplusplus */
> +#include <rte_log.h>

Why all the C++ guards, this is a private file.

> +error_cflags = ['-Wno-unused-value',
> +		'-Wno-unused-but-set-variable',
> +		'-Wno-unused-variable',
> +]

Please fix these errors instead of masking them

> +/**
> + * Atomically writes the link status information into global
> + * struct rte_eth_dev.
> + */
> +int hinic_dev_atomic_write_link_status(struct rte_eth_dev *dev,
> +				       struct rte_eth_link *link) {
> +	struct rte_eth_link *dst = &dev->data->dev_link;
> +	struct rte_eth_link *src = link;
> +
> +	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
> +	    *(uint64_t *)src) == 0)
> +		return HINIC_ERROR;
> +
> +	return HINIC_OK;
> +}

There already is code in rte_ethdev in current versions of DPDK to do this.