| Message ID | 20200423203140.19510-1-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace | expand |
On 4/23/20 1:31 PM, Luis R. Rodriguez wrote: > From: Luis Chamberlain <mcgrof@kernel.org> > > 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. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > Fixes: "firmware_loader: remove unused exports" > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Acked-by: Randy Dunlap <rdunlap@infradead.org> Tested-by: Randy Dunlap <rdunlap@infradead.org> thanks. > --- > drivers/base/firmware_loader/fallback.c | 3 +++ > drivers/base/firmware_loader/fallback_table.c | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c > index 1e9c96e3ed63..d9ac7296205e 100644 > --- a/drivers/base/firmware_loader/fallback.c > +++ b/drivers/base/firmware_loader/fallback.c > @@ -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 */ > diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c > index 0a737349f78f..46a731dede6f 100644 > --- a/drivers/base/firmware_loader/fallback_table.c > +++ b/drivers/base/firmware_loader/fallback_table.c > @@ -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[] = { >
On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote: > From: Luis Chamberlain <mcgrof@kernel.org> > > 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. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > Fixes: "firmware_loader: remove unused exports" Can't help but notice this strange form of the Fixes tag, is it intentional? > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
On Thu, Apr 23, 2020 at 06:05:44PM -0700, Jakub Kicinski wrote: > On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote: > > From: Luis Chamberlain <mcgrof@kernel.org> > > > > 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. > > > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Randy Dunlap <rdunlap@infradead.org> > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > 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
On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain wrote: > On Thu, Apr 23, 2020 at 06:05:44PM -0700, Jakub Kicinski wrote: > > On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote: > > > From: Luis Chamberlain <mcgrof@kernel.org> > > > > > > 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. > > > > > > Cc: Christoph Hellwig <hch@lst.de> > > > Cc: Randy Dunlap <rdunlap@infradead.org> > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > 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. 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..
On Thu, Apr 23, 2020 at 07:27:16PM -0700, Jakub Kicinski wrote: > On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain wrote: > > On Thu, Apr 23, 2020 at 06:05:44PM -0700, Jakub Kicinski wrote: > > > On Thu, 23 Apr 2020 20:31:40 +0000 Luis R. Rodriguez wrote: > > > > From: Luis Chamberlain <mcgrof@kernel.org> > > > > > > > > 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. > > > > > > > > Cc: Christoph Hellwig <hch@lst.de> > > > > Cc: Randy Dunlap <rdunlap@infradead.org> > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > 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. > > 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, On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > 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. 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).
On Fri, Apr 24, 2020 at 01:15:56PM +1000, Stephen Rothwell wrote: > Hi Luis, > > On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > 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. > > 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, On Fri, 24 Apr 2020 03:19:59 +0000 Luis Chamberlain <mcgrof@kernel.org> 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.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Apr 23, 2020 at 08:31:40PM +0000, Luis R. Rodriguez wrote: > From: Luis Chamberlain <mcgrof@kernel.org> > > 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. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > Fixes: "firmware_loader: remove unused exports" > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/base/firmware_loader/fallback.c | 3 +++ > drivers/base/firmware_loader/fallback_table.c | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c > index 1e9c96e3ed63..d9ac7296205e 100644 > --- a/drivers/base/firmware_loader/fallback.c > +++ b/drivers/base/firmware_loader/fallback.c > @@ -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 */ While nice, that does not fix the existing build error that people are having, right? > diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c > index 0a737349f78f..46a731dede6f 100644 > --- a/drivers/base/firmware_loader/fallback_table.c > +++ b/drivers/base/firmware_loader/fallback_table.c > @@ -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
On Fri, Apr 24, 2020 at 11:21:19AM +0200, Greg KH wrote: > On Thu, Apr 23, 2020 at 08:31:40PM +0000, Luis R. Rodriguez wrote: > > From: Luis Chamberlain <mcgrof@kernel.org> > > > > 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. > > > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Randy Dunlap <rdunlap@infradead.org> > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > Fixes: "firmware_loader: remove unused exports" > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > drivers/base/firmware_loader/fallback.c | 3 +++ > > drivers/base/firmware_loader/fallback_table.c | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c > > index 1e9c96e3ed63..d9ac7296205e 100644 > > --- a/drivers/base/firmware_loader/fallback.c > > +++ b/drivers/base/firmware_loader/fallback.c > > @@ -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 */ > > While nice, that does not fix the existing build error that people are > having, right? It does. > > diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c > > index 0a737349f78f..46a731dede6f 100644 > > --- a/drivers/base/firmware_loader/fallback_table.c > > +++ b/drivers/base/firmware_loader/fallback_table.c > > @@ -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. Sure. Luis
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 1e9c96e3ed63..d9ac7296205e 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -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 */ diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c index 0a737349f78f..46a731dede6f 100644 --- a/drivers/base/firmware_loader/fallback_table.c +++ b/drivers/base/firmware_loader/fallback_table.c @@ -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[] = {