[PATCH] sparc: use asm-generic/scatterlist.h

[PATCH] sparc: use asm-generic/scatterlist.h

[FUJITA Tomonori]

sparc's scatterlist structure is identical to the generic one.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/sparc/include/asm/scatterlist.h |   21 +--------------------
 1 files changed, 1 insertions(+), 20 deletions(-)

diff --git a/arch/sparc/include/asm/scatterlist.h b/arch/sparc/include/asm/scatterlist.h
index e580f55..d112025 100644
--- a/arch/sparc/include/asm/scatterlist.h
+++ b/arch/sparc/include/asm/scatterlist.h
@@ -1,27 +1,8 @@
 #ifndef _SPARC_SCATTERLIST_H
 #define _SPARC_SCATTERLIST_H
 
-#include <asm/page.h>
-#include <asm/types.h>
-
-struct scatterlist {
-#ifdef CONFIG_DEBUG_SG
-	unsigned long	sg_magic;
-#endif
-	unsigned long	page_link;
-	unsigned int	offset;
-
-	unsigned int	length;
-
-	dma_addr_t	dma_address;
-	__u32		dma_length;
-};
-
-#define sg_dma_address(sg)	((sg)->dma_address)
 #define sg_dma_len(sg)     	((sg)->dma_length)
 
-#define ISA_DMA_THRESHOLD	(~0UL)
-
-#define ARCH_HAS_SG_CHAIN
+#include <asm-generic/scatterlist.h>
 
 #endif /* !(_SPARC_SCATTERLIST_H) */
-- 
1.6.5

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[David Miller]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Fri, 26 Feb 2010 09:43:51 +0900

> sparc's scatterlist structure is identical to the generic one.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Applied, thank you.

BTW, the conditional sg_dma_len() definition cpp games done
in asm-generic/scatterlist.h might be superfluous these days.

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[FUJITA Tomonori]

On Fri, 26 Feb 2010 04:35:36 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Date: Fri, 26 Feb 2010 09:43:51 +0900
> 
> > sparc's scatterlist structure is identical to the generic one.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Applied, thank you.

Thanks,

> BTW, the conditional sg_dma_len() definition cpp games done
> in asm-generic/scatterlist.h might be superfluous these days.

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)

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.

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[David Miller]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Mon, 1 Mar 2010 15:05:48 +0900

> I'll clean up this in the next merge window.

Sounds great!

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[Arnd Bergmann]

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.
 
> 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.

	Arnd

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[FUJITA Tomonori]

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.

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)

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[Arnd Bergmann]

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

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[FUJITA Tomonori]

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).

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[Arnd Bergmann]

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

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[FUJITA Tomonori]

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.

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[Arnd Bergmann]

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

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[David Miller]

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.

Re: [PATCH] sparc: use asm-generic/scatterlist.h

[FUJITA Tomonori]

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.