2008-10-22 16:29:20

by Fabio Comolli

[permalink] [raw]
Subject: Possible bug in SCSI Kconfig

Hi.
In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:

config SCSI_WAIT_SCAN
tristate
default m
depends on SCSI
depends on MODULES

The tristate field is empty. This has the effect that this option is
not visible in menuconfig and so it's always selected. The default is
"m" for all architectures and so this module is always compiled if
SCSI and MODULES are both enabled.

I'm using a patch like this one:

config SCSI_WAIT_SCAN
- tristate
+ tristate "Wait until all the async scans are complete"
default m
depends on SCSI
depends on MODULES

to get rid of that module.

Of course, I have no idea if this is correct or the current behavior
is the expected one.

Regards,
Fabio


2008-10-22 18:36:46

by Stefan Richter

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

(adding Cc: LSML and author)

On 10/22/2008, Fabio Comolli wrote at LKML:
> Hi.
> In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:
>
> config SCSI_WAIT_SCAN
> tristate
> default m
> depends on SCSI
> depends on MODULES
>
> The tristate field is empty. This has the effect that this option is
> not visible in menuconfig and so it's always selected. The default is
> "m" for all architectures and so this module is always compiled if
> SCSI and MODULES are both enabled.
>
> I'm using a patch like this one:
>
> config SCSI_WAIT_SCAN
> - tristate
> + tristate "Wait until all the async scans are complete"
> default m
> depends on SCSI
> depends on MODULES
>
> to get rid of that module.
>
> Of course, I have no idea if this is correct or the current behavior
> is the expected one.
>
> Regards,
> Fabio

What's the correct behaviour is contentious. There have been complaints
that it shouldn't be built if it is not needed. However, how it
currently works is how those who added and merged that feature thought
that it should be.

Notes on your suggestion:

- If you make it a visible prompt, you should also add a help text.
No Kconfig prompts without good help text, please!

- The suggested prompt text doesn't describe the matter too well.
scsi_wait_scan rather is a module which userland can use to get
a signal for when scans (by some but not all transports) are done.
(The signal is the end of module initialization of the
scsi_wait_scan module.) I.e. the kernel as a whole doesn't
necessarily wait, just this module does.

You could use the comment in drivers/scsi/scsi_wait_scan.c and the
changelog of
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3e082a910d217b2e7b186077ebf5a1126a68c62f
as a basis for the Kconfig help text. You could actually add a help
text even to invisible prompts, just for documentation purposes.
--
Stefan Richter
-=====-==--- =-=- =-==-
http://arcgraph.de/sr/

2008-10-22 19:11:49

by James Bottomley

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

On Wed, 2008-10-22 at 18:29 +0200, Fabio Comolli wrote:
> Hi.
> In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:
>
> config SCSI_WAIT_SCAN
> tristate
> default m
> depends on SCSI
> depends on MODULES
>
> The tristate field is empty. This has the effect that this option is
> not visible in menuconfig and so it's always selected. The default is
> "m" for all architectures and so this module is always compiled if
> SCSI and MODULES are both enabled.
>
> I'm using a patch like this one:
>
> config SCSI_WAIT_SCAN
> - tristate
> + tristate "Wait until all the async scans are complete"
> default m
> depends on SCSI
> depends on MODULES
>
> to get rid of that module.
>
> Of course, I have no idea if this is correct or the current behavior
> is the expected one.

The point of all of this is that if you enable async scanning, you need
a method of waiting for the scans to complete, which is all this module
does (you insert it and it doesn't come back from the insertion until
all the pending async scans are complete). It's tristate because really
it only makes sense to be M or N. The consensus is that, given async
scanning is always an option, this function should always be built as a
module *if* modules and SCSI are enabled ... because you might have a
HBA module you enabled async scanning for and you need to wait.

There's really not much point giving the user the choice, since this
module is part of the initrd sequence ... it's not really anything a
user would want to use stand alone, hence there's no point giving a
choice about it.

James

2008-10-22 19:25:59

by Fabio Comolli

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

Hi.

