mmc: core: Lower max_seg_size if too high for DMA

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +14 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

@@ -415,6 +416,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 EXPORT_SYMBOL(mmc_alloc_host);
 
+static void mmc_check_max_seg_size(struct mmc_host *host)
+{
+	unsigned int max_seg_size = dma_get_max_seg_size(mmc_dev(host));
+
+	if (host->max_seg_size <= max_seg_size)
+		return;
+
+	dev_info(mmc_dev(host), "Lowering max_seg_size for DMA: %u vs %u\n",
+		 host->max_seg_size, max_seg_size);
+
+	host->max_seg_size = max_seg_size;
+}
+
 /**
  *	mmc_add_host - initialise host hardware
  *	@host: mmc host

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +1 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

drivers/mmc/core/host.c +12 -1

+
+       if (host->max_seg_size <= max_seg_size)
+               return;
+
+       dev_info(mmc_dev(host), "Lowering max_seg_size for DMA: %u vs %u\n",
+                host->max_seg_size, max_seg_size);
+
+       host->max_seg_size = max_seg_size;
+}
+
 /**
  *     mmc_add_host - initialise host hardware
  *     @host: mmc host

@@ -430,6 +444,8 @@ int mmc_add_host(struct mmc_host *host)
        WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
                !host->ops->enable_sdio_irq);
 
+       mmc_check_max_seg_size(host);
+
        err = device_add(&host->class_dev);
        if (err)
                return err;
--

2.19.1

Quoted 32 lines · Tony Lindgren

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +1 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

drivers/mmc/core/host.c struct mmc_host *mmc_alloc_host(int extra, struct device *dev) · line 421

Quoted 6 lines

drivers/mmc/core/host.c +3 -0

@@ -415,6 +416,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 EXPORT_SYMBOL(mmc_alloc_host);
 
+static void mmc_check_max_seg_size(struct mmc_host *host)
+{
+       unsigned int max_seg_size = dma_get_max_seg_size(mmc_dev(host));

Is dma_get_max_seg_size() really intended to be called for any struct device (representing the mmc controller) like this?

My understanding is that the dma_get_max_seg_size() is supposed to be called by using the DMA engine device, no?

Quoted 43 lines · Ulf Hansson

Quoted 32 lines · Tony Lindgren

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +1 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

drivers/mmc/core/host.c struct mmc_host *mmc_alloc_host(int extra, struct device *dev) · line 421

Quoted 6 lines

drivers/mmc/core/host.c +3 -0

@@ -415,6 +416,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 EXPORT_SYMBOL(mmc_alloc_host);
 
+static void mmc_check_max_seg_size(struct mmc_host *host)
+{
+       unsigned int max_seg_size = dma_get_max_seg_size(mmc_dev(host));

Is dma_get_max_seg_size() really intended to be called for any struct device (representing the mmc controller) like this?

Quoted 2 lines

My understanding is that the dma_get_max_seg_size() is supposed to be called by using the DMA engine device, no?

Oh good catch sounds like I'm calling it for the wrong device, need to check. In that case sounds like this can't be generic?

Quoted 45 lines · Tony Lindgren

Quoted 43 lines · Ulf Hansson

Quoted 32 lines · Tony Lindgren

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +1 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

drivers/mmc/core/host.c struct mmc_host *mmc_alloc_host(int extra, struct device *dev) · line 421

Quoted 6 lines

drivers/mmc/core/host.c +3 -0

@@ -415,6 +416,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 EXPORT_SYMBOL(mmc_alloc_host);
 
+static void mmc_check_max_seg_size(struct mmc_host *host)
+{
+       unsigned int max_seg_size = dma_get_max_seg_size(mmc_dev(host));

Is dma_get_max_seg_size() really intended to be called for any struct device (representing the mmc controller) like this?

Quoted 5 lines

Quoted 2 lines

My understanding is that the dma_get_max_seg_size() is supposed to be called by using the DMA engine device, no?

Oh good catch sounds like I'm calling it for the wrong device, need to check. In that case sounds like this can't be generic?

No, I don't think so as it's only the mmc host driver that knows about the DMA engine device.

Quoted 46 lines · Ulf Hansson

Quoted 45 lines · Tony Lindgren

Quoted 43 lines · Ulf Hansson

Quoted 32 lines · Tony Lindgren

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +1 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

drivers/mmc/core/host.c struct mmc_host *mmc_alloc_host(int extra, struct device *dev) · line 421

Quoted 6 lines

drivers/mmc/core/host.c +3 -0

@@ -415,6 +416,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 EXPORT_SYMBOL(mmc_alloc_host);
 
+static void mmc_check_max_seg_size(struct mmc_host *host)
+{
+       unsigned int max_seg_size = dma_get_max_seg_size(mmc_dev(host));

Is dma_get_max_seg_size() really intended to be called for any struct device (representing the mmc controller) like this?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

My understanding is that the dma_get_max_seg_size() is supposed to be called by using the DMA engine device, no?

Oh good catch sounds like I'm calling it for the wrong device, need to check. In that case sounds like this can't be generic?

No, I don't think so as it's only the mmc host driver that knows about the DMA engine device.

We're nearing the merge window, and this is a regression that is yet to be solved. It causes a kernel warning with backtrace, so it's very annoying.

The error is:

omap-dma-engine 4a056000.dma-controller: DMA-API: mapping sg segment longer than device claims to support [len=69632] [max=65536]

which indicates that we have a SG segment length that exceeds thte published maximum segment size for a device - in this case the DMA engine device. The maximum segment size for the DMA engine comes from the default per-device setting, in linux/dma-mapping.h, which is 64K.

However, omap_hsmmc sets:

        mmc->max_blk_size = 512;       /* Block Length at max can be 1024 */
        mmc->max_blk_count = 0xFFFF;    /* No. of Blocks is 16 bits */
        mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
        mmc->max_seg_size = mmc->max_req_size;

