[PATCH] scatterlist: define SG chain for arm architecture

[PATCH] scatterlist: define SG chain for arm architecture

[Fernando Guzman Lugo]

Define SG chain for ARM architecture in order to work
with big buffers. This patch is needed in order to
iovmm users can pass a big buffer to map it and ARM
architecture has not issues using SG chain.

This change was suggested by FUJITA Tomonori.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/include/asm/scatterlist.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/scatterlist.h b/arch/arm/include/asm/scatterlist.h
index 2f87870..3de0a0b 100644
--- a/arch/arm/include/asm/scatterlist.h
+++ b/arch/arm/include/asm/scatterlist.h
@@ -5,4 +5,6 @@
 #include <asm/types.h>
 #include <asm-generic/scatterlist.h>
 
+#define ARCH_HAS_SG_CHAIN
+
 #endif /* _ASMARM_SCATTERLIST_H */
-- 
1.6.3.3

Re: [PATCH] scatterlist: define SG chain for arm architecture

[Russell King - ARM Linux]

On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo wrote:
> Define SG chain for ARM architecture in order to work
> with big buffers. This patch is needed in order to
> iovmm users can pass a big buffer to map it and ARM
> architecture has not issues using SG chain.
> 
> This change was suggested by FUJITA Tomonori.

Have you audited all places where SG lists are used in the ARM
architecture to ensure that SG chain support is safe?  (That
goes for ARM generic code and ARM specific drivers.)

If the answer is no, this patch is unacceptable until such a
review has been carried out.

RE: [PATCH] scatterlist: define SG chain for arm architecture

[Guzman Lugo, Fernando]

 

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] 
> Sent: Friday, October 01, 2010 11:40 AM
> To: Guzman Lugo, Fernando
> Cc: linux-arm-kernel@lists.infradead.org; 
> Hiroshi.DOYU@nokia.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] scatterlist: define SG chain for arm architecture
> 
> On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo wrote:
> > Define SG chain for ARM architecture in order to work with big 
> > buffers. This patch is needed in order to iovmm users can 
> pass a big 
> > buffer to map it and ARM architecture has not issues using SG chain.
> > 
> > This change was suggested by FUJITA Tomonori.
> 
> Have you audited all places where SG lists are used in the 
> ARM architecture to ensure that SG chain support is safe?  
> (That goes for ARM generic code and ARM specific drivers.)

Unfortunately I only can test on omap platform. I am not so
Familiar with scatter gather code and what is the restriction
For one page size of SG list (maybe they can comment). However
For SG chaining seems there is no restriction with the exception
Of using the with function to assigned to page and iterate each
Element of the list and it has sometime when SG chaining was 
Introduce all the code should be use this new functions.

I am agree with you about make sure nothing is broken with the change.
Maybe some people of other arm platforms can comment if they have
Issues with the patch and test the change and if there is no complain
Then merge the change. If there are issues maybe I can send the patch
Just for omap platform. However, looks like there is no issues enabling
It to all arm, but again I am not so familiar with SG, so I will let
The experts to comment.

Thanks a lot,
Fernando.




> 
> If the answer is no, this patch is unacceptable until such a 
> review has been carried out.
> 

Re: [PATCH] scatterlist: define SG chain for arm architecture

[FUJITA Tomonori]

On Fri, 1 Oct 2010 17:39:58 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo wrote:
> > Define SG chain for ARM architecture in order to work
> > with big buffers. This patch is needed in order to
> > iovmm users can pass a big buffer to map it and ARM
> > architecture has not issues using SG chain.
> > 
> > This change was suggested by FUJITA Tomonori.
> 
> Have you audited all places where SG lists are used in the ARM
> architecture to ensure that SG chain support is safe?  (That
> goes for ARM generic code and ARM specific drivers.)

As far as I know, only SCSI actually uses SG chain feature (that is,
sending chained scatter lists). All SCSI drivers were converted when
we added SG chain feature. So what you need to care about is the dma
code.

There are only several files that actually use scatter list under
arch/arm.

>From a quick look, arch/arm/mm/dma-mapping.c,
arch/arm/common/dmabounce.c, arch/arm/kernel/dma.c, and
arch/arm/include/asm/dma-mapping.h looks fine.

