Re: [PATCH v5 04/20] media: rc: Replace open coded krealloc() for keymap

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 9/15/26 10:32, Sean Young wrote:
> Replace some ugly code with open coded krealloc() and remove
> superfluous member of struct rc_map.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/rc-main.c | 45 +++++++++++++++++++-------------------
>  include/media/rc-map.h     |  2 --
>  2 files changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index d4baaae3a834..53e39b7a489a 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -17,9 +17,8 @@
>  #include <linux/module.h>
>  #include "rc-core-priv.h"
>  
> -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */
> -#define IR_TAB_MIN_SIZE	256
> -#define IR_TAB_MAX_SIZE	8192
> +#define IR_TAB_MIN_ENTRIES	32
> +#define IR_TAB_MAX_ENTRIES	1024

This is better...

>  
>  static const struct {
>  	const char *name;
> @@ -214,21 +213,23 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
>  static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
>  			   const char *name, u64 rc_proto, size_t size)
>  {
> +	unsigned int alloc;
>  	rc_map->name = kstrdup(name, GFP_KERNEL);
>  	if (!rc_map->name)
>  		return -ENOMEM;
> +	alloc = roundup_pow_of_two(size);
>  	rc_map->rc_proto = rc_proto;
> -	rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
> -	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> -	rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
> +	rc_map->len = 0;
> +	rc_map->size = alloc;

...but this still says 'size'...

> +	rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);
>  	if (!rc_map->scan) {
>  		kfree(rc_map->name);
>  		rc_map->name = NULL;
>  		return -ENOMEM;
>  	}
>  
> -	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%u bytes)\n",
> -		rc_map->size, rc_map->alloc);
> +	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%zu bytes)\n",
> +		alloc, alloc * sizeof(struct rc_map_table));
>  	return 0;
>  }
>  
> @@ -262,38 +263,36 @@ static void ir_free_table(struct rc_map *rc_map)
>  static int ir_resize_table(struct rc_dev *dev, struct rc_map *rc_map,
>  			   gfp_t gfp_flags)
>  {
> -	unsigned int oldalloc = rc_map->alloc;
> -	unsigned int newalloc = oldalloc;
> -	struct rc_map_table *oldscan = rc_map->scan;
> +	unsigned int newsize = rc_map->size;
>  	struct rc_map_table *newscan;
>  
>  	if (rc_map->size == rc_map->len) {
>  		/* All entries in use -> grow keytable */
> -		if (rc_map->alloc >= IR_TAB_MAX_SIZE)
> +		if (newsize >= IR_TAB_MAX_ENTRIES)
>  			return -ENOMEM;
>  
> -		newalloc *= 2;
> -		dev_dbg(&dev->dev, "Growing table to %u bytes\n", newalloc);
> +		newsize *= 2;
> +
> +		dev_dbg(&dev->dev, "Growing table to %u entries\n", newsize);
>  	}
>  
> -	if ((rc_map->len * 3 < rc_map->size) && (oldalloc > IR_TAB_MIN_SIZE)) {
> +	if (rc_map->len * 3 < rc_map->size && rc_map->size > IR_TAB_MIN_ENTRIES) {

...which is especially confusing here where rc_map->size and IR_TAB_MIN_ENTRIES
are mixed.

This is not important for this series, so you can go ahead with it.

But personally I would prefer to rename rc_map->size to rc_map->entries and
ditto for newsize to newentries. That way it is clear that these variables
deal with the number of entries and not the size in bytes.

I mention it only because when I reviewed v4 it confused me.

Regards,

	Hans

>  		/* Less than 1/3 of entries in use -> shrink keytable */
> -		newalloc /= 2;
> -		dev_dbg(&dev->dev, "Shrinking table to %u bytes\n", newalloc);
> +		newsize /= 2;
> +		dev_dbg(&dev->dev, "Shrinking table to %u entries\n", newsize);
>  	}
>  
> -	if (newalloc == oldalloc)
> +	if (newsize == rc_map->size)
>  		return 0;
>  
> -	newscan = kmalloc(newalloc, gfp_flags);
> +	newscan = krealloc_array(rc_map->scan, newsize,
> +				 sizeof(struct rc_map_table), gfp_flags);
>  	if (!newscan)
>  		return -ENOMEM;
>  
> -	memcpy(newscan, rc_map->scan, rc_map->len * sizeof(struct rc_map_table));
>  	rc_map->scan = newscan;
> -	rc_map->alloc = newalloc;
> -	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> -	kfree(oldscan);
> +	rc_map->size = newsize;
> +
>  	return 0;
>  }
>  
> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> index d95ed3e96de2..f167c37179c8 100644
> --- a/include/media/rc-map.h
> +++ b/include/media/rc-map.h
> @@ -148,7 +148,6 @@ struct rc_map_table {
>   * @scan: pointer to struct &rc_map_table
>   * @size: Max number of entries
>   * @len: Number of entries that are in use
> - * @alloc: size of \*scan, in bytes
>   * @rc_proto: type of the remote controller protocol, as defined at
>   *	     enum &rc_proto
>   * @name: name of the key map table
> @@ -158,7 +157,6 @@ struct rc_map {
>  	struct rc_map_table	*scan;
>  	unsigned int		size;
>  	unsigned int		len;
> -	unsigned int		alloc;
>  	enum rc_proto		rc_proto;
>  	const char		*name;
>  	spinlock_t		lock;