Re: [UNIKRAFT/LIBLWIP PATCH v2 2/3] lib/lwip: Fetch features supported on the netdev

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

thanks for this patch. Here too, a little bit more documentation would
be nice. Comments inline.

regards,
Hugo

On Tue, 2020-06-30 at 13:15 +0200, Sharan Santhanam wrote:
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> ---
>  uknetdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/uknetdev.c b/uknetdev.c
> index 7047352..ef57014 100644
> --- a/uknetdev.c
> +++ b/uknetdev.c
> @@ -62,6 +62,10 @@
>  #define UKNETDEV_NETIF_NAME0 'e'
>  #define UKNETDEV_NETIF_NAME1 'n'
>  
> +struct lwip_netdev_data {
> +     uint32_t features;
> +};
> +
>  /*
>   * Global headroom settings for buffer allocations used on receive
>   * and transmit. We are taking the maximum of all uknetdev devices
> as
> @@ -372,6 +376,7 @@ err_t uknetdev_init(struct netif *nf)
>       struct uk_netdev_rxqueue_conf rxq_conf;
>       struct uk_netdev_txqueue_conf txq_conf;
>       struct uk_netdev_info info;
> +     struct lwip_netdev_data *lwip_data;
>       const struct uk_hwaddr *hwaddr;
>       unsigned int i;
>       int ret;
> @@ -379,6 +384,11 @@ err_t uknetdev_init(struct netif *nf)
>       UK_ASSERT(nf);
>       dev = netif_to_uknetdev(nf);
>       UK_ASSERT(dev);
> +#if CONFIG_UK_NETDEV_SCRATCH_SIZE < CONFIG_LWIP_UKNETDEV_SCRATCH
> +#error "Insufficient Scratch memory"

This is not supposed to happen, right? Having a comment about this here
would be nice:

/* This should not happen. CONFIG_UK_NETDEV_SCRATCH_SIZE is configured as the 
max of all scratch pad requirements by the Makefile macro uknetdev_scratch_mem. 
*/

> +#endif
> +
> +     lwip_data = (struct lwip_netdev_data *)dev->scratch_pad;

I don't really like the idea that an outdated
CONFIG_LWIP_UKNETDEV_SCRATCH could lead to insufficient scratch pad
size. This is going to create a lot of trouble if users rely on
outdated config data (this is a realistic case, right?).

What about a compile time guard that checks the size of struct
lwip_netdev_data and makes sure that CONFIG_LWIP_UKNETDEV_SCRATCH is
big enough?

In any case, there should be a comment about this here I think. :-)

>  
>       LWIP_ASSERT("uknetdev needs an input callback (netif_input
> or tcpip_input)",
>                   nf->input != NULL);
> @@ -411,6 +421,7 @@ err_t uknetdev_init(struct netif *nf)
>       uk_netdev_info_get(dev, &info);
>       if (!info.max_rx_queues || !info.max_tx_queues)
>               return ERR_IF;
> +     lwip_data->features = info.features;
>  
>       /*
>        * Update our global (rx|tx)_headroom setting that we use
> for