arm/mach-imx/dma-v1.c also looks like to use chained sg API properly
(but this isn't used with SCSI, right?).

Seems that arch/arm/plat-omap/iovmm.c can handle the chained SG
too. It doesn't allocate the chained SG now though.

Re: [PATCH] scatterlist: define SG chain for arm architecture

[Russell King - ARM Linux]

On Sun, Oct 03, 2010 at 08:02:52PM +0900, FUJITA Tomonori wrote:
> On Fri, 1 Oct 2010 17:39:58 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo wrote:
> > > Define SG chain for ARM architecture in order to work
> > > with big buffers. This patch is needed in order to
> > > iovmm users can pass a big buffer to map it and ARM
> > > architecture has not issues using SG chain.
> > > 
> > > This change was suggested by FUJITA Tomonori.
> > 
> > Have you audited all places where SG lists are used in the ARM
> > architecture to ensure that SG chain support is safe?  (That
> > goes for ARM generic code and ARM specific drivers.)
> 
> As far as I know, only SCSI actually uses SG chain feature (that is,
> sending chained scatter lists). All SCSI drivers were converted when
> we added SG chain feature. So what you need to care about is the dma
> code.
> 
> There are only several files that actually use scatter list under
> arch/arm.
> 
> >From a quick look, arch/arm/mm/dma-mapping.c,
> arch/arm/common/dmabounce.c, arch/arm/kernel/dma.c, and
> arch/arm/include/asm/dma-mapping.h looks fine.
> 
> arm/mach-imx/dma-v1.c also looks like to use chained sg API properly
> (but this isn't used with SCSI, right?).
> 
> Seems that arch/arm/plat-omap/iovmm.c can handle the chained SG
> too. It doesn't allocate the chained SG now though.

There's also RiscPC and its SCSI drivers could well be an issue for SG
chaining.  They have to manually save the SCSI 'data pointer' (down to
the byte position in the SG chain and be able to restore a previously
saved point.)  This code was written before SG chaining ever appeared,
and although it has been partly fixed up, it's probably buggy if SG
chaining were to be enabled.

If it's just OMAP users who wish to enable SG chaining, then the sane
thing to do would be to make the definition of the macro conditional
on OMAP.

Re: [PATCH] scatterlist: define SG chain for arm architecture

[FUJITA Tomonori]

On Sun, 3 Oct 2010 12:28:39 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Sun, Oct 03, 2010 at 08:02:52PM +0900, FUJITA Tomonori wrote:
> > On Fri, 1 Oct 2010 17:39:58 +0100
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 
> > > On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo wrote:
> > > > Define SG chain for ARM architecture in order to work
> > > > with big buffers. This patch is needed in order to
> > > > iovmm users can pass a big buffer to map it and ARM
> > > > architecture has not issues using SG chain.
> > > > 
> > > > This change was suggested by FUJITA Tomonori.
> > > 
> > > Have you audited all places where SG lists are used in the ARM
> > > architecture to ensure that SG chain support is safe?  (That
> > > goes for ARM generic code and ARM specific drivers.)
> > 
> > As far as I know, only SCSI actually uses SG chain feature (that is,
> > sending chained scatter lists). All SCSI drivers were converted when
> > we added SG chain feature. So what you need to care about is the dma
> > code.
> > 
> > There are only several files that actually use scatter list under
> > arch/arm.
> > 
> > >From a quick look, arch/arm/mm/dma-mapping.c,
> > arch/arm/common/dmabounce.c, arch/arm/kernel/dma.c, and
> > arch/arm/include/asm/dma-mapping.h looks fine.
> > 
> > arm/mach-imx/dma-v1.c also looks like to use chained sg API properly
> > (but this isn't used with SCSI, right?).
> > 
> > Seems that arch/arm/plat-omap/iovmm.c can handle the chained SG
> > too. It doesn't allocate the chained SG now though.
> 
> There's also RiscPC and its SCSI drivers could well be an issue for SG
> chaining.  They have to manually save the SCSI 'data pointer' (down to
> the byte position in the SG chain and be able to restore a previously
> saved point.)  This code was written before SG chaining ever appeared,
> and although it has been partly fixed up, it's probably buggy if SG
> chaining were to be enabled.

What SCSI drviers we need to fix?


> If it's just OMAP users who wish to enable SG chaining, then the sane
> thing to do would be to make the definition of the macro conditional
> on OMAP.

Yeah, that should work.

Re: [PATCH] scatterlist: define SG chain for arm architecture

[Guzman Lugo, Fernando]

On Sun, Oct 3, 2010 at 6:34 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Sun, 3 Oct 2010 12:28:39 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>
>> On Sun, Oct 03, 2010 at 08:02:52PM +0900, FUJITA Tomonori wrote:
>> > On Fri, 1 Oct 2010 17:39:58 +0100
>> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>> >
>> > > On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo
> wrote:
>> > > > Define SG chain for ARM architecture in order to work
>> > > > with big buffers. This patch is needed in order to
>> > > > iovmm users can pass a big buffer to map it and ARM
>> > > > architecture has not issues using SG chain.
>> > > >
>> > > > This change was suggested by FUJITA Tomonori.
>> > >
>> > > Have you audited all places where SG lists are used in the ARM
>> > > architecture to ensure that SG chain support is safe?  (That
>> > > goes for ARM generic code and ARM specific drivers.)
>> >
>> > As far as I know, only SCSI actually uses SG chain feature (that is,
>> > sending chained scatter lists). All SCSI drivers were converted when
>> > we added SG chain feature. So what you need to care about is the dma
>> > code.
>> >
>> > There are only several files that actually use scatter list under
>> > arch/arm.
>> >
>> > >From a quick look, arch/arm/mm/dma-mapping.c,
>> > arch/arm/common/dmabounce.c, arch/arm/kernel/dma.c, and
>> > arch/arm/include/asm/dma-mapping.h looks fine.
>> >
>> > arm/mach-imx/dma-v1.c also looks like to use chained sg API properly
>> > (but this isn't used with SCSI, right?).
>> >
>> > Seems that arch/arm/plat-omap/iovmm.c can handle the chained SG
>> > too. It doesn't allocate the chained SG now though.
>>
>> There's also RiscPC and its SCSI drivers could well be an issue for SG
>> chaining.  They have to manually save the SCSI 'data pointer' (down to
>> the byte position in the SG chain and be able to restore a previously
>> saved point.)  This code was written before SG chaining ever appeared,
>> and although it has been partly fixed up, it's probably buggy if SG
>> chaining were to be enabled.
>
> What SCSI drviers we need to fix?

Russell,

Any update with this? I hope it can be merged for 2.6.38.

>
>
>> If it's just OMAP users who wish to enable SG chaining, then the sane
>> thing to do would be to make the definition of the macro conditional
>> on OMAP.
>
> Yeah, that should work.

some like this in scatterlist.h is ok?

#ifdef CONFIG_ARCH_OMAP
#define ARCH_HAS_SG_CHAIN
#endif


Regards,
Fernando.
>

Re: [PATCH] scatterlist: define SG chain for arm architecture

[Guzman Lugo, Fernando]

On Thu, Dec 2, 2010 at 11:41 AM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Sun, Oct 3, 2010 at 6:34 AM, FUJITA Tomonori
> <fujita.tomonori@lab.ntt.co.jp> wrote:
>> On Sun, 3 Oct 2010 12:28:39 +0100
>> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>>
>>> On Sun, Oct 03, 2010 at 08:02:52PM +0900, FUJITA Tomonori wrote:
>>> > On Fri, 1 Oct 2010 17:39:58 +0100
>>> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>>> >
>>> > > On Fri, Oct 01, 2010 at 11:28:41AM -0500, Fernando Guzman Lugo
>> wrote:
>>> > > > Define SG chain for ARM architecture in order to work
>>> > > > with big buffers. This patch is needed in order to
>>> > > > iovmm users can pass a big buffer to map it and ARM
>>> > > > architecture has not issues using SG chain.
>>> > > >
>>> > > > This change was suggested by FUJITA Tomonori.
>>> > >
>>> > > Have you audited all places where SG lists are used in the ARM
>>> > > architecture to ensure that SG chain support is safe?  (That
>>> > > goes for ARM generic code and ARM specific drivers.)
>>> >
>>> > As far as I know, only SCSI actually uses SG chain feature (that is,
>>> > sending chained scatter lists). All SCSI drivers were converted when
>>> > we added SG chain feature. So what you need to care about is the dma
>>> > code.
>>> >
>>> > There are only several files that actually use scatter list under
>>> > arch/arm.
>>> >
>>> > >From a quick look, arch/arm/mm/dma-mapping.c,
>>> > arch/arm/common/dmabounce.c, arch/arm/kernel/dma.c, and
>>> > arch/arm/include/asm/dma-mapping.h looks fine.
>>> >
>>> > arm/mach-imx/dma-v1.c also looks like to use chained sg API properly
>>> > (but this isn't used with SCSI, right?).
>>> >
>>> > Seems that arch/arm/plat-omap/iovmm.c can handle the chained SG
>>> > too. It doesn't allocate the chained SG now though.
>>>
>>> There's also RiscPC and its SCSI drivers could well be an issue for SG
>>> chaining.  They have to manually save the SCSI 'data pointer' (down to
>>> the byte position in the SG chain and be able to restore a previously
>>> saved point.)  This code was written before SG chaining ever appeared,
>>> and although it has been partly fixed up, it's probably buggy if SG
>>> chaining were to be enabled.
>>
>> What SCSI drviers we need to fix?
>
> Russell,
>
> Any update with this? I hope it can be merged for 2.6.38.


Russell,

The motivation for this patch is:

The tidspbridge driver running in omap platform (omap3) is migrating
to use the iommu and iovmm modules form DMM propose. In order to map
user-side buffers into DSP virtual address we use the function
iommu_vmap() in iovmm.c file, the third parameter of that funcion is
of struct sg_table type, which contains all the pages that will be
mapped.

There is a restriction about the size of the sg list, that cannot be
bigger than PAGE_SIZE, so that the number of pages cannot be bigger
than SG_MAX_SINGLE_ALLOC. Therefore there is a restriction in the size
of the buffer which iommu_vmap() can map. However MM apps using
tidspbridge maps big buffers most of the time, so it is not possible
to use  iommu_vmap() if it cannot map big buffer. Enabling SG chain
for ARM resolves that problem.

If you have a better solution please let me know. Otherwise I will
follow your comment about make it a Kconfig entry to enable SG chain
for ARM.


Thanks a lot,
Fernando.

>
>>
>>
>>> If it's just OMAP users who wish to enable SG chaining, then the sane
>>> thing to do would be to make the definition of the macro conditional
>>> on OMAP.
>>
>> Yeah, that should work.
>
> some like this in scatterlist.h is ok?
>
> #ifdef CONFIG_ARCH_OMAP
> #define ARCH_HAS_SG_CHAIN
> #endif
>
>
> Regards,
> Fernando.
>>
>

[PATCH] scatterlist: define SG chain for arm architecture

[Fernando Guzman Lugo]

Define SG chain for ARM architecture in order to work
with big buffers. This patch is needed in order to
iovmm users can pass a big buffer to map it and ARM
architecture has not issues using SG chain.

This change was suggested by FUJITA Tomonori.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/include/asm/scatterlist.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/scatterlist.h b/arch/arm/include/asm/scatterlist.h
index 2f87870..3de0a0b 100644
--- a/arch/arm/include/asm/scatterlist.h
+++ b/arch/arm/include/asm/scatterlist.h
@@ -5,4 +5,6 @@
 #include <asm/types.h>
 #include <asm-generic/scatterlist.h>
 
+#define ARCH_HAS_SG_CHAIN
+
 #endif /* _ASMARM_SCATTERLIST_H */
-- 
1.6.3.3

RE: [PATCH] scatterlist: define SG chain for arm architecture

[Guzman Lugo, Fernando]

Please discard this patch, it should have sent to linux-arm list.

Regards,
Fernando.

> 
> ________________________________________
> From: Guzman Lugo, Fernando
> Sent: Thursday, September 30, 2010 10:22 PM
> To: tony@atomide.com
> Cc: Hiroshi.DOYU@nokia.com; linux-kernel@vger.kernel.org; linux-omap@vger.ke
> rnel.org; Guzman Lugo, Fernando
> Subject: [PATCH] scatterlist: define SG chain for arm architecture
> 
> Define SG chain for ARM architecture in order to work
> with big buffers. This patch is needed in order to
> iovmm users can pass a big buffer to map it and ARM
> architecture has not issues using SG chain.
> 
> This change was suggested by FUJITA Tomonori.
> 
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> ---
>  arch/arm/include/asm/scatterlist.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/scatterlist.h b/arch/arm/include/asm/scatt
> erlist.h
> index 2f87870..3de0a0b 100644
> --- a/arch/arm/include/asm/scatterlist.h
> +++ b/arch/arm/include/asm/scatterlist.h
> @@ -5,4 +5,6 @@
>  #include <asm/types.h>
>  #include <asm-generic/scatterlist.h>
> 
> +#define ARCH_HAS_SG_CHAIN
> +
>  #endif /* _ASMARM_SCATTERLIST_H */
> --
> 1.6.3.3
>