[PATCH] drop superfluous .align 16

[PATCH] drop superfluous .align 16

[Helge Deller]

This .align 16 is misplaced and should be in front of the ENTRY(lws_lock_start).
Works because of pure luck since we have a .align PAGE_SIZE before it instead.

Signed-off-by: Helge Deller <deller@gmx.de>

--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -640,7 +640,6 @@ END(sys_call_table64)
 	.align	PAGE_SIZE
 ENTRY(lws_lock_start)
 	/* lws locks */
-	.align 16
 	.rept 16
 	/* Keep locks aligned at 16-bytes */
 	.word 1

Re: [PATCH] drop superfluous .align 16

[Carlos O'Donell]

On Tue, Jul 8, 2008 at 4:06 PM, Helge Deller <deller@gmx.de> wrote:
> This .align 16 is misplaced and should be in front of the ENTRY(lws_lock_start).
> Works because of pure luck since we have a .align PAGE_SIZE before it instead.

This is not true, the .align 16 aligns almost any object in the
subsection including .word.

However, it is superfluous since we have a .align PAGE_SIZE, but I was
probably being safe in the event that someone put an object before
lws_lock_start.

> Signed-off-by: Helge Deller <deller@gmx.de>
>
> --- a/arch/parisc/kernel/syscall.S
> +++ b/arch/parisc/kernel/syscall.S
> @@ -640,7 +640,6 @@ END(sys_call_table64)
>        .align  PAGE_SIZE
>  ENTRY(lws_lock_start)
>        /* lws locks */
> -       .align 16
>        .rept 16
>        /* Keep locks aligned at 16-bytes */
>        .word 1

Cheers,
Carlos.

Re: [PATCH] drop superfluous .align 16

[Helge Deller]

Carlos O'Donell wrote:
> On Tue, Jul 8, 2008 at 4:06 PM, Helge Deller <deller@gmx.de> wrote:
>> This .align 16 is misplaced and should be in front of the ENTRY(lws_lock_start).
>> Works because of pure luck since we have a .align PAGE_SIZE before it instead.
> 
> This is not true, the .align 16 aligns almost any object in the
> subsection including .word.

Sure, but it does not take care of lws_lock_start itself.

> However, it is superfluous since we have a .align PAGE_SIZE, but I was
> probably being safe in the event that someone put an object before
> lws_lock_start.

What I mean is, that the lws_lock_start label should start at 16byte 
boundary, right? (it is due to the .align PAGE_SIZE).
Now, if e.g. the .word555 (see below) would be in there, then
lws_lock_start would be at PAGE_SIZE+4, while the .word1 would start at 
PAGE_SIZE+16, which is wrong for accessing the lws_lock_start variables, 
where the first entry should be "1".

Example:
        .align  PAGE_SIZE
        .word 5555  <<<- think this would be here.
ENTRY(lws_lock_start)
       /* lws locks */
      .align 16
      .rept 16
      /* Keep locks aligned at 16-bytes */
      .word 1

So, I still think it would be better to remove the .align16, or 
alternatively to move it in front of ENTRY(lws_lock_start) [to be on the 
safe side].

Am I really that wrong?

Helge

>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> --- a/arch/parisc/kernel/syscall.S
>> +++ b/arch/parisc/kernel/syscall.S
>> @@ -640,7 +640,6 @@ END(sys_call_table64)
>>        .align  PAGE_SIZE
>>  ENTRY(lws_lock_start)
>>        /* lws locks */
>> -       .align 16
>>        .rept 16
>>        /* Keep locks aligned at 16-bytes */
>>        .word 1
> 
> Cheers,
> Carlos.

Re: [PATCH] drop superfluous .align 16

[Carlos O'Donell]

On Wed, Jul 9, 2008 at 3:50 PM, Helge Deller <deller@gmx.de> wrote:
> So, I still think it would be better to remove the .align16, or
> alternatively to move it in front of ENTRY(lws_lock_start) [to be on the
> safe side].
>
> Am I really that wrong?

No, you are absolutely right. I forgot the locks are referenced via
lws_lock_start, and the symbol needs to be aligned. Sorry, I thought
there was another symbol elsewhere.

Signed-off-by: Carlos O'Donell <carlos@systemhalted.org>

Cheers,
Carlos.

Re: [PATCH] drop superfluous .align 16

[John David Anglin]

> On Wed, Jul 9, 2008 at 3:50 PM, Helge Deller <deller@gmx.de> wrote:
> > So, I still think it would be better to remove the .align16, or
> > alternatively to move it in front of ENTRY(lws_lock_start) [to be on the
> > safe side].
> >
> > Am I really that wrong?
> 
> No, you are absolutely right. I forgot the locks are referenced via
> lws_lock_start, and the symbol needs to be aligned. Sorry, I thought
> there was another symbol elsewhere.

This would be clearer if there was an ENTRY_ALIGNED macro (e.g.,
ENTRY_ALIGNED(lws_lock_start, 16)).  It's probably overkill though.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

[PATCH] - Patch series for parisc

[Helge Deller]

The following mails contain patches which I have sent to linux-parisc during 
the last weeks and which have not yet been applied.
I've cleaned them up for Linus' and Kyle's current trees.

Helge

[PATCH] drop superfluous .align 16

[Helge Deller]

This .align 16 is misplaced and should be in front of the ENTRY(lws_lock_start).
Works because of pure luck since we have a .align PAGE_SIZE before it instead.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
index 69b6eeb..c6ff81f 100644
--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -640,7 +640,6 @@ END(sys_call_table64)
 	.align	PAGE_SIZE
 ENTRY(lws_lock_start)
 	/* lws locks */
-	.align 16
 	.rept 16
 	/* Keep locks aligned at 16-bytes */
 	.word 1

Re: [PATCH] drop superfluous .align 16

[James Bottomley]

On Sun, 2008-08-24 at 20:51 +0200, Helge Deller wrote:
> This .align 16 is misplaced and should be in front of the ENTRY(lws_lock_start).
> Works because of pure luck since we have a .align PAGE_SIZE before it instead.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
> index 69b6eeb..c6ff81f 100644
> --- a/arch/parisc/kernel/syscall.S
> +++ b/arch/parisc/kernel/syscall.S
> @@ -640,7 +640,6 @@ END(sys_call_table64)
>  	.align	PAGE_SIZE
>  ENTRY(lws_lock_start)
>  	/* lws locks */
> -	.align 16
>  	.rept 16
>  	/* Keep locks aligned at 16-bytes */
>  	.word 1

I think I'd really rather keep this.  It may be technically superfluous
because of the .align PAGE_SIZE above it, but that belongs to the
syscall table.  If anyone ever moved this section in head.S, the align
16 ensures nothing goes wrong.

James

Re: [PATCH] drop superfluous .align 16

[Helge Deller]

James Bottomley wrote:
> On Sun, 2008-08-24 at 20:51 +0200, Helge Deller wrote:
>> This .align 16 is misplaced and should be in front of the ENTRY(lws_lock_start).
>> Works because of pure luck since we have a .align PAGE_SIZE before it instead.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
>> index 69b6eeb..c6ff81f 100644
>> --- a/arch/parisc/kernel/syscall.S
>> +++ b/arch/parisc/kernel/syscall.S
>> @@ -640,7 +640,6 @@ END(sys_call_table64)
>>  	.align	PAGE_SIZE
>>  ENTRY(lws_lock_start)
>>  	/* lws locks */
>> -	.align 16
>>  	.rept 16
>>  	/* Keep locks aligned at 16-bytes */
>>  	.word 1
> 
> I think I'd really rather keep this.  It may be technically superfluous
> because of the .align PAGE_SIZE above it, but that belongs to the
> syscall table.  

> If anyone ever moved this section in head.S, the align
> 16 ensures nothing goes wrong.

Hi James,

If the block is moved, then that's exactly what this .align does not 
ensure.
We had this discussion already here on the list. Please read the full 
thread: http://marc.info/?t=121554765000005&r=1&w=2

Helge

Re: [PATCH] drop superfluous .align 16

[Grant Grundler]

On Mon, Aug 25, 2008 at 02:31:40PM +0200, Helge Deller wrote:
> James Bottomley wrote:
>> On Sun, 2008-08-24 at 20:51 +0200, Helge Deller wrote:
>>> This .align 16 is misplaced and should be in front of the 
>>> ENTRY(lws_lock_start).
>>> Works because of pure luck since we have a .align PAGE_SIZE before it 
>>> instead.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>
>>> diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
>>> index 69b6eeb..c6ff81f 100644
>>> --- a/arch/parisc/kernel/syscall.S
>>> +++ b/arch/parisc/kernel/syscall.S
>>> @@ -640,7 +640,6 @@ END(sys_call_table64)
>>>  	.align	PAGE_SIZE
>>>  ENTRY(lws_lock_start)
>>>  	/* lws locks */
>>> -	.align 16
>>>  	.rept 16
>>>  	/* Keep locks aligned at 16-bytes */
>>>  	.word 1
>> I think I'd really rather keep this.  It may be technically superfluous
>> because of the .align PAGE_SIZE above it, but that belongs to the
>> syscall table.  
>
>> If anyone ever moved this section in head.S, the align
>> 16 ensures nothing goes wrong.
>
> Hi James,
>
> If the block is moved, then that's exactly what this .align does not 
> ensure.
> We had this discussion already here on the list. Please read the full 
> thread: http://marc.info/?t=121554765000005&r=1&w=2

Helge,
Thanks for tracking and resubmitting this...Carlos already ACK'd and I think
it would be good if kyle added it to his patch queue.

But I liked two other things proposed in this thread.
1) jejb's preference to document the alignement requirement 
2) jda's proposal for ENTRY_ALIGN() (which accomplishes (1))

