> 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.
