staging: vme: Adjusted VME_USER in Kconfig

PATCH Superseded Bruno Moreira-Guedes 2 participants 8 messages Apr 1, 2022 – Apr 12, 2022 Staging

Share & tools

Bruno Moreira-Guedes

author

2022-04-01 05:00 UTC #0 lore raw

PATCH 1/1 staging: vme: Adjusted VME_USER in Kconfig

Currently, the VME_USER driver is in the staging tree Kconfig, unlike other VME drivers already moved to the main portions of the kernel tree. Its configuration is, however, nested into the VME_BUS config option, which might be misleading.

Since the staging tree "[...] is used to hold stand-alone[1] drivers and filesystem that are not ready to be merged into the main portion of the Linux kernel tree [...]"[1], IMHO all staging drivers should appear nested into the Main Menu -> Device Drivers -> Staging Drivers to make sure the user don't pick it without being fully aware of its staging status as it could be the case in Menu -> Device Drivers -> VME bridge support (the current location).

With this change menuconfig users will clearly know this is not a driver in the main portion of the kernel tree and decide whether to build it or not with that clearly in mind.

This change goes into the same direction of commit 4b4cdf3979c3 ("STAGING: Move staging drivers back to staging-specific menu")

[1] https://lkml.org/lkml/2009/3/18/314

Signed-off-by Bruno Moreira-Guedes <[email protected]>

---
 drivers/staging/Kconfig | 2 ++
 drivers/vme/Kconfig     | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/staging/Kconfig +2 -0

index 932acb4e8cbc..0545850eb2ff 100644
@@ -88,4 +88,6 @@ source "drivers/staging/qlge/Kconfig"
 
 source "drivers/staging/wfx/Kconfig"
 
+source "drivers/staging/vme/devices/Kconfig"
+
 endif # STAGING

drivers/vme/Kconfig +0 -2

index 936392ca3c8c..c13dd9d2a604 100644
@@ -15,6 +15,4 @@ source "drivers/vme/bridges/Kconfig"
 
 source "drivers/vme/boards/Kconfig"
 
-source "drivers/staging/vme/devices/Kconfig"
-
 endif # VME

Signature · 1 line

2.35.1

reg Kroah-Hartman

maintainer

2022-04-01 06:08 UTC #1 lore raw

Quoted 54 lines · Bruno Moreira-Guedes

Currently, the VME_USER driver is in the staging tree Kconfig, unlike other VME drivers already moved to the main portions of the kernel tree. Its configuration is, however, nested into the VME_BUS config option, which might be misleading.
Since the staging tree "[...] is used to hold stand-alone[1] drivers and filesystem that are not ready to be merged into the main portion of the Linux kernel tree [...]"[1], IMHO all staging drivers should appear nested into the Main Menu -> Device Drivers -> Staging Drivers to make sure the user don't pick it without being fully aware of its staging status as it could be the case in Menu -> Device Drivers -> VME bridge support (the current location).
With this change menuconfig users will clearly know this is not a driver in the main portion of the kernel tree and decide whether to build it or not with that clearly in mind.
This change goes into the same direction of commit 4b4cdf3979c3 ("STAGING: Move staging drivers back to staging-specific menu")
[1] https://lkml.org/lkml/2009/3/18/314
Signed-off-by Bruno Moreira-Guedes <[email protected]>
---
 drivers/staging/Kconfig | 2 ++
 drivers/vme/Kconfig     | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/staging/Kconfig +2 -0
index 932acb4e8cbc..0545850eb2ff 100644
@@ -88,4 +88,6 @@ source "drivers/staging/qlge/Kconfig"
 
 source "drivers/staging/wfx/Kconfig"
 
+source "drivers/staging/vme/devices/Kconfig"
+
 endif # STAGING
drivers/vme/Kconfig +0 -2
index 936392ca3c8c..c13dd9d2a604 100644
@@ -15,6 +15,4 @@ source "drivers/vme/bridges/Kconfig"
 
 source "drivers/vme/boards/Kconfig"
 
-source "drivers/staging/vme/devices/Kconfig"
-
 endif # VME
Signature · 1 line
2.35.1

The problem with this change is that you just changed the initialization order of the drivers if they are built into the kernel. Are you sure that you can initialize a vme device driver before the vme bridge and bus code is run? I don't know if that will work properly, which is why the Kconfig entries are in the order they currently are in (we preserved the link order.)

It's not an obvious thing at all, sorry, but build order defines link order, which defines the order in which things are initialized in the kernel.

