From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v4 07/17] net/avp: driver registration Date: Thu, 16 Mar 2017 14:53:02 +0000 Message-ID: <6571ff47-1b94-bc74-ba0e-14dd8d32c0ed@intel.com> References: <1488414008-162839-1-git-send-email-allain.legacy@windriver.com> <1489432593-32390-1-git-send-email-allain.legacy@windriver.com> <1489432593-32390-8-git-send-email-allain.legacy@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, ian.jolliffe@windriver.com, bruce.richardson@intel.com, john.mcnamara@intel.com, keith.wiles@intel.com, thomas.monjalon@6wind.com, vincent.jardin@6wind.com, jerin.jacob@caviumnetworks.com, stephen@networkplumber.org, 3chas3@gmail.com To: Allain Legacy Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id C39691396 for ; Thu, 16 Mar 2017 15:53:28 +0100 (CET) In-Reply-To: <1489432593-32390-8-git-send-email-allain.legacy@windriver.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > Signed-off-by: Matt Peters <...> > +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, ð_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; > +} > + <...>