Message ID | 20211019181950.14679-1-tim.gardner@canonical.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [linux-next] net: enetc: unmap DMA in enetc_send_cmd() | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Single patches do not need cover letters |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Fixes tag looks correct |
netdev/checkpatch | warning | WARNING: line length of 88 exceeds 80 columns |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/header_inline | success | No static functions without inline keyword in header files |
> -----Original Message----- > From: Tim Gardner <tim.gardner@canonical.com> > Sent: Tuesday, October 19, 2021 9:20 PM [...] > Subject: [PATCH][linux-next] net: enetc: unmap DMA in enetc_send_cmd() > > Coverity complains of a possible dereference of a null return value. > > 5. returned_null: kzalloc returns NULL. [show details] > 6. var_assigned: Assigning: si_data = NULL return value from kzalloc. > 488 si_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL); > 489 cbd.length = cpu_to_le16(data_size); > 490 > 491 dma = dma_map_single(&priv->si->pdev->dev, si_data, > 492 data_size, DMA_FROM_DEVICE); > > While this kzalloc() is unlikely to fail, I did notice that the function > returned without unmapping si_data. > > Fix this by refactoring the error paths and checking for kzalloc() > failure. > > Fixes: 888ae5a3952ba ("net: enetc: add tc flower psfp offload driver") > Cc: Claudiu Manoil <claudiu.manoil@nxp.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org (open list) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > > I am curious why you do not need to call dma_sync_single_for_device() > before enetc_send_cmd() > in order to flush the contents of CPU cache to RAM. Is it because > __GFP_DMA marks > that page as uncached ? Or is it because of the SOC this runs on ? > Not sure how it works like this, but I think dma_alloc_coherent() should have been used here instead of the kmalloc + dma_map scheme. I don't have the means to test this particular code path however. Acked-by: Claudiu Manoil <claudiu.manoil@nxp.com>
On Tue, 19 Oct 2021 19:47:17 +0000 Claudiu Manoil wrote: > > I am curious why you do not need to call dma_sync_single_for_device() > > before enetc_send_cmd() > > in order to flush the contents of CPU cache to RAM. Is it because > > __GFP_DMA marks > > that page as uncached ? Or is it because of the SOC this runs on ? > > > > Not sure how it works like this, but I think dma_alloc_coherent() should have > been used here instead of the kmalloc + dma_map scheme. Using dma_alloc_coherent() seems simpler and more correct, let's do that instead.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c index 4577226d3c6a..a93c55b04287 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c @@ -486,14 +486,16 @@ static int enetc_streamid_hw_set(struct enetc_ndev_priv *priv, data_size = sizeof(struct streamid_data); si_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL); - cbd.length = cpu_to_le16(data_size); + if (!si_data) + return -ENOMEM; + cbd.length = cpu_to_le16(data_size); dma = dma_map_single(&priv->si->pdev->dev, si_data, data_size, DMA_FROM_DEVICE); if (dma_mapping_error(&priv->si->pdev->dev, dma)) { netdev_err(priv->si->ndev, "DMA mapping failed!\n"); - kfree(si_data); - return -ENOMEM; + err = -ENOMEM; + goto out; } cbd.addr[0] = cpu_to_le32(lower_32_bits(dma)); @@ -512,12 +514,10 @@ static int enetc_streamid_hw_set(struct enetc_ndev_priv *priv, err = enetc_send_cmd(priv->si, &cbd); if (err) - return -EINVAL; + goto out; - if (!enable) { - kfree(si_data); - return 0; - } + if (!enable) + goto out; /* Enable the entry overwrite again incase space flushed by hardware */ memset(&cbd, 0, sizeof(cbd)); @@ -560,7 +560,11 @@ static int enetc_streamid_hw_set(struct enetc_ndev_priv *priv, } err = enetc_send_cmd(priv->si, &cbd); - kfree(si_data); +out: + if (!dma_mapping_error(&priv->si->pdev->dev, dma)) + dma_unmap_single(&priv->si->pdev->dev, dma, data_size, DMA_FROM_DEVICE); + + kfree(si_data); return err; }
Coverity complains of a possible dereference of a null return value. 5. returned_null: kzalloc returns NULL. [show details] 6. var_assigned: Assigning: si_data = NULL return value from kzalloc. 488 si_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL); 489 cbd.length = cpu_to_le16(data_size); 490 491 dma = dma_map_single(&priv->si->pdev->dev, si_data, 492 data_size, DMA_FROM_DEVICE); While this kzalloc() is unlikely to fail, I did notice that the function returned without unmapping si_data. Fix this by refactoring the error paths and checking for kzalloc() failure. Fixes: 888ae5a3952ba ("net: enetc: add tc flower psfp offload driver") Cc: Claudiu Manoil <claudiu.manoil@nxp.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org (open list) Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- I am curious why you do not need to call dma_sync_single_for_device() before enetc_send_cmd() in order to flush the contents of CPU cache to RAM. Is it because __GFP_DMA marks that page as uncached ? Or is it because of the SOC this runs on ? rtg --- .../net/ethernet/freescale/enetc/enetc_qos.c | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-)