Re: [dpdk-dev] [PATCH v2 1/5] kni: add API to set link status on kernel interface

[Dan Gora <dg@adax.com>]

On Wed, Sep 26, 2018 at 1:42 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> There is nothing to "reflect" to the kernel interface, nor to apply to
>> the kernel interface.  This is exactly how every other kernel driver
>> works on link status changes.  There is no "netif_set_speed()'
>> function.  When a link status change occurs the kernel driver calls
>> netif_carrier_on/off() and prints a message like this one.
>
> I am not suggesting reflecting these into interface, I am just saying why do you
> print them?

Because the information is useful and because every other Ethernet
driver does the same thing when the link status changes.

See the Linux kernel:

ixgbe_watchdog_link_is_up(): drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
print_link_info(): drivers/net/ethernet/cavium/liquidio/lio_main.c
bnx2_report_link(): drivers/net/ethernet/broadcom/bnx2.c

and dozens of other examples.

Imagine you plug your 1G ethernet cable into a 10/100Mbps hub.  Yes
the link will be up, but it's the wrong speed.
Imagine you had accidentally forced your 1G connection to 10Mbps
fixed.  It will only come up at 10Mbps.  Wouldn't you like to know
that autoNeg was disabled?

> For example, "link->link_speed" this is coming as parameter to API, this API
> does nothing with this value, why print is here?

Because wouldn't you like to know if your link has connected at the
correct speed?  I would.

> I assume you are using "rte_eth_link" as parameter, instead of basic
> "link_status" to make it extendible in the feature. If so please print those
> other values when function extended, right now only link_status matters.

No, all that information matters.  If you have autoNeg disabled or are
connecting to a broken piece of equipment, the link can come up but be
at the wrong speed.

There is not much to extend this function with.  The only action we
can take, other than to print the information, is to write to the /sys
file to have the KNI kernel module call netif_carrier_on/off() for us.

>>>> @@ -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.
>>>
>>
>> Well, no it's not "wrong".  The interface has to be up to change the
>> link state, so this is a convenient place to do it.
>
> Are we agree that this function in unit test is to test packet processing part
> of KNI?
> And for example please check following coming patch:
> https://patches.dpdk.org/patch/44730/
>
> I think you want to benefit from "system(IFCONFIG TEST_KNI_PORT" up")" since
> interface needs to be up to set the link, but you can do same call, not have to
> use that one.

ok, I'll fix this.  Should I rebase the changes on top of this patch
(https://patches.dpdk.org/patch/44730/)?  Is that patch going to get
accepted soon?

>> Because then we'd have to add code to set the interface up and down
>> again, and there really is no point.  This is as good a place as any.
>> It does not affect the data transfer tests at all.
>
> Doesn't affect the data transfer, agreed, but why confusing data transfer test
> with link up/down calls? Why not have your function and set interface up and
> down again?

ok.  I was just trying to minimize the number of changes. I'll send a new patch.