[2/2] kni: set default carrier state to 'off'

Message ID 20180911232906.18352-3-dg@adax.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series kni: add API to set link status on kernel interface |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Dan Gora Sept. 11, 2018, 11:29 p.m. UTC
  Set the carrier state to 'off' when the interface is instantiated
or when it is marked 'up' or 'down'.  This is necessary to set the
interface to a known operational state until the carrier state is
changed with rte_kni_update_link().

Signed-off-by: Dan Gora <dg@adax.com>
---
 kernel/linux/kni/kni_misc.c | 2 ++
 kernel/linux/kni/kni_net.c  | 2 ++
 2 files changed, 4 insertions(+)
  

Comments

Ferruh Yigit Sept. 18, 2018, 4:15 p.m. UTC | #1
On 9/12/2018 12:29 AM, Dan Gora wrote:
> Set the carrier state to 'off' when the interface is instantiated
> or when it is marked 'up' or 'down'.  This is necessary to set the
> interface to a known operational state until the carrier state is
> changed with rte_kni_update_link().

Why setting to no-carrier mode by default? This will change the behavior of
interfaces and may effect others. And indeed I didn't get why this is required?

> 
> Signed-off-by: Dan Gora <dg@adax.com>
> ---
>  kernel/linux/kni/kni_misc.c | 2 ++
>  kernel/linux/kni/kni_net.c  | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index fa69f8e63..45649499d 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -466,6 +466,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>  		return -ENODEV;
>  	}
>  
> +	netif_carrier_off(net_dev);
> +
>  	ret = kni_run_thread(knet, kni, dev_info.force_bind);
>  	if (ret != 0)
>  		return ret;
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 7fcfa106c..1f8ba0700 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -133,6 +133,7 @@ kni_net_open(struct net_device *dev)
>  	struct kni_dev *kni = netdev_priv(dev);
>  
>  	netif_start_queue(dev);
> +	netif_carrier_off(dev);
>  
>  	memset(&req, 0, sizeof(req));
>  	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;
> @@ -152,6 +153,7 @@ kni_net_release(struct net_device *dev)
>  	struct kni_dev *kni = netdev_priv(dev);
>  
>  	netif_stop_queue(dev); /* can't transmit any more */
> +	netif_carrier_off(dev);
>  
>  	memset(&req, 0, sizeof(req));
>  	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;
>
  
Ferruh Yigit Sept. 18, 2018, 4:48 p.m. UTC | #2
On 9/18/2018 5:15 PM, Ferruh Yigit wrote:
> On 9/12/2018 12:29 AM, Dan Gora wrote:
>> Set the carrier state to 'off' when the interface is instantiated
>> or when it is marked 'up' or 'down'.  This is necessary to set the
>> interface to a known operational state until the carrier state is
>> changed with rte_kni_update_link().
> 
> Why setting to no-carrier mode by default? This will change the behavior of
> interfaces and may effect others. And indeed I didn't get why this is required?

I just read the other thread, including Igor's and your comment about starting
the interface down, overall I got your point but my concerns is if someone has a
solution based on assumption that interface will be up when created will be
affected.

> 
>>
>> Signed-off-by: Dan Gora <dg@adax.com>
>> ---
>>  kernel/linux/kni/kni_misc.c | 2 ++
>>  kernel/linux/kni/kni_net.c  | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
>> index fa69f8e63..45649499d 100644
>> --- a/kernel/linux/kni/kni_misc.c
>> +++ b/kernel/linux/kni/kni_misc.c
>> @@ -466,6 +466,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>>  		return -ENODEV;
>>  	}
>>  
>> +	netif_carrier_off(net_dev);
>> +
>>  	ret = kni_run_thread(knet, kni, dev_info.force_bind);
>>  	if (ret != 0)
>>  		return ret;
>> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
>> index 7fcfa106c..1f8ba0700 100644
>> --- a/kernel/linux/kni/kni_net.c
>> +++ b/kernel/linux/kni/kni_net.c
>> @@ -133,6 +133,7 @@ kni_net_open(struct net_device *dev)
>>  	struct kni_dev *kni = netdev_priv(dev);
>>  
>>  	netif_start_queue(dev);
>> +	netif_carrier_off(dev);
>>  
>>  	memset(&req, 0, sizeof(req));
>>  	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;
>> @@ -152,6 +153,7 @@ kni_net_release(struct net_device *dev)
>>  	struct kni_dev *kni = netdev_priv(dev);
>>  
>>  	netif_stop_queue(dev); /* can't transmit any more */
>> +	netif_carrier_off(dev);
>>  
>>  	memset(&req, 0, sizeof(req));
>>  	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;
>>
>
  
Dan Gora Sept. 18, 2018, 6:34 p.m. UTC | #3
On Tue, Sep 18, 2018 at 1:48 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> I just read the other thread, including Igor's and your comment about starting
> the interface down, overall I got your point but my concerns is if someone has a
> solution based on assumption that interface will be up when created will be
> affected.
>

The carrier state of the interface does not come up in the "up" state
currently.  It comes up either in the "down" state or in the "unknown"
state, which is really the worst of all worlds..

RHEL/Centos 7.5 running 'test->kni_autotest'

6: test_kni_port: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1450 qdisc
pfifo_fast state UNKNOWN group default qlen 1000
   link/ether ca:40:e8:72:68:29 brd ff:ff:ff:ff:ff:ff
   inet6 fe80::691c:755e:5257:83af/64 scope link noprefixroute
      valid_lft forever preferred_lft forever

-dan
  

Patch

diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index fa69f8e63..45649499d 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -466,6 +466,8 @@  kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 		return -ENODEV;
 	}
 
+	netif_carrier_off(net_dev);
+
 	ret = kni_run_thread(knet, kni, dev_info.force_bind);
 	if (ret != 0)
 		return ret;
diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index 7fcfa106c..1f8ba0700 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -133,6 +133,7 @@  kni_net_open(struct net_device *dev)
 	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_start_queue(dev);
+	netif_carrier_off(dev);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;
@@ -152,6 +153,7 @@  kni_net_release(struct net_device *dev)
 	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_stop_queue(dev); /* can't transmit any more */
+	netif_carrier_off(dev);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;