gnrc_netif: handle NETDEV_EVENT_LINK_UP/DOWN events

[benpicco]

Contribution description

If an interface goes offline we can to stop sending router solicitations on it.
But more importantly, if the interface gets online again, we need to send out a router solicitation immediately to get a fresh address.

Testing procedure

This was tested with a atwinc15x0 WiFi module. When it connects to a WiFi network it generates a NETDEV_EVENT_LINK_UP event, when the WiFi is no longer connected a NETDEV_EVENT_LINK_DOWN event is generated.

I used two custom functions to manage the netif state, but ifconfig should work too:

static void _netup(gnrc_netif_t *netif)
{
    netopt_state_t state = NETOPT_STATE_IDLE;
    while (gnrc_netapi_set(netif->pid, NETOPT_STATE, 0, &state, sizeof(state)) == -EBUSY) {}
}

static void _drop_global_addr(gnrc_netif_t *netif)
{
    ipv6_addr_t addr[CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF];
    int num = gnrc_netif_ipv6_addrs_get(netif, addr, sizeof(addr));
    for (int i = 0; i < num; ++i) {
        if (ipv6_addr_is_global(&addr[i])) {
            gnrc_netif_ipv6_addr_remove(netif, &addr[i]);
        }
    }
}

static void _netdown(gnrc_netif_t *netif)
{
    netopt_state_t state = NETOPT_STATE_SLEEP;
    while (gnrc_netapi_set(netif->pid, NETOPT_STATE, 0, &state, sizeof(state)) == -EBUSY) {}

    _drop_global_addr(netif);
}

Issues/PRs references

[jia200x]

I don't have hardware to test, but the approach seems reasonable to me.

[benpicco]

Currently the event is implemented by

• encx24j600
• atwinc15x0
• stm32_eth
• esp-eth
• esp_wifi

[maribu]

Code looks good to me. @fabian18 has recently dived deep into the NDP code, maybe he could also talk a quick look, as I'm not that familiar with the code.

[miri64]

LGTM, but @benpicco offline told me that sometimes the prefix is not received, regardless, after the interface is brought up. If that could be debugged (or at least some sniffing output to guess the problem) before this is merged, that would be nice.

[miri64]

If that could be debugged (or at least some sniffing output to guess the problem) before this is merged, that would be nice.

I meant with that primarily that it should be ensured that this problem is not introduced by this PR. If this is a general problem or if this PR only makes the cause of a bigger symptom more obvious, then I am fine with merging this PR as is.

[fabian18]

RIOT/core/msg.c

Lines 274 to 276 in 7ee1b58

	int msg_send_receive(msg_t *m, msg_t *reply, kernel_pid_t target_pid)
	{
	assert(thread_getpid() != target_pid);

This assertion is triggered, after the call to

netif_get_opt(&netif->netif, NETOPT_LINK, 0, &link, sizeof(netopt_enable_t));

,for me at least.

[fabian18]

What is still needed here?

I would wish that addresses are auto (re-)configured in gnrc_ipv6_nib_iface_up().

[benpicco]

What about addresses that were configured manually by the application?

[fabian18]

They are lost. The application must take care of that, I think.
If I add an address to an interface in Linux and put the interface down and up again, the manually configured address is gone.

[benpicco]

The problem is that without #17902 we don't have any way to notify the application that the interface is up again. With WiFi we could then just lose a manually configured address if the node gets out of range for some time.

[fabian18]

So the status in this PR is, as far as I understand. You would rather keep the addresses of an interface during a link restart, until there is a way to notify an application that a link has restarted, right? And if this PR is merged, your plan is to add address reconfiguration on link up and address dropping on link down in #17902? I was hoping to see the correct behavior in this PR already. But... if you prefer to keep it that way I think this PR is about ready. I don´t want to ACK a PR in a hurry, so let me go through this one more time in the evening or at latest tomorrow.

[benpicco]

Yes, I try not to do so many things at once in a PR.
This one already

• makes NETDEV_EVENT_LINK_UP mandatory
• makes router advertisements depend on NETDEV_EVENT_LINK_UP/NETDEV_EVENT_LINK_DOWN events

Dropping addresses/generating events to the application would be another behavior change that I'd rather keep separate.
We are not dropping addresses now and it's not immediately clear what side effects that will have, so one step at a time.

[fabian18]

I searched the code base for netdev_driver_t , and i think there are some more netdevs to be adjusted.

For netdev_test.c and nimble_netif.c I am not sure whether they must be adjusted because they do not call netdev->event_callback() at all. My guess is that they don´t have to be adjusted.

[fabian18]

I searched the code base for netdev_driver_t , and i think there are some more netdevs to be adjusted.

Oh sorry those are the netdevs which support NETDEV_EVENT_LINK_*. Ignore my last comment.

[fabian18]

Ok but cdc_ecm_netdev does not support NETDEV_EVENT_LINK_*. What about that?

[benpicco]

Thank you, missed that one.

[fabian18]

ACK

[benpicco]

I had to rebase to make static checks happy - spell checks have been fixed in a different PR.

[kaspar030]

I think this broke tests/gnrc_ipv6_nib samr21-xpro.

[benpicco]

But… how?
The test runs fine on native and does not contain anything hardware dependent.

[kaspar030]

main(): This is RIOT! (Version: 2022.10-devel-658-g6ac0bb-HEAD)                                                                                    
.........                                                                                                                                          
tests.test_get_next_hop_l2addr__link_local_after_handshake_iface (tests/gnrc_ipv6_nib/main.c 277) exp 6 was 4                                      
.                                                                                                                                                  
tests.test_get_next_hop_l2addr__link_local_after_handshake_iface_router (tests/gnrc_ipv6_nib/main.c 277) exp 6 was 4                               
.........................................                                                                                                          

[benpicco]

Huh this is weird - why does this not fail on native? I also see this on samd20-xpro - fix provided in #18635

[jia200x]

This PR broke the LoRa tests :/ (see https://forum.riot-os.org/t/lora-e5-dev-any-working-lora-examples/3755/13).