Misunderstanding with function cpm_dpalloc in Linux 2.6.x

Misunderstanding with function cpm_dpalloc in Linux 2.6.x

[Laurent Lagrange]

Hello,

I work on MPC boards with Linux 2.6.9 and I have a problem with the
cpm_dpalloc function.

In the file cpm2_common.c the function "uint cpm_dpalloc(uint size, uint
align)" is intended
to allocate a piece of memory in the dpram. This piece of memory has at
least "size" bytes
and I beleived that the returned address should be aligned on "align" bytes.

This function calls "void *rh_alloc(rh_info_t * info, int size, const char
*owner)" from rheap.c file.
In this function the alignment is only used to calculate the right size with
the formula :
size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
It seems to be right but the alignment is not used to retreive an aligned
start address.

So if I do the following sequential calls, I have the following results :
cpm_dpalloc(16, 8)	-> 	0XC0		-> right aligned
cpm_dpalloc(64, 64)	-> 	0XD0		-> wrong aligned -> must be 0x100
cpm_dpalloc(16, 8)	-> 	0X110		-> right aligned but wrong to use
cpm_dpalloc(24, 8)	-> 	0X1200		-> right aligned but wrong to use

I can have the wanted values if instead of size, I try to allocate (size +
align - 1) bytes
but I don't think it is the solution.

Perhaps, I don't understand the allocation theory.
Any idea ?

Thanks all
Laurent

Re: Misunderstanding with function cpm_dpalloc in Linux 2.6.x

[Pantelis Antoniou]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Wednesday 25 January 2006 15:48, Laurent Lagrange wrote:
> 
> Hello,
> 
> I work on MPC boards with Linux 2.6.9 and I have a problem with the
> cpm_dpalloc function.
> 
> In the file cpm2_common.c the function "uint cpm_dpalloc(uint size, uint
> align)" is intended
> to allocate a piece of memory in the dpram. This piece of memory has at
> least "size" bytes
> and I beleived that the returned address should be aligned on "align" bytes.
> 
> This function calls "void *rh_alloc(rh_info_t * info, int size, const char
> *owner)" from rheap.c file.
> In this function the alignment is only used to calculate the right size with
> the formula :
> size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
> It seems to be right but the alignment is not used to retreive an aligned
> start address.
> 
> So if I do the following sequential calls, I have the following results :
> cpm_dpalloc(16, 8)	-> 	0XC0		-> right aligned
> cpm_dpalloc(64, 64)	-> 	0XD0		-> wrong aligned -> must be 0x100
> cpm_dpalloc(16, 8)	-> 	0X110		-> right aligned but wrong to use
> cpm_dpalloc(24, 8)	-> 	0X1200		-> right aligned but wrong to use
> 
> I can have the wanted values if instead of size, I try to allocate (size +
> align - 1) bytes
> but I don't think it is the solution.
> 
> Perhaps, I don't understand the allocation theory.
> Any idea ?
> 
> Thanks all
> Laurent
> 
> 

Laurent Hi,

Yes this is a know bug. This patch fixes it.

Marcelo, please apply - we have this hanging for quite a while.

Pantelis

