Ethernet driver deinitialization (IDFGH-12421)

[mickeyl]

Following up on a message by @kostaond in #5697 (comment) where we discussed a missing deinit for the bridge example, I want to help getting this fixed.

To simplify things, I first want to get this working without the bridge functionality. Inversing the following init:

    // Create Ethernet interface with appropriate configuration
    auto netif = esp_netif_new(&cfg);

    // Attach Ethernet driver to TCP/IP stack
    ESP_ERROR_CHECK(esp_netif_attach(netif, esp_eth_new_netif_glue(eth_handles[0])));

    // Register user defined event handers
    ESP_ERROR_CHECK(esp_event_handler_register(ETH_EVENT, ESP_EVENT_ANY_ID, &eth_event_handler, NULL));
    ESP_ERROR_CHECK(esp_event_handler_register(IP_EVENT, IP_EVENT_ETH_GOT_IP, &got_ip_event_handler, NULL));

    // Start Ethernet driver state machine
    for (int i = 0; i < eth_port_cnt; i++) {
        ESP_ERROR_CHECK(esp_eth_start(eth_handles[i]));
    }

I figured I'd start w/

    // Stop Ethernet driver state machine
    for (int i = 0; i < eth_port_cnt; i++) {
        ESP_ERROR_CHECK(esp_eth_stop(eth_handles[i]));
    }

    // Unregister user defined event handlers
    esp_event_handler_unregister(ETH_EVENT, ESP_EVENT_ANY_ID, &eth_event_handler);
    esp_event_handler_unregister(IP_EVENT, IP_EVENT_ETH_GOT_IP, &got_ip_event_handler);

    // Delete Ethernet driver glue
    esp_eth_del_netif_glue((esp_eth_netif_glue_handle_t)netif->driver_handle);

    // Destroy Ethernet interface
    esp_netif_destroy(netif);

    // Destroy Ethernet drivers (technically part of the glue, but the glue is a private struct)
    example_eth_deinit(eth_handles, eth_port_cnt);
}

But this crashes in esp_eth_del_netif_glue during esp_eth_decrease like that:

[mickeyl]

Debugging more into this, it seems I can't get the glue reference via (esp_eth_netif_glue_handle_t)netif->driver_handle), because in esp_netif_attach the driver_handle seems to get rewritten by the means of a post_attach callback.

Storing the glue works (although it's a bit unfortunate that we have to story so many things, if they can be get by traversing the structures), but the next problem is then in esp_eth:272. The code is

    ESP_GOTO_ON_ERROR(phy->deinit(phy), err, TAG, "deinit phy failed");
    ESP_GOTO_ON_ERROR(mac->deinit(mac), err, TAG, "deinit mac failed");
    free(eth_driver);

however this doesn't lead to (in my case) w5500_del being called. And without this being called, I can't free the SPI bus. So the next question would be where/when is the proper chance to call this?

[mickeyl]

Augmenting the deinit sequence in esp_eth.c with

    ESP_GOTO_ON_ERROR(phy->deinit(phy), err, TAG, "deinit phy failed");
    ESP_GOTO_ON_ERROR(phy->del(phy), err, TAG, "del phy failed"); // new
    ESP_GOTO_ON_ERROR(mac->deinit(mac), err, TAG, "deinit mac failed");
    ESP_GOTO_ON_ERROR(mac->del(mac), err, TAG, "del mac failed"); // new

Together with the following new function in the ethernet_init component:

void example_eth_deinit(esp_eth_handle_t *eth_handles, uint8_t eth_cnt) {

    for (int i = 0; i < eth_cnt; i++) {
        esp_eth_driver_uninstall(eth_handles[i]);
    }
    free(eth_handles);

#ifdef CONFIG_EXAMPLE_USE_SPI_ETHERNET
    spi_bus_free(CONFIG_EXAMPLE_ETH_SPI_HOST);
#endif
}

seems to be a step in the right direction. I can bring up / bring down Ethernet a couple of times, before it crashes in dhcpserver.c:991 (parse_msg):

                pdhcps_pool = pback_node->pnode;

Is it possible that the DHCP server does not get properly shutdown, hence multiple servers are stepping on its others toes?

[mickeyl]

Ok, stopping the DHCP server manually before stopping the state machine on the drivers seems to be the missing link.

I can now bring down and bring up the Ethernet example multiple times -- seemingly without any memory leaks.

Before tackling the more complicated bridge example, I'd like to ask how we can proceed from here. It looks like there is little missing to make this work reliably, in particular calling del in the phy and mac and adding the aforementioned example_eth_deinit in the component.

Is there a special reason why the esp_eth_driver_uninstall does not already call the corresponding del functions?

[kostaond]

Is there a special reason why the esp_eth_driver_uninstall does not already call the corresponding del functions?

Yes, mac and phy were created (and resources allocated) by separated function call, not by esp_eth_driver_install. Therefore it's not job of esp_eth_driver_uninstall.

I'm currently in process of Ethernet driver update. The current issue with using example_eth_init is that you (user) lost reference to mac and phy instances and so you cannot delete them. This can be easily overcome as demonstrated by Ethernet init Component. However, I don't like that solution much when considering eth_hanler keeps the reference to both instances. Hence, I'm planning to update esp_eth driver to provide getter functions for these two instances. Along with esp_eth update, I'll update deinit sequence for both Ethernet init example/component.

[mickeyl]

Very good, thanks! I'll keep the aforementioned diff in my fork for now, but will wait for a proper solution then. And ­– perhaps ­– if I'm not asking too much… it would be great, if you could also extend the deinit to the bridge example ­– which has a lot more glue to do.

[KaeLL]

That's good to hear. I also tried my hand at deiniting, but gave up because I wasn't in much need of it, and because esp_netif itself can't to this day be deinitialized.

Since you touched on the subject of update/refactor of the esp_eth component, there's something I'd like to ask risking hijacking the issue. The W5100 doesn't expose its PHY by means of registers, only pins, which realistically, just get connected to GND or Vcc instead of traced elsewhere. Would it make sense to make the PHY layer implementation optional, either by having some default implementation stubs or something? This function alone is the only reason I have to implement this component. Of course, I'm assuming there are other SPI Ethernet uC's out there that don't expose their PHY in a useful way like this one, but I could be wrong, and this request may make no sense, so let me know.

[kostaond]

@KaeLL would espressif/esp-eth-drivers#21 help you solve your problem? The original intention of the PR was different, but it's Dummy PHY and it sounds exactly you need. If it helps, I'll update the README with your use case :)

[KaeLL]

I skimmed over it and seems like that is what I was looking for. I'll take a deeper look the next time I update my code.