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

[Sean Young]

On Tue, Sep 15, 2026 at 03:07:32PM +0200, Hans Verkuil wrote:
> 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@xxxxxxxx>
> > ---
> >  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'...

struct rc_map_table has a len and size member. I agree that size is not
a good name. How about capacity or cap]? I am not sure that entries is much
clearer that entries tbh, because what's the difference between len and
entries?

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

How about newcapicity or newcap? 

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

Thanks,

Sean