From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jennifer Dahm <jennifer.dahm@ni.com>
Subject: Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX
 checksum offloading
Date: Thu, 7 Jun 2018 11:43:15 -0500
Message-ID: <7569cbec-8aae-93ff-e442-4c270ac3002b@ni.com>
References: <1527284654-24835-1-git-send-email-jennifer.dahm@ni.com>
 <1527284654-24835-2-git-send-email-jennifer.dahm@ni.com>
 <ad25fd2b-a3a0-28d5-4dd1-a7ccc935fff2@microchip.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Cc: Nathan Sullivan <nathan.sullivan@ni.com>
To: Nicolas Ferre <nicolas.ferre@microchip.com>,
        netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mx0b-00010702.pphosted.com ([148.163.158.57]:57578 "EHLO
        mx0b-00010702.pphosted.com" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S932804AbeFGRSP (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 7 Jun 2018 13:18:15 -0400
In-Reply-To: <ad25fd2b-a3a0-28d5-4dd1-a7ccc935fff2@microchip.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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