Patchwork sparc: use asm-generic/scatterlist.h

login
register
mail settings
Submitter FUJITA Tomonori
Date March 2, 2010, 3:33 a.m.
Message ID <20100302123326H.fujita.tomonori@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/46622/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

FUJITA Tomonori - March 2, 2010, 3:33 a.m.
On Mon, 1 Mar 2010 12:29:51 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 01 March 2010, FUJITA Tomonori wrote:
> > On Fri, 26 Feb 2010 04:35:36 -0800 (PST)
> > David Miller <davem@davemloft.net> wrote:
> >
> > You are referring to the following code (I guess that this hack came
> > from x86)?
> > 
> > #if __BITS_PER_LONG == 64
> > #define sg_dma_len(sg)          ((sg)->dma_length)
> > #else
> > #define sg_dma_len(sg)          ((sg)->length)
> > #endif /* 64 bit */
> > 
> > if so, seems that you are right. we could simply have:
> > 
> > #define sg_dma_len(sg)          ((sg)->dma_length)
> 
> I did it the above way so it would work for any architecture that
> wants it. IIRC, similar constructs were used in multiple architectures
> before, using the __BITS_PER_LONG macro made this portable.

Yeah, but the following assumption are not true for sparc, parisc, and
even x86_32 so this ccp trick is not so useful:

/*
 * Normally, you have an iommu on 64 bit machines, but not on 32 bit
 * machines. Architectures that are differnt should override this.
*/

> > The current users of asm-generic/scatterlist.h are microblaze, s390,
> > score, sh, and x86.
> > 
> > The first three users don't support DMA so sg_dma_len doesn't matter
> > for them.
> > 
> > sh and x86_32 use sg->length, x86_64 uses sg->dma_length. However, sh
> > and x86_32 sets dma_length in dma_map_sg() so they can use
> > sg->dma_length.
> > 
> > I'll clean up this in the next merge window.
> 
> Ok, great. I think a good way to clean this up would be to convert
> all architectures to use asm-generic/scatterlist.h first, and then
> move it to linux/scatterlist once it is architecture intedepent.

If we go with such approach, then we could use something like the
following. There are only two kinds of scatterlist definitions (use
dma_length or not) so we can cover all the architectures.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 2, 2010, 12:03 p.m.
On Tuesday 02 March 2010, FUJITA Tomonori wrote:
> If we go with such approach, then we could use something like the
> following. There are only two kinds of scatterlist definitions (use
> dma_length or not) so we can cover all the architectures.
> 
> diff --git a/include/asm-generic/scatterlist.h b/include/asm-generic/scatterlist.h
> index 8b94544..1bf620d 100644
> --- a/include/asm-generic/scatterlist.h
> +++ b/include/asm-generic/scatterlist.h
> @@ -11,7 +11,9 @@ struct scatterlist {
>         unsigned int    offset;
>         unsigned int    length;
>         dma_addr_t      dma_address;
> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
>         unsigned int    dma_length;
> +#endif
>  };

Yes, that sounds good. If the only reason to need dma_length is virtual merging,
a clearer (from the Kconfig perspective, not from the implementation) name 
might be CONFIG_HAVE_IOMMU_VMERGE, similar to the CONFIG_IOMMU_VMERGE option
on PPC64 that determines the default for the virtual merging runtime option.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
FUJITA Tomonori - March 2, 2010, 12:25 p.m.
On Tue, 2 Mar 2010 13:03:25 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 02 March 2010, FUJITA Tomonori wrote:
> > If we go with such approach, then we could use something like the
> > following. There are only two kinds of scatterlist definitions (use
> > dma_length or not) so we can cover all the architectures.
> > 
> > diff --git a/include/asm-generic/scatterlist.h b/include/asm-generic/scatterlist.h
> > index 8b94544..1bf620d 100644
> > --- a/include/asm-generic/scatterlist.h
> > +++ b/include/asm-generic/scatterlist.h
> > @@ -11,7 +11,9 @@ struct scatterlist {
> >         unsigned int    offset;
> >         unsigned int    length;
> >         dma_addr_t      dma_address;
> > +#ifdef CONFIG_NEED_SG_DMA_LENGTH
> >         unsigned int    dma_length;
> > +#endif
> >  };
> 
> Yes, that sounds good. If the only reason to need dma_length is virtual merging,
> a clearer (from the Kconfig perspective, not from the implementation) name 
> might be CONFIG_HAVE_IOMMU_VMERGE, similar to the CONFIG_IOMMU_VMERGE option
> on PPC64 that determines the default for the virtual merging runtime option.

Yeah, but IIRC, Alpha, x86_64 GART, parisc, and IA64 don't have
CONFIG_ option for IOMMU virtual merging. I prefer to avoid to adding
something like CONFIG_HAVE_IOMMU_VMERGE for them. If we add
CONFIG_HAVE_IOMMU_VMERGE to them, it's a bit strange not to add the
feature to disable virtual merging for them (I guess GART already has
the feature though).

Actually, I want to use dma_length on all the architectures (if nobody
complains).
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 2, 2010, 1:38 p.m.
On Tuesday 02 March 2010, FUJITA Tomonori wrote:
> Yeah, but IIRC, Alpha, x86_64 GART, parisc, and IA64 don't have
> CONFIG_ option for IOMMU virtual merging. I prefer to avoid to adding
> something like CONFIG_HAVE_IOMMU_VMERGE for them. If we add
> CONFIG_HAVE_IOMMU_VMERGE to them, it's a bit strange not to add the
> feature to disable virtual merging for them (I guess GART already has
> the feature though).

