lkml.org 
[lkml]   [2019]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH V4 2/9] jump_label: Add the jump_label_can_update_check() helper
> Subject: Re: [PATCH V4 2/9] jump_label: Add the jump_label_can_update_check() helper

s/the/a/

On Mon, Feb 04, 2019 at 08:58:55PM +0100, Daniel Bristot de Oliveira wrote:
> Move the check of if a jump_entry is valid to a function.

s/of //

> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 288d630da22d..456c0d7cbb5b 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -374,22 +374,32 @@ static enum jump_label_type jump_label_type(struct jump_entry *entry)
> return enabled ^ branch;
> }
>
> +bool jump_label_can_update_check(struct jump_entry *entry, bool init)

static.

Also, "jump_label_can_update" is sufficient for a name AFAICT.

> +{
> + /*
> + * An entry->code of 0 indicates an entry which has been
> + * disabled because it was in an init text area.
> + */
> + if (init || !jump_entry_is_init(entry)) {
> + if (!kernel_text_address(jump_entry_code(entry))) {
> + WARN_ONCE(1, "can't patch jump_label at %pS",
> + (void *)jump_entry_code(entry));
> + return 0;
> + }
> + return 1;
> + }
> + return 0;

Those should be bools which it returns, no?

Also, I'd do the function this way, to make it more readable and not
have three returns back-to-back. :)

/*
* An entry->code of 0 indicates an entry which has been disabled because it
* was in an init text area.
*/
bool jump_label_can_update(struct jump_entry *entry, bool init)
{
if (!init && jump_entry_is_init(entry))
return false;

if (WARN_ON_ONCE(!kernel_text_address(jump_entry_code(entry))),
"can't patch jump_label at %pS", (void *)jump_entry_code(entry))
return false;

return true;
}

That second check could be even:

if (WARN_ON_ONCE(!kernel_text_address(jump_entry_code(entry))),
"can't patch jump_label at %pS", (void *)jump_entry_code(entry))
return false;
but that's not more readable than above, I'd say.

> static void __jump_label_update(struct static_key *key,
> struct jump_entry *entry,
> struct jump_entry *stop,
> bool init)
> {
> for_each_label_entry(key, entry, stop) {
> - /*
> - * An entry->code of 0 indicates an entry which has been
> - * disabled because it was in an init text area.
> - */
> - if (init || !jump_entry_is_init(entry)) {
> - if (kernel_text_address(jump_entry_code(entry)))
> - arch_jump_label_transform(entry, jump_label_type(entry));
> - else
> - WARN_ONCE(1, "can't patch jump_label at %pS",
> - (void *)jump_entry_code(entry));
> + if (jump_label_can_update_check(entry, init)) {
> + arch_jump_label_transform(entry,
> + jump_label_type(entry));

Yeah, let that one stick out.

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

\
 
 \ /
  Last update: 2019-02-05 08:22    [W:1.033 / U:5.912 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site