From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2582C3932E1; Tue, 22 Sep 2026 11:22:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790076154; cv=none; b=J6i5okOp3sEPhQdCH9VP361c4AOhY/EdjKLS8oO7b9VjYWJ0g4XwxnlnozrqmTiRFx2+2y52IYRiPjX/u7Yb0o3y6/awuIvSgdjgHTKG0IzJJTfmT1cOoz9c16iDgG4ASQ0lfkz0ItthscsLHNAjF1XuXX42QSmavZSXGw3Lvks= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790076154; c=relaxed/simple; bh=vGhE/XSASvxXZdTUjPbgbquNbUpYbaQ/lLuBauojj/s=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=SrT8HtzAPlF5u/LQ9/eVbKq8YG3pjqIwEalVLw85b2GRk2DGjBkPhUHzCttphJckQ19GQ1cReeENbSTQwEep0kI53NkI6cUw3DYZa/IT88HMUwume7YoKPZvUa4UbmfYQyQJaEpW9+ydVw0QWrfK6/nPLqQ5dtgDna2zDKLGBHE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Sw/7LRVq; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Sw/7LRVq" Received: by smtp.kernel.org (Postfix) with ESMTPSA id BC89C1F000FF; Tue, 22 Sep 2026 11:22:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1790076152; bh=fHkLOnz8tVHZVz6Gya/0gPv+0vLoHxjMIXNdeFar0fY=; h=Date:Subject:To:Cc:References:From:In-Reply-To; b=Sw/7LRVqD3vnxdmhjOwnQRszQpVxQDDh9LI3T88M/pogSuIAFg8E04Asqz+Qwb0nz 3U7CBlyaRK4xQIXWtvRZ9ouQ3JW1es38lMmaGF3LnY+s56dODc8eONHdPlcxGUuM3k gxN6cC+0mH+WoIcGQiHGFlGZakmJvqEJsVAV2BTw32H0aZml+KGcJS/QztWyXrmR4W B+fuIi3UcNjr/yLWcqT2YsgeXs7hsS4jB8+Tauig/sdeIUFA97LY+7fZP2k9zoqNVT rCfNuZK46xovVAHke0t5mjJPCaupqNYG9kNoZjM8MDcoy1vFrtdMfRr6lq4F9LGTgE Gje9JCKwpBZWA== Message-ID: <1885a637-54c9-4478-8ff1-6d01caaf603a@kernel.org> Date: Tue, 22 Sep 2026 13:22:30 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 04/20] media: rc: Replace open coded krealloc() for keymap To: Sean Young Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab , Rik van Riel , linux-kernel@vger.kernel.org References: <3b7094b8f0035d970fd61caca05abcdd3374146b.1789460680.git.sean@mess.org> <61897bc7-e210-4b81-b2fe-f133c962b152@kernel.org> From: Hans Verkuil Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 >>>> --- >>>> 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 >>>> #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 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 >