[PATCH 0/3] mmc: Fix scatter/gather on SDHCI

[PATCH 0/3] mmc: Fix scatter/gather on SDHCI

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Commit 158a6d3ce3bc ("iommu/dma: add a new dma_map_ops of
get_merge_boundary()") causes scatter/gather to break for SDHCI and
potentially other MMC hosts.

The reason is that the commit ends up tricking the block layer into
believing that there's effectively no limit on the segment size. While
this may be true for some device, it's certainly not true for all. The
DMA descriptors used by SDHCI, for example, have a 16-bit field that
contains the number of bytes to transmit for that particular transfer.
As a result of the segment size exceeding the capabilities of the
hardware, the scatterlist ends up containing entries that are too large
to fit into a single descriptor.

This small series fixes this by making the block layer respect the
segment size restrictions set for the device. It also prevents the MMC
queue code to attempt to overwrite the maximum segment size of a device
that may already have been set up. Finally it configures the maximum
segment size for SDHCI. The last step is technically not required
because the maximum segment size for SDHCI coincides with the default,
but I think it's better to be explicit here.

As a result, all entries in the scatterlist are now small enough to fit
into SDHCI DMA descriptors. Some improvements could be made to how the
scatterlist is packed. For example, the dma-iommu code compacts the SG
entries so that they result in segments less than the maximum segment,
but doesn't split up individual entries. This often results in holes in
the individual segments. In order to create full 64 KiB segments with
only the last segment being partial, the code would have to split up
individual entries. This should be possible but is not done as part of
this series.

Thierry

Thierry Reding (3):
  block: Respect the device's maximum segment size
  mmc: core: Respect MMC host's maximum segment size
  mmc: sdhci: Set DMA maximum segment size to 64 KiB

 block/blk-settings.c     | 24 +++++++++++++++---------
 drivers/mmc/core/queue.c |  2 --
 drivers/mmc/host/sdhci.c |  5 +++++
 drivers/mmc/host/sdhci.h |  1 +
 4 files changed, 21 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH 1/3] block: Respect the device's maximum segment size

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When enabling the DMA map merging capability for a queue, ensure that
the maximum segment size does not exceed the device's limit.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 block/blk-settings.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 70b39f14e974..9fb1288fbc12 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -738,12 +738,8 @@ void blk_queue_segment_boundary(struct request_queue *q, unsigned long mask)
 }
 EXPORT_SYMBOL(blk_queue_segment_boundary);
 
-/**
- * blk_queue_virt_boundary - set boundary rules for bio merging
- * @q:  the request queue for the device
- * @mask:  the memory boundary mask
- **/
-void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
+void __blk_queue_virt_boundary(struct request_queue *q, unsigned long mask,
+			       unsigned int max_segment_size)
 {
 	q->limits.virt_boundary_mask = mask;
 
@@ -754,7 +750,17 @@ void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
 	 * of that they are not limited by our notion of "segment size".
 	 */
 	if (mask)
-		q->limits.max_segment_size = UINT_MAX;
+		q->limits.max_segment_size = max_segment_size;
+}
+
+/**
+ * blk_queue_virt_boundary - set boundary rules for bio merging
+ * @q:  the request queue for the device
+ * @mask:  the memory boundary mask
+ **/
+void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
+{
+	__blk_queue_virt_boundary(q, mask, UINT_MAX);
 }
 EXPORT_SYMBOL(blk_queue_virt_boundary);
 
@@ -843,13 +849,13 @@ EXPORT_SYMBOL_GPL(blk_queue_write_cache);
 bool blk_queue_can_use_dma_map_merging(struct request_queue *q,
 				       struct device *dev)
 {
+	unsigned int max_segment_size = dma_get_max_seg_size(dev);
 	unsigned long boundary = dma_get_merge_boundary(dev);
 
 	if (!boundary)
 		return false;
 
-	/* No need to update max_segment_size. see blk_queue_virt_boundary() */
-	blk_queue_virt_boundary(q, boundary);
+	__blk_queue_virt_boundary(q, boundary, max_segment_size);
 
 	return true;
 }
