DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: "Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions)" <xuanziyang2@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Wangxiaoyun (Cloud,
	Network Chip Application Development Dept)"
	<cloud.wangxiaoyun@huawei.com>,
	zhouguoyang <zhouguoyang@huawei.com>,
	Shahar Belkar <shahar.belkar@huawei.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>,
	Luoxianjun <luoxianjun@huawei.com>
Subject: Re: [dpdk-dev] [PATCH v4 11/11] net/hinic: add support for basic device operations
Date: Wed, 12 Jun 2019 15:10:03 +0000
Message-ID: <EA3B33EA29B44042A5337C4DC59AF4505EEE43D3@dggeml512-mbx.china.huawei.com> (raw)

> 
> On 6/6/2019 12:07 PM, Ziyang Xuan wrote:
> > Add hinic PMD initialization and ethernet operatioins code.
> 
> Hi Xuan,
> 
> Previous patches puts the code without enabling them, this last patch
> registers the PMD with lots of new code, it is hard to review this PMD.
> 
> I think "OCTEON TX2" which also submitted this release [1] is good sample of
> how building the PMD incrementally, feature by feature, can you please
> check it?
> [1] https://patches.dpdk.org/user/todo/dpdk/?series=4848

OK, thanks.

> 
> >
> > Signed-off-by: Ziyang Xuan <xuanziyang2@huawei.com>
> > ---
> >  drivers/net/hinic/hinic_pmd_ethdev.c        | 2125 +++++++++++++++++++
> >  drivers/net/hinic/rte_pmd_hinic_version.map |    4 +
> 
> .map file needs to be added in the patch that adds "hinic/Makefile",
> otherwise shared build will fail for those patches in between.
> 
> <...>
> 
> > +
> > +/* Hinic PMD parameters */
> > +#define ETH_HINIC_FW_VER	"check_fw_version"
> > +
> > +static const char *const valid_params[] = {
> > +	ETH_HINIC_FW_VER,
> > +	NULL};
> 
> 
> Can you please document this devargs in hinic documentation, describe what
> it does, and perhaps provide a sample command line to use it.
> 
> <...>
> <...>
> 
> > +	snprintf(nic_dev->proc_dev_name,
> > +		 sizeof(nic_dev->proc_dev_name),
> > +		 "hinic-%.4x:%.2x:%.2x.%x",
> > +		 pci_dev->addr.domain, pci_dev->addr.bus,
> > +		 pci_dev->addr.devid, pci_dev->addr.function);
> > +
> > +	rte_eth_copy_pci_info(eth_dev, pci_dev);
> 
> You may not need this, can you please double check?

Yes, we use rte_eth_dev_pci_generic_probe, it can do this. And I will delete it here.

> 
> > +
> > +	/* clear RX ring mbuf allocated failed */
> > +	eth_dev->data->rx_mbuf_alloc_failed = 0;
> 
> At this stage all ethdev->data should be 0, is this assignment required?

I will check it, and delete it if not necessary.

> 
> <...>
> 
> > +/**
> > + * DPDK callback to close the device.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device structure.
> > + */
> > +void hinic_dev_close(struct rte_eth_dev *dev) {
> 
> You may want to 'RTE_ETH_DEV_CLOSE_REMOVE' flag to cause
> 'rte_eth_dev_close()'
> clean ethdev resources clean, please check other PMDs and ethdev API for
> sample usage.

Yes, I find it. I will fix it to set 'RTE_ETH_DEV_CLOSE_REMOVE' flag when initialize.

Thank you for your precious comments very much, I will fix these problems carefully.

             reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-12 15:10 Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions) [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-06-06 11:04 [dpdk-dev] [PATCH v4 00/11] A new net PMD - hinic Ziyang Xuan
2019-06-06 11:07 ` [dpdk-dev] [PATCH v4 11/11] net/hinic: add support for basic device operations Ziyang Xuan
2019-06-11 16:02   ` Ferruh Yigit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=EA3B33EA29B44042A5337C4DC59AF4505EEE43D3@dggeml512-mbx.china.huawei.com \
    --to=xuanziyang2@huawei.com \
    --cc=cloud.wangxiaoyun@huawei.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=luoxianjun@huawei.com \
    --cc=shahar.belkar@huawei.com \
    --cc=stephen@networkplumber.org \
    --cc=zhouguoyang@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox