linux-kernel - Re: [PATCH 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs

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:   Wed, 14 Mar 2018 14:49:00 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Sinan Kaya <okaya@...eaurora.org>
Cc:     Timur Tabi <timur@...eaurora.org>, Netdev <netdev@...r.kernel.org>,
        sulrich@...eaurora.org, linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/7] ixgbevf: eliminate duplicate barriers on
 weakly-ordered archs

On Wed, Mar 14, 2018 at 5:13 AM,  <okaya@...eaurora.org> wrote:
> On 2018-03-14 01:08, Timur Tabi wrote:
>>
>> On 3/13/18 10:20 PM, Sinan Kaya wrote:
>>>
>>> +/* Assumes caller has executed a write barrier to order memory and
>>> device
>>> + * requests.
>>> + */
>>>   static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32
>>> value)
>>>   {
>>> -       writel(value, ring->tail);
>>> +       writel_relaxed(value, ring->tail);
>>>   }
>>
>>
>> Why not put the wmb() in this function, or just get rid of the wmb()
>> in the rest of the file and keep this as writel?  That way, you can
>> avoid the comment and the risk that comes with it.
>
>
>
> Sure, both solutions will work. I want to see what the maintainer prefers. I
> can repost accordingly.

Actually I would argue this whole patch set is pointless. For starters
why is it we are only updating the Intel Ethernet drivers here? This
seems like something that is going to impact the whole kernel tree
since many of us have been writing drivers for some time assuming x86
style behavior.

It doesn't make sense to be optimizing the drivers for one subset of
architectures. The scope of the work needed to update the drivers for
this would be ridiculous. Also I don't see how this could be expected
to work on any other architecture when we pretty much need to have a
wmb() before calling the writel on x86 to deal with accesses between
coherent and non-coherent memory. It seems to me more like somebody
added what they considered to be an optimization somewhere that is a
workaround for a poorly written driver. Either that or the barrier is
serving a different purpose then the one we were using.

It would make more sense to put in the effort making writel and
writel_relaxed consistent between architectures before we go through
and start modifying driver code to support different architectures.