-- 
2.23.0

[PATCH 2/3] mmc: core: Respect MMC host's maximum segment size

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Do not overwrite the MMC host's configured maximum segment size for DMA
transfers. For devices behind an IOMMU, the queue's maximum segment size
may be larger than that of the MMC host, but that doesn't mean that the
MMC host actually supports it.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/mmc/core/queue.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 1e29b305767e..987b01f4cfb3 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -389,8 +389,6 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
 		blk_queue_max_segment_size(mq->queue,
 			round_down(host->max_seg_size, block_size));
 
-	dma_set_max_seg_size(mmc_dev(host), queue_max_segment_size(mq->queue));
-
 	INIT_WORK(&mq->recovery_work, mmc_mq_recovery_handler);
 	INIT_WORK(&mq->complete_work, mmc_blk_mq_complete_work);
 
-- 
2.23.0

[PATCH 3/3] mmc: sdhci: Set DMA maximum segment size to 64 KiB

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

DMA descriptors used with SDHCI have a 16-bit field that specifies the
number of bytes to transfer per segment. A segment's size may therefore
never exceed 64 KiB.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/mmc/host/sdhci.c | 5 +++++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d814dc004bad..b59d063646bf 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3555,6 +3555,10 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
 	 */
 	host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1;
 
+	/* ADMA supports a maximum of 64 KiB per descriptor */
+	dev->dma_parms = &host->dma_params;
+	dma_set_max_seg_size(dev, SZ_64K);
+
 	return host;
 }
 
@@ -4410,6 +4414,7 @@ EXPORT_SYMBOL_GPL(sdhci_remove_host);
 
 void sdhci_free_host(struct sdhci_host *host)
 {
+	host->mmc->parent->dma_parms = NULL;
 	mmc_free_host(host->mmc);
 }
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cf3d1ed91909..b543d31bbcdb 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -494,6 +494,7 @@ struct sdhci_host {
 	/* Internal data */
 	struct mmc_host *mmc;	/* MMC structure */
 	struct mmc_host_ops mmc_host_ops;	/* MMC host ops */
+	struct device_dma_parameters dma_params; /* DMA parameters */
 	u64 dma_mask;		/* custom DMA mask */
 
 #if IS_ENABLED(CONFIG_LEDS_CLASS)
-- 
2.23.0

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Christoph Hellwig]

On Mon, Sep 09, 2019 at 02:56:56PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> When enabling the DMA map merging capability for a queue, ensure that
> the maximum segment size does not exceed the device's limit.

We can't do that unfortunately.  If we use the virt_boundary setting
we do aggressive merges that there is no accounting for.  So we can't
limit the segment size.

And at least for the case how devices usually do the addressing
(page based on not sgl based) that needs the virt_boundary there isn't
really any concept like a segment anyway.

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Thierry Reding]

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

On Mon, Sep 09, 2019 at 06:13:31PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 09, 2019 at 02:56:56PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > When enabling the DMA map merging capability for a queue, ensure that
> > the maximum segment size does not exceed the device's limit.
> 
> We can't do that unfortunately.  If we use the virt_boundary setting
> we do aggressive merges that there is no accounting for.  So we can't
> limit the segment size.

But that's kind of the point here. My understanding is that the maximum
segment size in the device's DMA parameters defines the maximum size of
the segment that the device can handle.

In the particular case that I'm trying to fix, according to the SDHCI
specification, these devices can handle segments that are a maximum of
64 KiB in size. If we allow that segment size to be exceeded, the device
will no longer be able to handle them.

> And at least for the case how devices usually do the addressing
> (page based on not sgl based) that needs the virt_boundary there isn't
> really any concept like a segment anyway.

I do understand that aspect of it. However, devices that do the
addressing this way, wouldn't they want to set the maximum segment size
to something large (like UINT_MAX, which many users that don't have the
concept of a segment already do)?

If you disagree, do you have any alternative proposals other than
reverting the offending patch? linux-next is currently broken on all
systems where the SDHCI controller is behind an IOMMU.

Thierry

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

