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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/26/2018 7:56 PM, Dan Gora wrote:
> 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.

It would be useful if it writes the values of virtual interface, but this API
prints user input.

The virtual interface may have different value, this API doesn't change anything
related other than link status, so why print user provided value.

Won't you think it will be confusing if the virtual interface values are
different than what printed.
Or won't user will think API changed those values to printed one in interface?

> 
> 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.
>