So I can't take this change unless you are able to prove that it still works properly on the hardware that these drivers control. Do you have this hardware to test this change with?

thanks,

greg k-h

reg Kroah-Hartman

maintainer

2022-04-01 06:10 UTC #2 lore raw

Quoted 63 lines · reg Kroah-Hartman

Quoted 54 lines · Bruno Moreira-Guedes
Currently, the VME_USER driver is in the staging tree Kconfig, unlike other VME drivers already moved to the main portions of the kernel tree. Its configuration is, however, nested into the VME_BUS config option, which might be misleading.
Since the staging tree "[...] is used to hold stand-alone[1] drivers and filesystem that are not ready to be merged into the main portion of the Linux kernel tree [...]"[1], IMHO all staging drivers should appear nested into the Main Menu -> Device Drivers -> Staging Drivers to make sure the user don't pick it without being fully aware of its staging status as it could be the case in Menu -> Device Drivers -> VME bridge support (the current location).
With this change menuconfig users will clearly know this is not a driver in the main portion of the kernel tree and decide whether to build it or not with that clearly in mind.
This change goes into the same direction of commit 4b4cdf3979c3 ("STAGING: Move staging drivers back to staging-specific menu")
[1] https://lkml.org/lkml/2009/3/18/314
Signed-off-by Bruno Moreira-Guedes <[email protected]>
---
 drivers/staging/Kconfig | 2 ++
 drivers/vme/Kconfig     | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/staging/Kconfig +2 -0
index 932acb4e8cbc..0545850eb2ff 100644
@@ -88,4 +88,6 @@ source "drivers/staging/qlge/Kconfig"
 
 source "drivers/staging/wfx/Kconfig"
 
+source "drivers/staging/vme/devices/Kconfig"
+
 endif # STAGING
drivers/vme/Kconfig +0 -2
index 936392ca3c8c..c13dd9d2a604 100644
@@ -15,6 +15,4 @@ source "drivers/vme/bridges/Kconfig"
 
 source "drivers/vme/boards/Kconfig"
 
-source "drivers/staging/vme/devices/Kconfig"
-
 endif # VME
Signature · 1 line
2.35.1
The problem with this change is that you just changed the initialization order of the drivers if they are built into the kernel. Are you sure that you can initialize a vme device driver before the vme bridge and bus code is run? I don't know if that will work properly, which is why the Kconfig entries are in the order they currently are in (we preserved the link order.)

Quoted 7 lines

It's not an obvious thing at all, sorry, but build order defines link order, which defines the order in which things are initialized in the kernel.
So I can't take this change unless you are able to prove that it still works properly on the hardware that these drivers control. Do you have this hardware to test this change with?

Oh wait, it's the Makefile order that controls this, not the Kconfig order. Sorry for the noise here, it's still early...

So this change _should_ be fine, but it would be good if you could prove it still works with some build tests. How did you test this change?

thanks,

greg k-h

Bruno

author

2022-04-01 18:27 UTC #3 lore raw

Quoted 87 lines · reg Kroah-Hartman

Quoted 86 lines · reg Kroah-Hartman
Quoted 60 lines · Bruno Moreira-Guedes
Currently, the VME_USER driver is in the staging tree Kconfig, unlike
other VME drivers already moved to the main portions of the kernel tree.
Its configuration is, however, nested into the VME_BUS config option,
which might be misleading.
Since the staging tree "[...] is used to hold stand-alone[1] drivers and
filesystem that are not ready to be merged into the main portion of the
Linux kernel tree [...]"[1], IMHO all staging drivers should appear
nested into the Main Menu -> Device Drivers -> Staging Drivers to make
sure the user don't pick it without being fully aware of its staging
status as it could be the case in Menu -> Device Drivers -> VME bridge
support (the current location).
With this change menuconfig users will clearly know this is not a driver
in the main portion of the kernel tree and decide whether to build it or
not with that clearly in mind.
This change goes into the same direction of commit 4b4cdf3979c3 ("STAGING: Move staging drivers back to staging-specific menu")
[1] https://lkml.org/lkml/2009/3/18/314
Signed-off-by Bruno Moreira-Guedes <[email protected]>
---
drivers/staging/Kconfig | 2 ++
drivers/vme/Kconfig | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/staging/Kconfig +0 -0
index 932acb4e8cbc..0545850eb2ff 100644
@@ -88,4 +88,6 @@ source "drivers/staging/qlge/Kconfig"
source "drivers/staging/wfx/Kconfig"
drivers/staging/Kconfig +2 -0
+source "drivers/staging/vme/devices/Kconfig"
+
endif # STAGING
drivers/vme/Kconfig +0 -0
index 936392ca3c8c..c13dd9d2a604 100644
@@ -15,6 +15,4 @@ source "drivers/vme/bridges/Kconfig"
source "drivers/vme/boards/Kconfig"
drivers/vme/Kconfig +0 -2
-source "drivers/staging/vme/devices/Kconfig"
-
Quoted 5 lines
endif # VME
Signature · 1 line
2.35.1
The problem with this change is that you just changed the initialization
order of the drivers if they are built into the kernel. Are you sure
that you can initialize a vme device driver before the vme bridge and
bus code is run? I don't know if that will work properly, which is why
the Kconfig entries are in the order they currently are in (we preserved
the link order.)
It's not an obvious thing at all, sorry, but build order defines link
order, which defines the order in which things are initialized in the
kernel.

