From: Trent Piepho <tpiepho@impinj.com>
To: "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "cyrille.pitchen@wedev4u.fr" <cyrille.pitchen@wedev4u.fr>,
        "dwmw2@infradead.org" <dwmw2@infradead.org>,
        "computersforpeace@gmail.com" <computersforpeace@gmail.com>,
        "vigneshr@ti.com" <vigneshr@ti.com>,
        "boris.brezillon@free-electrons.com" 
        <boris.brezillon@free-electrons.com>,
        "richard@nod.at" <richard@nod.at>,
        "marek.vasut@gmail.com" <marek.vasut@gmail.com>
CC: "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "nicolas.ferre@microchip.com" <nicolas.ferre@microchip.com>,
        "radu.pirea@microchip.com" <radu.pirea@microchip.com>,
        "robh@kernel.org" <robh@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH 1/3] mtd: spi-nor: add optional DMA-safe bounce buffer for
 data transfer
Thread-Topic: [PATCH 1/3] mtd: spi-nor: add optional DMA-safe bounce buffer
 for data transfer
Thread-Index: AQHTfHXE8/fvhJVkOkO7c5x52lKY/6NWCpOAgAKMrwCAAIpvAIABAZKAgACCkIA=
Date: Fri, 29 Dec 2017 18:03:47 +0000
Message-ID: <1514570627.26695.114.camel@impinj.com>
References: <cover.1514087323.git.cyrille.pitchen@wedev4u.fr>
         <fc2440c6f52877f28286d89691049e5cba10e6a7.1514087323.git.cyrille.pitchen@wedev4u.fr>
         <1514317385.26695.39.camel@impinj.com>
         <1a7dc424-1ce0-6c64-fc52-bb88ec7db8fa@wedev4u.fr>
         <1514487276.26695.94.camel@impinj.com>
         <08be8b42-732a-bf28-40c4-f46bf9d71c80@ti.com>
In-Reply-To: <08be8b42-732a-bf28-40c4-f46bf9d71c80@ti.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <51651E7A11DB4D408180A3F356864551@namprd06.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b4ffe58-559a-433b-a110-08d54ee68265
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Dec 2017 18:03:47.9467
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 6de70f0f-7357-4529-a415-d8cbb7e93e5e
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR0601MB3756
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id vBTI3vM6001239
Content-Length: 2144
Lines: 54

On Fri, 2017-12-29 at 15:46 +0530, Vignesh R wrote:
> On Friday 29 December 2017 12:24 AM, Trent Piepho wrote:
> > 
> > > Vignesh has suggested to call virt_addr_valid() instead.
> > > I think Boris has also told me about this function.
> > > So it might be the right solution. What do you think about their proposal?
> > 
> > Not sure what exactly the differences are between these methods.  The
> > fact that each of the many existing DMA fixes uses slightly different
> > code to detect what is unsafe speaks to the difficulty of this problem!
> 
> My understanding based on Documentation/DMA-API-HOWTO.txt and
> Documentation/arm/memory.txt is that
> virt_addr_valid() will guarantee that address is in range of
> PAGE_OFFSET to high_memory-1 (Kernel direct-mapped RAM region) which is
> address range of buffers that are DMA'able.

There's code in gpmi-nand.c that does:

        /* first try to map the upper buffer directly */
        if (virt_addr_valid(this->upper_buf) &&
                !object_is_on_stack(this->upper_buf)) {
                sg_init_one(sgl, this->upper_buf, this->upper_len);

So whoever wrote that thought that stack objects needed an additional
test beyond virt_addr_valid.  But it does appear to be far more common
to depend on just virt_addr_valid, so perhaps the code in gpmi-nand is
in error.

> >  virt_addr_valid() is already used by spi-ti-qspi.  spi core uses for
> > the buffer map helper, but that code path is for buffers which are NOT
> > vmalloc or highmem, but are still not virt_addr_valid() for some other
> > reason.
> > 
> 
> 	if (vmalloced_buf || kmap_buf) {
> 		/* Handle vmalloc'd or kmap'd buffers */
> 		...
This stuff does get DMAed.  So I have to wonder, if spi.c thinks it can
use DMA with vmalloc or highmem, couldn't spi-not do the same instead
of the bounce buffer?

>         } else if (virt_addr_valid(buf)) {
> 		/* Handle kmalloc'd and such buffers */
>                 ...
> 	} else {
> 		/* Error if none of the above */

So what is this case here for?  It's some class that does not have a
valid virtual address and yet is not vmalloc or highmem.

> 		return -EINVAL;
> 	}
>