On Tue, 11 Jun 2024 15:26:05 -0700
Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index d09136e040d3..044ddce8f085 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -2244,6 +2244,121 @@ void __init memblock_free_all(void)
> > totalram_pages_add(pages);
> > }
> >
> > +/* Keep a table to reserve named memory */
> > +#define RESERVE_MEM_MAX_ENTRIES 8
> > +#define RESERVE_MEM_NAME_SIZE 16
> > +struct reserve_mem_table {
> > + char name[RESERVE_MEM_NAME_SIZE];
> > + phys_addr_t start;
> > + phys_addr_t size;
> > +};
> > +static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
> > +static int reserved_mem_count;
> > +
> > +/* Add wildcard region with a lookup name */
> > +static int __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> > + const char *name)
> > +{
> > + struct reserve_mem_table *map;
> > +
> > + if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
> > + return -EINVAL;
> > +
>
> I know I am picky, but name is never NULL, and strlen(name) is guaranteed to be > 0.
> Personally I'd suggest to check for strlen(name) >= RESERVE_MEM_NAME_SIZE together
> with !strlen(name) and drop the duplicate checks here (and, as side effect, avoid
> the pointless memory allocation if the name is invalid).
Yeah, it's now checked before hand. I'll remove it for v5.
>
> > + if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
> > + return -1;
> > +
> > + map = &reserved_mem_table[reserved_mem_count++];
> > + map->start = start;
> > + map->size = size;
> > + strscpy(map->name, name);
> > + return 0;
> > +}
> > +
> > +/*
> > + * Parse reserve_mem=nn:align:name
> > + */
> > +static int __init reserve_mem(char *p)
> > +{
> > + phys_addr_t start, size, align;
> > + char *name;
> > + char *oldp;
> > + int err;
> > +
> > + if (!p)
> > + return -EINVAL;
> > +
> > + oldp = p;
> > + size = memparse(p, &p);
> > + if (!size || p == oldp)
> > + return -EINVAL;
> > +
> > + if (*p != ':')
> > + return -EINVAL;
> > +
> > + align = memparse(p+1, &p);
> > + if (*p != ':')
> > + return -EINVAL;
> > +
> > + /*
> > + * memblock_phys_alloc() doesn't like a zero size align,
> > + * but it is OK for this command to have it.
> > + */
> > + if (align <= SMP_CACHE_BYTES)
>
> Any reason for using <= instead of < ?
Nope. Not sure why I did that. :-/
I'll fix that too.
Thanks,
-- Steve
>
> Guenter
>
> > + align = SMP_CACHE_BYTES;
> > +
> > + name = p + 1;
> > + if (!strlen(name))
> > + return -EINVAL;
> > +
> > + /* Make sure that name has text */
> > + for (p = name; *p; p++) {
> > + if (!isspace(*p))
> > + break;
> > + }
> > + if (!*p)
> > + return -EINVAL;
> > +
> > + start = memblock_phys_alloc(size, align);
> > + if (!start)
> > + return -ENOMEM;
> > +
> > + err = reserved_mem_add(start, size, name);
> > + if (err) {
> > + memblock_phys_free(start, size);
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +__setup("reserve_mem=", reserve_mem);
> > +
> > #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> > static const char * const flagname[] = {
> > [ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",
