From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH v2 1/5] kni: add API to set link status on
 kernel interface
Date: Wed, 26 Sep 2018 14:59:23 +0100
Message-ID: <671135e5-a666-4254-c5c6-672c3863146b@intel.com>
References: <20180911232906.18352-1-dg@adax.com>
 <20180919195549.5585-1-dg@adax.com> <20180919195549.5585-2-dg@adax.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: Igor Ryzhov <iryzhov@nfware.com>,
 Stephen Hemminger <stephen@networkplumber.org>
To: Dan Gora <dg@adax.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
Received: from mga05.intel.com (mga05.intel.com [192.55.52.43])
 by dpdk.org (Postfix) with ESMTP id 01AE31B171
 for <dev@dpdk.org>; Wed, 26 Sep 2018 15:59:27 +0200 (CEST)
In-Reply-To: <20180919195549.5585-2-dg@adax.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 9/19/2018 8:55 PM, Dan Gora wrote:
> Add a new API function to KNI, rte_kni_update_link() to allow DPDK
> applications to update the link status for KNI network interfaces in
> the linux kernel.
> 
> Signed-off-by: Dan Gora <dg@adax.com>

<...>

> +int __rte_experimental
> +rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link)
> +{
> +	char path[64];
> +	char carrier[2];
> +	const char *new_carrier;
> +	int fd, ret;
> +
> +	if (kni == NULL || link == NULL)
> +		return -1;
> +
> +	snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
> +		kni->name);
> +
> +	fd = open(path, O_RDWR);
> +	if (fd == -1) {
> +		RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
> +		return -1;
> +	}
> +
> +	ret = read(fd, carrier, 2);
> +	if (ret < 1) {
> +		/* Cannot read carrier until interface is marked
> +		 * 'up', so don't log an error.
> +		 */
> +		close(fd);
> +		return -1;
> +	}
> +
> +	new_carrier = (link->link_status == ETH_LINK_UP) ? "1" : "0";
> +	ret = write(fd, new_carrier, 1);
> +	if (ret < 1) {
> +		RTE_LOG(ERR, KNI, "Failed to write file: %s.\n", path);
> +		close(fd);
> +		return -1;
> +	}
> +
> +	if (strncmp(carrier, new_carrier, 1)) {
> +		if (link->link_status == ETH_LINK_UP) {
> +			RTE_LOG(INFO, KNI, "%s NIC Link is Up %d Mbps %s %s.\n",
> +				kni->name,
> +				link->link_speed,
> +				link->link_autoneg ? "(AutoNeg)" : "(Fixed)",
> +				link->link_duplex ?
> +					"Full Duplex" : "Half Duplex");

These link values are coming from user and not reflected to the kni interface,
printing them here can cause a misunderstanding that they have been applied.
I think only link status should be printed here.

<...>

> @@ -148,9 +239,16 @@ test_kni_loop(__rte_unused void *arg)
>  			ret = -1;
>  		if (system(IFCONFIG TEST_KNI_PORT" mtu 1400") == -1)
>  			ret = -1;
> +
> +		ret = kni_change_link();
> +		if (ret != 0) {
> +			test_kni_processing_flag = 1;
> +			return ret;
> +		}

I thinks this is wrong place to call kni_change_link(), this is test_kni_loop()
created by test_kni_processing() that does packet processing tests.

I think better to call directly from test_kni(), perhaps before
test_kni_processing() call?