* re: mvpp2: XDP TX support
@ 2020-07-06 13:59 Colin Ian King
2020-07-06 15:28 ` Matteo Croce
0 siblings, 1 reply; 3+ messages in thread
From: Colin Ian King @ 2020-07-06 13:59 UTC (permalink / raw)
To: Matteo Croce, Sven Auhagen
Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel
Hi,
Static analysis with Coverity has found a potential issue in the
following commit:
commit c2d6fe6163de80d7f7cf400ee351f56d6cdb7a5a
Author: Matteo Croce <mcroce@microsoft.com>
Date: Thu Jul 2 16:12:43 2020 +0200
mvpp2: XDP TX support
In source drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c in function
mvpp2_check_pagepool_dma, analysis is as follows:
4486 if (!priv->percpu_pools)
4487 return err;
4488
CID (#1 of 1): Array compared against 0 (NO_EFFECT)
array_null: Comparing an array to null is not useful: priv->page_pool,
since the test will always evaluate as true.
Was priv->page_pool formerly declared as a pointer?
4489 if (!priv->page_pool)
4490 return -ENOMEM;
4491
page_pool is declared as:
struct page_pool *page_pool[MVPP2_PORT_MAX_RXQ];
..it is an array and hence cannot be null, so the null check is
redundant. Later on there is a reference of priv->page_pool[0], so was
the check meant to be:
if (!priv->page_pool[0])
Colin
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: mvpp2: XDP TX support 2020-07-06 13:59 mvpp2: XDP TX support Colin Ian King @ 2020-07-06 15:28 ` Matteo Croce 2020-07-06 15:32 ` Colin Ian King 0 siblings, 1 reply; 3+ messages in thread From: Matteo Croce @ 2020-07-06 15:28 UTC (permalink / raw) To: Colin Ian King, Sven Auhagen Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel On Mon, 6 Jul 2020 14:59:22 +0100 Colin Ian King <colin.king@canonical.com> wrote: > Hi, > > Static analysis with Coverity has found a potential issue in the > following commit: > > commit c2d6fe6163de80d7f7cf400ee351f56d6cdb7a5a > Author: Matteo Croce <mcroce@microsoft.com> > Date: Thu Jul 2 16:12:43 2020 +0200 > > mvpp2: XDP TX support > > > In source drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c in function > mvpp2_check_pagepool_dma, analysis is as follows: > > > 4486 if (!priv->percpu_pools) > 4487 return err; > 4488 > CID (#1 of 1): Array compared against 0 (NO_EFFECT) > array_null: Comparing an array to null is not useful: priv->page_pool, > since the test will always evaluate as true. > > Was priv->page_pool formerly declared as a pointer? > > 4489 if (!priv->page_pool) > 4490 return -ENOMEM; > 4491 > > > page_pool is declared as: > > struct page_pool *page_pool[MVPP2_PORT_MAX_RXQ]; > > ..it is an array and hence cannot be null, so the null check is > redundant. Later on there is a reference of priv->page_pool[0], so > was the check meant to be: > > if (!priv->page_pool[0]) > > Colin Hi, yes, the check was meant to be 'if (!priv->page_pool[0])'. Maybe it's a copy/paste error from other points where 'page_pool' is a local variable. While at it, I've found that in case a page_pool allocation fails, I don't cleanup the previously allocated pools, and upon deallocation the pointer isn't set back to NULL. I should add something like: @@ -548,8 +548,10 @@ static int mvpp2_bm_pool_destroy(struct device *dev, struct mvpp2 *priv, val |= MVPP2_BM_STOP_MASK; mvpp2_write(priv, MVPP2_BM_POOL_CTRL_REG(bm_pool->id), val); - if (priv->percpu_pools) + if (priv->percpu_pools) { page_pool_destroy(priv->page_pool[bm_pool->id]); + priv->page_pool[bm_pool->id] = NULL; + } dma_free_coherent(dev, bm_pool->size_bytes, bm_pool->virt_addr, @@ -609,8 +611,15 @@ static int mvpp2_bm_init(struct device *dev, struct mvpp2 *priv) mvpp2_pools[pn].buf_num, mvpp2_pools[pn].pkt_size, dma_dir); - if (IS_ERR(priv->page_pool[i])) - return PTR_ERR(priv->page_pool[i]); + if (IS_ERR(priv->page_pool[i])) { + err = PTR_ERR(priv->page_pool[i]); + + for (i--; i >=0; i--) { + page_pool_destroy(priv->page_pool[i]); + priv->page_pool[i] = NULL; + } + return err; + } } } Looks sane to you? Regards, -- per aspera ad upstream ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: mvpp2: XDP TX support 2020-07-06 15:28 ` Matteo Croce @ 2020-07-06 15:32 ` Colin Ian King 0 siblings, 0 replies; 3+ messages in thread From: Colin Ian King @ 2020-07-06 15:32 UTC (permalink / raw) To: Matteo Croce, Sven Auhagen Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel On 06/07/2020 16:28, Matteo Croce wrote: > On Mon, 6 Jul 2020 14:59:22 +0100 > Colin Ian King <colin.king@canonical.com> wrote: > >> Hi, >> >> Static analysis with Coverity has found a potential issue in the >> following commit: >> >> commit c2d6fe6163de80d7f7cf400ee351f56d6cdb7a5a >> Author: Matteo Croce <mcroce@microsoft.com> >> Date: Thu Jul 2 16:12:43 2020 +0200 >> >> mvpp2: XDP TX support >> >> >> In source drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c in function >> mvpp2_check_pagepool_dma, analysis is as follows: >> >> >> 4486 if (!priv->percpu_pools) >> 4487 return err; >> 4488 >> CID (#1 of 1): Array compared against 0 (NO_EFFECT) >> array_null: Comparing an array to null is not useful: priv->page_pool, >> since the test will always evaluate as true. >> >> Was priv->page_pool formerly declared as a pointer? >> >> 4489 if (!priv->page_pool) >> 4490 return -ENOMEM; >> 4491 >> >> >> page_pool is declared as: >> >> struct page_pool *page_pool[MVPP2_PORT_MAX_RXQ]; >> >> ..it is an array and hence cannot be null, so the null check is >> redundant. Later on there is a reference of priv->page_pool[0], so >> was the check meant to be: >> >> if (!priv->page_pool[0]) >> >> Colin > > Hi, > > yes, the check was meant to be 'if (!priv->page_pool[0])'. > Maybe it's a copy/paste error from other points where 'page_pool' is a > local variable. > > While at it, I've found that in case a page_pool allocation fails, I > don't cleanup the previously allocated pools, and upon deallocation the > pointer isn't set back to NULL. > > I should add something like: > > @@ -548,8 +548,10 @@ static int mvpp2_bm_pool_destroy(struct device > *dev, struct mvpp2 *priv, val |= MVPP2_BM_STOP_MASK; > mvpp2_write(priv, MVPP2_BM_POOL_CTRL_REG(bm_pool->id), val); > > - if (priv->percpu_pools) > + if (priv->percpu_pools) { > page_pool_destroy(priv->page_pool[bm_pool->id]); > + priv->page_pool[bm_pool->id] = NULL; > + } > > dma_free_coherent(dev, bm_pool->size_bytes, > bm_pool->virt_addr, > @@ -609,8 +611,15 @@ static int mvpp2_bm_init(struct device *dev, > struct mvpp2 *priv) mvpp2_pools[pn].buf_num, > mvpp2_pools[pn].pkt_size, > dma_dir); > - if (IS_ERR(priv->page_pool[i])) > - return PTR_ERR(priv->page_pool[i]); > + if (IS_ERR(priv->page_pool[i])) { > + err = PTR_ERR(priv->page_pool[i]); > + > + for (i--; i >=0; i--) { > + > page_pool_destroy(priv->page_pool[i]); > + priv->page_pool[i] = NULL; > + } > + return err; > + } > } > } > > Looks sane to you? > > Regards, > Oh, good catch on the cleanups. Yes, that looks sane. Colin ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-06 15:32 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-06 13:59 mvpp2: XDP TX support Colin Ian King 2020-07-06 15:28 ` Matteo Croce 2020-07-06 15:32 ` Colin Ian King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).