On Wed, Oct 22, 2008 at 9:05 PM, James Bottomley
<[email protected]> wrote:
> On Wed, 2008-10-22 at 18:29 +0200, Fabio Comolli wrote:
>> Hi.
>> In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:
>>
>> config SCSI_WAIT_SCAN
>> tristate
>> default m
>> depends on SCSI
>> depends on MODULES
>>
>> The tristate field is empty. This has the effect that this option is
>> not visible in menuconfig and so it's always selected. The default is
>> "m" for all architectures and so this module is always compiled if
>> SCSI and MODULES are both enabled.
>>
>> I'm using a patch like this one:
>>
>> config SCSI_WAIT_SCAN
>> - tristate
>> + tristate "Wait until all the async scans are complete"
>> default m
>> depends on SCSI
>> depends on MODULES
>>
>> to get rid of that module.
>>
>> Of course, I have no idea if this is correct or the current behavior
>> is the expected one.
>
> The point of all of this is that if you enable async scanning, you need
> a method of waiting for the scans to complete, which is all this module
> does (you insert it and it doesn't come back from the insertion until
> all the pending async scans are complete). It's tristate because really
> it only makes sense to be M or N. The consensus is that, given async
> scanning is always an option, this function should always be built as a
> module *if* modules and SCSI are enabled ... because you might have a
> HBA module you enabled async scanning for and you need to wait.

OK, I see your point.

