Message-ID: <CAKv+Gu-pcODD6+e3uHL-gTOUbQcZrh1N5rvzMeVQkVqAYkPu6w@mail.gmail.com>
Date:   Sat, 26 May 2018 08:16:26 +0200
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Jassi Brar <jaswinder.singh@...aro.org>
Cc:     Robin Murphy <robin.murphy@....com>,
        "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Masahisa Kojima <masahisa.kojima@...aro.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>, nd <nd@....com>
Subject: Re: [PATCH] net: netsec: reduce DMA mask to 40 bits

On 26 May 2018 at 05:44, Jassi Brar <jaswinder.singh@...aro.org> wrote:
> On 26 May 2018 at 08:56, Jassi Brar <jaswinder.singh@...aro.org> wrote:
>> On 26 May 2018 at 01:07, Robin Murphy <robin.murphy@....com> wrote:
>>> On Sat, 26 May 2018 00:33:05 +0530
>>> Jassi Brar <jaswinder.singh@...aro.org> wrote:
>>>
>>>> On 25 May 2018 at 18:20, Ard Biesheuvel <ard.biesheuvel@...aro.org>
>>>> wrote:
>>>> > The netsec network controller IP can drive 64 address bits for DMA,
>>>> > and the DMA mask is set accordingly in the driver. However, the
>>>> > SynQuacer SoC, which is the only silicon incorporating this IP at
>>>> > the moment, integrates this IP in a manner that leaves address bits
>>>> > [63:40] unconnected.
>>>> >
>>>> > Up until now, this has not resulted in any problems, given that the
>>>> > DDR controller doesn't decode those bits to begin with. However,
>>>> > recent firmware updates for platforms incorporating this SoC allow
>>>> > the IOMMU to be enabled, which does decode address bits [47:40],
>>>> > and allocates top down from the IOVA space, producing DMA addresses
>>>> > that have bits set that have been left unconnected.
>>>> >
>>>> > Both the DT and ACPI (IORT) descriptions of the platform take this
>>>> > into account, and only describe a DMA address space of 40 bits
>>>> > (using either dma-ranges DT properties, or DMA address limits in
>>>> > IORT named component nodes). However, even though our IOMMU and bus
>>>> > layers may take such limitations into account by setting a narrower
>>>> > DMA mask when creating the platform device, the netsec probe()
>>>> > entrypoint follows the common practice of setting the DMA mask
>>>> > uncondionally, according to the capabilities of the IP block itself
>>>> > rather than to its integration into the chip.
>>>> >
>>>> > It is currently unclear what the correct fix is here. We could hack
>>>> > around it by only setting the DMA mask if it deviates from its
>>>> > default value of DMA_BIT_MASK(32). However, this makes it
>>>> > impossible for the bus layer to use DMA_BIT_MASK(32) as the bus
>>>> > limit, and so it appears that a more comprehensive approach is
>>>> > required to take DMA limits imposed by the SoC as a whole into
>>>> > account.
>>>> >
>>>> > In the mean time, let's limit the DMA mask to 40 bits. Given that
>>>> > there is currently only one SoC that incorporates this IP, this is
>>>> > a reasonable approach that can be backported to -stable and buys us
>>>> > some time to come up with a proper fix going forward.
>>>> >
>>>> I am sure you already thought about it, but why not let the platform
>>>> specify the bit mask for the driver (via some "bus-width" property),
>>>> to override the default 64 bit mask?
>>>
>>> Because lack of a property to describe the integration is not the
>>> problem. There are already at least two ways: the general DT/IORT
>>> properties for describing DMA addressing - which it would be a bit
>>> ungainly for a driver to parse for this reason, but not impossible -
>> ....
>>
>>
>>> and inferring it from a SoC-specific compatible - which is more
>>> appropriate, and what we happen to be able to do here.
>>>
>> Sorry, I am not sure I follow. This patch changes from 64-bits default
>> to 40-bits capability without checking for the parent SoC. If the next
>> generation implements the full 64-bit or just 32-bit bus, we'll be
>> back in the pit again. No?
>>
> Probably you meant we'll change the ethernet compatible string for
> differently capable SoC. OK, but here it is more of integration issue
> than controller version.
>
> Which makes me realise the extant compatible property for netsec is
> not so correct (it embeds the platform name). So I am ok either way.
>

The platform in question has a dma-ranges DT property at the root
level that only describes 40 bits' worth of DMA. Also, the ACPI
description in the IORT table of the IOMMU integration of the netsec
controller limits DMA to 40 bits. In the latter case, we actually
enter netsec_probe() with the correct value already assigned to the
DMA mask fields. (In the former case, the DMA limit is ignored
entirely)

In other words, we can already describe these SoC limitations and
distinguish them from device limitations. The problem is that drivers
ignore the existing values of DMA mask.

Robin has volunteered to look into fixing this, but this cannot be
done in a way that is suitable for -stable. In the mean time, we have
a single platform using this network IP in the field that cannot
upgrade its firmware to a version that describes the IOMMU, because
the existing DMA layer code will start driving address bits that are
correctly described as unconnected by the DT/ACPI tables.

So as a a workaround, until Robin fixes things properly, let's reduce
the DMA mask to 40 bits.

Powered by blists - more mailing lists