Re: [PATCH] staging: vme: Adjusted VME_USER in Kconfig

[Bruno]

On Fri, 2022-04-01 at 08:10 +0200, reg Kroah-Hartman wrote:
> On Fri, Apr 01, 2022 at 08:08:17AM +0200, reg Kroah-Hartman wrote:
> > On Fri, Apr 01, 2022 at 02:00:45AM -0300, Bruno Moreira-Guedes
> > wrote:
> > > 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 <codeagain@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/staging/Kconfig | 2 ++
> > >  drivers/vme/Kconfig     | 2 --
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> > > index 932acb4e8cbc..0545850eb2ff 100644
> > > --- a/drivers/staging/Kconfig
> > > +++ b/drivers/staging/Kconfig
> > > @@ -88,4 +88,6 @@ source "drivers/staging/qlge/Kconfig"
> > >  
> > >  source "drivers/staging/wfx/Kconfig"
> > >  
> > > +source "drivers/staging/vme/devices/Kconfig"
> > > +
> > >  endif # STAGING
> > > diff --git a/drivers/vme/Kconfig b/drivers/vme/Kconfig
> > > index 936392ca3c8c..c13dd9d2a604 100644
> > > --- a/drivers/vme/Kconfig
> > > +++ b/drivers/vme/Kconfig
> > > @@ -15,6 +15,4 @@ source "drivers/vme/bridges/Kconfig"
> > >  
> > >  source "drivers/vme/boards/Kconfig"
> > >  
> > > -source "drivers/staging/vme/devices/Kconfig"
> > > -
> > >  endif # VME
> > > -- 
> > > 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?
> 
> 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/

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

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

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