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.
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(+)
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[] = {
--
2.25.1
On 4/23/20 1:31 PM, 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=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 <[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]>
Acked-by: Randy Dunlap <[email protected]>
Tested-by: Randy Dunlap <[email protected]>
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[] = {
>
--
~Randy
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 <[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.
> > >
> > > 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.
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..
Hi Luis,
On Fri, 24 Apr 2020 02:14:20 +0000 Luis Chamberlain <[email protected]> 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).
--
Cheers,
Stephen Rothwell
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 <[email protected]> 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
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Thu, Apr 23, 2020 at 08:31:40PM +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=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 <[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(+)
>
> 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 <[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.
> >
> > 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(+)
> >
> > 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