On Fri, May 18 2018, Kamal Heib wrote:
> Simplify the code of allocate and cleanup RX ring resources by using
> helper functions, also make sure to free the allocated resources in
> cause of allocation failure.
>
> Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
> ---
> drivers/staging/mt7621-eth/mtk_eth_soc.c | 122 ++++++++++++++++++++-----------
> 1 file changed, 81 insertions(+), 41 deletions(-)
>
....
> static int mtk_dma_rx_alloc(struct mtk_eth *eth, struct mtk_rx_ring *ring)
> {
> - int i, pad = 0;
> + int err;
>
> ring->frag_size = mtk_max_frag_size(ETH_DATA_LEN);
> ring->rx_buf_size = mtk_max_buf_size(ring->frag_size);
> @@ -317,38 +366,23 @@ static int mtk_dma_rx_alloc(struct mtk_eth *eth, struct mtk_rx_ring *ring)
> ring->rx_data = kcalloc(ring->rx_ring_size, sizeof(*ring->rx_data),
> GFP_KERNEL);
> if (!ring->rx_data)
> - goto no_rx_mem;
> + return -ENOMEM;
Hi
I haven't tested this patch yet (will try to in the next day or so) but
it mostly looks good to me.
I would rather see the above "return -ENOMEM" as a "goto free_rx_data".
Having a single error-exit is generally more maintainable than having
lots of separate 'return's, and kfree() is quite happy to be asked to
kfree(NULL).
In fact, all of the free function (even dma_free_coherent) cope
ok will not having anything to free. As devm_kzallo() and kcalloc()
are used, all the pointers default to NULL which is safe. I would would
prefer a single 'nomem' label which did:
nomem:
dma_free_coherent(eth->dev, ring->rx_ring_size * sizeof(*ring->rx_dma),
ring->rx_dma, ring->rx_phys);
mtk_rx_free_frags(ring);
kfree(ring->rx_data);
return -ENOMEM;
But that is just my personal preference.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
