Message-ID: <CAJe_ZhcMw5sCfhSq-2O3GT2MCnYfVyoEQAj6U04_ffWeeZQ+Zw@mail.gmail.com>
Date:   Sun, 27 May 2018 10:03:48 +0530
From:   Jassi Brar <jaswinder.singh@...aro.org>
To:     Ard Biesheuvel <ard.biesheuvel@...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 11:46, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
> 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.
>
Yeah no point in introducing another dt property if this hack is
temporary until the core is fixed.
FWIW ... Acked-by: Jassi Brar <jaswinder.singh@...aro.org>

Thanks.

Powered by blists - more mailing lists