(2014/01/15 9:33), Steven Rostedt wrote:
> On Tue, 10 Dec 2013 16:42:15 +0100
> Petr Mladek <pmla...@suse.cz> wrote:
>
>> diff --git a/arch/x86/include/asm/alternative.h
>> b/arch/x86/include/asm/alternative.h
>> index 586747f5f41d..82ffe7e1529c 100644
>> --- a/arch/x86/include/asm/alternative.h
>> +++ b/arch/x86/include/asm/alternative.h
>> @@ -232,4 +232,40 @@ extern int text_poke_bp(void *addr, const void *opcode,
>> size_t len,
>> extern void text_poke_bp_or_die(void *addr, const void *opcode, size_t len,
>> void *handler);
>
> Small nit. If you can, place comments on the same line as the
> structure field.
>
>> +struct text_poke_bp_iter {
>> + /* information used to start iteration from the beginning */
>> + void *init;
>> + /* length of the patched instruction */
>> + size_t len;
>> + /* details about failure if any */
>> + int fail_count;
>> + void *fail_addr;
>
> The above should have the comments on the same line as the field.
> Something like this:
>
> void *init; /* information used to start
> iteration from the beginning */
>
> The comments for the function pointers below are fine.
>
>> + /* iteration over entries */
>> + void *(*start)(void *init);
>> + void *(*next)(void *prev);
>> + /* callback to get patched address */
>> + void *(*get_addr)(void *cur);
>> + /*
>> + * Callbacks to get the patched code. They could return NULL if no
>> + * patching is needed; This is useful for example in ftrace.
>> + */
>> + const void *(*get_opcode)(void *cur);
>> + const void *(*get_old_opcode)(void *cur);
>> + /*
>> + * Optional function that is called when the patching of the given
>> + * has finished. It might be NULL if no postprocess is needed.
>> + */
>> + int (*finish)(void *cur);
>> + /*
>> + * Helper function for int3 handler. It decides whether the given IP
>> + * is being patched or not.
>> + *
>> + * Try to implement it as fast as possible. It affects performance
>> + * of the system when the patching is in progress.
>> + */
>> + void *(*is_handled)(const unsigned long ip);
>> +};
>> +
>> +extern int text_poke_bp_list(struct text_poke_bp_iter *iter);
>> +
>> #endif /* _ASM_X86_ALTERNATIVE_H */
>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>> index 6436beec7b0c..8e57ac03a0e8 100644
>> --- a/arch/x86/kernel/alternative.c
>> +++ b/arch/x86/kernel/alternative.c
>> @@ -7,6 +7,7 @@
>> #include <linux/stringify.h>
>> #include <linux/kprobes.h>
>> #include <linux/mm.h>
>> +#include <linux/uaccess.h>
>> #include <linux/vmalloc.h>
>> #include <linux/memory.h>
>> #include <linux/stop_machine.h>
>> @@ -610,8 +611,11 @@ static void run_sync(void)
>> on_each_cpu(do_sync_core, NULL, 1);
>> }
>>
>> +static char bp_int3;
>
> bp_int3 is not going to be anything but 0xcc. Let's change that to:
>
> static char bp_int3 = 0xcc;
>
> And remove the other initializations.
just a comment.
If it is always 0xcc, it should be a const variable.
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
