Re: [PATCH v6 7/9] bgmac: simplify dma init/cleanup

["Rafał Miłecki" <zajec5@gmail.com>]

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> Instead of allocating buffers at device init time and initializing
> descriptors at device open, do both at the same time (during open).
> Free all buffers when closing the device.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/net/ethernet/broadcom/bgmac.c | 70 +++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index eafdbca..aad80a7 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -557,18 +557,26 @@ static void bgmac_dma_ring_desc_free(struct bgmac *bgmac,
>                           ring->dma_base);
>  }
>
> -static void bgmac_dma_free(struct bgmac *bgmac)
> +static void bgmac_dma_cleanup(struct bgmac *bgmac)
>  {
>         int i;
>
> -       for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
> +       for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
>                 bgmac_dma_tx_ring_free(bgmac, &bgmac->tx_ring[i]);
> -               bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
> -       }
> -       for (i = 0; i < BGMAC_MAX_RX_RINGS; i++) {
> +
> +       for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
>                 bgmac_dma_rx_ring_free(bgmac, &bgmac->rx_ring[i]);
> +}
> +
> +static void bgmac_dma_free(struct bgmac *bgmac)
> +{
> +       int i;
> +
> +       for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
> +               bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
> +
> +       for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
>                 bgmac_dma_ring_desc_free(bgmac, &bgmac->rx_ring[i]);
> -       }
>  }
>
>  static int bgmac_dma_alloc(struct bgmac *bgmac)
> @@ -693,8 +690,13 @@ static void bgmac_dma_init(struct bgmac *bgmac)
>                 if (ring->unaligned)
>                         bgmac_dma_rx_enable(bgmac, ring);
>
> -               for (j = 0; j < ring->num_slots; j++)
> +               for (j = 0; j < ring->num_slots; j++) {
> +                       err = bgmac_dma_rx_skb_for_slot(bgmac, &ring->slots[j]);
> +                       if (err)
> +                               return err;

I don't think I like this logic. Instead of allowing caller to just
detect an error and handle it, you require it to do a cleanup. I think
bgmac_dma_init shouldn't leave any mess after failing.

If you e.g. call alloc_netdev and it fails, you don't care about
freeing whatever is allocated before failing.

I think you should call bgmac_dma_cleanup directly in a bgmac_dma_init
in case of error.