netdev - Re: [PATCH RFC 4/5] net:stmmac: fix jumbo frame handling.

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]

Message-ID: <52667C76.4080305@st.com>
Date:	Tue, 22 Oct 2013 15:24:06 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Jimmy PERCHET <jimmy.perchet@...rot.com>
Cc:	<netdev@...r.kernel.org>, <jimmy.perchet@...il.com>
Subject: Re: [PATCH RFC 4/5] net:stmmac: fix jumbo frame handling.

On 10/21/2013 6:28 PM, Jimmy PERCHET wrote:
> On 21/10/2013 15:40, Giuseppe CAVALLARO wrote:
>> On 10/16/2013 5:24 PM, Jimmy Perchet wrote:
>>> This patch addresses several issues which prevent jumbo frames from working properly :
>>> .jumbo frames' last descriptor was not closed
>>> .several confusion regarding descriptor's max buffer size
>>> .frags could not be jumbo
>>>
>>> Signed-off-by: Jimmy Perchet <jimmy.perchet@...rot.com>
>>
>>
>> Jimmy, thx for thi patch. BElow some my first notes.
>
> Thanks a lot for this first review.

welcome

>
>> I'll continue to look at the patch to verify if I missed
>> soemthing. I kindly ask you, for the next version, to add
>> more comments especially in the function to prepare the
>> tx desc in order to help me on reviewing.
>
> Sure ;)
>
> I hope do v2 by next week.

ok thx, I'll try to help on reviewing for the v2 again.

>
> I'm OK with most of your comments. Some additional
> notes below:
>
>>>    }
>>> @@ -81,7 +81,7 @@ static inline void ndesc_end_tx_desc_on_ring(struct dma_desc *p, int ter)
>>>
>>>    static inline void norm_set_tx_desc_len_on_ring(struct dma_desc *p, int len)
>>>    {
>>> -    if (unlikely(len > BUF_SIZE_2KiB)) {
>>> +    if (unlikely(len >= BUF_SIZE_2KiB)) {
>>
>> we cannot manage a size of 2048 on normal desc
>>
>> Pls you should verify to not break the back-compatibility.
>
> IMHO, this actually fix the problem you think I create.
> In current code, if len is equal to 2048, buffer1_size is set to 2048,
> this is wrong because the max size is actually 2047...

IIRC, for normal descriptors, the TBS2/1 are just 11 bits
so the max programmable size is 2047 (0x7ff).

>
>>
>>>            p->des01.etx.buffer1_size = BUF_SIZE_2KiB - 1;
>>>            p->des01.etx.buffer2_size = len - p->des01.etx.buffer1_size;
>>>        } else
>
>
>
>>>
>>>    static void stmmac_refill_desc3(void *priv_ptr, struct dma_desc *p)
>>>    {
>>> @@ -103,13 +90,13 @@ static void stmmac_refill_desc3(void *priv_ptr, struct dma_desc *p)
>>>        if (unlikely(priv->plat->has_gmac))
>>>            /* Fill DES3 in case of RING mode */
>>>            if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
>>> -            p->des3 = p->des2 + BUF_SIZE_8KiB;
>>> +            p->des3 = p->des2 + BUF_SIZE_8KiB - 1;
>>
>> is it correct? can you check?
>
> The actual buffer's max size is 8191, so, in ring mode,
> the second buffer must start at p->des2 + 8191.
>
>>> -    priv->cur_tx++;
>>> +    priv->cur_tx += nb_desc;
>>
>> can we avoid to use the nb_desc?
> Actually, it is a preparation for my 5th patch : I want to write cur_tx only once.
> I can split this.

ok

>
>
>
> Best Regards,
> Jimmy
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html