linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
@ 2020-04-23 20:31 Luis R. Rodriguez
  2020-04-23 20:42 ` Randy Dunlap
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Luis R. Rodriguez @ 2020-04-23 20:31 UTC (permalink / raw)
  To: gregkh
  Cc: akpm, josh, rishabhb, kubakici, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Luis Chamberlain, Christoph Hellwig, Randy Dunlap,
	Stephen Rothwell

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 */
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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-23 20:31 [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace Luis R. Rodriguez
@ 2020-04-23 20:42 ` Randy Dunlap
  2020-04-24  1:05 ` Jakub Kicinski
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Randy Dunlap @ 2020-04-23 20:42 UTC (permalink / raw)
  To: Luis R. Rodriguez, gregkh
  Cc: akpm, josh, rishabhb, kubakici, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Stephen Rothwell

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[] = {
> 


-- 
~Randy

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-23 20:31 [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace Luis R. Rodriguez
  2020-04-23 20:42 ` Randy Dunlap
@ 2020-04-24  1:05 ` Jakub Kicinski
  2020-04-24  2:14   ` Luis Chamberlain
  2020-04-24  6:57 ` Christoph Hellwig
  2020-04-24  9:21 ` Greg KH
  3 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-04-24  1:05 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: gregkh, akpm, josh, rishabhb, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  1:05 ` Jakub Kicinski
@ 2020-04-24  2:14   ` Luis Chamberlain
  2020-04-24  2:27     ` Jakub Kicinski
  2020-04-24  3:15     ` Stephen Rothwell
  0 siblings, 2 replies; 12+ messages in thread
From: Luis Chamberlain @ 2020-04-24  2:14 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: gregkh, akpm, josh, rishabhb, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  2:14   ` Luis Chamberlain
@ 2020-04-24  2:27     ` Jakub Kicinski
  2020-04-24  2:31       ` Luis Chamberlain
  2020-04-24  3:15     ` Stephen Rothwell
  1 sibling, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-04-24  2:27 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: gregkh, akpm, josh, rishabhb, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

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..

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  2:27     ` Jakub Kicinski
@ 2020-04-24  2:31       ` Luis Chamberlain
  0 siblings, 0 replies; 12+ messages in thread
From: Luis Chamberlain @ 2020-04-24  2:31 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: gregkh, akpm, josh, rishabhb, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  2:14   ` Luis Chamberlain
  2020-04-24  2:27     ` Jakub Kicinski
@ 2020-04-24  3:15     ` Stephen Rothwell
  2020-04-24  3:19       ` Luis Chamberlain
  1 sibling, 1 reply; 12+ messages in thread
From: Stephen Rothwell @ 2020-04-24  3:15 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Jakub Kicinski, gregkh, akpm, josh, rishabhb, maco, andy.gross,
	david.brown, bjorn.andersson, linux-wireless, keescook, shuah,
	mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel,
	zajec5, nbroeking, markivx, broonie, dmitry.torokhov, dwmw2,
	torvalds, Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7,
	dan.rue, brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

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).

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  3:15     ` Stephen Rothwell
@ 2020-04-24  3:19       ` Luis Chamberlain
  2020-04-24  4:08         ` Stephen Rothwell
  0 siblings, 1 reply; 12+ messages in thread
From: Luis Chamberlain @ 2020-04-24  3:19 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Jakub Kicinski, gregkh, akpm, josh, rishabhb, maco, andy.gross,
	david.brown, bjorn.andersson, linux-wireless, keescook, shuah,
	mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel,
	zajec5, nbroeking, markivx, broonie, dmitry.torokhov, dwmw2,
	torvalds, Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7,
	dan.rue, brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  3:19       ` Luis Chamberlain
@ 2020-04-24  4:08         ` Stephen Rothwell
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Rothwell @ 2020-04-24  4:08 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Jakub Kicinski, gregkh, akpm, josh, rishabhb, maco, andy.gross,
	david.brown, bjorn.andersson, linux-wireless, keescook, shuah,
	mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel,
	zajec5, nbroeking, markivx, broonie, dmitry.torokhov, dwmw2,
	torvalds, Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7,
	dan.rue, brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

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.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-23 20:31 [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace Luis R. Rodriguez
  2020-04-23 20:42 ` Randy Dunlap
  2020-04-24  1:05 ` Jakub Kicinski
@ 2020-04-24  6:57 ` Christoph Hellwig
  2020-04-24  9:21 ` Greg KH
  3 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2020-04-24  6:57 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: gregkh, akpm, josh, rishabhb, kubakici, maco, andy.gross,
	david.brown, bjorn.andersson, linux-wireless, keescook, shuah,
	mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel,
	zajec5, nbroeking, markivx, broonie, dmitry.torokhov, dwmw2,
	torvalds, Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7,
	dan.rue, brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-23 20:31 [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace Luis R. Rodriguez
                   ` (2 preceding siblings ...)
  2020-04-24  6:57 ` Christoph Hellwig
@ 2020-04-24  9:21 ` Greg KH
  2020-04-24 18:00   ` Luis Chamberlain
  3 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2020-04-24  9:21 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: akpm, josh, rishabhb, kubakici, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace
  2020-04-24  9:21 ` Greg KH
@ 2020-04-24 18:00   ` Luis Chamberlain
  0 siblings, 0 replies; 12+ messages in thread
From: Luis Chamberlain @ 2020-04-24 18:00 UTC (permalink / raw)
  To: Greg KH
  Cc: akpm, josh, rishabhb, kubakici, maco, andy.gross, david.brown,
	bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
	dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
	markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
	Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7, dan.rue,
	brendanhiggins, yzaikin, linux-kernel, linux-fsdevel,
	Christoph Hellwig, Randy Dunlap, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-04-24 18:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 20:31 [PATCH] firmware_loader: re-export fw_fallback_config into firmware_loader's own namespace Luis R. Rodriguez
2020-04-23 20:42 ` Randy Dunlap
2020-04-24  1:05 ` Jakub Kicinski
2020-04-24  2:14   ` Luis Chamberlain
2020-04-24  2:27     ` Jakub Kicinski
2020-04-24  2:31       ` Luis Chamberlain
2020-04-24  3:15     ` Stephen Rothwell
2020-04-24  3:19       ` Luis Chamberlain
2020-04-24  4:08         ` Stephen Rothwell
2020-04-24  6:57 ` Christoph Hellwig
2020-04-24  9:21 ` Greg KH
2020-04-24 18:00   ` Luis Chamberlain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).