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 10:59 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 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.
>

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.

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

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

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.

-d