| 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 |
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.
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
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 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
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
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
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 -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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>
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 --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);
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(-) -- 2.25.1