| Message ID | 20170918130408.23114-2-antoine.tenart@free-electrons.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: mvpp2: various fixes | expand |
From: Antoine Tenart <antoine.tenart@free-electrons.com> Date: Mon, 18 Sep 2017 15:04:06 +0200 > The dev->dma_mask usually points to dev->coherent_dma_mask. This is an > issue as setting both of them will override the other. This is > problematic here as the PPv2 driver uses a 32-bit-mask for coherent > accesses (txq, rxq, bm) and a 40-bit mask for all other accesses due to > an hardware limitation. > > This can lead to a memory remap for all dma_map_single() calls when > dealing with memory above 4GB. > > Fixes: 2067e0a13cfe ("net: mvpp2: set dma mask and coherent dma mask on PPv2.2") > Reported-by: Stefan Chulski <stefanc@marvell.com> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> Yikes. I surrmise that if the platform has made dev->dma_mask point to &dev->coherent_dma_mask, it is because it does not allow the two settings to be set separately. By rearranging the pointer, you are bypassing that, and probably breaking things or creating a situation that the DMA mapping layer is not expecting. I want to know more about the situations where dma_mask is set to point to &coherent_dma_mask and how that is supposed to work. At a minimum this commit log message needs to go into more detail. Thanks.
Hi David, On Mon, Sep 18, 2017 at 05:18:58PM -0700, David Miller wrote: > From: Antoine Tenart <antoine.tenart@free-electrons.com> > Date: Mon, 18 Sep 2017 15:04:06 +0200 > > > The dev->dma_mask usually points to dev->coherent_dma_mask. This is an > > issue as setting both of them will override the other. This is > > problematic here as the PPv2 driver uses a 32-bit-mask for coherent > > accesses (txq, rxq, bm) and a 40-bit mask for all other accesses due to > > an hardware limitation. > > > > This can lead to a memory remap for all dma_map_single() calls when > > dealing with memory above 4GB. > > > > Fixes: 2067e0a13cfe ("net: mvpp2: set dma mask and coherent dma mask on PPv2.2") > > Reported-by: Stefan Chulski <stefanc@marvell.com> > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > > I surrmise that if the platform has made dev->dma_mask point to > &dev->coherent_dma_mask, it is because it does not allow the two > settings to be set separately. That's also the default when the platform does not allocate dma_mask. You have a point, that could be because it's not supported. But I don't know what would be a good check then. > By rearranging the pointer, you are bypassing that, and probably > breaking things or creating a situation that the DMA mapping > layer is not expecting. > > I want to know more about the situations where dma_mask is set to > point to &coherent_dma_mask and how that is supposed to work. From what I see in other parts of the kernel, dma_mask points to &coherent_dma_mask by default and having two different values for these two masks isn't a use case for many drivers. Antoine
From: Antoine Tenart <antoine.tenart@free-electrons.com> Date: Thu, 21 Sep 2017 16:24:13 +0200 > That's also the default when the platform does not allocate dma_mask. That's the problem that needs to be fixed then.
On Thu, Sep 21, 2017 at 10:07:18AM -0700, David Miller wrote: > From: Antoine Tenart <antoine.tenart@free-electrons.com> > Date: Thu, 21 Sep 2017 16:24:13 +0200 > > > That's also the default when the platform does not allocate dma_mask. > > That's the problem that needs to be fixed then. OK, I'll drop this patch until I find a proper solution. Thanks, Antoine
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c index dd0ee2691c86..7024d4dbb461 100644 --- a/drivers/net/ethernet/marvell/mvpp2.c +++ b/drivers/net/ethernet/marvell/mvpp2.c @@ -7969,9 +7969,25 @@ static int mvpp2_probe(struct platform_device *pdev) priv->tclk = clk_get_rate(priv->pp_clk); if (priv->hw_version == MVPP22) { + /* If dma_mask points to coherent_dma_mask, setting both will + * override the value of the other. This is problematic as the + * PPv2 driver uses a 32-bit-mask for coherent accesses (txq, + * rxq, bm) and a 40-bit mask for all other accesses. + */ + if (pdev->dev.dma_mask == &pdev->dev.coherent_dma_mask) { + pdev->dev.dma_mask = devm_kzalloc(&pdev->dev, + sizeof(*pdev->dev.dma_mask), + GFP_KERNEL); + if (!pdev->dev.dma_mask) { + err = -ENOMEM; + goto err_mg_clk; + } + } + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)); if (err) goto err_mg_clk; + /* Sadly, the BM pools all share the same register to * store the high 32 bits of their address. So they * must all have the same high 32 bits, which forces
The dev->dma_mask usually points to dev->coherent_dma_mask. This is an issue as setting both of them will override the other. This is problematic here as the PPv2 driver uses a 32-bit-mask for coherent accesses (txq, rxq, bm) and a 40-bit mask for all other accesses due to an hardware limitation. This can lead to a memory remap for all dma_map_single() calls when dealing with memory above 4GB. Fixes: 2067e0a13cfe ("net: mvpp2: set dma mask and coherent dma mask on PPv2.2") Reported-by: Stefan Chulski <stefanc@marvell.com> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/net/ethernet/marvell/mvpp2.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)