Regards,
Keith
> On Oct 12, 2016, at 9:56 AM, Yigit, Ferruh <ferruh.yigit at intel.com> wrote:
>
> On 10/11/2016 10:51 PM, Keith Wiles wrote:
>> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
>> on the local host. The PMD allows for DPDK and the host to
>> communicate using a raw device interface on the host and in
>> the DPDK application. The device created is a Tap device with
>> a L2 packet header.
>>
>> v5 - merge in changes from list review see related emails.
>> fixed checkpatch issues and many minor edits
>> v4 - merge with latest driver changes
>> v3 - fix includes by removing ifdef for other type besides Linux.
>> Fix the copyright notice in the Makefile
>> v2 - merge all of the patches into one patch.
>> Fix a typo on naming the tap device.
>> Update the maintainers list
>>
>> Signed-off-by: Keith Wiles <keith.wiles at intel.com>
>> ---
>
> <..>
>
>> diff --git a/config/common_base b/config/common_base
>> index f5d2eff..356c631 100644
>> --- a/config/common_base
>> +++ b/config/common_base
>> @@ -592,3 +592,8 @@ CONFIG_RTE_APP_TEST_RESOURCE_TAR=n
>> CONFIG_RTE_TEST_PMD=y
>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES=n
>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS=n
>> +
>> +#
>> +# Set TAP PMD to 'n' as it is only supported in Linux for now.
>
> This comments moved to final .config and creates confusion, can we
> remove it if you don't have a strong option to keep it?
What do you mean, the statement is confusing or causes problems?
>
> <..>
>
>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>> new file mode 100644
>> index 0000000..eed81ec
>> --- /dev/null
>> +++ b/doc/guides/nics/tap.rst
>
> <..>
>
>> +.. code-block:: console
>> +
>> + The interfaced name can be changed by adding the iface=foo0
>> + e.g. --vdev=eth_tap,iface=foo0 --vdev=eth_tap,iface=foo1, ...
>
> For all file:
> %s/eth_tap/net_tap/g, there are multiple lines with this usage
Missed that one.
>
>
> <..>
>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> new file mode 100644
>> index 0000000..c13aa1b
>> --- /dev/null
>> +++ b/drivers/net/tap/rte_eth_tap.c
>
> <..>
>
>> +
>> +struct tap_info {
>> + char name[RTE_ETH_NAME_MAX_LEN]; /* Interface name supplied/given */
>> + int speed; /* Speed of interface */
>> +};
>
> This struct can go away, it is not used, since with the updated code
> rte_pmd_tap_probe() used tap_name and speed as seperate variables
> instead of struct.
>
OK.
>
> <..>
>
>> +
>> + /* If the name is different that new name as default */
>> + if (name && strcmp(name, ifr.ifr_name))
>> + snprintf(name, RTE_ETH_NAME_MAX_LEN-1, "%s", ifr.ifr_name);
>
> syntax, space around "-"
>
> <..>
>
>> +
>> +static void
>> +tap_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *tap_stats)
>> +{
>> + unsigned i, imax;
>
> checkpatch complain about not using "unsigned int?
I ran checkpatch on the patch and saw no errors reported via the
scripts/checkpatch.sh. Which checkpatch are you using here?
>
>
> <..>
>
>> +static int
>> +tap_setup_queue(struct rte_eth_dev *dev,
>> + struct pmd_internals *internals,
>> + uint16_t qid)
>> +{
>> + struct rx_queue *rx = &internals->rxq[qid];
>> + struct tx_queue *tx = &internals->txq[qid];
>> + int fd;
>> +
>> + if ((fd = rx->fd) < 0)
>> + if ((fd = tx->fd) < 0) {
>> + RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
>> + dev->data->name, qid);
>> + if ((fd = tun_alloc(dev->data->name)) < 0) {
>
> checkpatch complain about assignment in the if condition
>
>
> <..>
>
>> + /* Now get the space available for data in the mbuf */
>> + buf_size = (uint16_t) (rte_pktmbuf_data_room_size(mp) -
>
> syntax, no space after cast
>
>
> <..>
>
>> + /* Create the first Tap device */
>> + if ((fd = tun_alloc(tap_name)) < 0) {
>
> checkpatch complains about assignment in if condition
>
>> + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", dev->data->name);
>> + rte_free(pmd);
>
> rte_free(data) or goto error_exit; ?
>
>> + rte_eth_dev_release_port(dev);
>> + return -EINVAL;
>> + }
>> +
>> + /* Presetup the fds to -1 as being not working */
>> + for(i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>> + pmd->fds[i] = -1;
>> + pmd->rxq[i].fd = -1;
>> + pmd->txq[i].fd = -1;
>> + }
>> +
>> + /* Take the TUN/TAP fd and place in the first location */
>> + pmd->rxq[0].fd = fd;
>> + pmd->txq[0].fd = fd;
>> + pmd->fds[0] = fd;
>> +
>> + if (pmd_mac_address(fd, dev, &pmd->eth_addr) < 0) {
>> + rte_free(pmd);
>
> rte_free(data) or goto error_exit; ?
>
>
> <..>
>
>> +static int
>> +set_interface_name(const char *key __rte_unused,
>> + const char *value,
>> + void *extra_args)
>> +{
>> + char *name = (char *)extra_args;
>> +
>> + if (value)
>> + snprintf(name, RTE_ETH_NAME_MAX_LEN-1, "%s", value);
>
> syntax, space around "-"
>
>> + else
>> + snprintf(name, RTE_ETH_NAME_MAX_LEN-1, "%s%d",
>
> syntax, space around "-"
>
>> + DEFAULT_TAP_NAME, (tap_unit-1));
>
> syntax, space around "-"
>
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +set_interface_speed(const char *key __rte_unused,
>> + const char *value,
>> + void *extra_args)
>> +{
>> + *(int *)extra_args = (value)? atoi(value) : ETH_SPEED_NUM_10G;
>
> syntax, space around "?"
>
> <..>
>
>> + kvlist = rte_kvargs_parse(params, valid_arguments);
>> + if (kvlist) {
>> + if (rte_kvargs_count(kvlist, ETH_TAP_SPEED_ARG) == 1) {
>> + ret = rte_kvargs_process(kvlist,
>> + ETH_TAP_SPEED_ARG,
>> + &set_interface_speed,
>> + &speed);
>
> whitespace, space and tab mixed
>
>> + if (ret == -1)
>> + goto leave;
>> + }
>> +
>> + if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
>> + ret = rte_kvargs_process(kvlist,
>> + ETH_TAP_IFACE_ARG,
>> + &set_interface_name,
>> + tap_name);
>
> whitespace, space and tab mixed
>
> <..>
>
>> +static int
>> +rte_pmd_tap_remove(const char *name)
>> +{
>> + struct rte_eth_dev *eth_dev = NULL;
>> + struct pmd_internals *internals;
>> + int i;
>> +
>> + RTE_LOG(INFO, PMD, "Closing TUN/TAP Ethernet device on numa %u\n",
>> + rte_socket_id());
>> +
>> + /* find the ethdev entry */
>> + eth_dev = rte_eth_dev_allocated(name);
>
> This may cause a problem. Device created by tap_name, but searching with
> name. I suspenct this will always return NULL.
>
> <..>
>
> Thanks,
> ferru
