firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
Subsystems (3)
Luis R. Rodriguez
maintainerPATCH 1/1 firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.
If CONFIG_FW_LOADER=m so the firmware_loader is modular but CONFIG_FW_LOADER_USER_HELPER=y we fail at mostpost with:
ERROR: modpost: "fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!
This happens because the variable fw_fallback_config is built into the kernel if CONFIG_FW_LOADER_USER_HELPER=y always, so we need to grant access to the firmware loader module by exporting it.
Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
--- drivers/base/firmware_loader/fallback.c | 3 +++ drivers/base/firmware_loader/fallback_table.c | 1 + 2 files changed, 4 insertions(+)
drivers/base/firmware_loader/fallback.c +3 -0
@@ -9,6 +9,7 @@#include <linux/umh.h> #include <linux/sysctl.h> #include <linux/vmalloc.h> +#include <linux/module.h> #include "fallback.h" #include "firmware.h"
@@ -17,6 +18,8 @@* firmware fallback mechanism */ +MODULE_IMPORT_NS (FIRMWARE_LOADER_PRIVATE ); +extern struct firmware_fallback_config fw_fallback_config;/* These getters are vetted to use int properly */
drivers/base/firmware_loader/fallback_table.c +1 -0
@@ -21,6 +21,7 @@ struct firmware_fallback_config fw_fallback_config = { .loading_timeout = 60, .old_timeout = 60, }; +EXPORT_SYMBOL_NS_GPL (fw_fallback_config,FIRMWARE_LOADER_PRIVATE );#ifdef CONFIG_SYSCTL struct ctl_table firmware_config_table[] = {
Signature · 1 line
2.25.1
Quoted 21 lines · Luis R. Rodriguez
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Quoted 6 lines
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"Reported-by Randy Dunlap <[email protected]>Signed-off-by Luis Chamberlain <[email protected]>
thanks.
Quoted 39 lines
--- drivers/base/firmware_loader/fallback.c | 3 +++ drivers/base/firmware_loader/fallback_table.c | 1 + 2 files changed, 4 insertions(+)drivers/base/firmware_loader/fallback.c +3
-0@@ -9,6 +9,7 @@#include <linux/umh.h> #include <linux/sysctl.h> #include <linux/vmalloc.h> +#include <linux/module.h> #include "fallback.h" #include "firmware.h" @@ -17,6 +18,8 @@* firmware fallback mechanism */ +MODULE_IMPORT_NS (FIRMWARE_LOADER_PRIVATE ); +extern struct firmware_fallback_config fw_fallback_config;/* These getters are vetted to use int properly */ drivers/base/firmware_loader/fallback_table.c +1
-0@@ -21,6 +21,7 @@ struct firmware_fallback_config fw_fallback_config = { .loading_timeout = 60, .old_timeout = 60, }; +EXPORT_SYMBOL_NS_GPL (fw_fallback_config,FIRMWARE_LOADER_PRIVATE );#ifdef CONFIG_SYSCTL struct ctl_table firmware_config_table[] = {
Signature · 1 line
~Randy
Quoted 21 lines · On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote:
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Quoted 4 lines
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"
Can't help but notice this strange form of the Fixes tag, is it intentional?
Quoted 2 lines
Reported-by Randy Dunlap <[email protected]>Signed-off-by Luis Chamberlain <[email protected]>
Luis Chamberlain
maintainerQuoted 22 lines · Jakub Kicinski
Quoted 21 lines · On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote:
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Quoted 7 lines
Quoted 4 lines
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"Can't help but notice this strange form of the Fixes tag, is it intentional?
Yeah, no there is no commit for the patch as the commit is ephemeral in a development tree not yet upstream, ie, not on Linus' tree yet. Using a commit here then makes no sense unless one wants to use a reference development tree in this case, as development trees are expected to rebase to move closer towards Linus' tree. When a tree rebases, the commit IDs change, and this is why the commit is ephemeral unless one uses a base tree / branch / tag.
Luis
Quoted 31 lines · On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain wrote:
Quoted 22 lines · Jakub Kicinski
Quoted 21 lines · On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote:
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Quoted 7 lines
Quoted 4 lines
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"Can't help but notice this strange form of the Fixes tag, is it intentional?
Quoted 7 lines
Yeah, no there is no commit for the patch as the commit is ephemeral in a development tree not yet upstream, ie, not on Linus' tree yet. Using a commit here then makes no sense unless one wants to use a reference development tree in this case, as development trees are expected to rebase to move closer towards Linus' tree. When a tree rebases, the commit IDs change, and this is why the commit is ephemeral unless one uses a base tree / branch / tag.
I'd think that either the commit is rebase-able and the fix can be squashed into it, or it's not and it has a stable commit id. But I guess it may get tricky around the edges..
Luis Chamberlain
maintainerQuoted 40 lines · Jakub Kicinski
Quoted 31 lines · On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain wrote:
Quoted 22 lines · Jakub Kicinski
Quoted 21 lines · On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote:
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Quoted 7 lines
Quoted 4 lines
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"Can't help but notice this strange form of the Fixes tag, is it intentional?
Quoted 7 lines
Yeah, no there is no commit for the patch as the commit is ephemeral in a development tree not yet upstream, ie, not on Linus' tree yet. Using a commit here then makes no sense unless one wants to use a reference development tree in this case, as development trees are expected to rebase to move closer towards Linus' tree. When a tree rebases, the commit IDs change, and this is why the commit is ephemeral unless one uses a base tree / branch / tag.
Quoted 3 lines
I'd think that either the commit is rebase-able and the fix can be squashed into it, or it's not and it has a stable commit id. But I guess it may get tricky around the edges..
I'll let Greg decide ;)
I did my part.
Luis
Hi Luis,
Quoted 6 lines · On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain <[email protected]> wrote:
Quoted 5 lines
Quoted 2 lines
Fixes "firmware_loader: remove unused exports"Can't help but notice this strange form of the Fixes tag, is it intentional?
Quoted 7 lines
Yeah, no there is no commit for the patch as the commit is ephemeral in a development tree not yet upstream, ie, not on Linus' tree yet. Using a commit here then makes no sense unless one wants to use a reference development tree in this case, as development trees are expected to rebase to move closer towards Linus' tree. When a tree rebases, the commit IDs change, and this is why the commit is ephemeral unless one uses a base tree / branch / tag.
That commit is in Greg's driver-core tree which never rebases, so the SHA1 can be considered immutable. This is (should be) true for most trees that are published in linux-next (I know it is not true for some).
Signature · 2 lines
Cheers, Stephen Rothwell
Luis Chamberlain
maintainerQuoted 17 lines · Stephen Rothwell
Hi Luis,
Quoted 6 lines · On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain <[email protected]> wrote:
Quoted 5 lines
Quoted 2 lines
Fixes "firmware_loader: remove unused exports"Can't help but notice this strange form of the Fixes tag, is it intentional?
Quoted 7 lines
Yeah, no there is no commit for the patch as the commit is ephemeral in a development tree not yet upstream, ie, not on Linus' tree yet. Using a commit here then makes no sense unless one wants to use a reference development tree in this case, as development trees are expected to rebase to move closer towards Linus' tree. When a tree rebases, the commit IDs change, and this is why the commit is ephemeral unless one uses a base tree / branch / tag.
Quoted 3 lines
That commit is in Greg's driver-core tree which never rebases, so the SHA1 can be considered immutable. This is (should be) true for most trees that are published in linux-next (I know it is not true for some).
Cool, but once merged on Linus' tree, I think it gets yet-another-commit ID right? So someone looking for:
git show commit-id-on-gregs-driver-core-tree
It would not work? Or would it?
Luis
Hi Luis,
Quoted 3 lines · On Fri, 24 Apr 2020 03:19:59 +0000 Luis Chamberlain <[email protected]> wrote:
Cool, but once merged on Linus' tree, I think it gets yet-another-commit ID right? So someone looking for:
No, Linus merges Greg's tree directly, so all the commits remain the same.
Signature · 2 lines
Cheers, Stephen Rothwell
Looks good,
Greg KH
maintainerQuoted 45 lines · Luis R. Rodriguez
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"Reported-by Randy Dunlap <[email protected]>Signed-off-by Luis Chamberlain <[email protected]>--- drivers/base/firmware_loader/fallback.c | 3 +++ drivers/base/firmware_loader/fallback_table.c | 1 + 2 files changed, 4 insertions(+)drivers/base/firmware_loader/fallback.c +1
-0@@ -9,6 +9,7 @@#include <linux/umh.h> #include <linux/sysctl.h> #include <linux/vmalloc.h> +#include <linux/module.h> #include "fallback.h" #include "firmware.h"
drivers/base/firmware_loader/fallback.c line 25
Quoted 8 lines
drivers/base/firmware_loader/fallback.c +2
-0@@ -17,6 +18,8 @@* firmware fallback mechanism */ +MODULE_IMPORT_NS (FIRMWARE_LOADER_PRIVATE ); +extern struct firmware_fallback_config fw_fallback_config;/* These getters are vetted to use int properly */
While nice, that does not fix the existing build error that people are having, right?
drivers/base/firmware_loader/fallback_table.c struct firmware_fallback_config fw_fallback_config = { · line 24
Quoted 9 lines
drivers/base/firmware_loader/fallback_table.c +1
-0@@ -21,6 +21,7 @@ struct firmware_fallback_config fw_fallback_config = { .loading_timeout = 60, .old_timeout = 60, }; +EXPORT_SYMBOL_NS_GPL (fw_fallback_config,FIRMWARE_LOADER_PRIVATE );
How about you send a patch that just reverts the single symbol change first, and then a follow-on patch that does this namespace addition. I can queue the first one up now, for 5.7-final, and the second one for 5.8-rc1.
thanks,
greg k-h
Luis Chamberlain
maintainerQuoted 49 lines · Greg KH
Quoted 48 lines · Luis R. Rodriguez
From: Luis Chamberlain <[email protected]>
Christoph's recent patch "
firmware_loader: remove unused exports", which is not merged upstream yet, removed two exported symbols. One is fine to remove since only built-in code uses it but the other is incorrect.If
CONFIG_FW_LOADER=mso thefirmware_loaderis modular butCONFIG_FW_LOADER_USER_HELPER=ywe fail at mostpost with:ERROR: modpost: "
fw_fallback_config" [drivers/base/firmware_loader/firmware_class.ko] undefined!This happens because the variable
fw_fallback_configis built into the kernel ifCONFIG_FW_LOADER_USER_HELPER=yalways, so we need to grant access to the firmware loader module by exporting it.Instead of just exporting it as we used to, take advantage of the new kernel symbol namespacing functionality, and export the symbol only to the firmware loader private namespace. This would prevent misuses from other drivers and makes it clear the goal is to keep this private to the firmware loader alone.
Cc Christoph Hellwig <[email protected]>Cc Randy Dunlap <[email protected]>Cc Stephen Rothwell <[email protected]>Fixes "firmware_loader: remove unused exports"Reported-by Randy Dunlap <[email protected]>Signed-off-by Luis Chamberlain <[email protected]>--- drivers/base/firmware_loader/fallback.c | 3 +++ drivers/base/firmware_loader/fallback_table.c | 1 + 2 files changed, 4 insertions(+)drivers/base/firmware_loader/fallback.c +1
-0@@ -9,6 +9,7 @@#include <linux/umh.h> #include <linux/sysctl.h> #include <linux/vmalloc.h> +#include <linux/module.h> #include "fallback.h" #include "firmware.h" @@ -17,6 +18,8 @@* firmware fallback mechanism */
Quoted 8 lines
drivers/base/firmware_loader/fallback.c line 25
Quoted 5 lines
drivers/base/firmware_loader/fallback.c +2
-0+MODULE_IMPORT_NS (FIRMWARE_LOADER_PRIVATE ); +extern struct firmware_fallback_config fw_fallback_config;/* These getters are vetted to use int properly */ While nice, that does not fix the existing build error that people are having, right?
It does.
Quoted 7 lines
Quoted 7 lines
drivers/base/firmware_loader/fallback_table.c +0
-0@@ -21,6 +21,7 @@ struct firmware_fallback_config fw_fallback_config = { .loading_timeout = 60, .old_timeout = 60,
Quoted 8 lines
drivers/base/firmware_loader/fallback_table.c struct firmware_fallback_config fw_fallback_config = { · line 24
Quoted 2 lines
drivers/base/firmware_loader/fallback_table.c +1
-0}; +EXPORT_SYMBOL_NS_GPL (fw_fallback_config,FIRMWARE_LOADER_PRIVATE );How about you send a patch that just reverts the single symbol change first, and then a follow-on patch that does this namespace addition. I can queue the first one up now, for 5.7-final, and the second one for 5.8-rc1.
Sure.
Luis