From: Ed Czeck <ed.czeck@atomicrules.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, John Miller <john.miller@atomicrules.com>,
Shepard Siegel <shepard.siegel@atomicrules.com>,
Stephen Hemminger <stephen@networkplumber.org>,
Adrien Mazarguil <adrien.mazarguil@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v5 7/7] net/ark: Arkville PMD component integration
Date: Tue, 28 Mar 2017 18:42:38 -0400 [thread overview]
Message-ID: <CALZ3GujJ79BwCivFVYDDi2JEDdYnsW5M2nQmkV+pHLOz_YvMPg@mail.gmail.com> (raw)
In-Reply-To: <c70bfd96-29f7-4f20-8804-4a70b0fc4507@intel.com>
On Tue, Mar 28, 2017 at 10:38 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:
> On 3/23/2017 11:01 PM, Ed Czeck wrote:
> > * Flesh out device configuration
> > * Add links dev_ops
> > * allow dynamic extension loading
> >
> > Signed-off-by: Shepard Siegel <shepard.siegel@atomicrules.com>
> > Signed-off-by: John Miller <john.miller@atomicrules.com>
> > Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
>
> <...>
>
> > +FW version = Y
>
> FW version is not required, as far as I can see, it requires
> fw_version_get eth_dev_ops implemented.
>
Entry removed.
>
> <...>
>
> > + /* We are a single function multi-port device. */
> > + const unsigned int numa_node = rte_socket_id();
> > + struct ether_addr adr;
> > +
> > + ret = ark_config_device(dev);
> > dev->dev_ops = &ark_eth_dev_ops;
> >
> > + dev->data->mac_addrs = rte_zmalloc("ark", ETHER_ADDR_LEN, 0);
> > + if (!dev->data->mac_addrs) {
> > + PMD_DRV_LOG(ERR,
> > + "Failed to allocated memory for storing mac
> address"
> > + );
> > + }
> > + ether_addr_copy((struct ether_addr *)&adr,
> &dev->data->mac_addrs[0]);
>
> "adr" has random value at this point, right? Why to copy it?
>
Code removed. The mac address is left as 0, since arkville does not
include a mac.
> > + int pc = 1;
> > + int p;
>
> I am aware some people prefer the declaring variables close to context,
> which is good idea, but if I remember correct, there was a patchset,
> from Adrien, to make DPDK C99 compatible, will this break it?
>
I moved the declarations to the top of the function to match the dpdk style.
> > +
> > + if (ark->user_ext.dev_get_port_count) {
> > + pc = ark->user_ext.dev_get_port_count(dev,
> ark->user_data);
> > + ark->num_ports = pc;
> > + } else {
> > + ark->num_ports = 1;
>
> Because pc has default value of "1", this if statement can be simplified.
>
Code has been simplified to 3 lines.
>
> > + }
> > + for (p = 0; p < pc; p++) {
> > + struct ark_port *port;
> > +
> > + port = &ark->port[p];
> > + struct rte_eth_dev_data *data = NULL;
> > +
> > + port->id = p;
> > +
> > + char name[RTE_ETH_NAME_MAX_LEN];
> > +
> > + snprintf(name, sizeof(name), "arketh%d",
> > + dev->data->port_id + p);
> > +
> > + if (p == 0) {
> > + /* First port is already allocated by DPDK */
> > + port->eth_dev = ark->eth_dev;
> > + continue;
> > + }
> > +
> > + /* reserve an ethdev entry */
> > + port->eth_dev = rte_eth_dev_allocate(name);
> > + if (!port->eth_dev) {
> > + PMD_DRV_LOG(ERR,
> > + "Could not allocate eth_dev for port
> %d\n",
> > + p);
> > + goto error;
> > + }
> > +
> > + data = rte_zmalloc_socket(name, sizeof(*data), 0,
> numa_node);
> > + if (!data) {
> > + PMD_DRV_LOG(ERR,
> > + "Could not allocate eth_dev for port
> %d\n",
> > + p);
> > + goto error;
> > + }
> > + data->port_id = ark->eth_dev->data->port_id + p;
>
> "ark->eth_dev->data->port_id" is port_id of the first physical ARK
> device, and it looks like each device may have multiple ports and "p" is
> the port_id within same device.
>
Arkville is a single PCIE function with 1 or more ports, lets call this p.
After initialization each there will be p additional devices, and each
device will have exactly 1 port.
We now understand the features of the rte_eth_dev_allocate(), and have
simplified this code avoid the unnecessary copies.
>
> From DPDK point of view, port_id is a global value incremented one by
> each eth port, so port_id is a unique value, why adding these two values?
>
> > + port->eth_dev->data = data;
>
> Why overwriting existing data value?
>
> > + port->eth_dev->device = &pci_dev->device;
> > +
>
> > + ark->user_ext.dev_init(dev, ark->a_bar, p);
> > + }
>
Unnecessary coping has been removed.
>
> <...>
>
> > static int
> > eth_ark_dev_uninit(struct rte_eth_dev *dev)
> > {
>
>
> Shouldn't uninit go thorough all ports ("for (p = 0; p < pc; p++) ") and
> uninit them all?
>
Arkville has one PCIE function, during initialization we determine the
number of ports, and from that one device for each port will be created.
At the end, there will be p devices, each with one port. So during device
uninit, there is only 1 port.
>
>
> <...>
>
> > +/*
> > + * The following functions are optional and are directly mapped
> > + * from the DPDK PMD ops structure.
> > + * Each function if implemented is called after the ARK PMD
> > + * implementation executes.
> > + */
> > +int dev_configure(struct rte_eth_dev *dev, void *user_data);
> > +int dev_start(struct rte_eth_dev *dev, void *user_data);
> > +void dev_stop(struct rte_eth_dev *dev, void *user_data);
> > +void dev_close(struct rte_eth_dev *dev, void *user_data);
> > +int link_update(struct rte_eth_dev *dev, int wait_to_complete,
> > + void *user_data);
> > +int dev_set_link_up(struct rte_eth_dev *dev, void *user_data);
> > +int dev_set_link_down(struct rte_eth_dev *dev, void *user_data);
> > +void stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
> > + void *user_data);
> > +void stats_reset(struct rte_eth_dev *dev, void *user_data);
> > +void mac_addr_add(struct rte_eth_dev *dev,
> > + struct ether_addr *macadr,
> > + uint32_t index,
> > + uint32_t pool,
> > + void *user_data);
> > +void mac_addr_remove(struct rte_eth_dev *dev, uint32_t index, void
> *user_data);
> > +void mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
> > + void *user_data);
>
> Where these functions are implemented? Do we need these declarations?
>
The following comment has been added to this file, which addresses your
question.
/*
* This is the template file for users who which to define a dynamic
* extension to the Arkville PMD. User's who create an extension
* should include this file and define the necessary and desired
* functions.
* Only 1 function is required for an extension, dev_init(); all other
* functions prototyped in this file are optional.
*/
Thanks
Ed.
next prev parent reply other threads:[~2017-03-28 22:42 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-23 1:03 [dpdk-dev] [PATCH v4 1/7] net/ark: PMD for Atomic Rules Arkville driver stub Ed Czeck
2017-03-23 1:03 ` [dpdk-dev] [PATCH v4 2/7] net/ark: HW API part 1 of 3 Ed Czeck
2017-03-23 11:38 ` Ferruh Yigit
2017-03-23 20:33 ` Ed Czeck
2017-03-29 1:05 ` [dpdk-dev] [PATCH v6 2/7] net/ark: Provide API for hardware modules mpu, rqp, and pktdir Ed Czeck
2017-03-29 21:32 ` [dpdk-dev] [PATCH v7 2/7] net/ark: provide API for hardware modules mpu rqp " Ed Czeck
2017-04-04 19:50 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-03-23 1:03 ` [dpdk-dev] [PATCH v4 3/7] net/ark: HW API part 2 of 3 Ed Czeck
2017-03-29 1:05 ` [dpdk-dev] [PATCH v6 3/7] net/ark: Provide API for hardware modules udm and ddm Ed Czeck
2017-03-29 21:33 ` [dpdk-dev] [PATCH v7 3/7] net/ark: provide " Ed Czeck
2017-04-04 19:50 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-03-23 1:03 ` [dpdk-dev] [PATCH v4 4/7] net/ark: HW API part 3 of 3 Ed Czeck
2017-03-29 1:06 ` [dpdk-dev] [PATCH v6 4/7] net/ark: Provide API for hardware modules pktchkr and pktgen Ed Czeck
2017-03-29 21:34 ` [dpdk-dev] [PATCH v7 4/7] net/ark: provide " Ed Czeck
2017-04-04 19:50 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-03-23 1:03 ` [dpdk-dev] [PATCH v4 5/7] net/ark: Packet TX support initial version Ed Czeck
2017-03-23 12:14 ` Ferruh Yigit
2017-03-23 21:44 ` Ed Czeck
2017-03-29 1:06 ` [dpdk-dev] [PATCH v6 " Ed Czeck
2017-03-29 21:34 ` [dpdk-dev] [PATCH v7 5/7] net/ark: packet Tx " Ed Czeck
2017-04-04 19:51 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-03-23 1:03 ` [dpdk-dev] [PATCH v4 6/7] net/ark: Packet RX " Ed Czeck
2017-03-23 12:14 ` Ferruh Yigit
2017-03-23 21:51 ` Ed Czeck
2017-03-29 1:06 ` [dpdk-dev] [PATCH v6 " Ed Czeck
2017-03-29 21:35 ` [dpdk-dev] [PATCH v7 6/7] net/ark: packet Rx " Ed Czeck
2017-04-04 19:51 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-03-23 1:03 ` [dpdk-dev] [PATCH v4 7/7] net/ark: Arkville PMD component integration Ed Czeck
2017-03-23 12:13 ` Ferruh Yigit
2017-03-23 22:19 ` Ed Czeck
2017-03-28 12:34 ` Ferruh Yigit
2017-03-29 1:07 ` [dpdk-dev] [PATCH v6 " Ed Czeck
2017-03-29 9:54 ` Ferruh Yigit
2017-03-29 21:35 ` [dpdk-dev] [PATCH v7 7/7] net/ark: arkville " Ed Czeck
2017-04-04 19:51 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-03-23 11:36 ` [dpdk-dev] [PATCH v4 1/7] net/ark: PMD for Atomic Rules Arkville driver stub Ferruh Yigit
2017-03-23 13:08 ` Ferruh Yigit
2017-03-23 19:46 ` Ed Czeck
2017-03-28 12:58 ` Ferruh Yigit
2017-03-28 21:11 ` Ed Czeck
2017-03-29 9:42 ` Ferruh Yigit
2017-03-23 22:59 ` [dpdk-dev] [PATCH v5 " Ed Czeck
2017-03-23 23:00 ` [dpdk-dev] [PATCH v5 2/7] net/ark: provide api to hardware module mpu, rqp, and pktdir Ed Czeck
2017-03-28 14:35 ` Ferruh Yigit
2017-03-28 20:14 ` Ed Czeck
2017-03-23 23:00 ` [dpdk-dev] [PATCH v5 3/7] net/ark: provide API hardware module udm and ddm Ed Czeck
2017-03-23 23:00 ` [dpdk-dev] [PATCH v5 4/7] net/ark: prrovide api for hardware module pktchkr and pktgen Ed Czeck
2017-03-23 23:00 ` [dpdk-dev] [PATCH v5 5/7] net/ark: Packet TX support initial version Ed Czeck
2017-03-23 23:01 ` [dpdk-dev] [PATCH v5 6/7] net/ark: Packet RX " Ed Czeck
2017-03-28 14:36 ` Ferruh Yigit
2017-03-28 21:59 ` Ed Czeck
2017-03-23 23:01 ` [dpdk-dev] [PATCH v5 7/7] net/ark: Arkville PMD component integration Ed Czeck
2017-03-28 14:38 ` Ferruh Yigit
2017-03-28 15:00 ` Adrien Mazarguil
2017-03-28 22:42 ` Ed Czeck [this message]
2017-03-28 14:34 ` [dpdk-dev] [PATCH v5 1/7] net/ark: PMD for Atomic Rules Arkville driver stub Ferruh Yigit
2017-03-28 22:38 ` Ed Czeck
2017-03-28 14:41 ` Ferruh Yigit
2017-03-29 1:04 ` [dpdk-dev] [PATCH v6 " Ed Czeck
2017-03-29 21:32 ` [dpdk-dev] [PATCH v7 1/7] net/ark: stub PMD for Atomic Rules Arkville Ed Czeck
2017-03-31 14:51 ` Ferruh Yigit
2017-03-31 15:09 ` Shepard Siegel
2017-04-04 20:58 ` Ed Czeck
2017-04-04 19:50 ` [dpdk-dev] [PATCH v8 " Ed Czeck
2017-04-07 10:54 ` Ferruh Yigit
Reply instructions:
You may reply publicly 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=CALZ3GujJ79BwCivFVYDDi2JEDdYnsW5M2nQmkV+pHLOz_YvMPg@mail.gmail.com \
--to=ed.czeck@atomicrules.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=john.miller@atomicrules.com \
--cc=shepard.siegel@atomicrules.com \
--cc=stephen@networkplumber.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).