RE: [PATCH 1/3] block: Respect the device's maximum segment size

[Yoshihiro Shimoda]

Hi Thierry,

> From: Thierry Reding, Sent: Tuesday, September 10, 2019 4:19 AM
> 
> On Mon, Sep 09, 2019 at 06:13:31PM +0200, Christoph Hellwig wrote:
> > On Mon, Sep 09, 2019 at 02:56:56PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > When enabling the DMA map merging capability for a queue, ensure that
> > > the maximum segment size does not exceed the device's limit.
> >
> > We can't do that unfortunately.  If we use the virt_boundary setting
> > we do aggressive merges that there is no accounting for.  So we can't
> > limit the segment size.
> 
> But that's kind of the point here. My understanding is that the maximum
> segment size in the device's DMA parameters defines the maximum size of
> the segment that the device can handle.
> 
> In the particular case that I'm trying to fix, according to the SDHCI
> specification, these devices can handle segments that are a maximum of
> 64 KiB in size. If we allow that segment size to be exceeded, the device
> will no longer be able to handle them.
> 
> > And at least for the case how devices usually do the addressing
> > (page based on not sgl based) that needs the virt_boundary there isn't
> > really any concept like a segment anyway.
> 
> I do understand that aspect of it. However, devices that do the
> addressing this way, wouldn't they want to set the maximum segment size
> to something large (like UINT_MAX, which many users that don't have the
> concept of a segment already do)?
> 
> If you disagree, do you have any alternative proposals other than
> reverting the offending patch? linux-next is currently broken on all
> systems where the SDHCI controller is behind an IOMMU.

I'm sorry for causing this trouble on your environment. I have a proposal to
resolve this issue. The mmc_host struct will have a new caps2 flag
like MMC_CAP2_MERGE_CAPABLE and add a condition into the queue.c like below.

+	if (host->caps2 & MMC_CAP2_MERGE_CAPABLE &&
+	    host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
	    dma_get_merge_boundary(mmc_dev(host)))
		host->can_dma_map_merge = 1;
	else
		host->can_dma_map_merge = 0;

After that, all mmc controllers disable the feature as default, and if a mmc
controller has such capable, the host driver should set the flag.
Ulf, is such a patch acceptable for v5.4-rcN? Anyway, I'll submit such a patch later.

Best regards,
Yoshihiro Shimoda

> Thierry

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Christoph Hellwig]

On Tue, Sep 10, 2019 at 02:03:17AM +0000, Yoshihiro Shimoda wrote:
> I'm sorry for causing this trouble on your environment. I have a proposal to
> resolve this issue. The mmc_host struct will have a new caps2 flag
> like MMC_CAP2_MERGE_CAPABLE and add a condition into the queue.c like below.
> 
> +	if (host->caps2 & MMC_CAP2_MERGE_CAPABLE &&
> +	    host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
> 	    dma_get_merge_boundary(mmc_dev(host)))
> 		host->can_dma_map_merge = 1;
> 	else
> 		host->can_dma_map_merge = 0;
> 
> After that, all mmc controllers disable the feature as default, and if a mmc
> controller has such capable, the host driver should set the flag.

That sounds sensible to me.  Alternatively we'd have to limit
max_sectors to 16-bit values for sdhci if using an iommu that can
merge.

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Thierry Reding]

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

