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
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 | [email protected]
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.
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
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;
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 <[email protected]>
> +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...
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
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.
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
> 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
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