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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

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