On Tue, Sep 10, 2019 at 02:03:17AM +0000, Yoshihiro Shimoda wrote:
> Hi Thierry,
> 
> > From: Thierry Reding, Sent: Tuesday, September 10, 2019 4:19 AM
> > 
> > On Mon, Sep 09, 2019 at 06:13:31PM +0200, Christoph Hellwig wrote:
> > > On Mon, Sep 09, 2019 at 02:56:56PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > When enabling the DMA map merging capability for a queue, ensure that
> > > > the maximum segment size does not exceed the device's limit.
> > >
> > > We can't do that unfortunately.  If we use the virt_boundary setting
> > > we do aggressive merges that there is no accounting for.  So we can't
> > > limit the segment size.
> > 
> > But that's kind of the point here. My understanding is that the maximum
> > segment size in the device's DMA parameters defines the maximum size of
> > the segment that the device can handle.
> > 
> > In the particular case that I'm trying to fix, according to the SDHCI
> > specification, these devices can handle segments that are a maximum of
> > 64 KiB in size. If we allow that segment size to be exceeded, the device
> > will no longer be able to handle them.
> > 
> > > And at least for the case how devices usually do the addressing
> > > (page based on not sgl based) that needs the virt_boundary there isn't
> > > really any concept like a segment anyway.
> > 
> > I do understand that aspect of it. However, devices that do the
> > addressing this way, wouldn't they want to set the maximum segment size
> > to something large (like UINT_MAX, which many users that don't have the
> > concept of a segment already do)?
> > 
> > If you disagree, do you have any alternative proposals other than
> > reverting the offending patch? linux-next is currently broken on all
> > systems where the SDHCI controller is behind an IOMMU.
> 
> I'm sorry for causing this trouble on your environment. I have a proposal to
> resolve this issue. The mmc_host struct will have a new caps2 flag
> like MMC_CAP2_MERGE_CAPABLE and add a condition into the queue.c like below.
> 
> +	if (host->caps2 & MMC_CAP2_MERGE_CAPABLE &&
> +	    host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
> 	    dma_get_merge_boundary(mmc_dev(host)))
> 		host->can_dma_map_merge = 1;
> 	else
> 		host->can_dma_map_merge = 0;
> 
> After that, all mmc controllers disable the feature as default, and if a mmc
> controller has such capable, the host driver should set the flag.
> Ulf, is such a patch acceptable for v5.4-rcN? Anyway, I'll submit such a patch later.

I'm sure that would work, but I think that's missing the point. It's not
that the setup isn't capable of merging at all. It just can't deal with
segments that are too large.

While I was debugging this, I was frequently seeing cases where the SG
was on the order of 40 entries initially and after dma_map_sg() it was
reduced to just 4 or 5.

So I think merging is still really useful if a setup supports it via an
IOMMU. I'm just not sure I see why we can't make the code respect what-
ever the maximum segment size that the driver may have configured. That
seems like it should allow us to get the best of both worlds.

Thierry

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

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Thierry Reding]

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

On Tue, Sep 10, 2019 at 08:13:48AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 10, 2019 at 02:03:17AM +0000, Yoshihiro Shimoda wrote:
> > I'm sorry for causing this trouble on your environment. I have a proposal to
> > resolve this issue. The mmc_host struct will have a new caps2 flag
> > like MMC_CAP2_MERGE_CAPABLE and add a condition into the queue.c like below.
> > 
> > +	if (host->caps2 & MMC_CAP2_MERGE_CAPABLE &&
> > +	    host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
> > 	    dma_get_merge_boundary(mmc_dev(host)))
> > 		host->can_dma_map_merge = 1;
> > 	else
> > 		host->can_dma_map_merge = 0;
> > 
> > After that, all mmc controllers disable the feature as default, and if a mmc
> > controller has such capable, the host driver should set the flag.
> 
> That sounds sensible to me.  Alternatively we'd have to limit
> max_sectors to 16-bit values for sdhci if using an iommu that can
> merge.

Isn't that effectively what dma_set_max_seg_size() is supposed to be
doing? That tells the DMA API what the maximum size of a segment can
be for the given device, right? If we make sure never to exceed that
when compacting the SG, the SG that we get back should map just fine
into the descriptors that SDHCI supports.

Now, for devices that support larger segments (or in fact no concept
of segments at all), if we set the maximum segment size to something
really big, isn't that going to automatically allow the huge, merged
segments that the original patch intended?

To me this sounds simply like an issue of the queue code thinking it
knows better than the driver and just overriding the maximum segment
size. Isn't that the real bug here that needs to be fixed?

Thierry

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

RE: [PATCH 1/3] block: Respect the device's maximum segment size

[Yoshihiro Shimoda]

Hi Thierry,

