| Message ID | 1527284654-24835-2-git-send-email-jennifer.dahm@ni.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: macb: Disable TX checksum offloading on all Zynq | expand |
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 <jennifer.dahm@ni.com> > --- > 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 0x00000010 > #define MACB_CAPS_JUMBO 0x00000020 > #define MACB_CAPS_GEM_HAS_PTP 0x00000040 > +#define MACB_CAPS_DISABLE_TX_HW_CSUM 0x00000080 Nitpicking: "DISABLED" tends to be at the end of the name... > #define MACB_CAPS_FIFO_MODE 0x10000000 > #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000 > #define MACB_CAPS_SG_DISABLED 0x40000000 > 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; >
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
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 0x00000010 #define MACB_CAPS_JUMBO 0x00000020 #define MACB_CAPS_GEM_HAS_PTP 0x00000040 +#define MACB_CAPS_DISABLE_TX_HW_CSUM 0x00000080 #define MACB_CAPS_FIFO_MODE 0x10000000 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000 #define MACB_CAPS_SG_DISABLED 0x40000000 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;
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 <jennifer.dahm@ni.com> --- drivers/net/ethernet/cadence/macb.h | 1 + drivers/net/ethernet/cadence/macb_main.c | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-)