While I think the runtime feature (actually a workaround for broken device
drivers) should be consistently used on all architectures, or removed
entirely, it's orthogonal to this discussion. I'm sure you'll come
up with a reasonable name for a new option if you introduce one.
CONFIG_HAVE_IOMMU_VMERGE and CONFIG_NEED_SG_DMA_LENGTH both seem ok
to me.

> Actually, I want to use dma_length on all the architectures (if nobody
> complains).

Fine with me as well. It wastes a small amount of memory but makes
the code more consistent.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
FUJITA Tomonori - March 2, 2010, 1:49 p.m.
On Tue, 2 Mar 2010 14:38:31 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 02 March 2010, FUJITA Tomonori wrote:
> > Yeah, but IIRC, Alpha, x86_64 GART, parisc, and IA64 don't have
> > CONFIG_ option for IOMMU virtual merging. I prefer to avoid to adding
> > something like CONFIG_HAVE_IOMMU_VMERGE for them. If we add
> > CONFIG_HAVE_IOMMU_VMERGE to them, it's a bit strange not to add the
> > feature to disable virtual merging for them (I guess GART already has
> > the feature though).
> 
> While I think the runtime feature (actually a workaround for broken device
> drivers)

What does "broken device drivers" mean here?

> should be consistently used on all architectures, or removed
> entirely, it's orthogonal to this discussion.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 2, 2010, 1:54 p.m.
On Tuesday 02 March 2010, FUJITA Tomonori wrote:
> On Tue, 2 Mar 2010 14:38:31 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Tuesday 02 March 2010, FUJITA Tomonori wrote:
> > > Yeah, but IIRC, Alpha, x86_64 GART, parisc, and IA64 don't have
> > > CONFIG_ option for IOMMU virtual merging. I prefer to avoid to adding
> > > something like CONFIG_HAVE_IOMMU_VMERGE for them. If we add
> > > CONFIG_HAVE_IOMMU_VMERGE to them, it's a bit strange not to add the
> > > feature to disable virtual merging for them (I guess GART already has
> > > the feature though).
> > 
> > While I think the runtime feature (actually a workaround for broken device
> > drivers)
> 
> What does "broken device drivers" mean here?

Broken in the sense that arch/powerpc/Kconfig describes CONFIG_IOMMU_VMERGE:

       Cause IO segments sent to a device for DMA to be merged virtually
       by the IOMMU when they happen to have been allocated contiguously.
       This doesn't add pressure to the IOMMU allocator. However, some
       drivers don't support getting large merged segments coming back
       from *_map_sg().

       Most drivers don't have this problem; it is safe to say Y here.

I don't know if this comment still applies to any drivers in the mainline
kernel, but it's possible.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 2, 2010, 1:55 p.m.
From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 2 Mar 2010 14:54:11 +0100

> Broken in the sense that arch/powerpc/Kconfig describes CONFIG_IOMMU_VMERGE:
> 
>        Cause IO segments sent to a device for DMA to be merged virtually
>        by the IOMMU when they happen to have been allocated contiguously.
>        This doesn't add pressure to the IOMMU allocator. However, some
>        drivers don't support getting large merged segments coming back
>        from *_map_sg().
> 
>        Most drivers don't have this problem; it is safe to say Y here.
> 
> I don't know if this comment still applies to any drivers in the mainline
> kernel, but it's possible.

That really has to be out of date these days.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
FUJITA Tomonori - March 2, 2010, 2:06 p.m.
On Tue, 02 Mar 2010 05:55:34 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> Date: Tue, 2 Mar 2010 14:54:11 +0100
> 
> > Broken in the sense that arch/powerpc/Kconfig describes CONFIG_IOMMU_VMERGE:
> > 
> >        Cause IO segments sent to a device for DMA to be merged virtually
> >        by the IOMMU when they happen to have been allocated contiguously.
> >        This doesn't add pressure to the IOMMU allocator. However, some
> >        drivers don't support getting large merged segments coming back
> >        from *_map_sg().
> > 
> >        Most drivers don't have this problem; it is safe to say Y here.
> > 
> > I don't know if this comment still applies to any drivers in the mainline
> > kernel, but it's possible.
> 
> That really has to be out of date these days.

Yeah, I think so.

I added dma_get_max_seg_size() several years ago so that device drives
can tell IOMMU about the maximum segment length that they can
handle. And the default limit (64K) should work for everyone, I think.

I guess that the comment was written when IOMMU was able to merge as
many segments as possible with ignoring the device limitation.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/asm-generic/scatterlist.h b/include/asm-generic/scatterlist.h
index 8b94544..1bf620d 100644
--- a/include/asm-generic/scatterlist.h
+++ b/include/asm-generic/scatterlist.h
@@ -11,7 +11,9 @@  struct scatterlist {
 	unsigned int	offset;
 	unsigned int	length;
 	dma_addr_t	dma_address;
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
 	unsigned int	dma_length;
+#endif
 };
 
 /*
@@ -22,17 +24,11 @@  struct scatterlist {
  * is 0.
  */
 #define sg_dma_address(sg)	((sg)->dma_address)
-#ifndef sg_dma_len
-/*
- * Normally, you have an iommu on 64 bit machines, but not on 32 bit
- * machines. Architectures that are differnt should override this.
- */
-#if __BITS_PER_LONG == 64
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
 #define sg_dma_len(sg)		((sg)->dma_length)
 #else
 #define sg_dma_len(sg)		((sg)->length)
-#endif /* 64 bit */
-#endif /* sg_dma_len */
+#endif
 
 #ifndef ISA_DMA_THRESHOLD
 #define ISA_DMA_THRESHOLD	(~0UL)