> From: Thierry Reding, Sent: Tuesday, September 10, 2019 4:31 PM
<snip>
> On Tue, Sep 10, 2019 at 02:03:17AM +0000, Yoshihiro Shimoda wrote:
> > Hi Thierry,
> >
> > > From: Thierry Reding, Sent: Tuesday, September 10, 2019 4:19 AM
> > >
> > > On Mon, Sep 09, 2019 at 06:13:31PM +0200, Christoph Hellwig wrote:
> > > > On Mon, Sep 09, 2019 at 02:56:56PM +0200, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > >
> > > > > When enabling the DMA map merging capability for a queue, ensure that
> > > > > the maximum segment size does not exceed the device's limit.
> > > >
> > > > We can't do that unfortunately.  If we use the virt_boundary setting
> > > > we do aggressive merges that there is no accounting for.  So we can't
> > > > limit the segment size.
> > >
> > > But that's kind of the point here. My understanding is that the maximum
> > > segment size in the device's DMA parameters defines the maximum size of
> > > the segment that the device can handle.
> > >
> > > In the particular case that I'm trying to fix, according to the SDHCI
> > > specification, these devices can handle segments that are a maximum of
> > > 64 KiB in size. If we allow that segment size to be exceeded, the device
> > > will no longer be able to handle them.
> > >
> > > > And at least for the case how devices usually do the addressing
> > > > (page based on not sgl based) that needs the virt_boundary there isn't
> > > > really any concept like a segment anyway.
> > >
> > > I do understand that aspect of it. However, devices that do the
> > > addressing this way, wouldn't they want to set the maximum segment size
> > > to something large (like UINT_MAX, which many users that don't have the
> > > concept of a segment already do)?
> > >
> > > If you disagree, do you have any alternative proposals other than
> > > reverting the offending patch? linux-next is currently broken on all
> > > systems where the SDHCI controller is behind an IOMMU.
> >
> > I'm sorry for causing this trouble on your environment. I have a proposal to
> > resolve this issue. The mmc_host struct will have a new caps2 flag
> > like MMC_CAP2_MERGE_CAPABLE and add a condition into the queue.c like below.
> >
> > +	if (host->caps2 & MMC_CAP2_MERGE_CAPABLE &&
> > +	    host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
> > 	    dma_get_merge_boundary(mmc_dev(host)))
> > 		host->can_dma_map_merge = 1;
> > 	else
> > 		host->can_dma_map_merge = 0;
> >
> > After that, all mmc controllers disable the feature as default, and if a mmc
> > controller has such capable, the host driver should set the flag.
> > Ulf, is such a patch acceptable for v5.4-rcN? Anyway, I'll submit such a patch later.
> 
> I'm sure that would work, but I think that's missing the point. It's not
> that the setup isn't capable of merging at all. It just can't deal with
> segments that are too large.

IIUC, since SDHCI has a strictly 64 KiB limitation on each segment,
the controller cannot handle the following example 1 case on the plain next-20190904.

For example 1:
 - Original scatter lists are 4 segments:
  sg[0]: .dma_address = 0x12340000, .length = 65536,
  sg[1]: .dma_address = 0x12350000, .length = 65536,
  sg[2]: .dma_address = 0x12360000, .length = 65536,
  sg[3]: .dma_address = 0x12370000, .length = 65536,

 - Merging the above segments will be a segment:
  sg[0]: .dma_address = 0x12340000, .length = 262144,

> While I was debugging this, I was frequently seeing cases where the SG
> was on the order of 40 entries initially and after dma_map_sg() it was
> reduced to just 4 or 5.

If each segment size is small, it can merge them.

For example 2:
 - Original scatter lists are 4 segments:
  sg[0]: .dma_address = 0x12340000, .length = 4096,
  sg[1]: .dma_address = 0x12341000, .length = 4096,
  sg[2]: .dma_address = 0x12342000, .length = 4096,
  sg[3]: .dma_address = 0x12343000, .length = 4096,

 - Merging the above segments will be a segment:
  sg[0]: .dma_address = 0x12340000, .length = 16384,

