[PATCH 00/11] drm,fbdev: Move video= option to drivers/video

[PATCH 00/11] drm,fbdev: Move video= option to drivers/video

[Thomas Zimmermann]

The kernel's video= option sets the initial video mode. It is shared
by fbdev and DRM, but located within the fbdev code. Move it to
drivers/video/ and adapt callers. Allows DRM (and others) to use the
option without depending on fbdev.

While at it, fix the interface of the lookup functions. This requires
a number of changes. First clarify the ownership of the option string
in patch 2. The helper fb_get_options() returns the video= parameter's
value. It's sometimes a copy and sometimes the original string. Hence
callers, mostly fbdev drivers, have just leaked the returned string.
Change this to always duplicate the option string in fb_get_options()
and transfer ownership of the copy to the caller. We can then start to
fix the memory leaks in the fbdev drivers.

There's a global video= setting and a number of per-output settings.
In patches 3 to 5, support explicit lookup of the global video option
and lookup the string in fbdev's modedb and in a PS3 driver. Then
avoid exporting the global setting's internal state variable in patch 6.

Finally, in patches 7 to 11, move the video= option to drivers/video.
It can be used directly in DRM and a PS3 driver. This fixes any memory
leaks from the returned option string. For fbdev drivers, the helper
fb_get_options() remains as an adapter aroudn the new interface.

Tested with DRM and fbdev and built for the PS3.

Thomas Zimmermann (11):
  fbdev: Fix contact info in fb_cmdline.c
  fbdev: Transfer video= option strings to caller; clarify ownership
  fbdev: Support NULL for name in option-string lookup
  drivers/ps3: Read video= option with fb_get_option()
  fbdev: Read video= option with fb_get_option() in modedb
  fbdev: Unexport fb_mode_option
  fbdev: Move option-string lookup into helper
  fbdev: Handle video= parameter in video/cmdline.c
  driver/ps3: Include <video/cmdline.h> for mode parsing
  drm: Include <video/cmdline.h> for mode parsing
  drm: Fix comment on mode parsing

 drivers/gpu/drm/Kconfig               |   2 +-
 drivers/gpu/drm/drm_connector.c       |   9 +-
 drivers/gpu/drm/drm_modes.c           |   3 +-
 drivers/ps3/ps3av.c                   |   9 +-
 drivers/video/Kconfig                 |   3 +
 drivers/video/Makefile                |   1 +
 drivers/video/cmdline.c               | 133 ++++++++++++++++++++++++++
 drivers/video/fbdev/Kconfig           |   5 +-
 drivers/video/fbdev/core/Makefile     |   3 +-
 drivers/video/fbdev/core/fb_cmdline.c |  94 +++++-------------
 drivers/video/fbdev/core/modedb.c     |   8 +-
 include/linux/fb.h                    |   1 -
 include/video/cmdline.h               |  20 ++++
 13 files changed, 202 insertions(+), 89 deletions(-)
 create mode 100644 drivers/video/cmdline.c
 create mode 100644 include/video/cmdline.h


base-commit: 1a019dd7a5d25f7c1c9b77931138290e28947e6a
-- 
2.39.1

[PATCH 01/11] fbdev: Fix contact info in fb_cmdline.c

[Thomas Zimmermann]

Fix Daniel's email address. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/video/fbdev/core/fb_cmdline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c
index 3b5bd666b952..6792010d6716 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -12,7 +12,7 @@
  * for more details.
  *
  * Authors:
- *    Vetter <danie.vetter@ffwll.ch>
+ *    Daniel Vetter <daniel.vetter@ffwll.ch>
  */
 #include <linux/init.h>
 #include <linux/fb.h>
-- 
2.39.1

[PATCH 02/11] fbdev: Transfer video= option strings to caller; clarify ownership

[Thomas Zimmermann]

In fb_get_options(), always duplicate the returned option string and
transfer ownership of the memory to the function's caller.

Until now, only the global option string got duplicated and transferred
to the caller; the per-driver options were owned by fb_get_options().
In the end, it was impossible for the function's caller to detect if
it had to release the string's memory buffer. Hence, all calling drivers
leak the memory buffer. The leaks have existed ever since, but drivers
only call fb_get_option() once as part of module initialization. So the
amount of leaked memory is not significant.

Fix the semantics of fb_get_option() by unconditionally transferring
ownership of the memory buffer to the caller. Later patches can resolve
the memory leaks in the fbdev drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fb_cmdline.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c
index 6792010d6716..702b00b71870 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -30,13 +30,17 @@ EXPORT_SYMBOL_GPL(fb_mode_option);
  *          (video=<name>:<options>)
  * @option: the option will be stored here
  *
