Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading

[Jennifer Dahm]

Hi Nicolas,

On 06/04/2018 10:13 AM, Nicolas Ferre wrote:

On 25/05/2018 at 23:44, Jennifer Dahm wrote:
diff --git a/drivers/net/ethernet/cadence/macb_main.c 
b/drivers/net/ethernet/cadence/macb_main.c

index 3e93df5..a5d564b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device 
*pdev)

  dev->hw_features |= MACB_NETIF_LSO;
    /* Checksum offload is only available on gem with packet 
buffer */

-    if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
-    dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+    if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
+    if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))


Why not the other way around? negating a "disabled" feature is always 
challenge ;-)



+    dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+    else
+    dev->hw_features |= NETIF_F_RXCSUM;
+    }
  if (bp->caps & MACB_CAPS_SG_DISABLED)
  dev->hw_features &= ~NETIF_F_SG;
  dev->features = dev->hw_features;


I can switch the ordering of the if-else clauses if that's what you're 
nitpicking. ;)


Alternatively, if you're asking why the flag is used to disable rather 
than enable checksum offloading: I was working under the assumption that 
this was an isolated bug, and so an opt-out would require less 
maintainance than an opt-in. If we discover that this is a problem 
across a wide variety of Cadence IP, it would definitely make sense to 
replace it with an opt-in (i.e. MACB_CAPS_TX_HW_CSUM_ENABLED).


Best,
Jennifer Dahm

Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading

[Nicolas Ferre]

On 25/05/2018 at 23:44, Jennifer Dahm wrote:

Certain PHYs have significant bugs in their TX checksum offloading
that cannot be solved in software. In order to accommodate these PHYS,
add a CAP to disable this hardware.

Signed-off-by: Jennifer Dahm 
---
  drivers/net/ethernet/cadence/macb.h  | 1 +
  drivers/net/ethernet/cadence/macb_main.c | 8 ++--
  2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h 
b/drivers/net/ethernet/cadence/macb.h
index 8665982..6b85e97 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -635,6 +635,7 @@
  #define MACB_CAPS_USRIO_DISABLED  0x0010
  #define MACB_CAPS_JUMBO   0x0020
  #define MACB_CAPS_GEM_HAS_PTP 0x0040
+#define MACB_CAPS_DISABLE_TX_HW_CSUM   0x0080


Nitpicking: "DISABLED" tends to be at the end of the name...


  #define MACB_CAPS_FIFO_MODE   0x1000
  #define MACB_CAPS_GIGABIT_MODE_AVAILABLE  0x2000
  #define MACB_CAPS_SG_DISABLED 0x4000
diff --git a/drivers/net/ethernet/cadence/macb_main.c 
b/drivers/net/ethernet/cadence/macb_main.c
index 3e93df5..a5d564b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device *pdev)
dev->hw_features |= MACB_NETIF_LSO;
  
  	/* Checksum offload is only available on gem with packet buffer */

-   if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
-   dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+   if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
+   if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))


Why not the other way around? negating a "disabled" feature is always 
challenge ;-)



+   dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+   else
+   dev->hw_features |= NETIF_F_RXCSUM;
+   }
if (bp->caps & MACB_CAPS_SG_DISABLED)
dev->hw_features &= ~NETIF_F_SG;
dev->features = dev->hw_features;




--
Nicolas Ferre

[RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading

[Jennifer Dahm]

Certain PHYs have significant bugs in their TX checksum offloading
that cannot be solved in software. In order to accommodate these PHYS,
add a CAP to disable this hardware.

Signed-off-by: Jennifer Dahm 
---
 drivers/net/ethernet/cadence/macb.h  | 1 +
 drivers/net/ethernet/cadence/macb_main.c | 8 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h 
b/drivers/net/ethernet/cadence/macb.h
index 8665982..6b85e97 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -635,6 +635,7 @@
 #define MACB_CAPS_USRIO_DISABLED   0x0010
 #define MACB_CAPS_JUMBO0x0020
 #define MACB_CAPS_GEM_HAS_PTP  0x0040
+#define MACB_CAPS_DISABLE_TX_HW_CSUM   0x0080
 #define MACB_CAPS_FIFO_MODE0x1000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE   0x2000
 #define MACB_CAPS_SG_DISABLED  0x4000
diff --git a/drivers/net/ethernet/cadence/macb_main.c 
b/drivers/net/ethernet/cadence/macb_main.c
index 3e93df5..a5d564b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device *pdev)
dev->hw_features |= MACB_NETIF_LSO;
 
/* Checksum offload is only available on gem with packet buffer */
-   if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
-   dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+   if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
+   if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))
+   dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+   else
+   dev->hw_features |= NETIF_F_RXCSUM;
+   }
if (bp->caps & MACB_CAPS_SG_DISABLED)
dev->hw_features &= ~NETIF_F_SG;
dev->features = dev->hw_features;
-- 
2.7.4