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.sie...@atomicrules.com>
> Signed-off-by: John Miller <john.mil...@atomicrules.com>
> Signed-off-by: Ed Czeck <ed.cz...@atomicrules.com>
<...>
> +[Features]
> +Queue start/stop = Y
> +Jumbo frame = Y
> +Scattered Rx = Y
> +Basic stats = Y
> +Stats per queue = Y
> +FW version = Y
FW version is not required, as far as I can see, it requires
fw_version_get eth_dev_ops implemented.
<...>
> + /* 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?
> + /*
> + * We will create additional devices based on the number of requested
> + * ports
> + */
> + 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?
> +
> + 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.
> + }
> + 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.
>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;
> + port->eth_dev->data->dev_private = ark;
> + port->eth_dev->driver = ark->eth_dev->driver;
> + port->eth_dev->dev_ops = ark->eth_dev->dev_ops;
> + port->eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst;
> + port->eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst;
> +
> + rte_eth_copy_pci_info(port->eth_dev, pci_dev);
> +
> + port->eth_dev->data->mac_addrs =
> + rte_zmalloc(name, ETHER_ADDR_LEN, 0);
> + if (!port->eth_dev->data->mac_addrs) {
> + PMD_DRV_LOG(ERR,
> + "Memory allocation for MAC failed!"
> + " Exiting.\n");
> + goto error;
> + }
> + ether_addr_copy(&adr,
> + &port->eth_dev->data->mac_addrs[0]);
> +
> + if (ark->user_ext.dev_init)
> + ark->user_data =
> + ark->user_ext.dev_init(dev, ark->a_bar, p);
> + }
<...>
> static int
> eth_ark_dev_uninit(struct rte_eth_dev *dev)
> {
> + struct ark_adapter *ark =
> + (struct ark_adapter *)dev->data->dev_private;
> +
> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> return 0;
>
> + if (ark->user_ext.dev_uninit)
> + ark->user_ext.dev_uninit(dev, ark->user_data);
> +
> + ark_pktgen_uninit(ark->pg);
> + ark_pktchkr_uninit(ark->pc);
> +
> dev->dev_ops = NULL;
> dev->rx_pkt_burst = NULL;
> dev->tx_pkt_burst = NULL;
> + if (dev->data->mac_addrs)
> + rte_free(dev->data->mac_addrs);
> + if (dev->data)
> + rte_free(dev->data);
> +
Shouldn't uninit go thorough all ports ("for (p = 0; p < pc; p++) ") and
uninit them all?
> return 0;
> }
>
<...>
> +/*
> + * 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 declerations?
