[PATCH] libata: fix broken Kconfig setup

[PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

The patch described in commit faa725332f39329288f52b7f872ffda866ba5b09
>   [PATCH] SCSI_SATA has to be a tristate

causes the PCI quirk in drivers/pci/quirk.c (quirk_intel_ide_combined)
to disappear, unless CONFIG_SCSI_SATA==y.  This, in turn, causes all
manner of booting and interaction problems between the IDE driver and
libata.

CONFIG_SCSI_SATA is truly a boolean option, not a tristate.
Since the Kconfig dependencies are insufficient to describe this (2.4
had dep_mbool), we need to resort to 'if'.

This fix is twofold:
1) Fix IDE/libata conflict by ensuring that CONFIG_SCSI_SATA symbol
   always exists (rather than bothering with CONFIG_SCSI_SATA_MODULE).
2) Restore CONFIG_SCSI_SATA's rightful boolean status, and employ
   'if' to obtain the proper menu behavior.

Please apply, so that people cursed with PATA/SATA "combined mode"
can have a working configuration again.


diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -437,7 +437,7 @@ config SCSI_IN2000
 source "drivers/scsi/megaraid/Kconfig.megaraid"
 
 config SCSI_SATA
-	tristate "Serial ATA (SATA) support"
+	bool "Serial ATA (SATA) support"
 	depends on SCSI
 	help
 	  This driver family supports Serial ATA host controllers
@@ -445,9 +445,11 @@ config SCSI_SATA
 
 	  If unsure, say N.
 
+if SCSI_SATA
+
 config SCSI_SATA_AHCI
 	tristate "AHCI SATA support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for AHCI Serial ATA.
 
@@ -455,7 +457,7 @@ config SCSI_SATA_AHCI
 
 config SCSI_SATA_SVW
 	tristate "ServerWorks Frodo / Apple K2 SATA support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for Broadcom/Serverworks/Apple K2
 	  SATA support.
@@ -464,7 +466,7 @@ config SCSI_SATA_SVW
 
 config SCSI_ATA_PIIX
 	tristate "Intel PIIX/ICH SATA support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for ICH5 Serial ATA.
 	  If PATA support was enabled previously, this enables
@@ -474,7 +476,7 @@ config SCSI_ATA_PIIX
 
 config SCSI_SATA_MV
 	tristate "Marvell SATA support"
-	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	depends on SCSI && PCI && EXPERIMENTAL
 	help
 	  This option enables support for the Marvell Serial ATA family.
 	  Currently supports 88SX[56]0[48][01] chips.
@@ -483,7 +485,7 @@ config SCSI_SATA_MV
 
 config SCSI_SATA_NV
 	tristate "NVIDIA SATA support"
-	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	depends on SCSI && PCI && EXPERIMENTAL
 	help
 	  This option enables support for NVIDIA Serial ATA.
 
@@ -491,7 +493,7 @@ config SCSI_SATA_NV
 
 config SCSI_SATA_PROMISE
 	tristate "Promise SATA TX2/TX4 support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for Promise Serial ATA TX2/TX4.
 
@@ -499,7 +501,7 @@ config SCSI_SATA_PROMISE
 
 config SCSI_SATA_QSTOR
 	tristate "Pacific Digital SATA QStor support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for Pacific Digital Serial ATA QStor.
 
@@ -507,7 +509,7 @@ config SCSI_SATA_QSTOR
 
 config SCSI_SATA_SX4
 	tristate "Promise SATA SX4 support"
-	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	depends on SCSI && PCI && EXPERIMENTAL
 	help
 	  This option enables support for Promise Serial ATA SX4.
 
@@ -515,7 +517,7 @@ config SCSI_SATA_SX4
 
 config SCSI_SATA_SIL
 	tristate "Silicon Image SATA support"
-	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	depends on SCSI && PCI && EXPERIMENTAL
 	help
 	  This option enables support for Silicon Image Serial ATA.
 
@@ -523,7 +525,7 @@ config SCSI_SATA_SIL
 
 config SCSI_SATA_SIS
 	tristate "SiS 964/180 SATA support"
-	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	depends on SCSI && PCI && EXPERIMENTAL
 	help
 	  This option enables support for SiS Serial ATA 964/180.
 
@@ -531,7 +533,7 @@ config SCSI_SATA_SIS
 
 config SCSI_SATA_ULI
 	tristate "ULi Electronics SATA support"
-	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	depends on SCSI && PCI && EXPERIMENTAL
 	help
 	  This option enables support for ULi Electronics SATA.
 
@@ -539,7 +541,7 @@ config SCSI_SATA_ULI
 
 config SCSI_SATA_VIA
 	tristate "VIA SATA support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for VIA Serial ATA.
 
@@ -547,12 +549,14 @@ config SCSI_SATA_VIA
 
 config SCSI_SATA_VITESSE
 	tristate "VITESSE VSC-7174 SATA support"
-	depends on SCSI_SATA && PCI
+	depends on SCSI && PCI
 	help
 	  This option enables support for Vitesse VSC7174 Serial ATA.
 
 	  If unsure, say N.
 
+endif	# if SCSI_SATA
+
 config SCSI_BUSLOGIC
 	tristate "BusLogic SCSI support"
 	depends on (PCI || ISA || MCA) && SCSI && ISA_DMA_API

Re: [PATCH] libata: fix broken Kconfig setup

[Matthias Urlichs]

Hi, Jeff Garzik wrote:

>  config SCSI_SATA
> -	tristate "Serial ATA (SATA) support"
> +	bool "Serial ATA (SATA) support"
>	depends on SCSI

