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@xxxxxxxxxx> > > > > > > > > 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:
signature.asc
Description: PGP signature