+ * The caller owns the string returned in @option and is
+ * responsible for releasing the memory.
+ *
  * NOTE: Needed to maintain backwards compatibility
  */
 int fb_get_options(const char *name, char **option)
 {
-	char *opt, *options = NULL;
+	const char *options = NULL;
 	int retval = 0;
 	int name_len = strlen(name), i;
+	char *opt;
 
 	if (name_len && ofonly && strncmp(name, "offb", 4))
 		retval = 1;
@@ -55,12 +59,16 @@ int fb_get_options(const char *name, char **option)
 	}
 	/* No match, pass global option */
 	if (!options && option && fb_mode_option)
-		options = kstrdup(fb_mode_option, GFP_KERNEL);
+		options = fb_mode_option;
 	if (options && !strncmp(options, "off", 3))
 		retval = 1;
 
-	if (option)
-		*option = options;
+	if (option) {
+		if (options)
+			*option = kstrdup(options, GFP_KERNEL);
+		else
+			*option = NULL;
+	}
 
 	return retval;
 }
-- 
2.39.1

[PATCH 03/11] fbdev: Support NULL for name in option-string lookup

[Thomas Zimmermann]

Ignore the per-driver video options if no driver name has been
specified to fb_get_option(). Return the global options in this
case.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fb_cmdline.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c
index 702b00b71870..cc8a88e8f308 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -39,13 +39,18 @@ int fb_get_options(const char *name, char **option)
 {
 	const char *options = NULL;
 	int retval = 0;
-	int name_len = strlen(name), i;
+	size_t name_len;
 	char *opt;
 
+	if (name)
+		name_len = strlen(name);
+
 	if (name_len && ofonly && strncmp(name, "offb", 4))
 		retval = 1;
 
 	if (name_len && !retval) {
+		unsigned int i;
+
 		for (i = 0; i < FB_MAX; i++) {
 			if (video_options[i] == NULL)
 				continue;
-- 
2.39.1

[PATCH 04/11] drivers/ps3: Read video= option with fb_get_option()

[Thomas Zimmermann]

Get the kernel's global video= parameter with fb_get_option(). Done
to unexport the internal fbdev state fb_mode_config. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/ps3/ps3av.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ps3/ps3av.c b/drivers/ps3/ps3av.c
index 516e6d14d32e..8f3e60f1bfe2 100644
--- a/drivers/ps3/ps3av.c
+++ b/drivers/ps3/ps3av.c
@@ -921,6 +921,9 @@ EXPORT_SYMBOL_GPL(ps3av_audio_mute);
 
 static int ps3av_probe(struct ps3_system_bus_device *dev)
 {
+#ifdef CONFIG_FB
+	char *mode_option = NULL;
+#endif
 	int res;
 	int id;
 
@@ -969,8 +972,12 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
 	ps3av_get_hw_conf(ps3av);
 
 #ifdef CONFIG_FB
-	if (fb_mode_option && !strcmp(fb_mode_option, "safe"))
-		safe_mode = 1;
+	fb_get_options(NULL, &mode_option);
+	if (mode_option) {
+		if (!strcmp(mode_option, "safe"))
+			safe_mode = 1;
+		kfree(mode_option);
+	}
 #endif /* CONFIG_FB */
 	id = ps3av_auto_videomode(&ps3av->av_hw_conf);
 	if (id < 0) {
-- 
2.39.1

[PATCH 05/11] fbdev: Read video= option with fb_get_option() in modedb

[Thomas Zimmermann]

Get the kernel's global video= parameter with fb_get_option(). Done
to unexport the internal fbdev state fb_mode_config. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/modedb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
index 6473e0dfe146..23cf8eba785d 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -620,6 +620,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 		 const struct fb_videomode *default_mode,
 		 unsigned int default_bpp)
 {
+	char *mode_option_buf = NULL;
 	int i;
 
 	/* Set up defaults */
@@ -635,8 +636,10 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 		default_bpp = 8;
 
 	/* Did the user specify a video mode? */
-	if (!mode_option)
-		mode_option = fb_mode_option;
+	if (!mode_option) {
+		fb_get_options(NULL, &mode_option_buf);
+		mode_option = mode_option_buf;
+	}
 	if (mode_option) {
 		const char *name = mode_option;
 		unsigned int namelen = strlen(name);
@@ -715,6 +718,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 			res_specified = 1;
 		}
 done:
+		kfree(mode_option_buf);
 		if (cvt) {
 			struct fb_videomode cvt_mode;
 			int ret;
-- 
2.39.1

[PATCH 06/11] fbdev: Unexport fb_mode_option

[Thomas Zimmermann]

There are no external users of fb_mode_option. Unexport the variable
and declare it static.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fb_cmdline.c | 4 +---
 include/linux/fb.h                    | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c
index cc8a88e8f308..512da89ff8b5 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -18,11 +18,9 @@
 #include <linux/fb.h>
 
 static char *video_options[FB_MAX] __read_mostly;
+static const char *fb_mode_option __read_mostly;
 static int ofonly __read_mostly;
 
-const char *fb_mode_option;
-EXPORT_SYMBOL_GPL(fb_mode_option);
-
 /**
  * fb_get_options - get kernel boot parameters
  * @name:   framebuffer name as it would appear in
diff --git a/include/linux/fb.h b/include/linux/fb.h
index daf336385613..054e8950b274 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -764,7 +764,6 @@ struct dmt_videomode {
 	const struct fb_videomode *mode;
 };
 
-extern const char *fb_mode_option;
 extern const struct fb_videomode vesa_modes[];
 extern const struct dmt_videomode dmt_modes[];
 
-- 
2.39.1

[PATCH 07/11] fbdev: Move option-string lookup into helper

[Thomas Zimmermann]

Move the lookup of the option string into an internal helper. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fb_cmdline.c | 60 ++++++++++++++++-----------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c
index 512da89ff8b5..f811c7b679e1 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -21,6 +21,36 @@ static char *video_options[FB_MAX] __read_mostly;
 static const char *fb_mode_option __read_mostly;
 static int ofonly __read_mostly;
 
+static const char *__fb_get_options(const char *name)
+{
+	const char *options = NULL;
+	size_t name_len = 0;
+
+	if (name)
+		name_len = strlen(name);
+
+	if (name_len) {
+		unsigned int i;
+		const char *opt;
+
+		for (i = 0; i < ARRAY_SIZE(video_options); ++i) {
+			if (!video_options[i])
+				continue;
+			if (video_options[i][0] == '\0')
+				continue;
+			opt = video_options[i];
+			if (!strncmp(opt, name, name_len) && opt[name_len] == ':')
+				options = opt + name_len + 1;
+		}
+	}
+
+	/* No match, return global options */
+	if (!options)
+		options = fb_mode_option;
+
+	return options;
+}
+
 /**
  * fb_get_options - get kernel boot parameters
  * @name:   framebuffer name as it would appear in
@@ -35,36 +65,18 @@ static int ofonly __read_mostly;
  */
 int fb_get_options(const char *name, char **option)
 {
-	const char *options = NULL;
 	int retval = 0;
-	size_t name_len;
-	char *opt;
+	const char *options;
 
-	if (name)
-		name_len = strlen(name);
-
-	if (name_len && ofonly && strncmp(name, "offb", 4))
+	if (name && ofonly && strncmp(name, "offb", 4))
 		retval = 1;
 
-	if (name_len && !retval) {
-		unsigned int i;
+	options = __fb_get_options(name);
 
-		for (i = 0; i < FB_MAX; i++) {
-			if (video_options[i] == NULL)
-				continue;
-			if (!video_options[i][0])
-				continue;
-			opt = video_options[i];
-			if (!strncmp(name, opt, name_len) &&
-			    opt[name_len] == ':')
-				options = opt + name_len + 1;
-		}
+	if (options) {
+		if (!strncmp(options, "off", 3))
+			retval = 1;
 	}
-	/* No match, pass global option */
-	if (!options && option && fb_mode_option)
-		options = fb_mode_option;
-	if (options && !strncmp(options, "off", 3))
-		retval = 1;
 
 	if (option) {
 		if (options)
-- 
2.39.1

[PATCH 08/11] fbdev: Handle video= parameter in video/cmdline.c

[Thomas Zimmermann]

Handle the command-line parameter video= in video/cmdline.c. Implement
the fbdev helper fb_get_options() on top. Will allows to handle the
kernel parameter in DRM without fbdev dependencies.

Note that __video_get_options() has the meaning of its return value
inverted compared to fb_get_options(). The new helper returns true if
the adapter has been enabled, and false otherwise.

There is the ofonly parameter, which disables output for non-OF-based
framebuffers. It is only for offb and looks like a workaround. The actual
purpose it not clear to me. Use 'video=off' or 'nomodeset' instead.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/Kconfig               |   2 +-
 drivers/video/Kconfig                 |   3 +
 drivers/video/Makefile                |   1 +
 drivers/video/cmdline.c               | 133 ++++++++++++++++++++++++++
 drivers/video/fbdev/Kconfig           |   5 +-
 drivers/video/fbdev/core/Makefile     |   3 +-
 drivers/video/fbdev/core/fb_cmdline.c |  93 +++---------------
 include/video/cmdline.h               |  20 ++++
 8 files changed, 172 insertions(+), 88 deletions(-)
 create mode 100644 drivers/video/cmdline.c
 create mode 100644 include/video/cmdline.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index dc0f94f02a82..81c8a99c744a 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -10,13 +10,13 @@ menuconfig DRM
 	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
 	select DRM_PANEL_ORIENTATION_QUIRKS
 	select HDMI
-	select FB_CMDLINE
 	select I2C
 	select DMA_SHARED_BUFFER
 	select SYNC_FILE
 # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
 # device and dmabuf fd. Let's make sure that is available for our userspace.
 	select KCMP
+	select VIDEO_CMDLINE
 	select VIDEO_NOMODESET
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 6d2fde6c5d11..bf05363d8906 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -11,6 +11,9 @@ config APERTURE_HELPERS
 	  Support tracking and hand-over of aperture ownership. Required
 	  by graphics drivers for firmware-provided framebuffers.
 
+config VIDEO_CMDLINE
+	bool
+
 config VIDEO_NOMODESET
 	bool
 	default n
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index a50eb528ed3c..831c9fa57a6c 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_APERTURE_HELPERS)    += aperture.o
 obj-$(CONFIG_VGASTATE)            += vgastate.o
+obj-$(CONFIG_VIDEO_CMDLINE)       += cmdline.o
 obj-$(CONFIG_VIDEO_NOMODESET)     += nomodeset.o
 obj-$(CONFIG_HDMI)                += hdmi.o
 
diff --git a/drivers/video/cmdline.c b/drivers/video/cmdline.c
new file mode 100644
index 000000000000..d3d257489c3d
--- /dev/null
+++ b/drivers/video/cmdline.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Based on the fbdev code in drivers/video/fbdev/core/fb_cmdline:
+ *
+ *  Copyright (C) 2014 Intel Corp
+ *  Copyright (C) 1994 Martin Schaller
+ *
+ *	2001 - Documented with DocBook
+ *	- Brad Douglas <brad@neruo.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors:
+ *    Daniel Vetter <daniel.vetter@ffwll.ch>
+ */
+
+#include <linux/fb.h> /* for FB_MAX */
+#include <linux/init.h>
+
+#include <video/cmdline.h>
+
+/*
+ * FB_MAX is the maximum number of framebuffer devices and also
+ * the maximum number of video= parameters. Although not directly
+ * related to each other, it makes sense to keep it that way.
+ */
+static const char *video_options[FB_MAX] __read_mostly;
+static const char *video_option __read_mostly;
+static int video_of_only __read_mostly;
+
+static const char *__video_get_option_string(const char *name)
+{
+	const char *options = NULL;
+	size_t name_len = 0;
+
+	if (name)
+		name_len = strlen(name);
+
+	if (name_len) {
+		unsigned int i;
+		const char *opt;
+
+		for (i = 0; i < ARRAY_SIZE(video_options); ++i) {
+			if (!video_options[i])
+				continue;
+			if (video_options[i][0] == '\0')
+				continue;
+			opt = video_options[i];
+			if (!strncmp(opt, name, name_len) && opt[name_len] == ':')
+				options = opt + name_len + 1;
+		}
+	}
+
+	/* No match, return global options */
+	if (!options)
+		options = video_option;
+
+	return options;
+}
+
+/**
+ * video_get_options - get kernel boot parameters
+ * @name:	name of the output as it would appear in the boot parameter
+ *		line (video=<name>:<options>)
+ *
+ * Looks up the video= options for the given name. Names are connector
+ * names with DRM, or driver names with fbdev. If no video option for
+ * the name has been specified, the function returns the global video=
+ * setting. A @name of NULL always returns the global video setting.
+ *
+ * Returns:
+ * The string of video options for the given name, or NULL if no video
+ * option has been specified.
+ */
+const char *video_get_options(const char *name)
+{
+	return __video_get_option_string(name);
+}
+EXPORT_SYMBOL(video_get_options);
+
+bool __video_get_options(const char *name, const char **options, bool is_of)
+{
+	bool enabled = true;
+	const char *opt = NULL;
+
+	if (video_of_only && !is_of)
+		enabled = false;
+
+	opt = __video_get_option_string(name);
+
+	if (options)
+		*options = opt;
+
+	return enabled;
+}
+EXPORT_SYMBOL(__video_get_options);
+
+/*
+ * Process command line options for video adapters. This function is
+ * a __setup and __init function. It only stores the options. Drivers
+ * have to call video_get_options() as necessary.
+ */
+static int __init video_setup(char *options)
+{
+	if (!options || !*options)
+		goto out;
+
+	if (!strncmp(options, "ofonly", 6)) {
+		video_of_only = true;
+		goto out;
+	}
+
+	if (strchr(options, ':')) {
+		/* named */
+		size_t i;
+
+		for (i = 0; i < ARRAY_SIZE(video_options); i++) {
+			if (!video_options[i]) {
+				video_options[i] = options;
+				break;
+			}
+		}
+	} else {
+		/* global */
+		video_option = options;
+	}
+
+out:
+	return 1;
+}
+__setup("video=", video_setup);
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index b2bed599e6c6..3623c1c0d34d 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -3,16 +3,13 @@
 # fbdev configuration
 #
 
-config FB_CMDLINE
-	bool
-
 config FB_NOTIFY
 	bool
 
 menuconfig FB
 	tristate "Support for frame buffer devices"
-	select FB_CMDLINE
 	select FB_NOTIFY
+	select VIDEO_CMDLINE
 	help
 	  The frame buffer device provides an abstraction for the graphics
 	  hardware. It represents the frame buffer of some video hardware and
diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 26cbc965497c..08fabce76b74 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -1,9 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_FB_CMDLINE)          += fb_cmdline.o
 obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
 obj-$(CONFIG_FB)                  += fb.o
 fb-y                              := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
-                                     modedb.o fbcvt.o
+                                     modedb.o fbcvt.o fb_cmdline.o
 fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
 
 ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c
index f811c7b679e1..4d1634c492ec 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -14,42 +14,12 @@
  * Authors:
  *    Daniel Vetter <daniel.vetter@ffwll.ch>
  */
-#include <linux/init.h>
-#include <linux/fb.h>
-
-static char *video_options[FB_MAX] __read_mostly;
-static const char *fb_mode_option __read_mostly;
-static int ofonly __read_mostly;
-
-static const char *__fb_get_options(const char *name)
-{
-	const char *options = NULL;
-	size_t name_len = 0;
-
-	if (name)
-		name_len = strlen(name);
 
-	if (name_len) {
-		unsigned int i;
-		const char *opt;
-
-		for (i = 0; i < ARRAY_SIZE(video_options); ++i) {
-			if (!video_options[i])
-				continue;
-			if (video_options[i][0] == '\0')
-				continue;
-			opt = video_options[i];
-			if (!strncmp(opt, name, name_len) && opt[name_len] == ':')
-				options = opt + name_len + 1;
-		}
-	}
+#include <linux/export.h>
+#include <linux/fb.h>
+#include <linux/string.h>
 
-	/* No match, return global options */
-	if (!options)
-		options = fb_mode_option;
-
-	return options;
-}
+#include <video/cmdline.h>
 
 /**
  * fb_get_options - get kernel boot parameters
@@ -65,17 +35,18 @@ static const char *__fb_get_options(const char *name)
  */
 int fb_get_options(const char *name, char **option)
 {
-	int retval = 0;
-	const char *options;
+	const char *options = NULL;
+	bool is_of = false;
+	bool enabled;
 
-	if (name && ofonly && strncmp(name, "offb", 4))
-		retval = 1;
+	if (name)
+		is_of = strncmp(name, "offb", 4);
 
-	options = __fb_get_options(name);
+	enabled = __video_get_options(name, &options, is_of);
 
 	if (options) {
 		if (!strncmp(options, "off", 3))
-			retval = 1;
+			enabled = false;
 	}
 
 	if (option) {
@@ -85,46 +56,6 @@ int fb_get_options(const char *name, char **option)
 			*option = NULL;
 	}
 
-	return retval;
+	return enabled ? 0 : 1; // 0 on success, 1 otherwise
 }
 EXPORT_SYMBOL(fb_get_options);
-
-/**
- *	video_setup - process command line options
- *	@options: string of options
- *
- *	Process command line options for frame buffer subsystem.
- *
- *	NOTE: This function is a __setup and __init function.
- *            It only stores the options.  Drivers have to call
- *            fb_get_options() as necessary.
- */
-static int __init video_setup(char *options)
-{
-	if (!options || !*options)
-		goto out;
-
-	if (!strncmp(options, "ofonly", 6)) {
-		ofonly = 1;
-		goto out;
-	}
-
-	if (strchr(options, ':')) {
-		/* named */
-		int i;
-
-		for (i = 0; i < FB_MAX; i++) {
-			if (video_options[i] == NULL) {
-				video_options[i] = options;
-				break;
-			}
-		}
-	} else {
-		/* global */
-		fb_mode_option = options;
-	}
-
-out:
-	return 1;
-}
-__setup("video=", video_setup);
diff --git a/include/video/cmdline.h b/include/video/cmdline.h
new file mode 100644
index 000000000000..26b80cdaef79
--- /dev/null
+++ b/include/video/cmdline.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef VIDEO_CMDLINE_H
+#define VIDEO_CMDLINE_H
+
+#include <linux/types.h>
+
+#if defined(CONFIG_VIDEO_CMDLINE)
+const char *video_get_options(const char *name);
+
+/* exported for compatibility with fbdev; don't use in new code */
+bool __video_get_options(const char *name, const char **option, bool is_of);
+#else
+static inline const char *video_get_options(const char *name)
+{
+	return NULL;
+}
+#endif
+
+#endif
-- 
2.39.1

[PATCH 09/11] driver/ps3: Include <video/cmdline.h> for mode parsing

[Thomas Zimmermann]

Include <video/cmdline.h> in ps3av.c to get video_get_options() and
avoid the dependency on <linux/fb.h>. The replaced function
fb_get_options() is just a tiny wrapper around video_get_opions(). No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/ps3/ps3av.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/ps3/ps3av.c b/drivers/ps3/ps3av.c
index 8f3e60f1bfe2..f6c9e56bdba7 100644
--- a/drivers/ps3/ps3av.c
+++ b/drivers/ps3/ps3av.c
@@ -11,13 +11,14 @@
 #include <linux/delay.h>
 #include <linux/notifier.h>
 #include <linux/ioctl.h>
-#include <linux/fb.h>
 #include <linux/slab.h>
 
 #include <asm/firmware.h>
 #include <asm/ps3av.h>
 #include <asm/ps3.h>
 
+#include <video/cmdline.h>
+
 #include "vuart.h"
 
 #define BUFSIZE          4096	/* vuart buf size */
@@ -921,9 +922,7 @@ EXPORT_SYMBOL_GPL(ps3av_audio_mute);
 
 static int ps3av_probe(struct ps3_system_bus_device *dev)
 {
-#ifdef CONFIG_FB
-	char *mode_option = NULL;
-#endif
+	const char *mode_option;
 	int res;
 	int id;
 
@@ -971,14 +970,9 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
 
 	ps3av_get_hw_conf(ps3av);
 
-#ifdef CONFIG_FB
-	fb_get_options(NULL, &mode_option);
-	if (mode_option) {
-		if (!strcmp(mode_option, "safe"))
-			safe_mode = 1;
-		kfree(mode_option);
-	}
-#endif /* CONFIG_FB */
+	mode_option = video_get_options(NULL);
+	if (mode_option && !strcmp(mode_option, "safe"))
+		safe_mode = 1;
 	id = ps3av_auto_videomode(&ps3av->av_hw_conf);
 	if (id < 0) {
 		printk(KERN_ERR "%s: invalid id :%d\n", __func__, id);
-- 
2.39.1

[PATCH 10/11] drm: Include <video/cmdline.h> for mode parsing

[Thomas Zimmermann]

Include <video/cmdline.h> in drm_connector.c to get video_get_options()
and avoid the dependency on <linux/fb.h>. The replaced function
fb_get_options() is just a tiny wrapper around video_get_opions(). No
functional changes.

Include <linux/property.h> to get fwnode_handle_put(), which had been
provided via <linux/fb.h>.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_connector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 9d0250c28e9b..ca5fb54b7aab 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -33,9 +33,11 @@
 #include <drm/drm_sysfs.h>
 #include <drm/drm_utils.h>
 
-#include <linux/fb.h>
+#include <linux/property.h>
 #include <linux/uaccess.h>
 
+#include <video/cmdline.h>
+
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
 
@@ -154,9 +156,10 @@ EXPORT_SYMBOL(drm_get_connector_type_name);
 static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *mode = &connector->cmdline_mode;
-	char *option = NULL;
+	const char *option;
 
-	if (fb_get_options(connector->name, &option))
+	option = video_get_options(connector->name);
+	if (!option)
 		return;
 
 	if (!drm_mode_parse_command_line_for_connector(option,
-- 
2.39.1

[PATCH 11/11] drm: Fix comment on mode parsing

[Thomas Zimmermann]

Do not claim that there's a default mode in the video= option parser.
if no option string has been given, the parser does nothing.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_modes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 40d482a01178..ac9a406250c5 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2339,8 +2339,7 @@ static int drm_mode_parse_cmdline_named_mode(const char *name,
  * @mode: preallocated drm_cmdline_mode structure to fill out
  *
  * This parses @mode_option command line modeline for modes and options to
- * configure the connector. If @mode_option is NULL the default command line
- * modeline in fb_mode_option will be parsed instead.
+ * configure the connector.
  *
  * This uses the same parameters as the fb modedb.c, except for an extra
  * force-enable, force-enable-digital and force-disable bit at the end::
-- 
2.39.1

Re: [PATCH 04/11] drivers/ps3: Read video= option with fb_get_option()

[Geoff Levand]

Hi Thomas,

On 2/9/23 05:55, Thomas Zimmermann wrote:
> Get the kernel's global video= parameter with fb_get_option(). Done
> to unexport the internal fbdev state fb_mode_config. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/ps3/ps3av.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

I wanted to test these changes on the PS3, but got this
error when trying to apply this patch set to Linux-6.2-rc7:

  Applying: fbdev: Handle video= parameter in video/cmdline.c
  error: patch failed: drivers/gpu/drm/Kconfig:10
  error: drivers/gpu/drm/Kconfig: patch does not apply

Is there a Linux kernel revision that these will apply to,
or is there a git repository I can pull them from?

-Geoff

Re: [PATCH 04/11] drivers/ps3: Read video= option with fb_get_option()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1331 bytes --]

Hi

Am 12.02.23 um 17:53 schrieb Geoff Levand:
> Hi Thomas,
> 
> On 2/9/23 05:55, Thomas Zimmermann wrote:
>> Get the kernel's global video= parameter with fb_get_option(). Done
>> to unexport the internal fbdev state fb_mode_config. No functional
>> changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/ps3/ps3av.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> I wanted to test these changes on the PS3, but got this
> error when trying to apply this patch set to Linux-6.2-rc7:
> 
>    Applying: fbdev: Handle video= parameter in video/cmdline.c
>    error: patch failed: drivers/gpu/drm/Kconfig:10
>    error: drivers/gpu/drm/Kconfig: patch does not apply
> 
> Is there a Linux kernel revision that these will apply to,
> or is there a git repository I can pull them from?

Thanks for testing.  My base version is a recent DRM development tree. 
The repo is at https://cgit.freedesktop.org/drm/drm-tip/, the branch is 
drm-tip.

If acceptable, I'd later like to merge the PS3 patches through DRM trees.

Best regards
Thomas

> 
> -Geoff

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 04/11] drivers/ps3: Read video= option with fb_get_option()

[Geoff Levand]

Hi,

On 2/13/23 03:29, Thomas Zimmermann wrote:
> Am 12.02.23 um 17:53 schrieb Geoff Levand:
>> On 2/9/23 05:55, Thomas Zimmermann wrote:
>>> Get the kernel's global video= parameter with fb_get_option(). Done
>>> to unexport the internal fbdev state fb_mode_config. No functional
>>> changes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/ps3/ps3av.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> I wanted to test these changes on the PS3, but got this
>> error when trying to apply this patch set to Linux-6.2-rc7:
>>
>>    Applying: fbdev: Handle video= parameter in video/cmdline.c
>>    error: patch failed: drivers/gpu/drm/Kconfig:10
>>    error: drivers/gpu/drm/Kconfig: patch does not apply
>>
>> Is there a Linux kernel revision that these will apply to,
>> or is there a git repository I can pull them from?
> 
> Thanks for testing.  My base version is a recent DRM development tree. The repo is at https://cgit.freedesktop.org/drm/drm-tip/, the branch is drm-tip.

I tested the drm-tip branch at c54b5fcf3e68 on PS3 and it
seemed to work OK.

Tested-by: Geoff Levand <geoff@infradead.org>

Re: [PATCH 01/11] fbdev: Fix contact info in fb_cmdline.c

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Fix Daniel's email address. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 02/11] fbdev: Transfer video= option strings to caller; clarify ownership

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> In fb_get_options(), always duplicate the returned option string and
> transfer ownership of the memory to the function's caller.
>
> Until now, only the global option string got duplicated and transferred
> to the caller; the per-driver options were owned by fb_get_options().
> In the end, it was impossible for the function's caller to detect if
> it had to release the string's memory buffer. Hence, all calling drivers
> leak the memory buffer. The leaks have existed ever since, but drivers
> only call fb_get_option() once as part of module initialization. So the
> amount of leaked memory is not significant.
>
> Fix the semantics of fb_get_option() by unconditionally transferring
> ownership of the memory buffer to the caller. Later patches can resolve
> the memory leaks in the fbdev drivers.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

> +	if (option) {
> +		if (options)
> +			*option = kstrdup(options, GFP_KERNEL);
> +		else
> +			*option = NULL;
> +	}
>

I know the old code wasn't checking if kstrdup() succeeded, but you should
do it here and let the caller know. And same if (!options). So I guess the
following check can be added (to be consistent with the rest of the code):

	if (!*option)
		retval = 1;

>  	return retval;
>  }
> -- 
> 2.39.1

Best regards,
Javier

Re: [PATCH 03/11] fbdev: Support NULL for name in option-string lookup

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Ignore the per-driver video options if no driver name has been
> specified to fb_get_option(). Return the global options in this
> case.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

I think you need to update the kernel-doc as well to mention that
@name could be NULL ?

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 04/11] drivers/ps3: Read video= option with fb_get_option()

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Get the kernel's global video= parameter with fb_get_option(). Done
> to unexport the internal fbdev state fb_mode_config. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 05/11] fbdev: Read video= option with fb_get_option() in modedb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Get the kernel's global video= parameter with fb_get_option(). Done
> to unexport the internal fbdev state fb_mode_config. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 06/11] fbdev: Unexport fb_mode_option

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> There are no external users of fb_mode_option. Unexport the variable
> and declare it static.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 07/11] fbdev: Move option-string lookup into helper

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Move the lookup of the option string into an internal helper. No
> functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 08/11] fbdev: Handle video= parameter in video/cmdline.c

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Handle the command-line parameter video= in video/cmdline.c. Implement
> the fbdev helper fb_get_options() on top. Will allows to handle the
> kernel parameter in DRM without fbdev dependencies.
>
> Note that __video_get_options() has the meaning of its return value
> inverted compared to fb_get_options(). The new helper returns true if
> the adapter has been enabled, and false otherwise.
>
> There is the ofonly parameter, which disables output for non-OF-based
> framebuffers. It is only for offb and looks like a workaround. The actual
> purpose it not clear to me. Use 'video=off' or 'nomodeset' instead.
>

s/it/is

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[..]

> +#include <linux/fb.h> /* for FB_MAX */

I believe including <uapi/linux/fb.h> is enough here.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 09/11] driver/ps3: Include <video/cmdline.h> for mode parsing

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Include <video/cmdline.h> in ps3av.c to get video_get_options() and
> avoid the dependency on <linux/fb.h>. The replaced function
> fb_get_options() is just a tiny wrapper around video_get_opions(). No
> functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 10/11] drm: Include <video/cmdline.h> for mode parsing

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Include <video/cmdline.h> in drm_connector.c to get video_get_options()
> and avoid the dependency on <linux/fb.h>. The replaced function
> fb_get_options() is just a tiny wrapper around video_get_opions(). No
> functional changes.
>
> Include <linux/property.h> to get fwnode_handle_put(), which had been
> provided via <linux/fb.h>.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 11/11] drm: Fix comment on mode parsing

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Do not claim that there's a default mode in the video= option parser.
> if no option string has been given, the parser does nothing.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 02/11] fbdev: Transfer video= option strings to caller; clarify ownership

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2166 bytes --]

