Re: [PATCH v9 RESEND 01/13] spi: imx: add dma_sync_sg_for_device after fallback from dma

From: Mark Brown <broonie@kernel.org>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Robin Gong <yibin.gong@nxp.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"matthias.schiffer@ew.tq-group.com" 
	<matthias.schiffer@ew.tq-group.com>,
	"martin.fuzzey@flowbird.group" <martin.fuzzey@flowbird.group>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"will.deacon@arm.com" <will.deacon@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
	"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v9 RESEND 01/13] spi: imx: add dma_sync_sg_for_device after fallback from dma
Date: Tue, 9 Jun 2020 14:36:27 +0100	[thread overview]
Message-ID: <20200609133627.GG4583@sirena.org.uk> (raw)
In-Reply-To: <bcfc3637-65af-577a-ddbd-890b6c83a6e6@arm.com>

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On Tue, Jun 09, 2020 at 11:00:33AM +0100, Robin Murphy wrote:

> Ah, I think I understand what's going on now. That's... really ugly :(

> Looking at the SPI core code, I think a better way to handle this would be
> to have your fallback path call spi_unmap_buf() directly (or perform the
> same actions, if exporting that to drivers is unacceptable), then make sure
> ->can_dma() returns false after that such that spi_unmap_msg() won't try to
> unmap it again. That's a lot more reasonable than trying to fake up a
> DMA_TO_DEVICE transfer in the middle of a DMA_FROM_DEVICE operation on the
> same buffer.

Ideally the driver would be checking in can_dma() if the DMA controller
is able to perform transactions rather than letting things run as far as 
trying to actually do the transfer, that's a whole lot cleaner and more
manageable than running into an error doing the transfer.  I'm surprised
that there's no DMA API way to figure this out TBH.

We'll also need some handling for this changing at runtime, we're not
expecting this to be dynamic at all - we're expecting it to be a static
property of the controller/transfer combination, we didn't contemplate
this varying randomly at runtime.  Instead of rechecking can_dma() we
ought to have a flag saying if we did the mapping (which the bodge Robin
suggests above could clear).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]