In other words, if SCSI is false then SCSI_SATA is false too.

So why are you doing

> +if SCSI_SATA
> +
>  config SCSI_SATA_AHCI
>  	tristate "AHCI SATA support"
> -	depends on SCSI_SATA && PCI
> +	depends on SCSI && PCI

and not just
  +     depends on PCI

?

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
If you go out of your mind, do it quietly, so as not to disturb those
around you.

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Matthias Urlichs wrote:

Please use the standard 'group reply' feature of your mailer, to ensure 
that me and other people in the thread are CC'd.


> Hi, Jeff Garzik wrote:
> 
> 
>> config SCSI_SATA
>>-	tristate "Serial ATA (SATA) support"
>>+	bool "Serial ATA (SATA) support"
>>	depends on SCSI
> 
> 
> In other words, if SCSI is false then SCSI_SATA is false too.
> 
> So why are you doing
> 
> 
>>+if SCSI_SATA
>>+
>> config SCSI_SATA_AHCI
>> 	tristate "AHCI SATA support"
>>-	depends on SCSI_SATA && PCI
>>+	depends on SCSI && PCI
> 
> 
> and not just
>   +     depends on PCI
> 
> ?

Because if SCSI==m, then a low-level driver cannot be ==y.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Linus Torvalds]

On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
> CONFIG_SCSI_SATA is truly a boolean option, not a tristate.
> Since the Kconfig dependencies are insufficient to describe this (2.4
> had dep_mbool), we need to resort to 'if'.

Two problems:

 - this is ugly as hell

   First, you change SCSI_SATA to be boolean, then you change the 
   depends-on to be "if". Which makes no sense. Once SCSI_SATA is a 
   boolean, then the "depends on" works fine, since SCSI_SATA can no 
   longer be "m" anyway (ie your comment about "dep_mbool" doesn't make 
   sense).

   Second, if you want "dep_mbool", then you can have dep_mbool in Kconfig 
   too. Just do

	depends on (XYZ != n)

   which gets you what you want (ie if XYZ is "m", then the inequality 
   will be "y")

   So you might as well have just done something like

	 config SCSI_SATA
	-       tristate "Serial ATA (SATA) support"
	-       depends on SCSI
	+       bool "Serial ATA (SATA) support"
	+       depends on SCSI != n

   and then just added SCSI to the "depends on" lines to get the "m" 
   config. No "if" needed.

 - anything that depends on a module had better only be _inside_ that 
   module. Ie the "dep_mbool" kind of behaviour should _not_ affect 
   anything outside the module. The reason? Maybe you build the module 
   _later_, and maybe you don't ever load it.

   So it appears that your dependence on quirk_intel_ide_combined() is 
   fundamentally broken for the "m" case _anyway_.

Anyway, the second thing means that the whole configuration is somewhat 
broken, but on the whole, why not this _much_ more trivial patch?

It's still broken, exactly because it depends on the modules being defined 
when compiling the whole kernel, but hey, we probably have other cases 
like that.

My suggestion being that we should make it unconditional, but maybe that 
breaks the case where we don't actually load the SATA module?

		Linus

---
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 11ca443..cec631b 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1233,7 +1233,7 @@ static void __init quirk_alder_ioapic(st
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_EESSC,	quirk_alder_ioapic );
 #endif
 
-#ifdef CONFIG_SCSI_SATA
+#if defined(CONFIG_SCSI_SATA) || defined(CONFIG_SCSI_SATA_MODULE)
 static void __devinit quirk_intel_ide_combined(struct pci_dev *pdev)
 {
 	u8 prog, comb, tmp;
@@ -1310,7 +1310,7 @@ static void __devinit quirk_intel_ide_co
 		request_region(0x170, 8, "libata");	/* port 1 */
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,    PCI_ANY_ID,	  quirk_intel_ide_combined );
-#endif /* CONFIG_SCSI_SATA */
+#endif /* CONFIG_SCSI_SATA[_MODULE] */
 
 
 int pcie_mch_quirk;

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
>>CONFIG_SCSI_SATA is truly a boolean option, not a tristate.
>>Since the Kconfig dependencies are insufficient to describe this (2.4
>>had dep_mbool), we need to resort to 'if'.
> 
> 
> Two problems:
> 
>  - this is ugly as hell
> 
>    First, you change SCSI_SATA to be boolean, then you change the 
>    depends-on to be "if". Which makes no sense. Once SCSI_SATA is a 
>    boolean, then the "depends on" works fine, since SCSI_SATA can no 
>    longer be "m" anyway (ie your comment about "dep_mbool" doesn't make 
>    sense).
> 
>    Second, if you want "dep_mbool", then you can have dep_mbool in Kconfig 
>    too. Just do
> 
> 	depends on (XYZ != n)
> 
>    which gets you what you want (ie if XYZ is "m", then the inequality 
>    will be "y")
> 
>    So you might as well have just done something like
> 
> 	 config SCSI_SATA
> 	-       tristate "Serial ATA (SATA) support"
> 	-       depends on SCSI
> 	+       bool "Serial ATA (SATA) support"
> 	+       depends on SCSI != n
> 
>    and then just added SCSI to the "depends on" lines to get the "m" 
>    config. No "if" needed.
> 
>  - anything that depends on a module had better only be _inside_ that 
>    module. Ie the "dep_mbool" kind of behaviour should _not_ affect 
>    anything outside the module. The reason? Maybe you build the module 
>    _later_, and maybe you don't ever load it.
> 
>    So it appears that your dependence on quirk_intel_ide_combined() is 
>    fundamentally broken for the "m" case _anyway_.

