code cleanup: was: Re: [PATCH v7 for-next 3/8] livepatch: Implement replace set for scoped atomic replace

[Petr Mladek <pmladek@suse.com>]

On Tue 2026-08-25 19:46:36, Yafang Shao wrote:
> The current bool replace flag is too coarse: it is either all or
> nothing. A livepatch with .replace=true replaces ALL existing
> livepatches, which is safe but inflexible. There is no way to have
> multiple independent livepatch sets coexist on the same system.
> 
> Replace it with a more flexible model using two new fields in
> struct klp_patch:
> 
> - provides: an unsigned int id identifying the patch replace set.
>   By default (provides=0), any livepatch replaces any other livepatch.
>
> - obsoletes: an optional array of unsigned int ids specifying
>   additional provides ids to be replaced. This allows a new patch
>   to explicitly obsolete patches from different replace sets.

> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -448,13 +449,32 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	return count;
>  }
>  
> -static ssize_t replace_show(struct kobject *kobj,
> +static ssize_t provides_show(struct kobject *kobj,
>  			    struct kobj_attribute *attr, char *buf)
>  {
>  	struct klp_patch *patch;
>  
>  	patch = container_of(kobj, struct klp_patch, kobj);
> -	return sysfs_emit(buf, "%d\n", patch->replace);
> +	return sysfs_emit(buf, "%u\n", patch->provides);
> +}
> +
> +static ssize_t obsoletes_show(struct kobject *kobj,
> +			      struct kobj_attribute *attr, char *buf)
> +{
> +	struct klp_patch *patch;
> +	unsigned int i;
> +	int len = 0;
> +
> +	patch = container_of(kobj, struct klp_patch, kobj);
> +	if (!patch->obsoletes || !patch->nr_obsoletes)
> +		return sysfs_emit(buf, "\n");
> +
> +	for (i = 0; i < patch->nr_obsoletes; i++)
> +		len += sysfs_emit_at(buf, len, "%u%s", patch->obsoletes[i],
> +				     i < patch->nr_obsoletes - 1 ? "," : "");

It would be easier if we swithed the oder of %u and %s:

		len += sysfs_emit_at(buf, len, "%s%u", patch->obsoletes[i],
				     i == 0, "" : ",");

> +
> +	len += sysfs_emit_at(buf, len, "\n");
> +	return len;
>  }
>  
>  static ssize_t stack_order_show(struct kobject *kobj,
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -498,7 +498,7 @@ void klp_try_complete_transition(void)
>  	 */
>  	if (!patch->enabled)
>  		klp_free_patch_async(patch);
> -	else if (patch->replace)
> +	else
>  		klp_free_replaced_patches_async(patch);

Nit: I would use this opportunity and invert the logic.
     Positive checks are always easier to read.
     I mean to use:

	if (patch->enabled)
		klp_free_replaced_patches_async(patch);
	else
		klp_free_patch_async(patch);

>  }
>  

Best Regards,
Petr