2014-01-03 15:53:30

by Jon Medhurst (Tixy)

[permalink] [raw]
Subject: Re: [PATCH 2/5] ARM: kprobes-test: use <asm/opcodes.h> for instruction accesses

On Mon, 2013-12-23 at 18:19 +0200, Taras Kondratiuk wrote:
> From: Ben Dooks <[email protected]>
>
> Ensure we read instructions in the correct endian-ness by using
> the <asm/opcodes.h> helper to transform them as necessary.
>
> Signed-off-by: Ben Dooks <[email protected]>
> [[email protected]: fix next_instruction() function]
> Signed-off-by: Taras Kondratiuk <[email protected]>
> ---

Apart from the spurious line removal (see below) then:

Acked-by: Jon Medhurst <[email protected]>

and you can include an ACK for the other patches in this series too.

Thanks.


> arch/arm/kernel/kprobes-test.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index 0cd63d0..96e3dbc 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -1329,7 +1329,8 @@ static void test_case_failed(const char *message)
> static unsigned long next_instruction(unsigned long pc)
> {
> #ifdef CONFIG_THUMB2_KERNEL
> - if ((pc & 1) && !is_wide_instruction(*(u16 *)(pc - 1)))
> + if ((pc & 1) &&
> + !is_wide_instruction(__mem_to_opcode_thumb16(*(u16 *)(pc - 1))))
> return pc + 2;
> else
> #endif
> @@ -1374,13 +1375,13 @@ static uintptr_t __used kprobes_test_case_start(const char *title, void *stack)
>
> if (test_case_is_thumb) {
> u16 *p = (u16 *)(test_code & ~1);
> - current_instruction = p[0];
> + current_instruction = __mem_to_opcode_thumb16(p[0]);
> if (is_wide_instruction(current_instruction)) {
> - current_instruction <<= 16;
> - current_instruction |= p[1];
> + u16 instr2 = __mem_to_opcode_thumb16(p[1]);
> + current_instruction = __opcode_thumb32_compose(current_instruction, instr2);
> }
> } else {
> - current_instruction = *(u32 *)test_code;
> + current_instruction = __mem_to_opcode_arm(*(u32 *)test_code);
> }
>
> if (current_title[0] == '.')
> @@ -1593,7 +1594,6 @@ static int run_test_cases(void (*tests)(void), const union decode_item *table)
> return 0;
> }
>
> -

I know the above blank line isn't needed but I believe the convention is
to avoid doing unrelated white-space clean-ups in patches.

> static int __init run_all_tests(void)
> {
> int ret = 0;


2014-01-09 10:55:06

by Taras Kondratiuk

[permalink] [raw]
Subject: Re: [PATCH 2/5] ARM: kprobes-test: use <asm/opcodes.h> for instruction accesses

On 3 January 2014 17:53, Jon Medhurst (Tixy) <[email protected]> wrote:
> On Mon, 2013-12-23 at 18:19 +0200, Taras Kondratiuk wrote:
>> From: Ben Dooks <[email protected]>
>>
>> Ensure we read instructions in the correct endian-ness by using
>> the <asm/opcodes.h> helper to transform them as necessary.
>>
>> Signed-off-by: Ben Dooks <[email protected]>
>> [[email protected]: fix next_instruction() function]
>> Signed-off-by: Taras Kondratiuk <[email protected]>
>> ---
>
> Apart from the spurious line removal (see below) then:
>
> Acked-by: Jon Medhurst <[email protected]>
>
> and you can include an ACK for the other patches in this series too.
>
> Thanks.

>> @@ -1593,7 +1594,6 @@ static int run_test_cases(void (*tests)(void), const union decode_item *table)
>> return 0;
>> }
>>
>> -
>
> I know the above blank line isn't needed but I believe the convention is
> to avoid doing unrelated white-space clean-ups in patches.

Right, I will remove it.
Should this series go through Russell's patch tracking system or it
can be pulled
to some tree?

--
Regards,
Taras Kondratiuk

2014-01-09 11:05:09

by Jon Medhurst (Tixy)

[permalink] [raw]
Subject: Re: [PATCH 2/5] ARM: kprobes-test: use <asm/opcodes.h> for instruction accesses

On Thu, 2014-01-09 at 12:54 +0200, Taras Kondratiuk wrote:
> On 3 January 2014 17:53, Jon Medhurst (Tixy) <[email protected]> wrote:
> > On Mon, 2013-12-23 at 18:19 +0200, Taras Kondratiuk wrote:
> >> From: Ben Dooks <[email protected]>
> >>
> >> Ensure we read instructions in the correct endian-ness by using
> >> the <asm/opcodes.h> helper to transform them as necessary.
> >>
> >> Signed-off-by: Ben Dooks <[email protected]>
> >> [[email protected]: fix next_instruction() function]
> >> Signed-off-by: Taras Kondratiuk <[email protected]>
> >> ---
> >
> > Apart from the spurious line removal (see below) then:
> >
> > Acked-by: Jon Medhurst <[email protected]>
> >
> > and you can include an ACK for the other patches in this series too.
> >
> > Thanks.
>
> >> @@ -1593,7 +1594,6 @@ static int run_test_cases(void (*tests)(void), const union decode_item *table)
> >> return 0;
> >> }
> >>
> >> -
> >
> > I know the above blank line isn't needed but I believe the convention is
> > to avoid doing unrelated white-space clean-ups in patches.
>
> Right, I will remove it.
> Should this series go through Russell's patch tracking system or it
> can be pulled
> to some tree?

I would assume as a pull request sent to Russell using the email alias
he uses for these things: [email protected] I'm sure he's
shout up if I'm wrong...

--
Tixy

2014-01-09 12:01:10

by Taras Kondratiuk

[permalink] [raw]
Subject: Re: [PATCH 2/5] ARM: kprobes-test: use <asm/opcodes.h> for instruction accesses

Thanks.
I'll prepare a pull request.

On 9 January 2014 13:04, Jon Medhurst (Tixy) <[email protected]> wrote:
> On Thu, 2014-01-09 at 12:54 +0200, Taras Kondratiuk wrote:
>> On 3 January 2014 17:53, Jon Medhurst (Tixy) <[email protected]> wrote:
>> > On Mon, 2013-12-23 at 18:19 +0200, Taras Kondratiuk wrote:
>> >> From: Ben Dooks <[email protected]>
>> >>
>> >> Ensure we read instructions in the correct endian-ness by using
>> >> the <asm/opcodes.h> helper to transform them as necessary.
>> >>
>> >> Signed-off-by: Ben Dooks <[email protected]>
>> >> [[email protected]: fix next_instruction() function]
>> >> Signed-off-by: Taras Kondratiuk <[email protected]>
>> >> ---
>> >
>> > Apart from the spurious line removal (see below) then:
>> >
>> > Acked-by: Jon Medhurst <[email protected]>
>> >
>> > and you can include an ACK for the other patches in this series too.
>> >
>> > Thanks.
>>
>> >> @@ -1593,7 +1594,6 @@ static int run_test_cases(void (*tests)(void), const union decode_item *table)
>> >> return 0;
>> >> }
>> >>
>> >> -
>> >
>> > I know the above blank line isn't needed but I believe the convention is
>> > to avoid doing unrelated white-space clean-ups in patches.
>>
>> Right, I will remove it.
>> Should this series go through Russell's patch tracking system or it
>> can be pulled
>> to some tree?
>
> I would assume as a pull request sent to Russell using the email alias
> he uses for these things: [email protected] I'm sure he's
> shout up if I'm wrong...
>
> --
> Tixy
>



--
Regards,
Taras Kondratiuk