[dpdk-dev] [PATCH v4 07/17] net/avp: driver registration

[Ferruh Yigit]

On 3/13/2017 7:16 PM, Allain Legacy wrote:
> Adds the initial framework for registering the driver against the support
> PCI device identifiers.
> 
> Signed-off-by: Allain Legacy <allain.legacy at windriver.com>
> Signed-off-by: Matt Peters <matt.peters at windriver.com>

<...>

> +static int eth_avp_dev_init(struct rte_eth_dev *eth_dev);
> +static int eth_avp_dev_uninit(struct rte_eth_dev *eth_dev);

I am for removing static function declarations by reordering functions,
and for this case even reordering not required I think, you can remove them.

> +
> +
> +#define AVP_DEV_TO_PCI(eth_dev) RTE_DEV_TO_PCI((eth_dev)->device)
> +
> +
> +#define AVP_MAX_MAC_ADDRS 1
> +#define AVP_MIN_RX_BUFSIZE ETHER_MIN_LEN
> +
> +
> +/*
> + * Defines the number of microseconds to wait before checking the response
> + * queue for completion.
> + */
> +#define AVP_REQUEST_DELAY_USECS (5000)
> +
> +/*
> + * Defines the number times to check the response queue for completion before
> + * declaring a timeout.
> + */
> +#define AVP_MAX_REQUEST_RETRY (100)
> +
> +/* Defines the current PCI driver version number */
> +#define AVP_DPDK_DRIVER_VERSION RTE_AVP_CURRENT_GUEST_VERSION
> +

I would suggest creating a avp_ethdev.h and move above defines there,
but of course this is at your will.

> +/*
> + * The set of PCI devices this driver supports
> + */
> +static const struct rte_pci_id pci_id_avp_map[] = {
> +	{ .vendor_id = RTE_AVP_PCI_VENDOR_ID,
> +	  .device_id = RTE_AVP_PCI_DEVICE_ID,
> +	  .subsystem_vendor_id = RTE_AVP_PCI_SUB_VENDOR_ID,
> +	  .subsystem_device_id = RTE_AVP_PCI_SUB_DEVICE_ID,
> +	  .class_id = RTE_CLASS_ANY_ID,
> +	},
> +
> +	{ .vendor_id = 0, /* sentinel */
> +	},
> +};
> +
> +
> +/*
> + * Defines the AVP device attributes which are attached to an RTE ethernet
> + * device
> + */
> +struct avp_dev {
> +	uint32_t magic; /**< Memory validation marker */
> +	uint64_t device_id; /**< Unique system identifier */
> +	struct ether_addr ethaddr; /**< Host specified MAC address */
> +	struct rte_eth_dev_data *dev_data;
> +	/**< Back pointer to ethernet device data */
> +	volatile uint32_t flags; /**< Device operational flags */
> +	uint8_t port_id; /**< Ethernet port identifier */
> +	struct rte_mempool *pool; /**< pkt mbuf mempool */
> +	unsigned int guest_mbuf_size; /**< local pool mbuf size */
> +	unsigned int host_mbuf_size; /**< host mbuf size */
> +	unsigned int max_rx_pkt_len; /**< maximum receive unit */
> +	uint32_t host_features; /**< Supported feature bitmap */
> +	uint32_t features; /**< Enabled feature bitmap */
> +	unsigned int num_tx_queues; /**< Negotiated number of transmit queues */
> +	unsigned int max_tx_queues; /**< Maximum number of transmit queues */
> +	unsigned int num_rx_queues; /**< Negotiated number of receive queues */
> +	unsigned int max_rx_queues; /**< Maximum number of receive queues */
> +
> +	struct rte_avp_fifo *tx_q[RTE_AVP_MAX_QUEUES]; /**< TX queue */
> +	struct rte_avp_fifo *rx_q[RTE_AVP_MAX_QUEUES]; /**< RX queue */
> +	struct rte_avp_fifo *alloc_q[RTE_AVP_MAX_QUEUES];
> +	/**< Allocated mbufs queue */
> +	struct rte_avp_fifo *free_q[RTE_AVP_MAX_QUEUES];
> +	/**< To be freed mbufs queue */
> +
> +	/* For request & response */
> +	struct rte_avp_fifo *req_q; /**< Request queue */
> +	struct rte_avp_fifo *resp_q; /**< Response queue */
> +	void *host_sync_addr; /**< (host) Req/Resp Mem address */
> +	void *sync_addr; /**< Req/Resp Mem address */
> +	void *host_mbuf_addr; /**< (host) MBUF pool start address */
> +	void *mbuf_addr; /**< MBUF pool start address */
> +} __rte_cache_aligned;
> +
> +/* RTE ethernet private data */
> +struct avp_adapter {
> +	struct avp_dev avp;
> +} __rte_cache_aligned;

if avp_ethdev.h created, structs also can go there.

> +
> +/* Macro to cast the ethernet device private data to a AVP object */
> +#define AVP_DEV_PRIVATE_TO_HW(adapter) \
> +	(&((struct avp_adapter *)adapter)->avp)
> +

<...>

> +	rte_eth_copy_pci_info(eth_dev, pci_dev);
> +
> +	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
> +
> +	/* Allocate memory for storing MAC addresses */
> +	eth_dev->data->mac_addrs = rte_zmalloc("avp_ethdev", ETHER_ADDR_LEN, 0);
> +	if (eth_dev->data->mac_addrs == NULL) {
> +		PMD_DRV_LOG(ERR, "Failed to allocate %d bytes needed to store MAC addresses\n",
> +			    ETHER_ADDR_LEN);
> +		return -ENOMEM;
> +	}
> +
> +	/* Get a mac from device config */
> +	ether_addr_copy(&avp->ethaddr, &eth_dev->data->mac_addrs[0]);

This copies MAC address from avp->ethaddr to eth_dev.
But at this point avp->ethaddr is all zero, is this the intention?

> +
> +	return 0;
> +}
> +

<...>