netdev - Re: [PATCH v5 net-next 4/7] net: mvneta: Convert to be 64 bits compatible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Dec 2016 12:48:39 +0100
From:   Marcin Wojtas <mw@...ihalf.com>
To:     Jisheng Zhang <jszhang@...vell.com>
Cc:     Gregory CLEMENT <gregory.clement@...e-electrons.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Nadav Haklai <nadavh@...vell.com>,
        Dmitri Epshtein <dima@...vell.com>,
        Yelena Krivosheev <yelena@...vell.com>
Subject: Re: [PATCH v5 net-next 4/7] net: mvneta: Convert to be 64 bits compatible

Hi Jisheng,

Which baseline do you use?

It took me really lot of time to catch why RX broke after rebase from
LKv4.1 to LKv4.4. Between those two, in commit:
97303480753e ("arm64: Increase the max granular size")
L1_CACHE_BYTES for all ARMv8 platforms was increased to 128B and so
did NET_SKB_PAD.

And 128 is more than the maximum that can fit into packet offset
[11:8]@0x1400. In such case this correction is needed. Did it answer
your doubts?

Best regards,
Marcin



2016-12-01 12:26 GMT+01:00 Jisheng Zhang <jszhang@...vell.com>:
> Hi Gregory, Marcin,
>
> On Wed, 30 Nov 2016 22:42:49 +0100 Gregory CLEMENT wrote:
>
>> From: Marcin Wojtas <mw@...ihalf.com>
>>
>> Prepare the mvneta driver in order to be usable on the 64 bits platform
>> such as the Armada 3700.
>>
>> [gregory.clement@...e-electrons.com]: this patch was extract from a larger
>> one to ease review and maintenance.
>>
>> Signed-off-by: Marcin Wojtas <mw@...ihalf.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@...e-electrons.com>
>> ---
>>  drivers/net/ethernet/marvell/mvneta.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> index 92b9af14c352..8ef03fb69bcd 100644
>> --- a/drivers/net/ethernet/marvell/mvneta.c
>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>> @@ -296,6 +296,12 @@
>>  /* descriptor aligned size */
>>  #define MVNETA_DESC_ALIGNED_SIZE     32
>>
>> +/* Number of bytes to be taken into account by HW when putting incoming data
>> + * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum packet
>> + * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.
>
> We also brought up this driver on 64bit platforms, we doesn't have this
> patch. Maybe I'm wrong, I'm trying to understand why we need this
> modification. Let's assume the NET_SKB_PAD is 64B, we call
> mvneta_rxq_offset_set(pp, rxq, 64),
>
> {
>         u32 val;
>
>         val = mvreg_read(pp, MVNETA_RXQ_CONFIG_REG(rxq->id));
>         val &= ~MVNETA_RXQ_PKT_OFFSET_ALL_MASK;
>
>         /* Offset is in */
>         val |= MVNETA_RXQ_PKT_OFFSET_MASK(offset >> 3);
> // then this will be "val |= 8;" it doesn't exceeds the max offset of
> MVNETA_RXQ_CONFIG_REG(q) register.
>
> Could you please kindly point out where I am wrong?
>
>> + */
>> +#define MVNETA_RX_PKT_OFFSET_CORRECTION              64
>> +
>>  #define MVNETA_RX_PKT_SIZE(mtu) \
>>       ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
>>             ETH_HLEN + ETH_FCS_LEN,                        \
>> @@ -416,6 +422,7 @@ struct mvneta_port {
>>       u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
>>
>>       u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
>> +     u16 rx_offset_correction;
>>  };
>>
>>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
>> @@ -1807,6 +1814,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>>               return -ENOMEM;
>>       }
>>
>> +     phys_addr += pp->rx_offset_correction;
>>       mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
>>       return 0;
>>  }
>> @@ -2782,7 +2790,7 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
>>       mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
>>
>>       /* Set Offset */
>> -     mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
>> +     mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD - pp->rx_offset_correction);
>>
>>       /* Set coalescing pkts and time */
>>       mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
>> @@ -4033,6 +4041,13 @@ static int mvneta_probe(struct platform_device *pdev)
>>
>>       pp->rxq_def = rxq_def;
>>
>> +     /* Set RX packet offset correction for platforms, whose
>> +      * NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
>> +      * platforms and 0B for 32-bit ones.
>
> Even we need this patch, I'm not sure this last comment is correct or not.
> NET_SKB_PAD is defined as:
>
> #define NET_SKB_PAD     max(32, L1_CACHE_BYTES)
>
> we have 64B cacheline 32bit platforms, on this platforms, the NET_SKB_PAD
> should be 64B as well.
>
> Thanks,
> Jisheng