netdev - Re: [v2,net-next,1/3] net: stmmac: enable multiple buffers

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: <abf8722f-5d3a-0bae-d6cd-6cc6fbb744cb@synopsys.com>
Date:   Fri, 24 Mar 2017 14:59:26 +0000
From:   Joao Pinto <Joao.Pinto@...opsys.com>
To:     Corentin Labbe <clabbe.montjoie@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        <Joao.Pinto@...opsys.com>
CC:     <davem@...emloft.net>, <peppe.cavallaro@...com>,
        <alexandre.torgue@...com>, <f.fainelli@...il.com>,
        <netdev@...r.kernel.org>
Subject: Re: [v2,net-next,1/3] net: stmmac: enable multiple buffers

Às 2:09 PM de 3/24/2017, Corentin Labbe escreveu:
> On Thu, Mar 23, 2017 at 07:10:59PM +0100, Thierry Reding wrote:
>> On Thu, Mar 23, 2017 at 05:27:08PM +0000, Joao Pinto wrote:
>>> Hi Thierry,
>>>
>>
>> Yes, I can submit a patch for that.
>>
>> After some more testing I did get a couple (roughly 2 out of 10)
>> successful boots (I'm booting over NFS using the EQOS), and given that
>> this pointed towards something related to uninitialized data, I changed
>> all occurrences of kmalloc_array() with kcalloc() and that I've gotten
>> 10 successful reboots out of 10.
>>
>> I still can't pinpoint why this is now necessary since previously the
>> kmalloc_array() was working just fine. The only thing I can think of is
>> that we're not properly initializing all fields of the new queue
>> structures, since that's the only thing that's changed with this commit.
>>
>> I haven't investigated in detail yet, but from nothing so far has jumped
>> out at me.
>>
>> Thierry
> 
> I have tried this change, but it made the situation worse on dwmac-sunxi (no network at all).
> 
> Joao, perhaps it's time to revert the faulty (and very huge) patch and rework it by splitting at least in two:
> - adding RX queue / adding TX queue
> And more if possible (like just adding an unused queue parameter) or a patch just for adding stmmac_free_tx_buffers() for example.
> I think it will help to find where the problem is.
> 
> And this time I will test them before applying:)
> 
> Regards
> Corentin Labbe
> 

Yes, I agree, it is better to revert and leave the tree functional for all.

@David Miller:
The multiple-buffer patch introduced some problems in some setups that are being
hard to debug, so Corentin gave the idea of reverting the until
commit 7bac4e1ec3ca2342929a39638d615c6b672c27a0 (net: stmmac: stmmac interrupt
treatment prepared for multiple queues), which I fully agree.

In my setup is ok, but the idea is to have everyone's setup working :), so lets
break them into smaller pieces, and let's only apply them when everyone confirms
that is working ok in your setups, agree?

What is the typical mechanism for this? I send a patch reverting them?

Thanks,
Joao