* [PATCH 0/3] mmc: Fix scatter/gather on SDHCI @ 2019-09-09 12:56 Thierry Reding 2019-09-09 12:56 ` [PATCH 1/3] block: Respect the device's maximum segment size Thierry Reding ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Thierry Reding @ 2019-09-09 12:56 UTC (permalink / raw) To: Jens Axboe, Ulf Hansson, Adrian Hunter Cc: Yoshihiro Shimoda, Christoph Hellwig, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-09 12:56 [PATCH 0/3] mmc: Fix scatter/gather on SDHCI Thierry Reding @ 2019-09-09 12:56 ` Thierry Reding 2019-09-09 16:13 ` Christoph Hellwig 2019-09-09 12:56 ` [PATCH 2/3] mmc: core: Respect MMC host's " Thierry Reding 2019-09-09 12:56 ` [PATCH 3/3] mmc: sdhci: Set DMA maximum segment size to 64 KiB Thierry Reding 2 siblings, 1 reply; 15+ messages in thread From: Thierry Reding @ 2019-09-09 12:56 UTC (permalink / raw) To: Jens Axboe, Ulf Hansson, Adrian Hunter Cc: Yoshihiro Shimoda, Christoph Hellwig, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-09 12:56 ` [PATCH 1/3] block: Respect the device's maximum segment size Thierry Reding @ 2019-09-09 16:13 ` Christoph Hellwig 2019-09-09 19:19 ` Thierry Reding 2019-09-12 0:57 ` Ming Lei 0 siblings, 2 replies; 15+ messages in thread From: Christoph Hellwig @ 2019-09-09 16:13 UTC (permalink / raw) To: Thierry Reding Cc: Jens Axboe, Ulf Hansson, Adrian Hunter, Yoshihiro Shimoda, Christoph Hellwig, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-09 16:13 ` Christoph Hellwig @ 2019-09-09 19:19 ` Thierry Reding 2019-09-10 2:03 ` Yoshihiro Shimoda 2019-09-12 0:57 ` Ming Lei 1 sibling, 1 reply; 15+ messages in thread From: Thierry Reding @ 2019-09-09 19:19 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, Ulf Hansson, Adrian Hunter, Yoshihiro Shimoda, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-09 19:19 ` Thierry Reding @ 2019-09-10 2:03 ` Yoshihiro Shimoda 2019-09-10 6:13 ` Christoph Hellwig ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Yoshihiro Shimoda @ 2019-09-10 2:03 UTC (permalink / raw) To: Thierry Reding, Christoph Hellwig, Ulf Hansson Cc: Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-10 2:03 ` Yoshihiro Shimoda @ 2019-09-10 6:13 ` Christoph Hellwig 2019-09-10 7:37 ` Thierry Reding 2019-09-10 7:30 ` Thierry Reding 2019-09-11 10:37 ` Christoph Hellwig 2 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2019-09-10 6:13 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Thierry Reding, Christoph Hellwig, Ulf Hansson, Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-10 6:13 ` Christoph Hellwig @ 2019-09-10 7:37 ` Thierry Reding 2019-09-11 10:36 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Thierry Reding @ 2019-09-10 7:37 UTC (permalink / raw) To: Christoph Hellwig Cc: Yoshihiro Shimoda, Ulf Hansson, Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-10 7:37 ` Thierry Reding @ 2019-09-11 10:36 ` Christoph Hellwig 0 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2019-09-11 10:36 UTC (permalink / raw) To: Thierry Reding Cc: Christoph Hellwig, Yoshihiro Shimoda, Ulf Hansson, Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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"). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-10 2:03 ` Yoshihiro Shimoda 2019-09-10 6:13 ` Christoph Hellwig @ 2019-09-10 7:30 ` Thierry Reding 2019-09-11 7:23 ` Yoshihiro Shimoda 2019-09-11 10:37 ` Christoph Hellwig 2 siblings, 1 reply; 15+ messages in thread From: Thierry Reding @ 2019-09-10 7:30 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Christoph Hellwig, Ulf Hansson, Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-10 7:30 ` Thierry Reding @ 2019-09-11 7:23 ` Yoshihiro Shimoda 0 siblings, 0 replies; 15+ messages in thread From: Yoshihiro Shimoda @ 2019-09-11 7:23 UTC (permalink / raw) To: Thierry Reding Cc: Christoph Hellwig, Ulf Hansson, Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-10 2:03 ` Yoshihiro Shimoda 2019-09-10 6:13 ` Christoph Hellwig 2019-09-10 7:30 ` Thierry Reding @ 2019-09-11 10:37 ` Christoph Hellwig 2 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2019-09-11 10:37 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Thierry Reding, Christoph Hellwig, Ulf Hansson, Jens Axboe, Adrian Hunter, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-09 16:13 ` Christoph Hellwig 2019-09-09 19:19 ` Thierry Reding @ 2019-09-12 0:57 ` Ming Lei 2019-09-12 8:19 ` Christoph Hellwig 1 sibling, 1 reply; 15+ messages in thread From: Ming Lei @ 2019-09-12 0:57 UTC (permalink / raw) To: Christoph Hellwig Cc: Thierry Reding, Jens Axboe, Ulf Hansson, Adrian Hunter, Yoshihiro Shimoda, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] block: Respect the device's maximum segment size 2019-09-12 0:57 ` Ming Lei @ 2019-09-12 8:19 ` Christoph Hellwig 0 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2019-09-12 8:19 UTC (permalink / raw) To: Ming Lei Cc: Christoph Hellwig, Thierry Reding, Jens Axboe, Ulf Hansson, Adrian Hunter, Yoshihiro Shimoda, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] mmc: core: Respect MMC host's maximum segment size 2019-09-09 12:56 [PATCH 0/3] mmc: Fix scatter/gather on SDHCI Thierry Reding 2019-09-09 12:56 ` [PATCH 1/3] block: Respect the device's maximum segment size Thierry Reding @ 2019-09-09 12:56 ` Thierry Reding 2019-09-09 12:56 ` [PATCH 3/3] mmc: sdhci: Set DMA maximum segment size to 64 KiB Thierry Reding 2 siblings, 0 replies; 15+ messages in thread From: Thierry Reding @ 2019-09-09 12:56 UTC (permalink / raw) To: Jens Axboe, Ulf Hansson, Adrian Hunter Cc: Yoshihiro Shimoda, Christoph Hellwig, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] mmc: sdhci: Set DMA maximum segment size to 64 KiB 2019-09-09 12:56 [PATCH 0/3] mmc: Fix scatter/gather on SDHCI Thierry Reding 2019-09-09 12:56 ` [PATCH 1/3] block: Respect the device's maximum segment size Thierry Reding 2019-09-09 12:56 ` [PATCH 2/3] mmc: core: Respect MMC host's " Thierry Reding @ 2019-09-09 12:56 ` Thierry Reding 2 siblings, 0 replies; 15+ messages in thread From: Thierry Reding @ 2019-09-09 12:56 UTC (permalink / raw) To: Jens Axboe, Ulf Hansson, Adrian Hunter Cc: Yoshihiro Shimoda, Christoph Hellwig, Simon Horman, Jon Hunter, linux-block, linux-mmc, linux-tegra 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-09-12 8:19 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-09 12:56 [PATCH 0/3] mmc: Fix scatter/gather on SDHCI Thierry Reding 2019-09-09 12:56 ` [PATCH 1/3] block: Respect the device's maximum segment size Thierry Reding 2019-09-09 16:13 ` Christoph Hellwig 2019-09-09 19:19 ` Thierry Reding 2019-09-10 2:03 ` Yoshihiro Shimoda 2019-09-10 6:13 ` Christoph Hellwig 2019-09-10 7:37 ` Thierry Reding 2019-09-11 10:36 ` Christoph Hellwig 2019-09-10 7:30 ` Thierry Reding 2019-09-11 7:23 ` Yoshihiro Shimoda 2019-09-11 10:37 ` Christoph Hellwig 2019-09-12 0:57 ` Ming Lei 2019-09-12 8:19 ` Christoph Hellwig 2019-09-09 12:56 ` [PATCH 2/3] mmc: core: Respect MMC host's " Thierry Reding 2019-09-09 12:56 ` [PATCH 3/3] mmc: sdhci: Set DMA maximum segment size to 64 KiB Thierry Reding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).