Re: [PATCH 3/3] net: stmmac: Prefer kcalloc() over kmalloc_array()

[Niklas Cassel]

(resending mail without SPAM header)

Hi Thierry

Sorry that I missed your previous email,
for some reason it got stuck in the spam filter.

Really good catch. This patch fixes the random
RX brokenness for me. Thanks!

It would be nice with a Fixes tag though,
but lately there's been so many changes,
so it might be hard to point to a single commit.

Nevertheless:

Tested-by: Niklas Cassel 

On 03/28/2017 03:57 PM, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Some of the data in the new queue structures seems to not be properly
> initialized, causing undefined behaviour (networking will work about 2
> out of 10 tries). kcalloc() will zero the allocated memory and results
> in 10 out of 10 successful boots.
> 
> Signed-off-by: Thierry Reding 
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 37 
> +++
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec5bba85c529..845320bc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1412,13 +1412,12 @@ static void free_dma_desc_resources(struct 
> stmmac_priv *priv)
>  static int alloc_rx_dma_desc_resources(struct stmmac_priv *priv)
>  {
>   u32 rx_count = priv->plat->rx_queues_to_use;
> + struct stmmac_rx_queue *rx_q;
>   int ret = -ENOMEM;
>   u32 queue = 0;
>  
>   /* Allocate RX queues array */
> - priv->rx_queue = kmalloc_array(rx_count,
> -sizeof(struct stmmac_rx_queue),
> -GFP_KERNEL);
> + priv->rx_queue = kcalloc(rx_count, sizeof(*rx_q), GFP_KERNEL);
>   if (!priv->rx_queue) {
>   kfree(priv->rx_queue);
>   return -ENOMEM;
> @@ -1426,20 +1425,19 @@ static int alloc_rx_dma_desc_resources(struct 
> stmmac_priv *priv)
>  
>   /* RX queues buffers and DMA */
>   for (queue = 0; queue < rx_count; queue++) {
> - struct stmmac_rx_queue *rx_q = >rx_queue[queue];
> + rx_q = >rx_queue[queue];
>  
>   rx_q->queue_index = queue;
>   rx_q->priv_data = priv;
>  
> - rx_q->rx_skbuff_dma = kmalloc_array(DMA_RX_SIZE,
> - sizeof(dma_addr_t),
> - GFP_KERNEL);
> + rx_q->rx_skbuff_dma = kcalloc(DMA_RX_SIZE, sizeof(dma_addr_t),
> +   GFP_KERNEL);
>   if (!rx_q->rx_skbuff_dma)
>   goto err_dma_buffers;
>  
> - rx_q->rx_skbuff = kmalloc_array(DMA_RX_SIZE,
> - sizeof(struct sk_buff *),
> - GFP_KERNEL);
> + rx_q->rx_skbuff = kcalloc(DMA_RX_SIZE,
> +   sizeof(struct sk_buff *),
> +   GFP_KERNEL);
>   if (!rx_q->rx_skbuff)
>   goto err_dma_buffers;
>  
> @@ -1477,33 +1475,32 @@ static int alloc_rx_dma_desc_resources(struct 
> stmmac_priv *priv)
>  static int alloc_tx_dma_desc_resources(struct stmmac_priv *priv)
>  {
>   u32 tx_count = priv->plat->tx_queues_to_use;
> + struct stmmac_tx_queue *tx_q;
>   int ret = -ENOMEM;
>   u32 queue = 0;
>  
>   /* Allocate TX queues array */
> - priv->tx_queue = kmalloc_array(tx_count,
> -sizeof(struct stmmac_tx_queue),
> -GFP_KERNEL);
> + priv->tx_queue = kcalloc(tx_count, sizeof(*tx_q), GFP_KERNEL);
>   if (!priv->tx_queue)
>   return -ENOMEM;
>  
>   /* TX queues buffers and DMA */
>   for (queue = 0; queue < tx_count; queue++) {
> - struct stmmac_tx_queue *tx_q = >tx_queue[queue];
> + tx_q = >tx_queue[queue];
>  
>   tx_q->queue_index = queue;
>   tx_q->priv_data = priv;
>  
> - tx_q->tx_skbuff_dma = kmalloc_array(DMA_TX_SIZE,
> -   sizeof(struct stmmac_tx_info),
> -   GFP_KERNEL);
> + tx_q->tx_skbuff_dma = kcalloc(DMA_TX_SIZE,
> +   sizeof(struct stmmac_tx_info),
> +   GFP_KERNEL);
>  
>   if (!tx_q->tx_skbuff_dma)
>   goto err_dma_buffers;
>  
> - tx_q->tx_skbuff = kmalloc_array(DMA_TX_SIZE,
> - sizeof(struct sk_buff *),
> - GFP_KERNEL);
> + tx_q->tx_skbuff = kcalloc(DMA_TX_SIZE,
> +   sizeof(struct sk_buff *),
> 

[PATCH 3/3] net: stmmac: Prefer kcalloc() over kmalloc_array()

[Thierry Reding]

From: Thierry Reding 

Some of the data in the new queue structures seems to not be properly
initialized, causing undefined behaviour (networking will work about 2
out of 10 tries). kcalloc() will zero the allocated memory and results
in 10 out of 10 successful boots.

Signed-off-by: Thierry Reding 
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 37 +++
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ec5bba85c529..845320bc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1412,13 +1412,12 @@ static void free_dma_desc_resources(struct stmmac_priv 
*priv)
 static int alloc_rx_dma_desc_resources(struct stmmac_priv *priv)
 {
u32 rx_count = priv->plat->rx_queues_to_use;
+   struct stmmac_rx_queue *rx_q;
int ret = -ENOMEM;
u32 queue = 0;
 
/* Allocate RX queues array */
-   priv->rx_queue = kmalloc_array(rx_count,
-  sizeof(struct stmmac_rx_queue),
-  GFP_KERNEL);
+   priv->rx_queue = kcalloc(rx_count, sizeof(*rx_q), GFP_KERNEL);
if (!priv->rx_queue) {
kfree(priv->rx_queue);
return -ENOMEM;
@@ -1426,20 +1425,19 @@ static int alloc_rx_dma_desc_resources(struct 
stmmac_priv *priv)
 
/* RX queues buffers and DMA */
for (queue = 0; queue < rx_count; queue++) {
-   struct stmmac_rx_queue *rx_q = >rx_queue[queue];
+   rx_q = >rx_queue[queue];
 
rx_q->queue_index = queue;
rx_q->priv_data = priv;
 
-   rx_q->rx_skbuff_dma = kmalloc_array(DMA_RX_SIZE,
-   sizeof(dma_addr_t),
-   GFP_KERNEL);
+   rx_q->rx_skbuff_dma = kcalloc(DMA_RX_SIZE, sizeof(dma_addr_t),
+ GFP_KERNEL);
if (!rx_q->rx_skbuff_dma)
goto err_dma_buffers;
 
-   rx_q->rx_skbuff = kmalloc_array(DMA_RX_SIZE,
-   sizeof(struct sk_buff *),
-   GFP_KERNEL);
+   rx_q->rx_skbuff = kcalloc(DMA_RX_SIZE,
+ sizeof(struct sk_buff *),
+ GFP_KERNEL);
if (!rx_q->rx_skbuff)
goto err_dma_buffers;
 
@@ -1477,33 +1475,32 @@ static int alloc_rx_dma_desc_resources(struct 
stmmac_priv *priv)
 static int alloc_tx_dma_desc_resources(struct stmmac_priv *priv)
 {
u32 tx_count = priv->plat->tx_queues_to_use;
+   struct stmmac_tx_queue *tx_q;
int ret = -ENOMEM;
u32 queue = 0;
 
/* Allocate TX queues array */
-   priv->tx_queue = kmalloc_array(tx_count,
-  sizeof(struct stmmac_tx_queue),
-  GFP_KERNEL);
+   priv->tx_queue = kcalloc(tx_count, sizeof(*tx_q), GFP_KERNEL);
if (!priv->tx_queue)
return -ENOMEM;
 
/* TX queues buffers and DMA */
for (queue = 0; queue < tx_count; queue++) {
-   struct stmmac_tx_queue *tx_q = >tx_queue[queue];
+   tx_q = >tx_queue[queue];
 
tx_q->queue_index = queue;
tx_q->priv_data = priv;
 
-   tx_q->tx_skbuff_dma = kmalloc_array(DMA_TX_SIZE,
- sizeof(struct stmmac_tx_info),
- GFP_KERNEL);
+   tx_q->tx_skbuff_dma = kcalloc(DMA_TX_SIZE,
+ sizeof(struct stmmac_tx_info),
+ GFP_KERNEL);
 
if (!tx_q->tx_skbuff_dma)
goto err_dma_buffers;
 
-   tx_q->tx_skbuff = kmalloc_array(DMA_TX_SIZE,
-   sizeof(struct sk_buff *),
-   GFP_KERNEL);
+   tx_q->tx_skbuff = kcalloc(DMA_TX_SIZE,
+ sizeof(struct sk_buff *),
+ GFP_KERNEL);
if (!tx_q->tx_skbuff)
goto err_dma_buffers;
 
-- 
2.12.0