But in my case I have SCSI enabled only because it's SELECTed by ATA
(it's just a laptop) and I don't have any hba's and never will. So no
async scan.

Maybe this module should be enabled if async scan is.

>
> There's really not much point giving the user the choice, since this
> module is part of the initrd sequence ... it's not really anything a
> user would want to use stand alone, hence there's no point giving a
> choice about it.
>

I don't use initrd at all. I just noticed the existence of this module
while trying to have a non modular kernel. This failed because of
ipw2200 and after having enabled modules again I just found this
scsi_wait_scan.ko.

> James
>
>
>

Regards,
Fabio

2008-10-22 19:27:37

by Matthew Wilcox

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

On Wed, Oct 22, 2008 at 09:25:46PM +0200, Fabio Comolli wrote:
> I don't use initrd at all. I just noticed the existence of this module
> while trying to have a non modular kernel. This failed because of
> ipw2200 and after having enabled modules again I just found this
> scsi_wait_scan.ko.

If you turn off CONFIG_MODULES, the module goes away.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2008-10-22 19:30:00

by Fabio Comolli

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

Hi.

On Wed, Oct 22, 2008 at 8:35 PM, Stefan Richter
<[email protected]> wrote:
> (adding Cc: LSML and author)
>
> On 10/22/2008, Fabio Comolli wrote at LKML:
>> Hi.
>> In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:
>>
>> config SCSI_WAIT_SCAN
>> tristate
>> default m
>> depends on SCSI
>> depends on MODULES
>>
>> The tristate field is empty. This has the effect that this option is
>> not visible in menuconfig and so it's always selected. The default is
>> "m" for all architectures and so this module is always compiled if
>> SCSI and MODULES are both enabled.
>>
>> I'm using a patch like this one:
>>
>> config SCSI_WAIT_SCAN
>> - tristate
>> + tristate "Wait until all the async scans are complete"
>> default m
>> depends on SCSI
>> depends on MODULES
>>
>> to get rid of that module.
>>
>> Of course, I have no idea if this is correct or the current behavior
>> is the expected one.
>>
>> Regards,
>> Fabio
>
> What's the correct behaviour is contentious. There have been complaints
> that it shouldn't be built if it is not needed. However, how it
> currently works is how those who added and merged that feature thought
> that it should be.
>
> Notes on your suggestion:
>
> - If you make it a visible prompt, you should also add a help text.
> No Kconfig prompts without good help text, please!
>
> - The suggested prompt text doesn't describe the matter too well.
> scsi_wait_scan rather is a module which userland can use to get
> a signal for when scans (by some but not all transports) are done.
> (The signal is the end of module initialization of the
> scsi_wait_scan module.) I.e. the kernel as a whole doesn't
> necessarily wait, just this module does.
>

Well, I didn't mean to have the patch merged. My point was just to
understand if the Kconfig was correct.

> You could use the comment in drivers/scsi/scsi_wait_scan.c and the
> changelog of
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3e082a910d217b2e7b186077ebf5a1126a68c62f
> as a basis for the Kconfig help text. You could actually add a help
> text even to invisible prompts, just for documentation purposes.

Well, the first line of comment in that file reads "This is a simple
module to wait until all the async scans are complete." More or less
the text I added to the tristate.

> --
> Stefan Richter
> -=====-==--- =-=- =-==-
> http://arcgraph.de/sr/
>

Regards,
Fabio

2008-10-22 19:32:10

by Fabio Comolli

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

Hi.

On Wed, Oct 22, 2008 at 9:26 PM, Matthew Wilcox <[email protected]> wrote:
> On Wed, Oct 22, 2008 at 09:25:46PM +0200, Fabio Comolli wrote:
>> I don't use initrd at all. I just noticed the existence of this module
>> while trying to have a non modular kernel. This failed because of
>> ipw2200 and after having enabled modules again I just found this
>> scsi_wait_scan.ko.
>
> If you turn off CONFIG_MODULES, the module goes away.

Unfortunately this is not an option because of ipw2200. I don't use
initrd and if I turn off CONFIG_MODULES the kernel hangs forever
requesting the firmware.

>
> --
> Matthew Wilcox Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. We can't possibly take such
> a retrograde step."
>

Regards,
Fabio

2008-10-22 19:35:17

by Matthew Wilcox

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

On Wed, Oct 22, 2008 at 09:31:51PM +0200, Fabio Comolli wrote:
> Unfortunately this is not an option because of ipw2200. I don't use
> initrd and if I turn off CONFIG_MODULES the kernel hangs forever
> requesting the firmware.

Have you investigated CONFIG_EXTRA_FIRMWARE?

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2008-10-22 19:44:38

by James Bottomley

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

On Wed, 2008-10-22 at 21:25 +0200, Fabio Comolli wrote:
> Hi.
>
> On Wed, Oct 22, 2008 at 9:05 PM, James Bottomley
> <[email protected]> wrote:
> > On Wed, 2008-10-22 at 18:29 +0200, Fabio Comolli wrote:
> >> Hi.
> >> In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:
> >>
> >> config SCSI_WAIT_SCAN
> >> tristate
> >> default m
> >> depends on SCSI
> >> depends on MODULES
> >>
> >> The tristate field is empty. This has the effect that this option is
> >> not visible in menuconfig and so it's always selected. The default is
> >> "m" for all architectures and so this module is always compiled if
> >> SCSI and MODULES are both enabled.
> >>
> >> I'm using a patch like this one:
> >>
> >> config SCSI_WAIT_SCAN
> >> - tristate
> >> + tristate "Wait until all the async scans are complete"
> >> default m
> >> depends on SCSI
> >> depends on MODULES
> >>
> >> to get rid of that module.
> >>
> >> Of course, I have no idea if this is correct or the current behavior
> >> is the expected one.
> >
> > The point of all of this is that if you enable async scanning, you need
> > a method of waiting for the scans to complete, which is all this module
> > does (you insert it and it doesn't come back from the insertion until
> > all the pending async scans are complete). It's tristate because really
> > it only makes sense to be M or N. The consensus is that, given async
> > scanning is always an option, this function should always be built as a
> > module *if* modules and SCSI are enabled ... because you might have a
> > HBA module you enabled async scanning for and you need to wait.
>
> OK, I see your point.
>
> But in my case I have SCSI enabled only because it's SELECTed by ATA
> (it's just a laptop) and I don't have any hba's and never will. So no
> async scan.

Presumably you have a laptop hard disk. That's currently SCSI if you
use ATA ... although when ATA moves out of SCSI it will no longer be, so
you could regard this as a temporary condition.

> Maybe this module should be enabled if async scan is.

But that's the point: async scan is always "enabled" it just might not
be the default, that's what the CONFIG_SCSI_SCAN_ASYNC controls: the
default value (but you can always turn it on with the kernel boot or
SCSI module option). Async scan is also functional for /drivers/ata and
other hot plug type busses, so it still makes sense for them as well.

> > There's really not much point giving the user the choice, since this
> > module is part of the initrd sequence ... it's not really anything a
> > user would want to use stand alone, hence there's no point giving a
> > choice about it.
> >
>
> I don't use initrd at all. I just noticed the existence of this module
> while trying to have a non modular kernel. This failed because of
> ipw2200 and after having enabled modules again I just found this
> scsi_wait_scan.ko.

Well, we do use CONFIG_MODULES to try to discern whether the user wants
modules or not. However, if you enable modules, we'd need some
telepathic configurator to tell us if you plan to use SCSI HBA modules
or not, so the safest course is always to enable it. The real point is
that you have to be a real power user to answer N correctly to this, so
it's better not to confuse the remaining 97% with the option because
they could easily get wrong ... the 3% who're sure they don't want it
can simply delete it.

James

2008-10-22 19:45:30

by Fabio Comolli

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

Hi.

On Wed, Oct 22, 2008 at 9:34 PM, Matthew Wilcox <[email protected]> wrote:
> On Wed, Oct 22, 2008 at 09:31:51PM +0200, Fabio Comolli wrote:
>> Unfortunately this is not an option because of ipw2200. I don't use
>> initrd and if I turn off CONFIG_MODULES the kernel hangs forever
>> requesting the firmware.
>
> Have you investigated CONFIG_EXTRA_FIRMWARE?

Wow! You made my day:

ipw2200: Intel(R) PRO/Wireless 2200/2915 Network Driver, 1.2.2kmr
ipw2200: Copyright(c) 2003-2006 Intel Corporation
ipw2200 0000:06:05.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20
ipw2200: Detected Intel PRO/Wireless 2200BG Network Connection
firmware: using built-in firmware ipw2200-bss.fw

Thanks!

>
> --
> Matthew Wilcox Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. We can't possibly take such
> a retrograde step."
>

2008-10-22 19:49:19

by Fabio Comolli

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

Hi.

On Wed, Oct 22, 2008 at 9:44 PM, James Bottomley
<[email protected]> wrote:
> On Wed, 2008-10-22 at 21:25 +0200, Fabio Comolli wrote:
>> Hi.
>>
>> On Wed, Oct 22, 2008 at 9:05 PM, James Bottomley
>> <[email protected]> wrote:
>> > On Wed, 2008-10-22 at 18:29 +0200, Fabio Comolli wrote:
>> >> Hi.
>> >> In kernel 2.6.27.2 - drivers/scsi/Kconfig we have:
>> >>
>> >> config SCSI_WAIT_SCAN
>> >> tristate
>> >> default m
>> >> depends on SCSI
>> >> depends on MODULES
>> >>
>> >> The tristate field is empty. This has the effect that this option is
>> >> not visible in menuconfig and so it's always selected. The default is
>> >> "m" for all architectures and so this module is always compiled if
>> >> SCSI and MODULES are both enabled.
>> >>
>> >> I'm using a patch like this one:
>> >>
>> >> config SCSI_WAIT_SCAN
>> >> - tristate
>> >> + tristate "Wait until all the async scans are complete"
>> >> default m
>> >> depends on SCSI
>> >> depends on MODULES
>> >>
>> >> to get rid of that module.
>> >>
>> >> Of course, I have no idea if this is correct or the current behavior
>> >> is the expected one.
>> >
>> > The point of all of this is that if you enable async scanning, you need
>> > a method of waiting for the scans to complete, which is all this module
>> > does (you insert it and it doesn't come back from the insertion until
>> > all the pending async scans are complete). It's tristate because really
>> > it only makes sense to be M or N. The consensus is that, given async
>> > scanning is always an option, this function should always be built as a
>> > module *if* modules and SCSI are enabled ... because you might have a
>> > HBA module you enabled async scanning for and you need to wait.
>>
>> OK, I see your point.
>>
>> But in my case I have SCSI enabled only because it's SELECTed by ATA
>> (it's just a laptop) and I don't have any hba's and never will. So no
>> async scan.
>
> Presumably you have a laptop hard disk. That's currently SCSI if you
> use ATA ... although when ATA moves out of SCSI it will no longer be, so
> you could regard this as a temporary condition.

Ok.

>
>> Maybe this module should be enabled if async scan is.
>
> But that's the point: async scan is always "enabled" it just might not
> be the default, that's what the CONFIG_SCSI_SCAN_ASYNC controls: the
> default value (but you can always turn it on with the kernel boot or
> SCSI module option). Async scan is also functional for /drivers/ata and
> other hot plug type busses, so it still makes sense for them as well.
>

I see the point.

>> > There's really not much point giving the user the choice, since this
>> > module is part of the initrd sequence ... it's not really anything a
>> > user would want to use stand alone, hence there's no point giving a
>> > choice about it.
>> >
>>
>> I don't use initrd at all. I just noticed the existence of this module
>> while trying to have a non modular kernel. This failed because of
>> ipw2200 and after having enabled modules again I just found this
>> scsi_wait_scan.ko.
>
> Well, we do use CONFIG_MODULES to try to discern whether the user wants
> modules or not. However, if you enable modules, we'd need some
> telepathic configurator to tell us if you plan to use SCSI HBA modules
> or not, so the safest course is always to enable it. The real point is
> that you have to be a real power user to answer N correctly to this, so
> it's better not to confuse the remaining 97% with the option because
> they could easily get wrong ... the 3% who're sure they don't want it
> can simply delete it.
>

Thanks for your explanation.

> James
>

Regards,
Fabio

>
>

2008-10-23 17:31:06

by Stefan Richter

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

James Bottomley wrote:
> On Wed, 2008-10-22 at 21:25 +0200, Fabio Comolli wrote:
>> But in my case I have SCSI enabled only because it's SELECTed by ATA
>> (it's just a laptop) and I don't have any hba's and never will. So no
>> async scan.
>
> Presumably you have a laptop hard disk. That's currently SCSI if you
> use ATA ... although when ATA moves out of SCSI it will no longer be, so
> you could regard this as a temporary condition.

What USB storage? And more.

>> Maybe this module should be enabled if async scan is.
>
> But that's the point: async scan is always "enabled" it just might not
> be the default, that's what the CONFIG_SCSI_SCAN_ASYNC controls: the
> default value (but you can always turn it on with the kernel boot or
> SCSI module option). Async scan is also functional for /drivers/ata and
> other hot plug type busses, so it still makes sense for them as well.
...
> Well, we do use CONFIG_MODULES to try to discern whether the user wants
> modules or not. However, if you enable modules, we'd need some
> telepathic configurator to tell us if you plan to use SCSI HBA modules
> or not, so the safest course is always to enable it. The real point is
> that you have to be a real power user to answer N correctly to this, so
> it's better not to confuse the remaining 97% with the option because
> they could easily get wrong ...

Is it remotely possible that SCSI_WAIT_SCAN could depend on or be
selected by transports which actually cooperate with scsi_wait_scan?

Also remember:
- Writing "Depending your initrd, this module may be necessary for
the system to boot up." and "If unsure, say M." in the Kconfig help
text is always an option.
- I haven't built an initrd since ages, but I presume that initrd
build scripts will complain if an expected module is missing.
- scsi_wait_scan is a special method to wait for devices during boot
which only works with some hardware types. More general methods are
available and have been in use far longer than scsi_wait_scan
exists. Nobody fundamentally needs scsi_wait_scan, it is only a
convenient tool for some users who choose to use it.
Special features are traditionally per default off in Kconfig and
are supposed to be actively enabled.

The sentence "this module may be necessary for the system to boot up" is
actually applicable to a myriad of options which have a prompt and
default to off. The only difference is that most of these options are
easier to explain. And that's what it boils down to: You omitted the
prompt because you felt it was too hard to explain.

> the 3% who're sure they don't want it can simply delete it.

Yeah, or remove the line from .config, or... change scsi/Kconfig to get
a prompt.
--
Stefan Richter
-=====-==--- =-=- =-===
http://arcgraph.de/sr/

2008-10-23 17:39:34

by Matthew Wilcox

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

On Thu, Oct 23, 2008 at 07:30:33PM +0200, Stefan Richter wrote:
> - scsi_wait_scan is a special method to wait for devices during boot
> which only works with some hardware types. More general methods are
> available and have been in use far longer than scsi_wait_scan
> exists. Nobody fundamentally needs scsi_wait_scan, it is only a
> convenient tool for some users who choose to use it.
> Special features are traditionally per default off in Kconfig and
> are supposed to be actively enabled.

What 'general methods' would those be?

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2008-10-23 19:00:21

by Stefan Richter

[permalink] [raw]
Subject: Re: Possible bug in SCSI Kconfig

Matthew Wilcox wrote:
> On Thu, Oct 23, 2008 at 07:30:33PM +0200, Stefan Richter wrote:
>> - scsi_wait_scan is a special method to wait for devices during boot
>> which only works with some hardware types. More general methods are
>> available and have been in use far longer than scsi_wait_scan
>> exists. Nobody fundamentally needs scsi_wait_scan, it is only a
>> convenient tool for some users who choose to use it.
>> Special features are traditionally per default off in Kconfig and
>> are supposed to be actively enabled.
>
> What 'general methods' would those be?

Waiting for the desired numbers of devices to show up. Or for a known
device.

scsi_wait_scan is optimal though if it is known at the outset how many
devices (or how many at maximum) will show up.
--
Stefan Richter
-=====-==--- =-=- =-===
http://arcgraph.de/sr/