CONFIG_SCSI_SATA does two things:
* Enables/disables the display of the SATA driver menu.
* Enables/disables the compiled-in PCI quirk.

Both of these are boolean, and have absolutely nothing to do with modules.

The original problem that caused me to merge the problematic patch (my 
fault for bad merge) was that Kconfig wouldn't allow SATA drivers to be 
built as modules if you did
	bool SCSI_SATA depends on SCSI
	tristate SCSI_SATA_driver depends on SCSI_SATA

That's why I used an 'if' in my patch.  I suppose we could split it into 
two Kconfig options, one for the menu (CONFIG_SCSI_SATA) using 'if', and 
one specifically for the PCI quirk.  Such a split would make it obvious 
that CONFIG_SCSI_SATA controls display of a menu, and nothing more.

Comments?

The entire situation is a hack, because we're fighting the 
always-built-in IDE driver for control of the hardware.  IDE driver 
picks up the PATA+SATA device even though it isn't listed in 
drivers/ide/pci/piix.c PCI table, because it blindly probes at the IDE 
ports after exhausting other options.


> Anyway, the second thing means that the whole configuration is somewhat 
> broken, but on the whole, why not this _much_ more trivial patch?

Because it's fundamental a boolean, and has -zero- to do with modules. 
Encouraging people to think otherwise will just lead to more confusion.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Linus Torvalds]

On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
> CONFIG_SCSI_SATA does two things:
> * Enables/disables the display of the SATA driver menu.
> * Enables/disables the compiled-in PCI quirk.
> 
> Both of these are boolean, and have absolutely nothing to do with modules.

You ignore the biggest thing it does:
 - it is the depends-on for the actual low-level drivers

IOW, the _biggest_ reason for it existing at all is in fact _not_ a 
boolean. It very much is a tristate. When it's "m" the SATA driver menu 
_should_ show.

Also, as already mentioned, that compiled-in PCI quirk is _wrong_. The 
fact that somebody asked for SCSI_SATA should not change Intel settings. 
Maybe somebody hass a separate SATA card, and has enabled support for 
_that_, but wants the on-board thing to work with legacy drivers? The way 
he'd have done that is to enable SCSI_SATA, but _not_ enable 
SCSI_ATA_PIIX.

So regardless, that PCI quirk is wrong wrong _wrong_.

Btw, if you want to really hide things (and not just gray them out) I 
think you should do a

	menu "SATA low-level drivers"
		depends on SCSI_SATA != n

	..

	endmenu

around the SATA drivers.

> Because it's fundamental a boolean, and has -zero- to do with modules.
> Encouraging people to think otherwise will just lead to more confusion.

I disagree. It is no more fundamentally boolean than anything else that 
controls modules. It's a tristate, because it chooses between the 
low-level drivers being tristate.

I also think that the _only_ thing your ugly patch fixes was totally wrong 
for wholly other reasons anyway. If that quirk is needed, it really looks 
like it should be

	#if defined(CONFIG_SCSI_SATA_PIIX) || defined(CONFIG_SCSI_SATA_PIIX_MODULE)
	..
	#endif

or something like that. It has _nothing_ to do with whether the user has 
enabled SATA or not.

		Linus

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
>>CONFIG_SCSI_SATA does two things:
>>* Enables/disables the display of the SATA driver menu.
>>* Enables/disables the compiled-in PCI quirk.
>>
>>Both of these are boolean, and have absolutely nothing to do with modules.
> 
> 
> You ignore the biggest thing it does:
>  - it is the depends-on for the actual low-level drivers

That dependency for each driver exists solely for menu display purposes. 
  There is no code dependency.


> IOW, the _biggest_ reason for it existing at all is in fact _not_ a 
> boolean. It very much is a tristate. When it's "m" the SATA driver menu 
> _should_ show.

The only operational difference between CONFIG_SCSI_SATA=y and 
CONFIG_SCSI_SATA=m is that CONFIG_SCSI_SATA=m restricts the drivers from 
being compiled in -- a silly and needless restriction.

The elimination of 'y' as an option should propagate from CONFIG_SCSI.


> Also, as already mentioned, that compiled-in PCI quirk is _wrong_. The 
> fact that somebody asked for SCSI_SATA should not change Intel settings. 
> Maybe somebody hass a separate SATA card, and has enabled support for 
> _that_, but wants the on-board thing to work with legacy drivers? The way 
> he'd have done that is to enable SCSI_SATA, but _not_ enable 
> SCSI_ATA_PIIX.

Agreed this is a _theoretical_ problem.

Never heard of this being an issue in the real world, because the IDE 
driver locks up on a lot of the Intel hardware in question.  That was 
one of the original reason for the split PATA/SATA driver configuration, 
for this wonky combined mode.


> Btw, if you want to really hide things (and not just gray them out) I 
> think you should do a
> 
> 	menu "SATA low-level drivers"
> 		depends on SCSI_SATA != n
> 
> 	..
> 
> 	endmenu
> 
> around the SATA drivers.

No preference whether its hidden or greyed out.

CONFIG_SCSI_SATA is just a switch to enable listing a set of drivers, 
just like CONFIG_NET_PCI (which I note is a bool), CONFIG_NET_ISA (a 
bool), ...


