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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 14 Apr 2015 09:24:35 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Felix Fietkau <nbd@...nwrt.org>
Cc:	Network Development <netdev@...r.kernel.org>,
	Hauke Mehrtens <hauke@...ke-m.de>
Subject: Re: [PATCH v6 7/9] bgmac: simplify dma init/cleanup

On 14 April 2015 at 01:42, Felix Fietkau <nbd@...nwrt.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@...nwrt.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html