diff mbox series

[v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset

Message ID 20200901150438.228887-1-ulf.hansson@linaro.org
State New
Headers show
Series [v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset | expand

Commit Message

Ulf Hansson Sept. 1, 2020, 3:04 p.m. UTC
The commit cd57d07b1e4e ("sh: don't allow non-coherent DMA for NOMMU") made
CONFIG_NO_DMA to be set for some platforms, for good reasons.
Consequentially, CONFIG_HAS_DMA doesn't get set, which makes the DMA
mapping interface to be built as stub functions, but also prevent the
mmc_spi driver from being built as it depends on CONFIG_HAS_DMA.

It turns out that for some odd cases, the driver still relied on the DMA
mapping interface, even if the DMA was not actively being used.

To fixup the behaviour, let's drop the build dependency for CONFIG_HAS_DMA.
Moreover, as to allow the driver to succeed probing, let's move the DMA
initializations behind "#ifdef CONFIG_HAS_DMA".

Fixes: cd57d07b1e4e ("sh: don't allow non-coherent DMA for NOMMU")
Reported-by: Rich Felker <dalias@libc.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Drop build dependency to CONFIG_HAS_DMA.
	- Rephrase commit message and its header, to reflect the updated change.

---
 drivers/mmc/host/Kconfig   |  2 +-
 drivers/mmc/host/mmc_spi.c | 86 +++++++++++++++++++++++---------------
 2 files changed, 53 insertions(+), 35 deletions(-)

Comments

Christoph Hellwig Sept. 1, 2020, 3:06 p.m. UTC | #1
On Tue, Sep 01, 2020 at 05:04:38PM +0200, Ulf Hansson wrote:
> +#ifdef CONFIG_HAS_DMA
> +static int mmc_spi_dma_alloc(struct mmc_spi_host *host)
> +{
> +	struct spi_device *spi = host->spi;
> +	struct device *dev;
> +
> +	if (!spi->master->dev.parent->dma_mask)
> +		return 0;

I still don't think this makes sense, as the dma_mask should always
be non-NULL here.
Ulf Hansson Sept. 1, 2020, 3:36 p.m. UTC | #2
On Tue, 1 Sep 2020 at 17:06, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Sep 01, 2020 at 05:04:38PM +0200, Ulf Hansson wrote:
> > +#ifdef CONFIG_HAS_DMA
> > +static int mmc_spi_dma_alloc(struct mmc_spi_host *host)
> > +{
> > +     struct spi_device *spi = host->spi;
> > +     struct device *dev;
> > +
> > +     if (!spi->master->dev.parent->dma_mask)
> > +             return 0;
>
> I still don't think this makes sense, as the dma_mask should always
> be non-NULL here.

If that is the case, I wonder how the driver could even have worked without DMA.

Because in the existing code, host->dma_dev gets assigned to
spi->master->dev.parent->dma_mask - which seems to turn on the DMA
usage in the driver.

What am I missing?

Kind regards
Uffe
Christoph Hellwig Sept. 1, 2020, 3:40 p.m. UTC | #3
On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > I still don't think this makes sense, as the dma_mask should always
> > be non-NULL here.
> 
> If that is the case, I wonder how the driver could even have worked without DMA.
> 
> Because in the existing code, host->dma_dev gets assigned to
> spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> usage in the driver.
> 
> What am I missing?

Do you know of other non-DMA users?  For SH nommu it probably worked
because SH nommu used to provide a DMA implementation that worked
fine for streaming maps, but was completely broken for coherent
allocation.  And this driver appears to only use the former.
Ulf Hansson Sept. 2, 2020, 8:31 a.m. UTC | #4
On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > I still don't think this makes sense, as the dma_mask should always
> > > be non-NULL here.
> >
> > If that is the case, I wonder how the driver could even have worked without DMA.
> >
> > Because in the existing code, host->dma_dev gets assigned to
> > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > usage in the driver.
> >
> > What am I missing?
>
> Do you know of other non-DMA users?  For SH nommu it probably worked

I don't know of other non-DMA users. As I said, I wish someone could
step in and take better care of mmc_spi - as I know it's being used a
lot.

> because SH nommu used to provide a DMA implementation that worked
> fine for streaming maps, but was completely broken for coherent
> allocation.  And this driver appears to only use the former.

Alright, so you are saying the DMA support may potentially never have
been optional to this driver. In any case, I can remove the check in
$subject patch, as it shouldn't matter.

Anyway, let's see what Rich thinks of this. I am curious to see if the
patch works on his SH boards - as I haven't been able to test it.

Kind regards
Uffe
Rich Felker Sept. 2, 2020, 1:34 p.m. UTC | #5
On Tue, Sep 01, 2020 at 05:40:49PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > I still don't think this makes sense, as the dma_mask should always
> > > be non-NULL here.
> > 
> > If that is the case, I wonder how the driver could even have worked without DMA.
> > 
> > Because in the existing code, host->dma_dev gets assigned to
> > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > usage in the driver.
> > 
> > What am I missing?
> 
> Do you know of other non-DMA users?  For SH nommu it probably worked
> because SH nommu used to provide a DMA implementation that worked
> fine for streaming maps, but was completely broken for coherent
> allocation.  And this driver appears to only use the former.

On the system we're using it on, there's no DMA whatsoever. It just
used to allow memory allocations suitable for DMA (which any
allocation vacuuously is when there's no DMA) but now it doesn't, due
to your change.

Just below the if block in question in this thread is:

	host->readback.is_dma_mapped = (host->dma_dev != NULL);

and similar conditions appear elsewhere all over the file in the form
of if (host->dma_dev). Of course doing DMA requires a link to a DMA
controller device, and plenty SPI devices (most obviously bit-banged
ones) lack DMA.

The right condition for the block in question here is probably just
checking dma_dev instead of dma_mask or CONFIG_HAS_DMA, but it seems
useful to optimize out the code if CONFIG_HAS_DMA is false, anyway.

Rich
Rich Felker Sept. 2, 2020, 1:44 p.m. UTC | #6
On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > I still don't think this makes sense, as the dma_mask should always
> > > > be non-NULL here.
> > >
> > > If that is the case, I wonder how the driver could even have worked without DMA.
> > >
> > > Because in the existing code, host->dma_dev gets assigned to
> > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > usage in the driver.
> > >
> > > What am I missing?
> >
> > Do you know of other non-DMA users?  For SH nommu it probably worked
> 
> I don't know of other non-DMA users. As I said, I wish someone could
> step in and take better care of mmc_spi - as I know it's being used a
> lot.
> 
> > because SH nommu used to provide a DMA implementation that worked
> > fine for streaming maps, but was completely broken for coherent
> > allocation.  And this driver appears to only use the former.
> 
> Alright, so you are saying the DMA support may potentially never have
> been optional to this driver. In any case, I can remove the check in
> $subject patch, as it shouldn't matter.

DMA support was always optional, because even on systems where DMA is
present, it doesn't necessarily mean the SPI controller uses DMA. In
particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
always worked. See my previous reply to Christoph about host->dma_dev
for my current-best understanding of what's going on here.

> Anyway, let's see what Rich thinks of this. I am curious to see if the
> patch works on his SH boards - as I haven't been able to test it.

I'll rebuild and retest just to confirm, but I already tested a
functionally equivalent patch that just did the #ifdef inline (rather
than moving the logic out to separate functions) and it worked fine.

Rich
Geert Uytterhoeven Sept. 2, 2020, 3:51 p.m. UTC | #7
Hi Rich,

On Wed, Sep 2, 2020 at 5:43 PM Rich Felker <dalias@libc.org> wrote:
> On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> > On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> > > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > > I still don't think this makes sense, as the dma_mask should always
> > > > > be non-NULL here.
> > > >
> > > > If that is the case, I wonder how the driver could even have worked without DMA.
> > > >
> > > > Because in the existing code, host->dma_dev gets assigned to
> > > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > > usage in the driver.
> > > >
> > > > What am I missing?
> > >
> > > Do you know of other non-DMA users?  For SH nommu it probably worked
> >
> > I don't know of other non-DMA users. As I said, I wish someone could
> > step in and take better care of mmc_spi - as I know it's being used a
> > lot.
> >
> > > because SH nommu used to provide a DMA implementation that worked
> > > fine for streaming maps, but was completely broken for coherent
> > > allocation.  And this driver appears to only use the former.
> >
> > Alright, so you are saying the DMA support may potentially never have
> > been optional to this driver. In any case, I can remove the check in
> > $subject patch, as it shouldn't matter.
>
> DMA support was always optional, because even on systems where DMA is
> present, it doesn't necessarily mean the SPI controller uses DMA. In
> particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
> always worked. See my previous reply to Christoph about host->dma_dev
> for my current-best understanding of what's going on here.
>
> > Anyway, let's see what Rich thinks of this. I am curious to see if the
> > patch works on his SH boards - as I haven't been able to test it.
>
> I'll rebuild and retest just to confirm, but I already tested a
> functionally equivalent patch that just did the #ifdef inline (rather
> than moving the logic out to separate functions) and it worked fine.

Hence, Tested-by? ;-)

Thanks!

Gr{oetje,eeting}s,

                        Geert
Rich Felker Sept. 3, 2020, 12:41 a.m. UTC | #8
On Wed, Sep 02, 2020 at 05:51:16PM +0200, Geert Uytterhoeven wrote:
> Hi Rich,
> 
> On Wed, Sep 2, 2020 at 5:43 PM Rich Felker <dalias@libc.org> wrote:
> > On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> > > On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> > > > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > > > I still don't think this makes sense, as the dma_mask should always
> > > > > > be non-NULL here.
> > > > >
> > > > > If that is the case, I wonder how the driver could even have worked without DMA.
> > > > >
> > > > > Because in the existing code, host->dma_dev gets assigned to
> > > > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > > > usage in the driver.
> > > > >
> > > > > What am I missing?
> > > >
> > > > Do you know of other non-DMA users?  For SH nommu it probably worked
> > >
> > > I don't know of other non-DMA users. As I said, I wish someone could
> > > step in and take better care of mmc_spi - as I know it's being used a
> > > lot.
> > >
> > > > because SH nommu used to provide a DMA implementation that worked
> > > > fine for streaming maps, but was completely broken for coherent
> > > > allocation.  And this driver appears to only use the former.
> > >
> > > Alright, so you are saying the DMA support may potentially never have
> > > been optional to this driver. In any case, I can remove the check in
> > > $subject patch, as it shouldn't matter.
> >
> > DMA support was always optional, because even on systems where DMA is
> > present, it doesn't necessarily mean the SPI controller uses DMA. In
> > particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
> > always worked. See my previous reply to Christoph about host->dma_dev
> > for my current-best understanding of what's going on here.
> >
> > > Anyway, let's see what Rich thinks of this. I am curious to see if the
> > > patch works on his SH boards - as I haven't been able to test it.
> >
> > I'll rebuild and retest just to confirm, but I already tested a
> > functionally equivalent patch that just did the #ifdef inline (rather
> > than moving the logic out to separate functions) and it worked fine.
> 
> Hence, Tested-by? ;-)