>>Because it's fundamental a boolean, and has -zero- to do with modules.
>>Encouraging people to think otherwise will just lead to more confusion.
> 
> 
> I disagree. It is no more fundamentally boolean than anything else that 
> controls modules. It's a tristate, because it chooses between the 
> low-level drivers being tristate.
> 
> I also think that the _only_ thing your ugly patch fixes was totally wrong 
> for wholly other reasons anyway. If that quirk is needed, it really looks 
> like it should be
> 
> 	#if defined(CONFIG_SCSI_SATA_PIIX) || defined(CONFIG_SCSI_SATA_PIIX_MODULE)
> 	..
> 	#endif

If IDE is compiled in, IDE SATA option is not enabled, and ata_piix or 
ahci are used.

Do we really want to do

	#if defined (CONFIG_IDE_GENERIC) &&
	       !defined(CONFIG_IDE_BLK_DEV_SATA) &&
	    (
	       defined(CONFIG_SCSI_SATA_PIIX) ||
	       defined(CONFIG_SCSI_SATA_PIIX_MODULE) ||
	       defined(CONFIG_SCSI_SATA_AHCI ||
	       defined(CONFIG_SCSI_SATA_AHCI_MODULE)
	    )

?

At that point it seems easier to solve at the Kconfig level, perhaps 
defining CONFIG_SATA_INTEL_COMBINED at the end.  And then with the quirk 
issue out of the way, CONFIG_SCSI_SATA becomes purely a boolean 
enable/disable-this-menu switch.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Linus Torvalds]

On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
> The only operational difference between CONFIG_SCSI_SATA=y and
> CONFIG_SCSI_SATA=m is that CONFIG_SCSI_SATA=m restricts the drivers from being
> compiled in -- a silly and needless restriction.
> 
> The elimination of 'y' as an option should propagate from CONFIG_SCSI.

Sure. You can do it that way too, as I mentioned in the original mail 
about why it was ugly.

But the point is:

 WHY?

Your insistence on CONFIG_SCSI_SATA being a boolean only results in uglier 
configuration, for no gain. And it's not even TRUE. It's only true if you 
consider it to be a "do we support SATA or not" thing. It's not true if 
you consider it "how do we support SATA" thing.

In other words, a tristate makes total sense, if you say that 
CONFIG_SCSI_SATA describes how SATA is supported.

> Agreed this is a _theoretical_ problem.

But it's just another sign of you thinking about that config variable the 
wrong way.

> CONFIG_SCSI_SATA is just a switch to enable listing a set of drivers, just
> like CONFIG_NET_PCI (which I note is a bool), CONFIG_NET_ISA (a bool), ...

No it's not.

CONFIG_NET_BOOL is not a tristate, because it can't be a module. There's 
nothing that forces network drivers to be modules only. Same goes for 
NET_ISA.

In contrast, CONFIG_SCSI=m _does_ force SATA drivers to be module only. 

See? They are not the same at all.

> At that point it seems easier to solve at the Kconfig level, perhaps defining
> CONFIG_SATA_INTEL_COMBINED at the end.

Sure, that makes sense.

>  And then with the quirk issue out of
> the way, CONFIG_SCSI_SATA becomes purely a boolean enable/disable-this-menu
> switch.

No it does not. You continue to ignore the fact that it's not an 
enable/disable thing. It's a "can we enable SATA drivers" vs "can we 
enable SATA drivers as modules" vs "do we do any SATA drivers at all?" 
thing.

A tristate.

Adding the SCSI config is a pure hack because you refuse to see the fact 
that CONFIG_SCSI_SATA _is_ a tristate. Thinking it is a boolean causes you 
to then have to mix in other issues.

		Linus

Re: [PATCH] libata: fix broken Kconfig setup

[Linus Torvalds]

On Mon, 17 Oct 2005, Linus Torvalds wrote:
> 
> >  And then with the quirk issue out of
> > the way, CONFIG_SCSI_SATA becomes purely a boolean enable/disable-this-menu
> > switch.
> 
> No it does not. You continue to ignore the fact that it's not an 
> enable/disable thing. It's a "can we enable SATA drivers" vs "can we 
> enable SATA drivers as modules" vs "do we do any SATA drivers at all?" 
> thing.
> 
> A tristate.

Btw, if you want to have the _question_ always be y/n only, that's easy 
enough to do, just make that one do

	config SATA_MENU
		bool "Want to see SATA drivers"
		depends on SCSI != n

	config SCSI_SATA
		tristate
		depends on SCSI && SATA_MENU
		default y

and now you have a totally sensible setup, where the low-level drivers can 
depend on something sane. 

I don't think it _buys_ you anything, but hey, at least it's logical. 

