Messages in this thread | | | Date | Thu, 6 Jun 2013 21:20:14 -0700 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH v1 3/4] firmware loader: allow distribution to choose default search paths |
| |
On Fri, Jun 07, 2013 at 09:24:25AM +0800, Ming Lei wrote: > On Fri, Jun 7, 2013 at 3:47 AM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 06, 2013 at 08:01:49PM +0800, Ming Lei wrote: > >> For some distributions(e.g. android), firmware images aren't put > >> under kernel built-in search paths, so introduce one Kconfig > >> option to allow distributions or users to choose its specific default > >> search paths, which are always tried before searching from kernel > >> built-in paths in direct loading. > >> > >> Also this patch introduces fw_get_fw_file_from_paths to cover all > >> search paths, and fw_get_filesystem_firmware is simpified a bit. > >> > >> Cc: Takashi Iwai <tiwai@suse.de> > >> Signed-off-by: Ming Lei <ming.lei@canonical.com> > >> --- > >> drivers/base/Kconfig | 14 ++++++++++ > >> drivers/base/firmware_class.c | 61 ++++++++++++++++++++++++++++++++++------- > >> 2 files changed, 65 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > >> index 07abd9d..2be10e4 100644 > >> --- a/drivers/base/Kconfig > >> +++ b/drivers/base/Kconfig > >> @@ -156,6 +156,20 @@ config FW_LOADER_USER_HELPER > >> no longer required unless you have a special firmware file that > >> resides in a non-standard path. > >> > >> +config FW_CUSTOMIZED_PATH > >> + string "default firmware search paths for direct loading" > >> + help > >> + On some distribution(e.g. android), firmware images aren't > >> + put under kernel built-in search paths, so provide this option > >> + for distributions to choose a distribution specific firmware > >> + search path. The option allows to choose more than one path, > >> + and paths are seperated with colon like $PATH(e.g. on android, > >> + the option might look as "/etc/firmware:/vendor/firmware"). > >> + Each path should be a absolute path, and relative path will be > >> + ignored. > >> + > >> + If you are unsure about this, don't choose here. > >> + > >> config DEBUG_DRIVER > >> bool "Driver Core verbose debug messages" > >> depends on DEBUG_KERNEL > >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > >> index 6ede229..051db83 100644 > >> --- a/drivers/base/firmware_class.c > >> +++ b/drivers/base/firmware_class.c > >> @@ -265,8 +265,13 @@ static void fw_free_buf(struct firmware_buf *buf) > >> > >> /* direct firmware loading support */ > >> static char fw_path_para[256]; > >> + > >> +/* search runtime paths first, then static pre-defined paths */ > >> static const char * const fw_path[] = { > >> fw_path_para, > >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH > >> + CONFIG_FW_CUSTOMIZED_PATH, > >> +#endif > >> "/lib/firmware/updates/" UTS_RELEASE, > >> "/lib/firmware/updates", > >> "/lib/firmware/" UTS_RELEASE, > >> @@ -314,6 +319,50 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf > >> return true; > >> } > >> > >> +static bool fw_get_file_firmware(const char *path, > >> + struct firmware_buf *buf) > >> +{ > >> + struct file *file; > >> + bool success; > >> + > >> + file = filp_open(path, O_RDONLY, 0); > >> + if (IS_ERR(file)) > >> + return false; > >> + success = fw_read_file_contents(file, buf); > >> + fput(file); > >> + > >> + return success; > >> +} > >> + > >> +/* The path in @paths is seperated by ';' */ > > > > No it isn't. > > Will fix it. > > > > >> +static bool fw_get_file_fw_from_paths(const char *paths, char *path, > >> + struct firmware_buf *buf) > >> +{ > >> + int len, start, end = -1; > >> + char *pos; > >> + > >> + do { > >> + start = end + 1; > >> + pos = strchr(&paths[start], ':'); > > > > As you have an array of paths now, why are you doing the ':' check > > still? Don't do that, just allow the person building the kernel to add > > one path to the loader, that should be all that we need, right? > > The distribution may have put firmware under more than one places, > for example, android put firmwares under "/etc/firmware" and > "/vendor/firmware", so I am afraid that only allowing one path or two > paths isn't flexible enough.
How about we wait until someone complains about this? Don't add features that aren't needed, especially complex ones like this.
thanks,
greg k-h
| |