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

[Hans Verkuil]

On 9/15/26 17:43, Sean Young wrote:
> On Tue, Sep 15, 2026 at 03:56:15PM +0100, Sean Young wrote:
>> 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?
> 
> Turns out this is a bit of a mess. All the keymaps in
> drivers/media/rc/keymaps/ set the size member for the number of entries they
> have - which really should be len. So, renaming this member of struct
> rc_map_table will mean patching all the keymap entries. Nothing too complex
> but maybe this is something for the next patch series.
> 
> It would be useful to hear what you think of I'm proposing though:
> 
> 1. Rename size to cap in struct rc_map_table (so it will have a len and cap)
> 2. Rename newsize to newcap in ir_resize_table() and related functions
> 3. In every keymap, replace:
> 	.size     = ARRAY_SIZE(empty),
>    With
> 	.len      = ARRAY_SIZE(empty),

The problem with 'size' and 'len' is that it is not obvious what they refer to:
bytes? Number of elements in an array? Most commonly size and len refer to number of
bytes.

For the number of array elements I would typically use 'elems' or possibly 'entries'.
Or sometimes nr_of_'something'.

> 
> This will be part of next series for rc-core, probably for the next release
>  cycle.

Of course, that doesn't belong in this series.

In any case, for the series:

Acked-by: Hans Verkuil <hverkuil+cisco@xxxxxxxxxx>

Regards,

	Hans

> 
> 
> Thanks,
> Sean
> 
>>
>>>> +	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
>