Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754131AbdL1Syo (ORCPT ); Thu, 28 Dec 2017 13:54:44 -0500 Received: from mail-co1nam03on0116.outbound.protection.outlook.com ([104.47.40.116]:29817 "EHLO NAM03-CO1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751220AbdL1Syl (ORCPT ); Thu, 28 Dec 2017 13:54:41 -0500 From: Trent Piepho To: "linux-mtd@lists.infradead.org" , "linux@armlinux.org.uk" , "broonie@kernel.org" , "cyrille.pitchen@wedev4u.fr" , "dwmw2@infradead.org" , "computersforpeace@gmail.com" , "vigneshr@ti.com" , "boris.brezillon@free-electrons.com" , "richard@nod.at" , "marek.vasut@gmail.com" CC: "linux-spi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "nicolas.ferre@microchip.com" , "radu.pirea@microchip.com" , "robh@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/6NWCpOAgAKMrwCAAIpvAA== Date: Thu, 28 Dec 2017 18:54:39 +0000 Message-ID: <1514487276.26695.94.camel@impinj.com> References: <1514317385.26695.39.camel@impinj.com> <1a7dc424-1ce0-6c64-fc52-bb88ec7db8fa@wedev4u.fr> In-Reply-To: <1a7dc424-1ce0-6c64-fc52-bb88ec7db8fa@wedev4u.fr> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=tpiepho@impinj.com; x-originating-ip: [216.243.31.162] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;MWHPR0601MB3755;7:4Avwh15ABNAT06cadRREwll9yH/pnodZTohihD9kp7Ur5CNbKKWrVcHO//H3mxMwx9CFMhSusltdch7RAp6VOi/HylcqecUbDTFQzna1nikWJSILH8tVs+xsxzdfyGID9JROJ3FQls+A3DXaPyX73iwrU5bGI7DhnEB3kHWwMCs+E+OwY85AK46afgwR0NSHFpXT067MSnYtOhkwFh2mvjhjqSkp+E+VUfRu1bIWVIGeCK0mRD64QBvJ7sE85lQ8 x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 52790349-d850-410b-969d-08d54e2472c5 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(5600026)(4604075)(3008032)(2017052603307)(7153060);SRVR:MWHPR0601MB3755; x-ms-traffictypediagnostic: MWHPR0601MB3755: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(17755550239193); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040470)(2401047)(5005006)(8121501046)(3231023)(944501075)(10201501046)(3002001)(93006095)(93001095)(6041268)(20161123564045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123562045)(6072148)(201708071742011);SRVR:MWHPR0601MB3755;BCL:0;PCL:0;RULEID:(100000803101)(100110400095);SRVR:MWHPR0601MB3755; x-forefront-prvs: 05352A48BE x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(376002)(346002)(39850400004)(39380400002)(366004)(396003)(76104003)(189003)(199004)(78114003)(54094003)(24454002)(377424004)(52314003)(25786009)(66066001)(68736007)(5250100002)(103116003)(6116002)(93886005)(97736004)(6486002)(3660700001)(14454004)(86362001)(102836004)(3280700002)(6512007)(6246003)(2201001)(59450400001)(6506007)(6436002)(53936002)(8936002)(2906002)(5660300001)(478600001)(81156014)(305945005)(105586002)(81166006)(217423001)(8676002)(76176011)(4326008)(36756003)(229853002)(7736002)(561944003)(106356001)(7416002)(2900100001)(39060400002)(99286004)(2501003)(54906003)(4001150100001)(110136005)(2950100002)(316002)(3846002)(921003)(1121003);DIR:OUT;SFP:1102;SCL:1;SRVR:MWHPR0601MB3755;H:MWHPR0601MB3753.namprd06.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; x-microsoft-antispam-message-info: sW9zt5N/XclDxRItPyE2U28/0dJgwgdebxRQPmZRnFV9RxoWGPHftEvTfOXWJ2XYM27RAfPthj1FCioh9E+Ngg== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: <19614AEFEB7D234F8E504F8164313343@namprd06.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: impinj.com X-MS-Exchange-CrossTenant-Network-Message-Id: 52790349-d850-410b-969d-08d54e2472c5 X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Dec 2017 18:54:39.3352 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 6de70f0f-7357-4529-a415-d8cbb7e93e5e X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR0601MB3755 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id vBSItlsl003121 Content-Length: 5966 Lines: 134 On Thu, 2017-12-28 at 11:39 +0100, Cyrille Pitchen wrote: > Le 26/12/2017 à 20:43, Trent Piepho a écrit : > > On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: > > > > > > Then the patch adds two hardware capabilities for SPI flash controllers, > > > SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. > > > > Are there any drivers for which a bounce buffer is NOT needed when the > > tx/rx buffer is not in DMA safe memory? Maybe it would make more sense > > to invert the sense of these flags, so that they indicate the driver > > does not need DMA safe buffers, if that is the uncommon/non-existent > > case, so that fewer drivers need to be modified to to be fixed? > > > > It doesn't sound safe for a first step. I don't know if some of the > spi-flash controllers are embedded inside systems with small memory and > don't care about DMA transfers. Maybe they don't plan to use anything else > but PIO transfers. Then why taking the risk to exhaust the memory on systems > that would not use the bounce buffer anyway? This would certainly be the case when the driver does not even support DMA in the first place. This also makes me wonder, how inefficient does this become when it uses a bounce buffer for small transfer that would not use DMA anyway? In the spi_flash_read() interface for spi masters, there is a master method spi_flash_can_dma() callback used by the spi core to tell if each transfer can be DMAed. Should something like that be used here, to ask the master if it would use dma on the buffer? This might also prevent allocation of the bounce buffer if the only DMA unsafe transfers are tiny control ops with stack variables that wouldn't use DMA, e.g. the stuff spi_nor_read_sfdp_dma_unsafe() does. > About the memory loss when forcing the SNOR_HWCAPS_*_BOUNCE in m25p80.c, I > justify it because the m25p80 has to be compliant with the SPI sub-system > requirements but currently is not. However I've taken care not to allocate > the bounce buffer as long as we use only DMA-safe buffers. Another possibility to reduce memory usage: make the buffer smaller when first allocated by being just enough for the needed space. Grow it (by powers of two?) until it reaches the max allowed size. No reason to use a 256 kB buffer if DMA unsafe operations never get that big. > Here at the MTD side, the main (only ?) source of DMA-unsafe buffers is > the UBIFS (JFFS2 too ?) layer. Then I've assumed that systems using such a > file-system should already have enough system memory. I saw a note in one of the existing DMA fixes that JFFS2 was the source of the unsafe buffers, so probably there too. > > 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! 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. > > > +static int spi_nor_get_bounce_buffer(struct spi_nor *nor, > > > + u_char **buffer, > > > + size_t *buffer_size) > > > +{ > > > + > > > + *buffer = nor->bounce_buffer; > > > + *buffer_size = size; > > > > So the buffer is returned via the parameter, and also via a field > > inside nor. Seems redundant. Consider address could be returned via > > the function return value coupled with PTR_ERR() for the error cases. > > Or not return address at all since it's available via nor- > > > bounce_buffer. > > Why not. It would require more lines though. I guess it's purely a matter of taste. Well, also consider that you don't need to even return the buffer pointer at all, since it's available as nor->bounce_buffer. Which it is used as in spi_nor_write() and spi_nor_read(). > > This pattern, check if bounce is enabled, check if address is dma- > > unsafe, get bounce buffer, seems to be very common. Could it be > > refactored into one helper? > > > > u_char *buffer = spi_nor_check_bounce(nor, buf, len, &buffer_size); > > The conditions that define the value of 'use_bounce' also depend on the type > of operation, read or write, hence on the two different flags > SNOR_F_USE_RD_BOUNCE and SNOR_F_USE_WR_BOUNCE. Just pass one of those flags as an argument to tell what direction it is in. Though I wonder if using a bounce buffer for only one direction will ever be used. > > Besides, 'use_bounce' is also tested later in spi_nor_read(), sst_write() > and sst_write(). > > So I don't really see how the spi_nor_check_bounce() function you propose > could be that different from spi_nor_get_bounce_buffer(). > > > if (IS_ERR(buffer)) > > return PTR_ERR(buffer); > > // buffer = nor->bounce_buffer or buf, whichever is correct > > // buffer_size = len or bounce buffer size, whichever is correct > > > > Could spi_nor_read_sfdp_dma_unsafe() also use this buffer? > > > > I didn't use the bounce buffer in spi_nor_read_sfdp_dma_unsafe() on > purpose: the bounce buffer, when needed, is allocated once for all to limit > performance loss. However, to avoid increasing the memory footprint, if not > absolutely needed the bounce buffer is not allocated at all. spi-nor tries to provide a common implementation of DMA bounce buffers, yet spi-nor itself has two different DMA bounce buffer implementations. I think the real answer for spi_nor_read_sfdp_dma_unsafe() is that it shouldn't be written that way and the function should go away. The two call sites should just kmalloc the struct they read into instead of placing it on the stack. The dma_unsafe wrapper kmallocs a buffer anyway, so it's not like there is any more allocation going on.