Btw, wouldn't it be much nicer to also have this all in a totally separate 
Kconfig file? That SCSI Kconfig file is one of the biggest ones (yeah, 
drivers/net/Kconfig is bigger, but hey, that's not a surprise, is it ;)

		Linus

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Mon, 17 Oct 2005, Linus Torvalds wrote:
> 
>>> And then with the quirk issue out of
>>>the way, CONFIG_SCSI_SATA becomes purely a boolean enable/disable-this-menu
>>>switch.
>>
>>No it does not. You continue to ignore the fact that it's not an 
>>enable/disable thing. It's a "can we enable SATA drivers" vs "can we 
>>enable SATA drivers as modules" vs "do we do any SATA drivers at all?" 
>>thing.
>>
>>A tristate.
> 
> 
> Btw, if you want to have the _question_ always be y/n only, that's easy 
> enough to do, just make that one do
> 
> 	config SATA_MENU
> 		bool "Want to see SATA drivers"
> 		depends on SCSI != n
> 
> 	config SCSI_SATA
> 		tristate
> 		depends on SCSI && SATA_MENU
> 		default y
> 
> and now you have a totally sensible setup, where the low-level drivers can 
> depend on something sane. 
> 
> I don't think it _buys_ you anything, but hey, at least it's logical. 

That's a reasonable solution.  I think it does buy you reduced user 
confusion.


> Btw, wouldn't it be much nicer to also have this all in a totally separate 
> Kconfig file? That SCSI Kconfig file is one of the biggest ones (yeah, 
> drivers/net/Kconfig is bigger, but hey, that's not a surprise, is it ;)

Honestly, I've been pondering moving all libata drivers to drivers/ata 
anyway...

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Linus Torvalds]

On Mon, 17 Oct 2005, Jeff Garzik wrote:
> Linus Torvalds wrote:
>
> > Btw, if you want to have the _question_ always be y/n only, that's easy
> > enough to do, just make that one do
> > 
> > 	config SATA_MENU
> > 		bool "Want to see SATA drivers"
> > 		depends on SCSI != n
> > 
> > 	config SCSI_SATA
> > 		tristate
> > 		depends on SCSI && SATA_MENU
> > 		default y
> > 
> > and now you have a totally sensible setup, where the low-level drivers can
> > depend on something sane. 
> > I don't think it _buys_ you anything, but hey, at least it's logical. 
> 
> That's a reasonable solution.  I think it does buy you reduced user confusion.

The thing that worries me is that while the question may appear a bit more 
straightforward that way, I actually think it makes the end result _less_ 
so.

Let's say that I'm a clueless user, and I just don't realize that SATA 
depends on SCSI. After all, to a user, SATA sure as hell isn't SCSI, 
that's just an implementation detail inside the kernel.

So I've happened to say "m" to SCSI (for whatever reason - don't ask why 
users do strange things, but maybe I realize that USB storage needs it), 
and now I see the question for SATA. And I say "y".

And then I wonder why I can only select my sata drivers as modules. I 
didn't ask for SATA as a module, but they refuse to say "m".

Now, with SCSI_SATA as a straight M/n choice (or whatever), if I had SCSI 
as a module, at least I'll see at SATA selection time that I can only 
compile SATA drivers as modules. I might wonder at that time why, but I 
think it's less confusing there (and we could even mention it in the 
help-text).

I dunno. 

The _best_ choice as far as I can tell, is to just dis-associate SATA from 
SCSI entirely. Even if it's an implementation choice, we could make it a 
"select SCSI" instead of "depends on SCSI", so that from a _logical_ 
standpoint the user could just select SATA support without even knowing 
that the kernel happens to need the SCSI layer for it.

I think that's what USB storage ended up doing, exactly because it 
confused people too badly that they had to select SCSI in order to be able 
to say "I want USB disk supprt".

Of course, eventually I still hope that SATA could be done on the block 
layer instead of even depending on SCSI at all, but hey, that's a totally 
different issue.

			Linus

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Linus Torvalds wrote:
> The _best_ choice as far as I can tell, is to just dis-associate SATA from 
> SCSI entirely. Even if it's an implementation choice, we could make it a 
> "select SCSI" instead of "depends on SCSI", so that from a _logical_ 
> standpoint the user could just select SATA support without even knowing 
> that the kernel happens to need the SCSI layer for it.

Yep.  That would happen as a side effect of moving the code to 
drivers/ata, even.


> Of course, eventually I still hope that SATA could be done on the block 
> layer instead of even depending on SCSI at all, but hey, that's a totally 
> different issue.

If you look at libata-scsi, the code is simply a SCSI simulator that 
calls a _clean_ and _separate_ libATA API.

Other code -- such as a block-layer driver -- could use this same API. 
I think Bart has mentioned he has early code to do this, or at least 
ideas on how to do it.

I made a promise to you, to do it at the block layer, and I intend to 
keep my promise.  :)  It just takes years to get there.  The two main 
reasons for using SCSI were/are:

* provides a bunch of useful _generic_ infrastructure

* has a very high Just Works(tm) value for distro installers and users, 
where code already exists for /dev/sdX.  I learned the hard way with 
drivers/block/sx8.c that adding a new block device involves coordination 
with multiple distros :(

I dream of a /dev/disk, perhaps reusing and expanding /dev/sdX's block 
majors, but that may be unrealistic.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Alistair John Strachan]

On Monday 17 October 2005 18:25, Linus Torvalds wrote:
> On Mon, 17 Oct 2005, Jeff Garzik wrote:
> > Linus Torvalds wrote:
> > > Btw, if you want to have the _question_ always be y/n only, that's easy
> > > enough to do, just make that one do
> > >
> > > 	config SATA_MENU
> > > 		bool "Want to see SATA drivers"
> > > 		depends on SCSI != n
> > >
> > > 	config SCSI_SATA
> > > 		tristate
> > > 		depends on SCSI && SATA_MENU
> > > 		default y
> > >
> > > and now you have a totally sensible setup, where the low-level drivers
> > > can depend on something sane.
> > > I don't think it _buys_ you anything, but hey, at least it's logical.
> >
> > That's a reasonable solution.  I think it does buy you reduced user
> > confusion.
>
> The thing that worries me is that while the question may appear a bit more
> straightforward that way, I actually think it makes the end result _less_
> so.
>
> Let's say that I'm a clueless user, and I just don't realize that SATA
> depends on SCSI. After all, to a user, SATA sure as hell isn't SCSI,
> that's just an implementation detail inside the kernel.
>
> So I've happened to say "m" to SCSI (for whatever reason - don't ask why
> users do strange things, but maybe I realize that USB storage needs it),
> and now I see the question for SATA. And I say "y".
>
> And then I wonder why I can only select my sata drivers as modules. I
> didn't ask for SATA as a module, but they refuse to say "m".
>
> Now, with SCSI_SATA as a straight M/n choice (or whatever), if I had SCSI
> as a module, at least I'll see at SATA selection time that I can only
> compile SATA drivers as modules. I might wonder at that time why, but I
> think it's less confusing there (and we could even mention it in the
> help-text).
>
[snip]

