Re: [PATCH v2 061/101] fbdev/ps3fb: Duplicate video-mode option string

From: Thomas Zimmermann
Date: Fri Mar 10 2023 - 07:21:59 EST


Hi Geert

Am 10.03.23 um 09:18 schrieb Geert Uytterhoeven:
Hi Thomas,

On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
Assume that the driver does not own the option string or its substrings
and hence duplicate the option string for the video mode. Allocate the
copy's memory with kstrdup() and free it in the module's exit function.

Done in preparation of switching the driver to struct option_iter and
constifying the option string.

v2:
* replace static memory with kstrdup()/kfree() (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Thanks for the upodate!

--- a/drivers/video/fbdev/ps3fb.c
+++ b/drivers/video/fbdev/ps3fb.c
@@ -260,6 +260,7 @@ static const struct fb_videomode ps3fb_modedb[] = {
static int ps3fb_mode;
module_param(ps3fb_mode, int, 0);

+static char *mode_option_buf;

Do you really need this variable? It contains the same value
as mode_option below.
This is a common pattern in several patches.

In most cases, the extra variable is necessary. mode_option, or a similar variable, is often also used to store module parameters or is initialized from a static string. In those cases we obviously cannot free the memory. Hence the additional variable for the freeing the allocation.

I reviewed the patches and it looks like kyrofb, ps3fb, and sm712fb might be able to work without the _buf variable. I'll try to update those.

Best regards
Thomas


static char *mode_option;

static int ps3fb_cmp_mode(const struct fb_videomode *vmode,
@@ -1276,8 +1277,11 @@ static int __init ps3fb_setup(void)
continue;
if (!strncmp(this_opt, "mode:", 5))
ps3fb_mode = simple_strtoul(this_opt + 5, NULL, 0);
- else
- mode_option = this_opt;
+ else {
+ kfree(mode_option_buf);
+ mode_option_buf = kstrdup(this_opt, GFP_KERNEL); // ignore errors
+ mode_option = mode_option_buf;
+ }
}
return 0;
}
@@ -1294,6 +1298,7 @@ static void __exit ps3fb_exit(void)
{
pr_debug(" -> %s:%d\n", __func__, __LINE__);
ps3_system_bus_driver_unregister(&ps3fb_driver);
+ kfree(mode_option_buf);
pr_debug(" <- %s:%d\n", __func__, __LINE__);
}

Gr{oetje,eeting}s,

Geert


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature