Re: [PATCH v2 2/3] mm: change type of state in struct memory_block

On Wed, Oct 29, 2025 at 07:56:30PM +0000, Israel Batista wrote:
> The state of a memory block should be restricted to values specified
> in the documentation of the memory hotplug API. However, since the
> state field in the memory_block struct was defined as an unsigned
> long, this restriction was not enforced at compile time.
>
> With the introduction of the enum memory_block_state, it is now possible
> to incorporate the desired semantics in the field declaration and
> enforce these restrictions at compile time.

Well I'm not sure how much is enforced at compile time, the compiler treats
the enum very loosely as if it were simply an int. BUt it does make it
clear what you expect this field to contain!

>
> Signed-off-by: Israel Batista <[email protected]>

This LGTM as a reasonable change though, so:

Reviewed-by: Lorenzo Stoakes <[email protected]>

On basis that you fixup the formatting issue raised by Randy.

> ---
>  drivers/base/memory.c  | 2 +-
>  include/linux/memory.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 6d84a02cfa5d..3d17dd774947 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -198,7 +198,7 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>  		break;
>  	default:
>  		WARN_ON(1);
> -		return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state);
> +		return sysfs_emit(buf, "ERROR-UNKNOWN-%d\n", mem->state);
>  	}
>
>  	return sysfs_emit(buf, "%s\n", output);
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index f4e358477c6a..36d733283329 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -78,7 +78,7 @@ enum memory_block_state {
>
>  struct memory_block {
>  	unsigned long start_section_nr;
> -	unsigned long state;		/* serialized by the dev->lock */
> +	enum memory_block_state state; /* serialized by the dev->lock */

I doubt it will be an isuse, but this obviously changes the layout of the
struct.

However since this will be an int and you have a bunch of ints here it should
all pad reasonably.

>  	int online_type;		/* for passing data to online routine */
>  	int nid;			/* NID for this memory block */
>  	/*
> --
> 2.51.0
>