So I've hacked your patch to include those two things.

I've only build-tested this for 64-bit builds.

thanks,
grant

Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Grant Grundler <grundler@parisc-linux.org>


diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
index 69b6eeb..faf8b37 100644
--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -638,11 +638,10 @@ END(sys_call_table64)
 	*/
 	.section .data
 	.align	PAGE_SIZE
-ENTRY(lws_lock_start)
+
+ENTRY_ALIGN(lws_lock_start,16)
 	/* lws locks */
-	.align 16
 	.rept 16
-	/* Keep locks aligned at 16-bytes */
 	.word 1
 	.word 0 
 	.word 0
diff --git a/include/asm-parisc/linkage.h b/include/asm-parisc/linkage.h
index 0b19a72..2c6cbe2 100644
--- a/include/asm-parisc/linkage.h
+++ b/include/asm-parisc/linkage.h
@@ -17,6 +17,11 @@
 	ALIGN !\
 name:
 
+#define ENTRY_ALIGN(name,alignval) \
+	.export name !\
+	.align alignval !\
+name:
+
 #ifdef CONFIG_64BIT
 #define ENDPROC(name) \
 	END(name)

Re: [PATCH] drop superfluous .align 16

[Carlos O'Donell]

On Tue, Aug 26, 2008 at 12:26 AM, Grant Grundler
<grundler@parisc-linux.org> wrote:
> Thanks for tracking and resubmitting this...Carlos already ACK'd and I think
> it would be good if kyle added it to his patch queue.
>
> But I liked two other things proposed in this thread.
> 1) jejb's preference to document the alignement requirement
> 2) jda's proposal for ENTRY_ALIGN() (which accomplishes (1))
>
> So I've hacked your patch to include those two things.
>
> I've only build-tested this for 64-bit builds.
>
> thanks,
> grant
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Grant Grundler <grundler@parisc-linux.org>
Signed-off-by: Carlos O'Donell <carlos@systemhalted.org>