Quoted 8 lines

Quoted 5 lines
So I can't take this change unless you are able to prove that it still
works properly on the hardware that these drivers control. Do you have
this hardware to test this change with?
Oh wait, it's the Makefile order that controls this, not the Kconfig order. Sorry for the noise here, it's still early...

No worries, your previous message was quite helpful to make realize some scenarios I wasn't considering at first. I did a more throrough inspection of how this patch impacts everything thanks to this observations.

I don't have the hardware so indeed I'm avoiding changes that would need it to be tested, and as far as I'm properly aware my patch just changes the places of things in the config targets. Build is protected from such changes through some Makefile validations such as in drivers/staging/Makefile:
| obj-$(CONFIG_VME_BUS) += vme/

Quoted 4 lines

So this change _should_ be fine, but it would be good if you could prove it still works with some build tests. How did you test this change?

At first I ran menuconfig and tested if it was still properly setting CONFIG_VME_USER. Then I built with CONFIG_VME_USER=m and CONFIG_VME_USER=n to check if it would build the module.

After your first e-mail I realized I didn't account for CONFIG_VME_USER=y on my tests. I have now successfuly built with this option too. Are those enough tests for this situation?

Quoted 5 lines

thanks,
greg k-h

With my tests in my, I have found two other things that I think are remarkable to mention. First one is a missing depends on line for VME_BRIDGE in drivers/staging/vme/devices/Kconfig, not visible because they were in the same tree, but now unveiled. I'm fixing it, do you think it's best to add it in the same patch?

Finally, not directly related with the patch, yet remarkable, I happened to notice something. When probing the vme_user module (compiled with CONFIG_VME_USER=m), I naturally get the following messages on my log and command output for modprobe vme_user:
| [177666.590400] vme_user: module is from the staging directory, the quality is unknown, you have been warned.

| [177666.601166] vme_user: VME User Space Access Driver
| [177666.602111] vme_user: No cards, skipping registration modprobe:
| ERROR: could not insert 'vme_user': No such device

While this is completely expected, the message about the code from staging directory does not appear when compiled with CONFIG_VME_USER=y, as shows a grep -i vme on the console log:

| [0.000000] Linux version 5.17.0lsa-t-vme_user=y-13483-gfeb94431c35c- dirty (bruno@AN5Bruno) (gcc (GCC) 11.2.0, GNU ld (GNU Binutils) 2.38) #7 SMP PREEMPT_DYNAMIC Fri Apr 1 14:33:16 -03 2022
| [1.974450] vme_user: VME User Space Access Driver
| [ 1.975405] vme_user: No cards, skipping registration

Do you think it would be interesting for a future patch to provide some output when drivers from the staging tree are present in the running kernel image?

Signature · 3 lines

Sincerely,
Bruno | Pronouns: they/them/theirs
IRC: CodeAgain

Greg Kroah-Hartman

maintainer

2022-04-03 11:05 UTC #4 lore raw

You sent this twice?

Anyway...

Quoted 5 lines · Bruno

With my tests in my, I have found two other things that I think are remarkable to mention. First one is a missing depends on line for VME_BRIDGE in drivers/staging/vme/devices/Kconfig, not visible because they were in the same tree, but now unveiled. I'm fixing it, do you think it's best to add it in the same patch?

Make that a second patch, and resend it as part of a patch series since your first patch here is gone from my queue.

Quoted 6 lines