Pretty much this exact thing happened to me. SATA=y when SCSI=y, then I 
selected my mainboard's SATA chipset (NFORCE=y), then a few kernels later I 
went back to set SCSI=m (I can't remember the rationale, something to do with 
udev and me thinking I didn't need to compile SCSI into the kernel).

Of course, without asking me, this changed my SATA chipset driver to a module, 
and the resulting kernel wouldn't get to init (because I was attempting to 
boot from a disc on the SATA controller).

This particular issue is perhaps more difficult to resolve, but I think this 
illustrates that a conceptual link between SCSI and SATA is a bad idea at the 
KConfig level (even if, within the kernel, SATA depends on parts of SCSI).

-- 
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [PATCH] libata: fix broken Kconfig setup

[Randy.Dunlap]

On Wed, 19 Oct 2005, Alistair John Strachan wrote:

> On Monday 17 October 2005 18:25, Linus Torvalds wrote:
> > On Mon, 17 Oct 2005, Jeff Garzik wrote:
> > > Linus Torvalds wrote:
> > > > Btw, if you want to have the _question_ always be y/n only, that's easy
> > > > enough to do, just make that one do
> > > >
> > > > 	config SATA_MENU
> > > > 		bool "Want to see SATA drivers"
> > > > 		depends on SCSI != n
> > > >
> > > > 	config SCSI_SATA
> > > > 		tristate
> > > > 		depends on SCSI && SATA_MENU
> > > > 		default y
> > > >
> > > > and now you have a totally sensible setup, where the low-level drivers
> > > > can depend on something sane.
> > > > I don't think it _buys_ you anything, but hey, at least it's logical.
> > >
> > > That's a reasonable solution.  I think it does buy you reduced user
> > > confusion.
> >
> > The thing that worries me is that while the question may appear a bit more
> > straightforward that way, I actually think it makes the end result _less_
> > so.
> >
> > Let's say that I'm a clueless user, and I just don't realize that SATA
> > depends on SCSI. After all, to a user, SATA sure as hell isn't SCSI,
> > that's just an implementation detail inside the kernel.
> >
> > So I've happened to say "m" to SCSI (for whatever reason - don't ask why
> > users do strange things, but maybe I realize that USB storage needs it),
> > and now I see the question for SATA. And I say "y".
> >
> > And then I wonder why I can only select my sata drivers as modules. I
> > didn't ask for SATA as a module, but they refuse to say "m".
> >
> > Now, with SCSI_SATA as a straight M/n choice (or whatever), if I had SCSI
> > as a module, at least I'll see at SATA selection time that I can only
> > compile SATA drivers as modules. I might wonder at that time why, but I
> > think it's less confusing there (and we could even mention it in the
> > help-text).
> >
> [snip]
>
> Pretty much this exact thing happened to me. SATA=y when SCSI=y, then I
> selected my mainboard's SATA chipset (NFORCE=y), then a few kernels later I
> went back to set SCSI=m (I can't remember the rationale, something to do with
> udev and me thinking I didn't need to compile SCSI into the kernel).
>
> Of course, without asking me, this changed my SATA chipset driver to a module,
> and the resulting kernel wouldn't get to init (because I was attempting to
> boot from a disc on the SATA controller).
>
> This particular issue is perhaps more difficult to resolve, but I think this
> illustrates that a conceptual link between SCSI and SATA is a bad idea at the
> KConfig level (even if, within the kernel, SATA depends on parts of SCSI).

I agree and it seems that Jeff expects to change that.
I think that what I said (or I guess I "asked") here makes sense:
  http://marc.theaimsgroup.com/?l=linux-kernel&m=112839490116475&w=2

-- 
~Randy

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

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

Here's a patch for the quirk.  I'll split this off from the sata-menu 
discussion.  Jesse, I presume this also fixes the problem for you?


This change makes quirk_intel_ide_combined() dependent on the precise 
conditions under which it is needed:
* IDE is built in
* IDE SATA option is not set
* ata_piix or ahci drivers are enabled

This fixes an issue where some modular configurations would not cause 
the quirk to be enabled.

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1178 bytes --]

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1233,7 +1233,7 @@ static void __init quirk_alder_ioapic(st
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_EESSC,	quirk_alder_ioapic );
 #endif
 
-#ifdef CONFIG_SCSI_SATA
+#ifdef CONFIG_SCSI_SATA_INTEL_COMBINED
 static void __devinit quirk_intel_ide_combined(struct pci_dev *pdev)
 {
 	u8 prog, comb, tmp;
@@ -1310,7 +1310,7 @@ static void __devinit quirk_intel_ide_co
 		request_region(0x170, 8, "libata");	/* port 1 */
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,    PCI_ANY_ID,	  quirk_intel_ide_combined );
-#endif /* CONFIG_SCSI_SATA */
+#endif /* CONFIG_SCSI_SATA_INTEL_COMBINED */
 
 
 int pcie_mch_quirk;
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -553,6 +553,11 @@ config SCSI_SATA_VITESSE
 
 	  If unsure, say N.
 