which ends up telling the block layer that we support a maximum segment size of 65535*512, so of course it _will_ pass SG lists where a segment is longer than 64K.

The problem here is that the HSMMC driver doesn't take account of the DMA engine device's capabilities.

We have something of an odd situation in that the omap-dma device's maximum SG size depends on the "address width" - it's 64K transfers of whatever unit "address width" is, so the current implementation of per-device parameters doesn't exactly work. That means the default 64K limit at the device-level is reasonable.

The only thing I can think of doing is adding into omap_hsmmc:

        mmc->max_seg_size = min(mmc->max_req_size,
                                min(dma_get_max_seg_size(host->rx_chan->device->dev),
                                    dma_get_max_seg_size(host->tx_chan->device->dev)));

to limit the maximum segment size to that of the device _and_ dma engine's capabilities.

Doing this solves the problem for me.

Quoted 97 lines · Russell King - ARM Linux

Quoted 46 lines · Ulf Hansson

Quoted 45 lines · Tony Lindgren

Quoted 43 lines · Ulf Hansson

Quoted 32 lines · Tony Lindgren

With CONFIG_DMA_API_DEBUG_SG a device may produce the following warning:

"DMA-API: mapping sg segment longer than device claims to support"

We default to 64KiB if a DMA engine driver does not initialize dma_parms and call dma_set_max_seg_size(). This may be lower that what many MMC drivers do with mmc->max_seg_size = mmc->max_blk_size * mmc->max_blk_count.

Let's do a sanity check for max_seg_size being higher than what DMA supports in mmc_add_host() and lower it as needed.

Cc Kishon Vijay Abraham I <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Cc Russell King <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/mmc/core/host.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mmc/core/host.c +1 -0

@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/of.h>

drivers/mmc/core/host.c struct mmc_host *mmc_alloc_host(int extra, struct device *dev) · line 421

Quoted 6 lines

drivers/mmc/core/host.c +3 -0

@@ -415,6 +416,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 EXPORT_SYMBOL(mmc_alloc_host);
 