Finally, not directly related with the patch, yet remarkable, I happened to notice something. When probing the vme_user module (compiled with CONFIG_VME_USER=m), I naturally get the following messages on my log and command output for modprobe vme_user:
| [177666.590400] vme_user: module is from the staging directory, the quality is unknown, you have been warned.

That is expected.

Quoted 3 lines

While this is completely expected, the message about the code from staging directory does not appear when compiled with CONFIG_VME_USER=y, as shows a grep -i vme on the console log:

That is because you built the driver into the tree, so there is nothing to cause the taint code to run as there is no module loader involved.

It's expected and works the same for all staging drivers. Try it yourself with a different one to verify this.

Quoted 9 lines

| [0.000000] Linux version 5.17.0lsa-t-vme_user=y-13483-gfeb94431c35c- dirty (bruno@AN5Bruno) (gcc (GCC) 11.2.0, GNU ld (GNU Binutils) 2.38) #7 SMP PREEMPT_DYNAMIC Fri Apr 1 14:33:16 -03 2022
| [1.974450] vme_user: VME User Space Access Driver
| [ 1.975405] vme_user: No cards, skipping registration
Do you think it would be interesting for a future patch to provide some output when drivers from the staging tree are present in the running kernel image?

If you can figure out how to do so, that would be interesting to see.

thanks,

greg k-h

Bruno Moreira-Guedes

author

2022-04-12 15:14 UTC #5 lore raw

Quoted 10 lines · Greg Kroah-Hartman

Quoted 5 lines · Bruno
With my tests in my, I have found two other things that I think are remarkable to mention. First one is a missing depends on line for VME_BRIDGE in drivers/staging/vme/devices/Kconfig, not visible because they were in the same tree, but now unveiled. I'm fixing it, do you think it's best to add it in the same patch?
Make that a second patch, and resend it as part of a patch series since your first patch here is gone from my queue.

This patch is already sent, so I'll trim most of this message to avoid duplicating the discussions. There's only one thing I'd like some input first, if you don't mind.

Quoted 5 lines

Quoted 3 lines
Do you think it would be interesting for a future patch to provide some output when drivers from the staging tree are present in the running kernel image?
If you can figure out how to do so, that would be interesting to see.

I think I might have figured out. In "include/modules.h" and "include/init.h" I happened to notice the driver initialization is handled by some macros. After some inspection through gcc -E and looking how they are defined, I figured out a scenario (when MODULE is not defined) where the module_init() macro is defined as (among other things) an inline initcall function that wraps the driver initialization function.

So I thought about implementing it by creating a -DSTAGING flag in drivers/staging/Makefile, and then using this macro to make an #ifdef STAGING to add a similar inline wrapping function, except that in this case the function makes a pr_warning() before calling the initialization function.

Do you think it would be a good way of solving that?

Quoted 4 lines

thanks,
greg k-h

Sincerely,
Bruno

Greg Kroah-Hartman

maintainer

2022-04-12 15:54 UTC #6 lore raw

Quoted 29 lines · Bruno Moreira-Guedes

Quoted 10 lines · Greg Kroah-Hartman
Quoted 5 lines · Bruno
With my tests in my, I have found two other things that I think are remarkable to mention. First one is a missing depends on line for VME_BRIDGE in drivers/staging/vme/devices/Kconfig, not visible because they were in the same tree, but now unveiled. I'm fixing it, do you think it's best to add it in the same patch?
Make that a second patch, and resend it as part of a patch series since your first patch here is gone from my queue.
This patch is already sent, so I'll trim most of this message to avoid duplicating the discussions. There's only one thing I'd like some input first, if you don't mind.
Quoted 5 lines
Quoted 3 lines
Do you think it would be interesting for a future patch to provide some output when drivers from the staging tree are present in the running kernel image?
If you can figure out how to do so, that would be interesting to see.
I think I might have figured out. In "include/modules.h" and "include/init.h" I happened to notice the driver initialization is handled by some macros. After some inspection through gcc -E and looking how they are defined, I figured out a scenario (when MODULE is not defined) where the module_init() macro is defined as (among other things) an inline initcall function that wraps the driver initialization function.

Quoted 7 lines

So I thought about implementing it by creating a -DSTAGING flag in drivers/staging/Makefile, and then using this macro to make an #ifdef STAGING to add a similar inline wrapping function, except that in this case the function makes a pr_warning() before calling the initialization function.
Do you think it would be a good way of solving that?

Yes, that would be a possible way, try it and see!

greg k-h

Bruno Moreira-Guedes

author

2022-04-01 19:26 UTC #7 lore raw