Confirmed that this version of the patch works too. Thus,

Tested-by: Rich Felker <dalias@libc.org>
Ulf Hansson Sept. 3, 2020, 8:10 a.m. UTC | #9
On Thu, 3 Sep 2020 at 02:41, Rich Felker <dalias@libc.org> wrote:
>
> On Wed, Sep 02, 2020 at 05:51:16PM +0200, Geert Uytterhoeven wrote:
> > Hi Rich,
> >
> > On Wed, Sep 2, 2020 at 5:43 PM Rich Felker <dalias@libc.org> wrote:
> > > On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> > > > On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> > > > > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > > > > I still don't think this makes sense, as the dma_mask should always
> > > > > > > be non-NULL here.
> > > > > >
> > > > > > If that is the case, I wonder how the driver could even have worked without DMA.
> > > > > >
> > > > > > Because in the existing code, host->dma_dev gets assigned to
> > > > > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > > > > usage in the driver.
> > > > > >
> > > > > > What am I missing?
> > > > >
> > > > > Do you know of other non-DMA users?  For SH nommu it probably worked
> > > >
> > > > I don't know of other non-DMA users. As I said, I wish someone could
> > > > step in and take better care of mmc_spi - as I know it's being used a
> > > > lot.
> > > >
> > > > > because SH nommu used to provide a DMA implementation that worked
> > > > > fine for streaming maps, but was completely broken for coherent
> > > > > allocation.  And this driver appears to only use the former.
> > > >
> > > > Alright, so you are saying the DMA support may potentially never have
> > > > been optional to this driver. In any case, I can remove the check in
> > > > $subject patch, as it shouldn't matter.
> > >
> > > DMA support was always optional, because even on systems where DMA is
> > > present, it doesn't necessarily mean the SPI controller uses DMA. In
> > > particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
> > > always worked. See my previous reply to Christoph about host->dma_dev
> > > for my current-best understanding of what's going on here.
> > >
> > > > Anyway, let's see what Rich thinks of this. I am curious to see if the
> > > > patch works on his SH boards - as I haven't been able to test it.
> > >
> > > I'll rebuild and retest just to confirm, but I already tested a
> > > functionally equivalent patch that just did the #ifdef inline (rather
> > > than moving the logic out to separate functions) and it worked fine.
> >
> > Hence, Tested-by? ;-)
>
> Confirmed that this version of the patch works too. Thus,
>
> Tested-by: Rich Felker <dalias@libc.org>

