linux-kernel - Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcpJzMUtrN2kBhWs90G3n6_NWTBhw3MX2WpuhsDt7zmjQ@mail.gmail.com>
Date: Fri, 22 Aug 2025 09:08:42 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jean-François Lessard <jefflessard3@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Geert Uytterhoeven <geert@...ux-m68k.org>, devicetree@...r.kernel.org, 
	linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Andreas Färber <afaerber@...e.de>, 
	Boris Gjenero <boris.gjenero@...il.com>, Christian Hewitt <christianshewitt@...il.com>, 
	Heiner Kallweit <hkallweit1@...il.com>, Paolo Sabatino <paolo.sabatino@...il.com>, 
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix
 display controllers driver

On Fri, Aug 22, 2025 at 5:20 AM Jean-François Lessard
<jefflessard3@...il.com> wrote:
> Le 21 août 2025 16 h 19 min 23 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
> >On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard
> ><jefflessard3@...il.com> wrote:
> >> Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
> >> >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard
> >> ><jefflessard3@...il.com> wrote:

...

> >> >> +#define TM16XX_DRIVER_NAME "tm16xx"

> >> The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage
> >> in registration and module macros.
> >> If helpful, I can add a leading /* module name */ header comment.
> >
> >Instead of an unneeded comment it seems better to use explicit string
> >literal in all cases (two?).
>
> I'm surprised by this preference since driver name macros are very common
> practice, but I'll use explicit string literals to align on this preference.

Usually we put a macro to something which (theoretically) might
change. The driver name is part of an ABI and I prefer it to be
explicit as we do not break an ABI. Once introduced it can't be
modified or removed. Also it's better to see clearly exactly at the
place in use in the code the name as it's easier to (git) grep for
something similar. With a macro I would need to grep at least twice to
see the users.

...

> >> >> +       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
> >> >> +       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
> >> >> +       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
> >> >> +       if (!keypad->state || !keypad->last_state || !keypad->changes) {
> >> >> +               ret = -ENOMEM;
> >
> >> >> +               goto free_keypad;
> >
> >(Hit send too early that time...) This goto is bad. It means
> >misunderstanding of the devm concept. See below.
>
> I can assure I understand the devm paradigm. The keypad probe is optional,
> failure doesn't fail the main driver probe but only generates a warning. The
> cleanup code prevents memory from staying allocated until device removal
> in this specific optional failure case. However, if you insist, I'll remove the
> cleanup and let devm handle it normally.

Assume you have a separate feature, let's say keypad driver for some
complex HW, like this one, and you have even a separate (library)
driver for it. Then you want to introduce some kind of library
functions to probe and remove the only keypad part, here are two
options:
- follow the library pattern with plain (non-devm) k*alloc() in probe
and kfree in remove
- use driver pattern with devm

If you choose the second one, it will be weird to call devm_kfree().
The rule of thumb the devm_$FREE_MY_RESOURSE() *must* be *well*
justified. Because it's exceptional. Losing 1kb memory or so is not
enough to justify.

> >> >> +       }
> >> >> +
> >> >> +       input = devm_input_allocate_device(display->dev);

> >> >> +free_bitmaps:
> >> >> +       devm_kfree(display->dev, keypad->state);
> >> >> +       devm_kfree(display->dev, keypad->last_state);
> >> >> +       devm_kfree(display->dev, keypad->changes);
> >> >> +free_keypad:
> >> >> +       devm_kfree(display->dev, keypad);
> >> >> +       return ret;
> >
> >No way. We don't do that, If required it signals about exceptional
> >case (0.01% probability) or misunderstanding of devm:
> >- managed resources are managed by core, no need to call for free
> >- using managed resources in the contexts when object lifetime is
> >short is incorrect, needs to be switched to the plain alloc (nowadays
> >with __free() from cleanup.h to have RAII enabled)
> >
> >Choose one of these and fix the code accordingly.
>
> Same as above.

-- 
With Best Regards,
Andy Shevchenko