+config SCSI_SATA_INTEL_COMBINED
+	bool
+	depends on IDE=y && !BLK_DEV_IDE_SATA && (SCSI_SATA_AHCI || SCSI_ATA_PIIX)
+	default y
+
 config SCSI_BUSLOGIC
 	tristate "BusLogic SCSI support"
 	depends on (PCI || ISA || MCA) && SCSI && ISA_DMA_API

Re: [PATCH] libata: fix broken Kconfig setup

[Sergey Vlasov]

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

On Mon, 17 Oct 2005 13:01:57 -0400 Jeff Garzik wrote:

> Here's a patch for the quirk.  I'll split this off from the sata-menu 
> discussion.  Jesse, I presume this also fixes the problem for you?
> 
> 
> This change makes quirk_intel_ide_combined() dependent on the precise 
> conditions under which it is needed:
> * IDE is built in
> * IDE SATA option is not set
> * ata_piix or ahci drivers are enabled
> 
> This fixes an issue where some modular configurations would not cause 
> the quirk to be enabled.
> 
> Signed-off-by: Jeff Garzik <jgarzik@pobox.com>

> +config SCSI_SATA_INTEL_COMBINED
> +	bool
> +	depends on IDE=y && !BLK_DEV_IDE_SATA && (SCSI_SATA_AHCI || SCSI_ATA_PIIX)
> +	default y

The IDE=y part seems to be incorrect - quirk_intel_ide_combined() is
needed even with modular IDE.  Without this quirk you will get one of
these configurations depending on the module load order:

1) ata_piix loads first - it grabs the whole controller, including the
PATA port; the IDE module loaded later finds nothing.

2) IDE modules are loaded first - without the quirk IDE drivers will
grab the whole controller, including the SATA part.

The binding you get with builtin IDE (ata_piix/ahci for SATA, generic
IDE driver for PATA) would be impossible to get with modular IDE without
the quirk, which does not seem to be good...

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Sergey Vlasov wrote:
> The IDE=y part seems to be incorrect - quirk_intel_ide_combined() is
> needed even with modular IDE.  Without this quirk you will get one of
> these configurations depending on the module load order:
> 
> 1) ata_piix loads first - it grabs the whole controller, including the
> PATA port; the IDE module loaded later finds nothing.
> 
> 2) IDE modules are loaded first - without the quirk IDE drivers will
> grab the whole controller, including the SATA part.
> 
> The binding you get with builtin IDE (ata_piix/ahci for SATA, generic
> IDE driver for PATA) would be impossible to get with modular IDE without
> the quirk, which does not seem to be good...

This is a reasonable point, but the rare person who runs modular IDE on 
these PATA/SATA combined mode beasts can certainly tell the IDE driver 
to not probe certain ports.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Linus Torvalds]

On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
> If IDE is compiled in, IDE SATA option is not enabled, and ata_piix or ahci
> are used.

How about this diff instead?

It's really quite clean and understandable, and it makes it very clear 
what's going on from a configuration standpoint, imnsho. And it does the 
right thing when AHCI/PIIX is compiled as a SATA module (well, as right as 
this approach ever can).

Of course, somebody should check that it really is just the AHCI and PIIX 
drivers that want the quirk, but I think the _approach_ is obvious.

		Linus

----
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 11ca443..7bb5725 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1233,7 +1233,7 @@ static void __init quirk_alder_ioapic(st
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_EESSC,	quirk_alder_ioapic );
 #endif
 
-#ifdef CONFIG_SCSI_SATA
+#ifdef CONFIG_INTEL_SATA_QUIRK
 static void __devinit quirk_intel_ide_combined(struct pci_dev *pdev)
 {
 	u8 prog, comb, tmp;
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 20019b8..49ef1c6 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -445,9 +445,14 @@ config SCSI_SATA
 
 	  If unsure, say N.
 
+config INTEL_SATA_QUIRK
+	bool
+	default n
+
 config SCSI_SATA_AHCI
 	tristate "AHCI SATA support"
 	depends on SCSI_SATA && PCI
+	select INTEL_SATA_QUIRK
 	help
 	  This option enables support for AHCI Serial ATA.
 
@@ -465,6 +470,7 @@ config SCSI_SATA_SVW
 config SCSI_ATA_PIIX
 	tristate "Intel PIIX/ICH SATA support"
 	depends on SCSI_SATA && PCI
+	select INTEL_SATA_QUIRK
 	help
 	  This option enables support for ICH5 Serial ATA.
 	  If PATA support was enabled previously, this enables

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Mon, 17 Oct 2005, Jeff Garzik wrote:
> 
>>If IDE is compiled in, IDE SATA option is not enabled, and ata_piix or ahci
>>are used.
> 
> 
> How about this diff instead?

Nope, it elides critical logic.

1) The quirk should not be enabled if IDE driver is a module.  No reason 
to perform the nasty hack at all, as the user controls module load order.

2) The quirk should not be enabled if IDE driver is not built at all. 
Standard resource reservation code works as expected here.

3) The quirk should not be enabled if CONFIG_BLK_DEV_IDE_SATA is 
enabled, which indicates that the IDE driver gets preference for the 
Intel SATA hardware in question.


> It's really quite clean and understandable, and it makes it very clear 
> what's going on from a configuration standpoint, imnsho. And it does the 
> right thing when AHCI/PIIX is compiled as a SATA module (well, as right as 
> this approach ever can).

My patch works fine when ahci/piix is compiled as a module.  That's the 
configuration I personally tested.