I have applied the patch for fixes, thanks for testing!

Christoph, when it comes to the check of
"spi->master->dev.parent->dma_mask", I am keeping it for now. I am
simply not sure that all spi masters assign the pointer (even if most
are platform drivers). I think it's better that we remove that check
in a separate patch - to get it tested.

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9c89a5b780e8..9a34c827c96e 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -602,7 +602,7 @@  config MMC_GOLDFISH
 
 config MMC_SPI
 	tristate "MMC/SD/SDIO over SPI"
-	depends on SPI_MASTER && HAS_DMA
+	depends on SPI_MASTER
 	select CRC7
 	select CRC_ITU_T
 	help
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 39bb1e30c2d7..5055a7eb134a 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1278,6 +1278,52 @@  mmc_spi_detect_irq(int irq, void *mmc)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_HAS_DMA
+static int mmc_spi_dma_alloc(struct mmc_spi_host *host)
+{
+	struct spi_device *spi = host->spi;
+	struct device *dev;
+
+	if (!spi->master->dev.parent->dma_mask)
+		return 0;
+
+	dev = spi->master->dev.parent;
+
+	host->ones_dma = dma_map_single(dev, host->ones, MMC_SPI_BLOCKSIZE,
+					DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, host->ones_dma))
+		return -ENOMEM;
+
+	host->data_dma = dma_map_single(dev, host->data, sizeof(*host->data),
+					DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(dev, host->data_dma)) {
+		dma_unmap_single(dev, host->ones_dma, MMC_SPI_BLOCKSIZE,
+				 DMA_TO_DEVICE);
+		return -ENOMEM;
+	}
+
+	dma_sync_single_for_cpu(dev, host->data_dma, sizeof(*host->data),
+				DMA_BIDIRECTIONAL);
+
+	host->dma_dev = dev;
+	return 0;
+}
+
+static void mmc_spi_dma_free(struct mmc_spi_host *host)
+{
+	if (!host->dma_dev)
+		return;
+
+	dma_unmap_single(host->dma_dev, host->ones_dma, MMC_SPI_BLOCKSIZE,
+			 DMA_TO_DEVICE);
+	dma_unmap_single(host->dma_dev, host->data_dma,	sizeof(*host->data),
+			 DMA_BIDIRECTIONAL);
+}
+#else
+static inline mmc_spi_dma_alloc(struct mmc_spi_host *host) { return 0; }
+static inline void mmc_spi_dma_free(struct mmc_spi_host *host) {}
+#endif
+
 static int mmc_spi_probe(struct spi_device *spi)
 {
 	void			*ones;
@@ -1374,23 +1420,9 @@  static int mmc_spi_probe(struct spi_device *spi)
 	if (!host->data)
 		goto fail_nobuf1;
 
-	if (spi->master->dev.parent->dma_mask) {
-		struct device	*dev = spi->master->dev.parent;
-
-		host->dma_dev = dev;
-		host->ones_dma = dma_map_single(dev, ones,
-				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
-		if (dma_mapping_error(dev, host->ones_dma))
-			goto fail_ones_dma;
-		host->data_dma = dma_map_single(dev, host->data,
-				sizeof(*host->data), DMA_BIDIRECTIONAL);
-		if (dma_mapping_error(dev, host->data_dma))
-			goto fail_data_dma;
-
-		dma_sync_single_for_cpu(host->dma_dev,
-				host->data_dma, sizeof(*host->data),
-				DMA_BIDIRECTIONAL);
-	}
+	status = mmc_spi_dma_alloc(host);
+	if (status)
+		goto fail_dma;
 
 	/* setup message for status/busy readback */
 	spi_message_init(&host->readback);
@@ -1458,20 +1490,12 @@  static int mmc_spi_probe(struct spi_device *spi)
 fail_add_host:
 	mmc_remove_host(mmc);
 fail_glue_init:
-	if (host->dma_dev)
-		dma_unmap_single(host->dma_dev, host->data_dma,
-				sizeof(*host->data), DMA_BIDIRECTIONAL);
-fail_data_dma:
-	if (host->dma_dev)
-		dma_unmap_single(host->dma_dev, host->ones_dma,
-				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
-fail_ones_dma:
+	mmc_spi_dma_free(host);
+fail_dma:
 	kfree(host->data);
-
 fail_nobuf1:
 	mmc_free_host(mmc);
 	mmc_spi_put_pdata(spi);
-
 nomem:
 	kfree(ones);
 	return status;
@@ -1489,13 +1513,7 @@  static int mmc_spi_remove(struct spi_device *spi)
 
 	mmc_remove_host(mmc);
 
-	if (host->dma_dev) {
-		dma_unmap_single(host->dma_dev, host->ones_dma,
-			MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
-		dma_unmap_single(host->dma_dev, host->data_dma,
-			sizeof(*host->data), DMA_BIDIRECTIONAL);
-	}
-
+	mmc_spi_dma_free(host);
 	kfree(host->data);
 	kfree(host->ones);