netdev - Re: [PATCH 2/3] net: socionext: Add Synquacer NetSec driver

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, 30 Nov 2017 17:37:27 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     David Miller <davem@...emloft.net>,
        Dan Williams <dan.j.williams@...el.com>,
        Will Deacon <will.deacon@....com>
Cc:     Jassi Brar <jassisinghbrar@...il.com>,
        "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Arnd Bergmann <arnd.bergmann@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jaswinder Singh <jaswinder.singh@...aro.org>
Subject: Re: [PATCH 2/3] net: socionext: Add Synquacer NetSec driver

(+ Dan, Will)

On 30 November 2017 at 17:29, David Miller <davem@...emloft.net> wrote:
> From: jassisinghbrar@...il.com
> Date: Thu, 30 Nov 2017 21:43:16 +0530
>
>> +     priv->eeprom_base = devm_memremap(&pdev->dev, eeprom_res->start,
>> +                                       resource_size(eeprom_res),
>> +                                       MEMREMAP_WT);
>> +     if (!priv->eeprom_base) {
>> +             dev_err(&pdev->dev, "devm_memremap() failed for EEPROM\n");
>> +             ret = -ENXIO;
>> +             goto free_ndev;
>> +     }
>
> dev_memremap() is implemented via memremap() which for MEMREMAP_WT is
> in turn implemented using ioremap_wt() which returns an "__iomem"
> pointer.
>
> The memremap() function talks about __iomem being about side effects.
> That's not really the full story.  The __iomem annotation is also
> about whether a special accessor is necessary to "dereference" the
> pointer.  On sparc64, for example, the ioremap_*() functions return a
> physical address not a virtual one.  So you cannot directly
> dereference pointers that are returned from the ioremap*() interfaces.
>
> You'll also need to mark priv->eeprom_base as "__iomem".
>
> devm_memremap() returns a straight "void *" without the __iomem
> annotation, and this is wrong then ioremap_*() is used.

Well, the whole point of using memremap() instead of ioremap() is that
the region has memory semantics, i.e., we read the MAC address and the
DMA engine microcode from it. If memremap() itself is flawed in this
regard, I agree we should fix it. But as I understand it, this is
really an implementation problem in memremap() [the fact that it falls
back to ioremap()] and not a problem in this driver.

So what alternative would you propose in this case?

BTW, this should be IOREMAP_WC not IOREMAP_WT, because the EEPROM on
the platform in question does not tolerate cached mappings (or rather,
shareable mappings). ioremap_wt() happens to result in device mappings
rather than writethrough cacheable mappings, but this is another
problem in itself. Once arm64 fixes ioremap_wt(), this code will no
longer work on the SynQuacer SoC.