+static void mmc_check_max_seg_size(struct mmc_host *host)
+{
+       unsigned int max_seg_size = dma_get_max_seg_size(mmc_dev(host));

Is dma_get_max_seg_size() really intended to be called for any struct device (representing the mmc controller) like this?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

My understanding is that the dma_get_max_seg_size() is supposed to be called by using the DMA engine device, no?

Oh good catch sounds like I'm calling it for the wrong device, need to check. In that case sounds like this can't be generic?

No, I don't think so as it's only the mmc host driver that knows about the DMA engine device.

We're nearing the merge window, and this is a regression that is yet to be solved. It causes a kernel warning with backtrace, so it's very annoying.

The error is:

omap-dma-engine 4a056000.dma-controller: DMA-API: mapping sg segment longer than device claims to support [len=69632] [max=65536]

which indicates that we have a SG segment length that exceeds thte published maximum segment size for a device - in this case the DMA engine device. The maximum segment size for the DMA engine comes from the default per-device setting, in linux/dma-mapping.h, which is 64K.

However, omap_hsmmc sets:

        mmc->max_blk_size = 512;       /* Block Length at max can be 1024 */
        mmc->max_blk_count = 0xFFFF;    /* No. of Blocks is 16 bits */
        mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
        mmc->max_seg_size = mmc->max_req_size;

which ends up telling the block layer that we support a maximum segment size of 65535*512, so of course it _will_ pass SG lists where a segment is longer than 64K.

The problem here is that the HSMMC driver doesn't take account of the DMA engine device's capabilities.

We have something of an odd situation in that the omap-dma device's maximum SG size depends on the "address width" - it's 64K transfers of whatever unit "address width" is, so the current implementation of per-device parameters doesn't exactly work. That means the default 64K limit at the device-level is reasonable.

The only thing I can think of doing is adding into omap_hsmmc:

        mmc->max_seg_size = min(mmc->max_req_size,
                                min(dma_get_max_seg_size(host->rx_chan->device->dev),
                                    dma_get_max_seg_size(host->tx_chan->device->dev)));

Quoted 4 lines

to limit the maximum segment size to that of the device _and_ dma engine's capabilities.

Doing this solves the problem for me.

Thanks for the suggestion - it sounds like a reasonable way to fix the problem, at least for now.

Do you want to send to patch or do you expect someone else to do it?

Quoted 11 lines

Quoted 10 lines

The only thing I can think of doing is adding into omap_hsmmc:

        mmc->max_seg_size = min(mmc->max_req_size,
                                min(dma_get_max_seg_size(host->rx_chan->device->dev),
                                    dma_get_max_seg_size(host->tx_chan->device->dev)));

to limit the maximum segment size to that of the device _and_ dma engine's capabilities.

Doing this solves the problem for me.

Quoted 2 lines

Thanks for the suggestion - it sounds like a reasonable way to fix the problem, at least for now.

I don't think there's any "at least for now" about this approach - it's not something that the MMC core can know about, because whether a driver uses DMA engine or not, and how many channels it uses is completely driver specific.

The only questionable thing is around the dma_get_max_seg_size() interface, but the only way that's going to get solved is to eliminate it entirely as it isn't a per-device property with some DMA engines such as omap-dma - it's a per-request property. That also means killing the check in the DMA debug code, which isn't going to go down very well.

Quoted 1 line

Do you want to send to patch or do you expect someone else to do it?

I'll send a patch once I've checked the corner cases, and whether we should go further and include other DMA parameters from the dma engine.

Russell,

Quoted 11 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 10 lines

The only thing I can think of doing is adding into omap_hsmmc:

        mmc->max_seg_size = min(mmc->max_req_size,
                                min(dma_get_max_seg_size(host->rx_chan->device->dev),
                                    dma_get_max_seg_size(host->tx_chan->device->dev)));

to limit the maximum segment size to that of the device _and_ dma engine's capabilities.

Doing this solves the problem for me.

Quoted 7 lines

Quoted 2 lines

Thanks for the suggestion - it sounds like a reasonable way to fix the problem, at least for now.

I don't think there's any "at least for now" about this approach - it's not something that the MMC core can know about, because whether a driver uses DMA engine or not, and how many channels it uses is completely driver specific.

I agree.

Quoted 7 lines

The only questionable thing is around the dma_get_max_seg_size() interface, but the only way that's going to get solved is to eliminate it entirely as it isn't a per-device property with some DMA engines such as omap-dma - it's a per-request property. That also means killing the check in the DMA debug code, which isn't going to go down very well.

Quoted 5 lines

Quoted 1 line

Do you want to send to patch or do you expect someone else to do it?

I'll send a patch once I've checked the corner cases, and whether we should go further and include other DMA parameters from the dma engine.

There is one thing which I'm trying to figure out:
sDMA (and EDMA also) have limitation on the length of each sg segment.

To make things a bit more interesting the limit is not in bytes, but in number of bursts:

65535 * burst * addr_width = max_sg_segment_len in bytes.

We can not set this to a static value as depending on the addr_width and burst it can be different for each transfer.

We're nearing the merge window, and this is a regression that is yet to be solved. It causes a kernel warning with backtrace, so it's very annoying.

The error is:

omap-dma-engine 4a056000.dma-controller: DMA-API: mapping sg segment longer than device claims to support [len=69632] [max=65536]

which indicates that we have a SG segment length that exceeds thte published maximum segment size for a device - in this case the DMA engine device. The maximum segment size for the DMA engine comes from the default per-device setting, in linux/dma-mapping.h, which is 64K.

However, omap_hsmmc sets:

        mmc->max_blk_size = 512;       /* Block Length at max can be 1024 */
        mmc->max_blk_count = 0xFFFF;    /* No. of Blocks is 16 bits */
        mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
        mmc->max_seg_size = mmc->max_req_size;

which ends up telling the block layer that we support a maximum segment size of 65535*512, so of course it _will_ pass SG lists where a segment is longer than 64K.

The problem here is that the HSMMC driver doesn't take account of the DMA engine device's capabilities.

We have something of an odd situation in that the omap-dma device's maximum SG size depends on the "address width" - it's 64K transfers of whatever unit "address width" is, so the current implementation of per-device parameters doesn't exactly work. That means the default 64K limit at the device-level is reasonable.

The only thing I can think of doing is adding into omap_hsmmc:

	mmc->max_seg_size = min(mmc->max_req_size,
				min(dma_get_max_seg_size(host->rx_chan->device->dev),
				    dma_get_max_seg_size(host->tx_chan->device->dev)));

to limit the maximum segment size to that of the device _and_ dma engine's capabilities.

Doing this solves the problem for me.

Quoted 8 lines · Russell King - ARM Linux

The only thing I can think of doing is adding into omap_hsmmc:

	mmc->max_seg_size = min(mmc->max_req_size,
				min(dma_get_max_seg_size(host->rx_chan->device->dev),
				    dma_get_max_seg_size(host->tx_chan->device->dev)));

to limit the maximum segment size to that of the device _and_ dma engine's capabilities.

Make sense.

Quoted 1 line

Doing this solves the problem for me.