[PATCH net 0/3] net: mvpp2: various fixes

[PATCH net 0/3] net: mvpp2: various fixes

[Antoine Tenart]

Hi all,

This series contains various fixes for the Marvell PPv2 driver. Please
have a thorough look at patch 1/3 ("net: mvpp2: fix the dma_mask and
coherent_dma_mask settings for PPv2.2") as I'm not 100% sure about the
fix implementation.

Thanks!
Antoine

Antoine Tenart (1):
  net: mvpp2: fix the dma_mask and coherent_dma_mask settings for PPv2.2

Stefan Chulski (1):
  net: mvpp2: fix parsing fragmentation detection

Yan Markman (1):
  net: mvpp2: fix port list indexing

 drivers/net/ethernet/marvell/mvpp2.c | 44 ++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

-- 
2.13.5

[PATCH net 1/3] net: mvpp2: fix the dma_mask and coherent_dma_mask settings for PPv2.2

[Antoine Tenart]

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(+)

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
-- 
2.13.5

[PATCH net 2/3] net: mvpp2: fix parsing fragmentation detection

[Antoine Tenart]

From: Stefan Chulski <stefanc@marvell.com>

Parsing fragmentation detection failed due to wrong configured
parser TCAM entry's. Some traffic was marked as fragmented in RX
descriptor, even it wasn't IP fragmented. The hardware also failed to
calculate checksums which lead to use software checksum and caused
performance degradation.

Fixes: 3f518509dedc ("ethernet: Add new driver for Marvell Armada 375 network unit")
Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 7024d4dbb461..56d474414cfa 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -676,6 +676,7 @@ enum mvpp2_tag_type {
 #define MVPP2_PRS_RI_L3_MCAST			BIT(15)
 #define MVPP2_PRS_RI_L3_BCAST			(BIT(15) | BIT(16))
 #define MVPP2_PRS_RI_IP_FRAG_MASK		0x20000
+#define MVPP2_PRS_RI_IP_FRAG_TRUE		BIT(17)
 #define MVPP2_PRS_RI_UDF3_MASK			0x300000
 #define MVPP2_PRS_RI_UDF3_RX_SPECIAL		BIT(21)
 #define MVPP2_PRS_RI_L4_PROTO_MASK		0x1c00000
@@ -2315,7 +2316,7 @@ static int mvpp2_prs_ip4_proto(struct mvpp2 *priv, unsigned short proto,
 	    (proto != IPPROTO_IGMP))
 		return -EINVAL;
 
-	/* Fragmented packet */
+	/* Not fragmented packet */
 	tid = mvpp2_prs_tcam_first_free(priv, MVPP2_PE_FIRST_FREE_TID,
 					MVPP2_PE_LAST_FREE_TID);
 	if (tid < 0)
@@ -2334,8 +2335,12 @@ static int mvpp2_prs_ip4_proto(struct mvpp2 *priv, unsigned short proto,
 				  MVPP2_PRS_SRAM_OP_SEL_UDF_ADD);
 	mvpp2_prs_sram_ai_update(&pe, MVPP2_PRS_IPV4_DIP_AI_BIT,
 				 MVPP2_PRS_IPV4_DIP_AI_BIT);
-	mvpp2_prs_sram_ri_update(&pe, ri | MVPP2_PRS_RI_IP_FRAG_MASK,
-				 ri_mask | MVPP2_PRS_RI_IP_FRAG_MASK);
+	mvpp2_prs_sram_ri_update(&pe, ri, ri_mask | MVPP2_PRS_RI_IP_FRAG_MASK);
+
+	mvpp2_prs_tcam_data_byte_set(&pe, 2, 0x00,
+				     MVPP2_PRS_TCAM_PROTO_MASK_L);
+	mvpp2_prs_tcam_data_byte_set(&pe, 3, 0x00,
+				     MVPP2_PRS_TCAM_PROTO_MASK);
 
 	mvpp2_prs_tcam_data_byte_set(&pe, 5, proto, MVPP2_PRS_TCAM_PROTO_MASK);
 	mvpp2_prs_tcam_ai_update(&pe, 0, MVPP2_PRS_IPV4_DIP_AI_BIT);
@@ -2346,7 +2351,7 @@ static int mvpp2_prs_ip4_proto(struct mvpp2 *priv, unsigned short proto,
 	mvpp2_prs_shadow_set(priv, pe.index, MVPP2_PRS_LU_IP4);
 	mvpp2_prs_hw_write(priv, &pe);
 
-	/* Not fragmented packet */
+	/* Fragmented packet */
 	tid = mvpp2_prs_tcam_first_free(priv, MVPP2_PE_FIRST_FREE_TID,
 					MVPP2_PE_LAST_FREE_TID);
 	if (tid < 0)
@@ -2358,8 +2363,11 @@ static int mvpp2_prs_ip4_proto(struct mvpp2 *priv, unsigned short proto,
 	pe.sram.word[MVPP2_PRS_SRAM_RI_CTRL_WORD] = 0x0;
 	mvpp2_prs_sram_ri_update(&pe, ri, ri_mask);
 
-	mvpp2_prs_tcam_data_byte_set(&pe, 2, 0x00, MVPP2_PRS_TCAM_PROTO_MASK_L);
-	mvpp2_prs_tcam_data_byte_set(&pe, 3, 0x00, MVPP2_PRS_TCAM_PROTO_MASK);
+	mvpp2_prs_sram_ri_update(&pe, ri | MVPP2_PRS_RI_IP_FRAG_TRUE,
+				 ri_mask | MVPP2_PRS_RI_IP_FRAG_MASK);
+
+	mvpp2_prs_tcam_data_byte_set(&pe, 2, 0x00, 0x0);
+	mvpp2_prs_tcam_data_byte_set(&pe, 3, 0x00, 0x0);
 
 	/* Update shadow table and hw entry */
 	mvpp2_prs_shadow_set(priv, pe.index, MVPP2_PRS_LU_IP4);
-- 
2.13.5

[PATCH net 3/3] net: mvpp2: fix port list indexing

[Antoine Tenart]

From: Yan Markman <ymarkman@marvell.com>

The private port_list array has a list of pointers to mvpp2_port
instances. This list is allocated given the number of ports enabled in
the device tree, but the pointers are set using the port-id property. If
on a single port is enabled, the port_list array will be of size 1, but
when registering the port, if its id is not 0 the driver will crash.
Other crashes were encountered in various situations.

This fixes the issue by using an index not equal to the value of the
port-id property.

Fixes: 3f518509dedc ("ethernet: Add new driver for Marvell Armada 375 network unit")
Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 56d474414cfa..e7889464e97e 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -7504,7 +7504,7 @@ static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
 /* Ports initialization */
 static int mvpp2_port_probe(struct platform_device *pdev,
 			    struct device_node *port_node,
-			    struct mvpp2 *priv)
+			    struct mvpp2 *priv, int index)
 {
 	struct device_node *phy_node;
 	struct phy *comphy;
@@ -7678,7 +7678,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 	}
 	netdev_info(dev, "Using %s mac address %pM\n", mac_from, dev->dev_addr);
 
-	priv->port_list[id] = port;
+	priv->port_list[index] = port;
 	return 0;
 
 err_free_port_pcpu:
@@ -8029,10 +8029,12 @@ static int mvpp2_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize ports */
+	i = 0;
 	for_each_available_child_of_node(dn, port_node) {
-		err = mvpp2_port_probe(pdev, port_node, priv);
+		err = mvpp2_port_probe(pdev, port_node, priv, i);
 		if (err < 0)
 			goto err_mg_clk;
+		i++;
 	}
 
 	platform_set_drvdata(pdev, priv);
-- 
2.13.5

Re: [PATCH net 1/3] net: mvpp2: fix the dma_mask and coherent_dma_mask settings for PPv2.2

[David Miller]

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.

Re: [PATCH net 1/3] net: mvpp2: fix the dma_mask and coherent_dma_mask settings for PPv2.2

[Antoine Tenart]

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

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH net 1/3] net: mvpp2: fix the dma_mask and coherent_dma_mask settings for PPv2.2

[David Miller]

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.

Re: [PATCH net 1/3] net: mvpp2: fix the dma_mask and coherent_dma_mask settings for PPv2.2

[Antoine Tenart]

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

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com