> So I think merging is still really useful if a setup supports it via an
> IOMMU. I'm just not sure I see why we can't make the code respect what-
> ever the maximum segment size that the driver may have configured. That
> seems like it should allow us to get the best of both worlds.

I agree about merging is useful for the case of the "example 2".

By the way, I checked dma-iommu.c ,and then I found the __finalise_sg() has
a condition "seg_mask" that is from dma_get_seg_boundary(). So, I'm guessing
if the sdhci.c calls dma_set_seg_boundary() with 0x0000ffff, the issue disappears.
This is because the dma-iommu.c will not merge the segments even if the case of
example 1. What do you think?

Best regards,
Yoshihiro Shimoda

> Thierry

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Christoph Hellwig]

On Tue, Sep 10, 2019 at 09:37:39AM +0200, Thierry Reding wrote:
> > > After that, all mmc controllers disable the feature as default, and if a mmc
> > > controller has such capable, the host driver should set the flag.
> > 
> > That sounds sensible to me.  Alternatively we'd have to limit
> > max_sectors to 16-bit values for sdhci if using an iommu that can
> > merge.
> 
> Isn't that effectively what dma_set_max_seg_size() is supposed to be
> doing? That tells the DMA API what the maximum size of a segment can
> be for the given device, right? If we make sure never to exceed that
> when compacting the SG, the SG that we get back should map just fine
> into the descriptors that SDHCI supports.

dma_set_max_seg_size() does indeed instruct the iommu drivers about
the merging capabilities (btw, swiotlb should be able to implement
this kind of merging as well, but that is a different discussion).

But the problem is that you don't just change the dma_set_max_seg_size,
but also the block layer max segment size setting, which is used for
block layer merges.  And we don't have the accounting for the first and
last segment in a request (those that are being merged to), so if you
enable the virt_boundary segments can grow to a size only limited by the
maximum request size.  We could add that accounting with a bit of
work, it's just that for devices that typicall use the virt boundary
there is no point (their actually segment is a page and not related
to the Linux "segment").

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Christoph Hellwig]

On Tue, Sep 10, 2019 at 02:03:17AM +0000, Yoshihiro Shimoda wrote:
> I'm sorry for causing this trouble on your environment. I have a proposal to
> resolve this issue. The mmc_host struct will have a new caps2 flag
> like MMC_CAP2_MERGE_CAPABLE and add a condition into the queue.c like below.
> 
> +	if (host->caps2 & MMC_CAP2_MERGE_CAPABLE &&
> +	    host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
> 	    dma_get_merge_boundary(mmc_dev(host)))
> 		host->can_dma_map_merge = 1;
> 	else
> 		host->can_dma_map_merge = 0;
> 
> After that, all mmc controllers disable the feature as default, and if a mmc
> controller has such capable, the host driver should set the flag.
> Ulf, is such a patch acceptable for v5.4-rcN? Anyway, I'll submit such a patch later.

FYI, I'd love to see such a patch, even if only as a temporary band-aid
so that I don't have to revert the series before sending the pull
request to Linus.

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Ming Lei]

On Mon, Sep 09, 2019 at 06:13:31PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 09, 2019 at 02:56:56PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > When enabling the DMA map merging capability for a queue, ensure that
> > the maximum segment size does not exceed the device's limit.
> 
> We can't do that unfortunately.  If we use the virt_boundary setting
> we do aggressive merges that there is no accounting for.  So we can't
> limit the segment size.

Could you explain a bit why we can't do that?

The segment size limit is basically removed since the following commit
200a9aff7b02 ("block: remove the segment size check in bio_will_gap").

Before that commit, the max segment size limit worked.


Thanks,
Ming

Re: [PATCH 1/3] block: Respect the device's maximum segment size

[Christoph Hellwig]

On Thu, Sep 12, 2019 at 08:57:29AM +0800, Ming Lei wrote:
> Could you explain a bit why we can't do that?
> 
> The segment size limit is basically removed since the following commit
> 200a9aff7b02 ("block: remove the segment size check in bio_will_gap").
> 
> Before that commit, the max segment size limit worked.

No, it didn't as explained in the commit log.  It worked for some
cases but not others.