Hi

Am 17.02.23 um 09:37 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> In fb_get_options(), always duplicate the returned option string and
>> transfer ownership of the memory to the function's caller.
>>
>> Until now, only the global option string got duplicated and transferred
>> to the caller; the per-driver options were owned by fb_get_options().
>> In the end, it was impossible for the function's caller to detect if
>> it had to release the string's memory buffer. Hence, all calling drivers
>> leak the memory buffer. The leaks have existed ever since, but drivers
>> only call fb_get_option() once as part of module initialization. So the
>> amount of leaked memory is not significant.
>>
>> Fix the semantics of fb_get_option() by unconditionally transferring
>> ownership of the memory buffer to the caller. Later patches can resolve
>> the memory leaks in the fbdev drivers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +	if (option) {
>> +		if (options)
>> +			*option = kstrdup(options, GFP_KERNEL);
>> +		else
>> +			*option = NULL;
>> +	}
>>
> 
> I know the old code wasn't checking if kstrdup() succeeded, but you should

Kstrdup uses kmalloc, which already warns about failed allocations. I 
think it's discouraged to warn again. (Wasn't there a warning in sparse 
or checkpatch?)  So I'd rather leave it as is.

> do it here and let the caller know. And same if (!options). So I guess the
> following check can be added (to be consistent with the rest of the code):
> 
> 	if (!*option)
> 		retval = 1;