> Of course, somebody should check that it really is just the AHCI and PIIX 
> drivers that want the quirk,

It is.  I wrote 100% of the code, and further, the quirk specifically 
enumerates which PCI IDs are affected, making it easy to verify.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Jesse Barnes]

On Monday, October 17, 2005 8:14 am, Linus Torvalds wrote:
>    So it appears that your dependence on quirk_intel_ide_combined() is
>    fundamentally broken for the "m" case _anyway_.

Correct.  The quirk itself is a fragile hack.


> Anyway, the second thing means that the whole configuration is
> somewhat broken, but on the whole, why not this _much_ more trivial
> patch?
>
> It's still broken, exactly because it depends on the modules being
> defined when compiling the whole kernel, but hey, we probably have
> other cases like that.
>
> My suggestion being that we should make it unconditional, but maybe
> that breaks the case where we don't actually load the SATA module?

It does, since it prevents one of the ports from being bound by the 
legacy IDE driver.  But the whole thing is rather hackish to begin with, 
and I prefer this hack to the existing code (in fact, Andrew already 
queued up a patch from me in -mm that looks just like yours).

Ultimately, when libata gets ATAPI support, I think we just have to 
declare libata and legacy IDE to be incompatible for combined mode 
devices and remove the quirk.  Then whichever driver loads first will 
get the whole device, as it should.

Jesse

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Jesse Barnes wrote:
> It does, since it prevents one of the ports from being bound by the 
> legacy IDE driver.  But the whole thing is rather hackish to begin with, 
> and I prefer this hack to the existing code (in fact, Andrew already 
> queued up a patch from me in -mm that looks just like yours).
> 
> Ultimately, when libata gets ATAPI support, I think we just have to 
> declare libata and legacy IDE to be incompatible for combined mode 
> devices and remove the quirk.  Then whichever driver loads first will 
> get the whole device, as it should.

I would love to remove the quirk completely!

Unfortunately combined mode is a runtime BIOS configuration, and there 
is also the lockup issue I mentioned in another email.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Jesse Barnes]

On Monday, October 17, 2005 10:03 am, Jeff Garzik wrote:
> Jesse Barnes wrote:
> > It does, since it prevents one of the ports from being bound by the
> > legacy IDE driver.  But the whole thing is rather hackish to begin
> > with, and I prefer this hack to the existing code (in fact, Andrew
> > already queued up a patch from me in -mm that looks just like
> > yours).
> >
> > Ultimately, when libata gets ATAPI support, I think we just have to
> > declare libata and legacy IDE to be incompatible for combined mode
> > devices and remove the quirk.  Then whichever driver loads first
> > will get the whole device, as it should.
>
> I would love to remove the quirk completely!
>
> Unfortunately combined mode is a runtime BIOS configuration, and there
> is also the lockup issue I mentioned in another email.

So sometimes the legacy IDE driver will lock up when it tries to drive 
both ports in a combined configuration?  In that case, can't we just 
disable the legacy IDE driver for these chips and force the use of the 
libata version?

Jesse

Re: [PATCH] libata: fix broken Kconfig setup

[Jeff Garzik]

Jesse Barnes wrote:
> So sometimes the legacy IDE driver will lock up when it tries to drive 
> both ports in a combined configuration?  In that case, can't we just 

-sometimes- When it tries to drive the SATA port, it locks up.  My best 
guess is that this is due to the fact that SATA emulates IDE shadow 
registers in silicon, and the IDE driver does something weird that 
confused the silicon's IDE emulation logic.

Under SATA, the IDE shadow registers are nothing but a buffer.  Writing 
to the Command or Control registers causes this buffer to be batched 
into a SATA frame (a "FIS"), and sent to the device.


> disable the legacy IDE driver for these chips and force the use of the 
> libata version?

More than a little ugly:  The piix driver already excludes the SATA 
device (unless CONFIG_BLK_DEV_IDE_SATA is defined), so the driver that 
picks up the IDE is the non-PCI "generic IDE" legacy driver.

You would need to add code somewhere in a non-PCI driver to specifically 
exclude a few PCI devices.

Removing the quirk means users/distros would simply have to know to 
disable CONFIG_IDE completely.  Doable, but also guaranteed to generate 
bug reports.

	Jeff

Re: [PATCH] libata: fix broken Kconfig setup

[Alan Cox]

> So sometimes the legacy IDE driver will lock up when it tries to drive 
> both ports in a combined configuration?  In that case, can't we just 
> disable the legacy IDE driver for these chips and force the use of the 
> libata version?

Now that libata is beginning to behave well I'd vote for that option,
however in kernel libata lacks several essential items for PATA feature
parity (HPA, ATAPI, suspend/resume, correct tuning). It's getting there
and I've got some more stuff waiting for Jeff, but it isn't there yet

Re: [PATCH] libata: fix broken Kconfig setup

[Jesse Barnes]

On Thursday, October 20, 2005 7:14 am, Alan Cox wrote:
> Now that libata is beginning to behave well I'd vote for that option,
> however in kernel libata lacks several essential items for PATA
> feature parity (HPA, ATAPI, suspend/resume, correct tuning). It's
> getting there and I've got some more stuff waiting for Jeff, but it
> isn't there yet

Yeah, that seems like the best thing to do in the long run.  Of course it 
has to wait until libata at least has ATAPI support I'd imagine.  Jeff 
also mentioned that it would require changes to the legacy IDE driver to 
prevent it from binding to certain PCI devices (and given that it just 
uses I/O ports that might get hackish).

Jesse