[-- Attachment #2: cpm2-dpalloc.patch --]
[-- Type: text/x-diff, Size: 4160 bytes --]

diff --git a/arch/ppc/lib/rheap.c b/arch/ppc/lib/rheap.c
--- a/arch/ppc/lib/rheap.c
+++ b/arch/ppc/lib/rheap.c
@@ -425,17 +425,21 @@ void *rh_detach_region(rh_info_t * info,
 	return (void *)s;
 }
 
-void *rh_alloc(rh_info_t * info, int size, const char *owner)
+void *rh_alloc_align(rh_info_t * info, int size, int alignment, const char *owner)
 {
 	struct list_head *l;
 	rh_block_t *blk;
 	rh_block_t *newblk;
 	void *start;
 
-	/* Validate size */
-	if (size <= 0)
+	/* Validate size, (must be power of two) */
+	if (size <= 0 || (alignment & (alignment - 1)) != 0)
 		return ERR_PTR(-EINVAL);
 
+	/* given alignment larger that default rheap alignment */
+	if (alignment > info->alignment)
+		size += alignment - 1;
+
 	/* Align to configured alignment */
 	size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
 
@@ -478,9 +482,21 @@ void *rh_alloc(rh_info_t * info, int siz
 
 	attach_taken_block(info, newblk);
 
+	/* for larger alignment return fixed up pointer  */
+	/* this is no problem with the deallocator since */
+	/* we scan for pointers that lie in the blocks   */
+	if (alignment > info->alignment)
+		start = (void *)(((unsigned long)start + alignment - 1) &
+				~(alignment - 1));
+
 	return start;
 }
 
+void *rh_alloc(rh_info_t * info, int size, const char *owner)
+{
+	return rh_alloc_align(info, size, info->alignment, owner);
+}
+
 /* allocate at precisely the given address */
 void *rh_alloc_fixed(rh_info_t * info, void *start, int size, const char *owner)
 {
diff --git a/arch/ppc/syslib/cpm2_common.c b/arch/ppc/syslib/cpm2_common.c
--- a/arch/ppc/syslib/cpm2_common.c
+++ b/arch/ppc/syslib/cpm2_common.c
@@ -148,8 +148,7 @@ uint cpm_dpalloc(uint size, uint align)
 	unsigned long flags;
 
 	spin_lock_irqsave(&cpm_dpmem_lock, flags);
-	cpm_dpmem_info.alignment = align;
-	start = rh_alloc(&cpm_dpmem_info, size, "commproc");
+	start = rh_alloc_align(&cpm_dpmem_info, size, align, "commproc");
 	spin_unlock_irqrestore(&cpm_dpmem_lock, flags);
 
 	return (uint)start;
@@ -170,13 +169,12 @@ int cpm_dpfree(uint offset)
 EXPORT_SYMBOL(cpm_dpfree);
 
 /* not sure if this is ever needed */
-uint cpm_dpalloc_fixed(uint offset, uint size, uint align)
+uint cpm_dpalloc_fixed(uint offset, uint size)
 {
 	void *start;
 	unsigned long flags;
 
 	spin_lock_irqsave(&cpm_dpmem_lock, flags);
-	cpm_dpmem_info.alignment = align;
 	start = rh_alloc_fixed(&cpm_dpmem_info, (void *)offset, size, "commproc");
 	spin_unlock_irqrestore(&cpm_dpmem_lock, flags);
 
diff --git a/include/asm-ppc/commproc.h b/include/asm-ppc/commproc.h
--- a/include/asm-ppc/commproc.h
+++ b/include/asm-ppc/commproc.h
@@ -74,7 +74,7 @@ static inline long IS_DPERR(const uint o
 extern	cpm8xx_t	*cpmp;		/* Pointer to comm processor */
 extern uint cpm_dpalloc(uint size, uint align);
 extern int cpm_dpfree(uint offset);
-extern uint cpm_dpalloc_fixed(uint offset, uint size, uint align);
+extern uint cpm_dpalloc_fixed(uint offset, uint size);
 extern void cpm_dpdump(void);
 extern void *cpm_dpram_addr(uint offset);
 extern void cpm_setbrg(uint brg, uint rate);
diff --git a/include/asm-ppc/cpm2.h b/include/asm-ppc/cpm2.h
--- a/include/asm-ppc/cpm2.h
+++ b/include/asm-ppc/cpm2.h
@@ -112,7 +112,7 @@ extern		cpm_cpm2_t	*cpmp;	 /* Pointer to
 
 extern uint cpm_dpalloc(uint size, uint align);
 extern int cpm_dpfree(uint offset);
-extern uint cpm_dpalloc_fixed(uint offset, uint size, uint align);
+extern uint cpm_dpalloc_fixed(uint offset, uint size);
 extern void cpm_dpdump(void);
 extern void *cpm_dpram_addr(uint offset);
 extern void cpm_setbrg(uint brg, uint rate);
diff --git a/include/asm-ppc/rheap.h b/include/asm-ppc/rheap.h
--- a/include/asm-ppc/rheap.h
+++ b/include/asm-ppc/rheap.h
@@ -62,6 +62,10 @@ extern int rh_attach_region(rh_info_t * 
 /* Detach a free region */
 extern void *rh_detach_region(rh_info_t * info, void *start, int size);
 
+/* Allocate the given size from the remote heap (with alignment) */
+extern void *rh_alloc_align(rh_info_t * info, int size, int alignment,
+		const char *owner);
+
 /* Allocate the given size from the remote heap */
 extern void *rh_alloc(rh_info_t * info, int size, const char *owner);
 

Linux 2.6.x : cpm_dpalloc alignment bug perhaps not fully resolved

[Laurent Lagrange]

Hello Pantelis,

Few months ago (25 January 2006), I sent a mail about an alignment bug in
cpm_dpalloc.
I applied and verified the provided patch. I was very satisfied with the
result.

Today I port a driver from Linux 2.4 to Linux 2.6 and I have strange
results.

The driver allocates rx and tx bds (8 bytes aligned) in the module_init for
4 SCC ports.
That is always right. Then one port is opened by an application and a user
configuration
is set via an ioctl (set_conf).

This ioctl first frees the old bds :
	cpm_dpfree(chan->rx_bd_offset);
	cpm_dpfree(chan->tx_bd_offset);
then allocates the new ones :
	chan->rx_bd_offset = cpm_dpalloc(sizeof(cbd_t) * chan->conf.rx_bufnbr, 8);
	chan->tx_bd_offset = cpm_dpalloc(sizeof(cbd_t) * chan->conf.tx_bufnbr, 8);
with rx_bufnbr == 8 and tx_bufnbr == 2

module_init
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a8
SCC2 rx_bd_offset=1c0
SCC2 tx_bd_offset=208
SCC3 rx_bd_offset=220
SCC3 tx_bd_offset=260
SCC4 rx_bd_offset=278
SCC4 tx_bd_offset=2c0
set_conf
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a4	-> ???

module_init
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a8
SCC2 rx_bd_offset=1c0
SCC2 tx_bd_offset=208
SCC3 rx_bd_offset=220
SCC3 tx_bd_offset=260
SCC4 rx_bd_offset=278
SCC4 tx_bd_offset=2c0
set_conf
SCC2 rx_bd_offset=1c0
SCC2 tx_bd_offset=202	-> ???

module_init
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a8
SCC2 rx_bd_offset=1c0
SCC2 tx_bd_offset=208
SCC3 rx_bd_offset=220
SCC3 tx_bd_offset=260
SCC4 rx_bd_offset=278
SCC4 tx_bd_offset=2c0
set_conf
SCC3 rx_bd_offset=220
SCC3 tx_bd_offset=260	-> ok

module_init
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a8
SCC2 rx_bd_offset=1c0
SCC2 tx_bd_offset=208
SCC3 rx_bd_offset=220
SCC3 tx_bd_offset=260
SCC4 rx_bd_offset=278
SCC4 tx_bd_offset=2c0
set_conf
SCC4 rx_bd_offset=278
SCC4 tx_bd_offset=2c0	-> ok

WARNING : if I only uses the SCC1 port without allocating bds for the other
ports,
I can free and reallocate the bds for the SCC1 port as many times I want.

module_init
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a8
set_conf
SCC1 rx_bd_offset=160
SCC1 tx_bd_offset=1a8	-> ok

Really, I don't understand how this can arise.
Any idea ?

Thanks
Laurent