Re: Possible bug in SCSI Kconfig

Re: Possible bug in SCSI Kconfig

[Stefan Richter]

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

Re: Possible bug in SCSI Kconfig

[Fabio Comolli]

Hi.

On Wed, Oct 22, 2008 at 8:35 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> 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

Re: Possible bug in SCSI Kconfig

[James Bottomley]

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

Re: Possible bug in SCSI Kconfig

[Fabio Comolli]

Hi.

On Wed, Oct 22, 2008 at 9:05 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> 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

Re: Possible bug in SCSI Kconfig

[Matthew Wilcox]

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

Re: Possible bug in SCSI Kconfig

[Fabio Comolli]

Hi.

On Wed, Oct 22, 2008 at 9:26 PM, Matthew Wilcox <matthew@wil.cx> 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

Re: Possible bug in SCSI Kconfig

[Matthew Wilcox]

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

Re: Possible bug in SCSI Kconfig

[Fabio Comolli]

Hi.

On Wed, Oct 22, 2008 at 9:34 PM, Matthew Wilcox <matthew@wil.cx> 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."
>

Re: Possible bug in SCSI Kconfig

[James Bottomley]

On Wed, 2008-10-22 at 21:25 +0200, Fabio Comolli wrote:
> Hi.
> 
> On Wed, Oct 22, 2008 at 9:05 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> 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

Re: Possible bug in SCSI Kconfig

[Fabio Comolli]

Hi.

On Wed, Oct 22, 2008 at 9:44 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> 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
>> <James.Bottomley@hansenpartnership.com> 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

>
>

Re: Possible bug in SCSI Kconfig

[Stefan Richter]

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/

Re: Possible bug in SCSI Kconfig

[Matthew Wilcox]

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

Re: Possible bug in SCSI Kconfig

[Stefan Richter]

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/