I agree with both Helge's original patch and this one. Thanks for
making an ENTRY_ALIGN macro. The macro keeps the symbol and alignment
together. Well done.

Cheers,
Carlos.

Re: [PATCH] drop superfluous .align 16

[James Bottomley]

On Tue, 2008-08-26 at 08:42 -0400, Carlos O'Donell wrote:
> On Tue, Aug 26, 2008 at 12:26 AM, Grant Grundler
> <grundler@parisc-linux.org> wrote:
> > Thanks for tracking and resubmitting this...Carlos already ACK'd and I think
> > it would be good if kyle added it to his patch queue.
> >
> > But I liked two other things proposed in this thread.
> > 1) jejb's preference to document the alignement requirement
> > 2) jda's proposal for ENTRY_ALIGN() (which accomplishes (1))
> >
> > So I've hacked your patch to include those two things.
> >
> > I've only build-tested this for 64-bit builds.
> >
> > thanks,
> > grant
> >
> > Signed-off-by: Helge Deller <deller@gmx.de>
> > Signed-off-by: Grant Grundler <grundler@parisc-linux.org>

Can we get this right, please ... we'll get into awful penguin trouble
if he catches us misusing the DCO:

> Signed-off-by: Carlos O'Donell <carlos@systemhalted.org>

That's not signed off by you.  You only get to add a signed-off-by if
the patch physically passes through your hands.  That means that Grant's
above is correct because he modified the patch and resent it, but yours
isn't because you did nothing with it (and you didn't pass it on).

You can add Acked-by: if you like (that usually means I'm the person in
charge of the relevant code and I approved sending it through someone
else's tree) or Reviewed-by: (I actually looked through the patch and it
seems to be fine to me).

James

Re: [PATCH] drop superfluous .align 16

[Carlos O'Donell]

On Tue, Aug 26, 2008 at 10:10 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Tue, 2008-08-26 at 08:42 -0400, Carlos O'Donell wrote:
>> On Tue, Aug 26, 2008 at 12:26 AM, Grant Grundler
>> <grundler@parisc-linux.org> wrote:
>> > Thanks for tracking and resubmitting this...Carlos already ACK'd and I think
>> > it would be good if kyle added it to his patch queue.
>> >
>> > But I liked two other things proposed in this thread.
>> > 1) jejb's preference to document the alignement requirement
>> > 2) jda's proposal for ENTRY_ALIGN() (which accomplishes (1))
>> >
>> > So I've hacked your patch to include those two things.
>> >
>> > I've only build-tested this for 64-bit builds.
>> >
>> > thanks,
>> > grant
>> >
>> > Signed-off-by: Helge Deller <deller@gmx.de>
>> > Signed-off-by: Grant Grundler <grundler@parisc-linux.org>
>
> Can we get this right, please ... we'll get into awful penguin trouble
> if he catches us misusing the DCO:
>
>> Signed-off-by: Carlos O'Donell <carlos@systemhalted.org>
>
> That's not signed off by you.  You only get to add a signed-off-by if
> the patch physically passes through your hands.  That means that Grant's
> above is correct because he modified the patch and resent it, but yours
> isn't because you did nothing with it (and you didn't pass it on).
>
> You can add Acked-by: if you like (that usually means I'm the person in
> charge of the relevant code and I approved sending it through someone
> else's tree) or Reviewed-by: (I actually looked through the patch and it
> seems to be fine to me).

Quite right. Sorry for the cut-n-paste.

Since I wrote the code, I consider myself the maintainer.

Acked-by: Carlos O'Donell <carlos@systemhalted.org>

Cheers,
Carlos.