In reply to reg Kroah-Hartman

Quoted 87 lines · reg Kroah-Hartman

Quoted 86 lines · reg Kroah-Hartman
Quoted 60 lines · Bruno Moreira-Guedes
Currently, the VME_USER driver is in the staging tree Kconfig, unlike
other VME drivers already moved to the main portions of the kernel tree.
Its configuration is, however, nested into the VME_BUS config option,
which might be misleading.
Since the staging tree "[...] is used to hold stand-alone[1] drivers and
filesystem that are not ready to be merged into the main portion of the
Linux kernel tree [...]"[1], IMHO all staging drivers should appear
nested into the Main Menu -> Device Drivers -> Staging Drivers to make
sure the user don't pick it without being fully aware of its staging
status as it could be the case in Menu -> Device Drivers -> VME bridge
support (the current location).
With this change menuconfig users will clearly know this is not a driver
in the main portion of the kernel tree and decide whether to build it or
not with that clearly in mind.
This change goes into the same direction of commit 4b4cdf3979c3 ("STAGING: Move staging drivers back to staging-specific menu")
[1] https://lkml.org/lkml/2009/3/18/314
Signed-off-by Bruno Moreira-Guedes <[email protected]>
---
drivers/staging/Kconfig | 2 ++
drivers/vme/Kconfig | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/staging/Kconfig +0 -0
index 932acb4e8cbc..0545850eb2ff 100644
@@ -88,4 +88,6 @@ source "drivers/staging/qlge/Kconfig"
source "drivers/staging/wfx/Kconfig"
drivers/staging/Kconfig +2 -0
+source "drivers/staging/vme/devices/Kconfig"
+
endif # STAGING
drivers/vme/Kconfig +0 -0
index 936392ca3c8c..c13dd9d2a604 100644
@@ -15,6 +15,4 @@ source "drivers/vme/bridges/Kconfig"
source "drivers/vme/boards/Kconfig"
drivers/vme/Kconfig +0 -2
-source "drivers/staging/vme/devices/Kconfig"
-
Quoted 5 lines
endif # VME
Signature · 1 line
2.35.1
The problem with this change is that you just changed the initialization
order of the drivers if they are built into the kernel. Are you sure
that you can initialize a vme device driver before the vme bridge and
bus code is run? I don't know if that will work properly, which is why
the Kconfig entries are in the order they currently are in (we preserved
the link order.)
It's not an obvious thing at all, sorry, but build order defines link
order, which defines the order in which things are initialized in the
kernel.

Quoted 8 lines

Quoted 5 lines
So I can't take this change unless you are able to prove that it still
works properly on the hardware that these drivers control. Do you have
this hardware to test this change with?
Oh wait, it's the Makefile order that controls this, not the Kconfig order. Sorry for the noise here, it's still early...

No problem, your previous message was quite helpful to make realize some scenarios I wasn't considering at first. I did a more throrough inspection of how this patch impacts everything thanks to this observations.

Quoted 4 lines

So this change _should_ be fine, but it would be good if you could prove
it still works with some build tests. How did you test this change?

At first I ran menuconfig and tested if it was still properly setting CONFIG_VME_USER. Then I built with CONFIG_VME_USER=m and CONFIG_VME_USER=n to check if it would build the module.

After your first e-mail I realized I didn't account for CONFIG_VME_USER=y on my tests. I have now successfuly built with this option too. Are those enough tests for this situation?

Quoted 4 lines

thanks,
greg k-h

While testing and checking, I have found two other things that I think are remarkable to mention. First one is a missing depends on line for VME_BRIDGE in drivers/staging/vme/devices/Kconfig, not visible because they were in the same tree, but now unveiled. I'm fixing it, do you think it's best to add it in the same patch?

| [177666.590400] vme_user: module is from the staging directory, the quality is unknown, you have been warned.

| [177666.601166] vme_user: VME User Space Access Driver
| [177666.602111] vme_user: No cards, skipping registration modprobe:
| ERROR: could not insert 'vme_user': No such device

While this is completely expected, the message about the code from staging directory does not appear when compiled with CONFIG_VME_USER=y, as shows a grep -i vme on the console log:

Do you think it would be interesting for a future patch to provide some output when drivers from the staging tree are present in the running kernel image?

NOTE: This message was sent earlier but due to some deliverability issues didn't make to destinations. I apologize in advance if it ends up duplicating.

Signature · 3 lines

Sincerely,
Bruno | Pronouns: they/them/theirs
IRC: CodeAgain