Why is that needed for consistency?

Retval is the state of the output: enabled or not. If there are no 
options, retval should be 0(=enabled). 1(=disabled) is only set by 
video=off or that ofonly thing.

Best regards
Thomas

> 
>>   	return retval;
>>   }
>> -- 
>> 2.39.1
> 
> Best regards,
> Javier
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 02/11] fbdev: Transfer video= option strings to caller; clarify ownership

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 17.02.23 um 09:37 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> In fb_get_options(), always duplicate the returned option string and
>>> transfer ownership of the memory to the function's caller.
>>>
>>> Until now, only the global option string got duplicated and transferred
>>> to the caller; the per-driver options were owned by fb_get_options().
>>> In the end, it was impossible for the function's caller to detect if
>>> it had to release the string's memory buffer. Hence, all calling drivers
>>> leak the memory buffer. The leaks have existed ever since, but drivers
>>> only call fb_get_option() once as part of module initialization. So the
>>> amount of leaked memory is not significant.
>>>
>>> Fix the semantics of fb_get_option() by unconditionally transferring
>>> ownership of the memory buffer to the caller. Later patches can resolve
>>> the memory leaks in the fbdev drivers.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>> 
>> [...]
>> 
>>> +	if (option) {
>>> +		if (options)
>>> +			*option = kstrdup(options, GFP_KERNEL);
>>> +		else
>>> +			*option = NULL;
>>> +	}
>>>
>> 
>> I know the old code wasn't checking if kstrdup() succeeded, but you should
>
> Kstrdup uses kmalloc, which already warns about failed allocations. I 
> think it's discouraged to warn again. (Wasn't there a warning in sparse 
> or checkpatch?)  So I'd rather leave it as is.
>

I didn't mean to warn but to return an error code.

>> do it here and let the caller know. And same if (!options). So I guess the
>> following check can be added (to be consistent with the rest of the code):
>> 
>> 	if (!*option)
>> 		retval = 1;
>
> Why is that needed for consistency?
>
> Retval is the state of the output: enabled or not. If there are no 
> options, retval should be 0(=enabled). 1(=disabled) is only set by 
> video=off or that ofonly thing.
>

Ah, I see. I misundertood what retval was about. Forget this comment then.

Maybe while you are there could have another patch to document the return
value in the fb_get_options() kernel-doc?

And this patch looks good to me too after your explanations.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier