2007-09-25 05:23:26

by peer chen

[permalink] [raw]
Subject: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Add the ahci controller legacy mode support to sata_nv.
Move the DIDs of legacy mode from ahci.c to sata_nv.c

The patch base on kernel 2.6.23-rc8

Signed-off-by: Peer Chen <[email protected]>
---
diff -uprN -X linux-2.6.23-rc8-vanilla/Documentation/dontdiff linux-2.6.23-rc8-vanilla/drivers/ata/ahci.c linux-2.6.23-rc8/drivers/ata/ahci.c
--- linux-2.6.23-rc8-vanilla/drivers/ata/ahci.c 2007-09-25 11:53:40.000000000 -0400
+++ linux-2.6.23-rc8/drivers/ata/ahci.c 2007-09-25 11:53:25.000000000 -0400
@@ -434,14 +434,6 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(NVIDIA, 0x044d), board_ahci }, /* MCP65 */
{ PCI_VDEVICE(NVIDIA, 0x044e), board_ahci }, /* MCP65 */
{ PCI_VDEVICE(NVIDIA, 0x044f), board_ahci }, /* MCP65 */
- { PCI_VDEVICE(NVIDIA, 0x045c), board_ahci }, /* MCP65 */
- { PCI_VDEVICE(NVIDIA, 0x045d), board_ahci }, /* MCP65 */
- { PCI_VDEVICE(NVIDIA, 0x045e), board_ahci }, /* MCP65 */
- { PCI_VDEVICE(NVIDIA, 0x045f), board_ahci }, /* MCP65 */
- { PCI_VDEVICE(NVIDIA, 0x0550), board_ahci }, /* MCP67 */
- { PCI_VDEVICE(NVIDIA, 0x0551), board_ahci }, /* MCP67 */
- { PCI_VDEVICE(NVIDIA, 0x0552), board_ahci }, /* MCP67 */
- { PCI_VDEVICE(NVIDIA, 0x0553), board_ahci }, /* MCP67 */
{ PCI_VDEVICE(NVIDIA, 0x0554), board_ahci }, /* MCP67 */
{ PCI_VDEVICE(NVIDIA, 0x0555), board_ahci }, /* MCP67 */
{ PCI_VDEVICE(NVIDIA, 0x0556), board_ahci }, /* MCP67 */
@@ -450,10 +442,6 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(NVIDIA, 0x0559), board_ahci }, /* MCP67 */
{ PCI_VDEVICE(NVIDIA, 0x055a), board_ahci }, /* MCP67 */
{ PCI_VDEVICE(NVIDIA, 0x055b), board_ahci }, /* MCP67 */
- { PCI_VDEVICE(NVIDIA, 0x07f0), board_ahci }, /* MCP73 */
- { PCI_VDEVICE(NVIDIA, 0x07f1), board_ahci }, /* MCP73 */
- { PCI_VDEVICE(NVIDIA, 0x07f2), board_ahci }, /* MCP73 */
- { PCI_VDEVICE(NVIDIA, 0x07f3), board_ahci }, /* MCP73 */
{ PCI_VDEVICE(NVIDIA, 0x07f4), board_ahci }, /* MCP73 */
{ PCI_VDEVICE(NVIDIA, 0x07f5), board_ahci }, /* MCP73 */
{ PCI_VDEVICE(NVIDIA, 0x07f6), board_ahci }, /* MCP73 */
@@ -462,10 +450,6 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(NVIDIA, 0x07f9), board_ahci }, /* MCP73 */
{ PCI_VDEVICE(NVIDIA, 0x07fa), board_ahci }, /* MCP73 */
{ PCI_VDEVICE(NVIDIA, 0x07fb), board_ahci }, /* MCP73 */
- { PCI_VDEVICE(NVIDIA, 0x0ad0), board_ahci }, /* MCP77 */
- { PCI_VDEVICE(NVIDIA, 0x0ad1), board_ahci }, /* MCP77 */
- { PCI_VDEVICE(NVIDIA, 0x0ad2), board_ahci }, /* MCP77 */
- { PCI_VDEVICE(NVIDIA, 0x0ad3), board_ahci }, /* MCP77 */
{ PCI_VDEVICE(NVIDIA, 0x0ad4), board_ahci }, /* MCP77 */
{ PCI_VDEVICE(NVIDIA, 0x0ad5), board_ahci }, /* MCP77 */
{ PCI_VDEVICE(NVIDIA, 0x0ad6), board_ahci }, /* MCP77 */
diff -uprN -X linux-2.6.23-rc8-vanilla/Documentation/dontdiff linux-2.6.23-rc8-vanilla/drivers/ata/sata_nv.c linux-2.6.23-rc8/drivers/ata/sata_nv.c
--- linux-2.6.23-rc8-vanilla/drivers/ata/sata_nv.c 2007-09-25 11:31:03.000000000 -0400
+++ linux-2.6.23-rc8/drivers/ata/sata_nv.c 2007-09-25 11:19:48.000000000 -0400
@@ -266,6 +266,7 @@ static void nv_adma_tf_read(struct ata_p
enum nv_host_type
{
GENERIC,
+ AHCI_LEG,
NFORCE2,
NFORCE3 = NFORCE2, /* NF2 == NF3 as far as sata_nv is concerned */
CK804,
@@ -287,6 +288,29 @@ static const struct pci_device_id nv_pci
{ PCI_VDEVICE(NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE_MCP61_SATA), GENERIC },
{ PCI_VDEVICE(NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE_MCP61_SATA2), GENERIC },
{ PCI_VDEVICE(NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE_MCP61_SATA3), GENERIC },
+ { PCI_VDEVICE(NVIDIA, 0x45c), AHCI_LEG }, /* MCP65 */
+ { PCI_VDEVICE(NVIDIA, 0x45d), AHCI_LEG }, /* MCP65 */
+ { PCI_VDEVICE(NVIDIA, 0x45e), AHCI_LEG }, /* MCP65 */
+ { PCI_VDEVICE(NVIDIA, 0x45f), AHCI_LEG }, /* MCP65 */
+ { PCI_VDEVICE(NVIDIA, 0x550), AHCI_LEG }, /* MCP67 */
+ { PCI_VDEVICE(NVIDIA, 0x551), AHCI_LEG }, /* MCP67 */
+ { PCI_VDEVICE(NVIDIA, 0x552), AHCI_LEG }, /* MCP67 */
+ { PCI_VDEVICE(NVIDIA, 0x553), AHCI_LEG }, /* MCP67 */
+ { PCI_VDEVICE(NVIDIA, 0x7f0), AHCI_LEG }, /* MCP73 */
+ { PCI_VDEVICE(NVIDIA, 0x7f1), AHCI_LEG }, /* MCP73 */
+ { PCI_VDEVICE(NVIDIA, 0x7f2), AHCI_LEG }, /* MCP73 */
+ { PCI_VDEVICE(NVIDIA, 0x7f3), AHCI_LEG }, /* MCP73 */
+ { PCI_VDEVICE(NVIDIA, 0xad0), AHCI_LEG }, /* MCP77 */
+ { PCI_VDEVICE(NVIDIA, 0xad1), AHCI_LEG }, /* MCP77 */
+ { PCI_VDEVICE(NVIDIA, 0xad2), AHCI_LEG }, /* MCP77 */
+ { PCI_VDEVICE(NVIDIA, 0xad3), AHCI_LEG }, /* MCP77 */
+ { PCI_VDEVICE(NVIDIA, 0xab4), AHCI_LEG }, /* MCP79 */
+ { PCI_VDEVICE(NVIDIA, 0xab5), AHCI_LEG }, /* MCP79 */
+ { PCI_VDEVICE(NVIDIA, 0xab6), AHCI_LEG }, /* MCP79 */
+ { PCI_VDEVICE(NVIDIA, 0xab7), AHCI_LEG }, /* MCP79 */
+ { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+ PCI_ANY_ID, PCI_ANY_ID,
+ PCI_CLASS_STORAGE_IDE<<8, 0xffff00, AHCI_LEG },

{ } /* terminate list */
};
@@ -365,6 +389,30 @@ static const struct ata_port_operations
.port_start = ata_port_start,
};

+static const struct ata_port_operations nv_ahci_leg_ops = {
+ .port_disable = ata_port_disable,
+ .tf_load = ata_tf_load,
+ .tf_read = ata_tf_read,
+ .exec_command = ata_exec_command,
+ .check_status = ata_check_status,
+ .dev_select = ata_std_dev_select,
+ .bmdma_setup = ata_bmdma_setup,
+ .bmdma_start = ata_bmdma_start,
+ .bmdma_stop = ata_bmdma_stop,
+ .bmdma_status = ata_bmdma_status,
+ .qc_prep = ata_qc_prep,
+ .qc_issue = ata_qc_issue_prot,
+ .freeze = ata_bmdma_freeze,
+ .thaw = ata_bmdma_thaw,
+ .error_handler = nv_error_handler,
+ .post_internal_cmd = ata_bmdma_post_internal_cmd,
+ .data_xfer = ata_data_xfer,
+ .irq_clear = ata_bmdma_irq_clear,
+ .irq_on = ata_irq_on,
+ .irq_ack = ata_irq_ack,
+ .port_start = ata_port_start,
+};
+
static const struct ata_port_operations nv_nf2_ops = {
.port_disable = ata_port_disable,
.tf_load = ata_tf_load,
@@ -463,6 +511,17 @@ static const struct ata_port_info nv_por
.port_ops = &nv_generic_ops,
.irq_handler = nv_generic_interrupt,
},
+ /* ahci legacy */
+ {
+ .sht = &nv_sht,
+ .flags = ATA_FLAG_SATA | ATA_FLAG_SLAVE_POSS |
+ ATA_FLAG_HRST_TO_RESUME,
+ .pio_mask = NV_PIO_MASK,
+ .mwdma_mask = NV_MWDMA_MASK,
+ .udma_mask = NV_UDMA_MASK,
+ .port_ops = &nv_ahci_leg_ops,
+ .irq_handler = nv_generic_interrupt,
+ },
/* nforce2/3 */
{
.sht = &nv_sht,
-

--------------
Peer Chen
2007-09-25


2007-09-25 07:08:52

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Peer Chen wrote:
> Add the ahci controller legacy mode support to sata_nv.
> Move the DIDs of legacy mode from ahci.c to sata_nv.c
>
> The patch base on kernel 2.6.23-rc8
>
> Signed-off-by: Peer Chen <[email protected]>

I don't understand why these are being moved?

If an interface can be driven via the AHCI driver, that is greatly
preferred over the sata_nv driver.

Jeff



2007-09-25 07:53:15

by peer chen

[permalink] [raw]
Subject: Re: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

We have three mode for one controller - IDE/RAID/AHCI, we want sata_nv being load when user select the IDE mode in BIOS, load ahci driver if RAID/AHCI being selected, which will verify if our legacy mode work well and have additional option if there is any
bug for the ahci mode.

----------------
Peer Chen
2007-09-25

-------------------------------------------------------------
?????ˣ?Jeff Garzik
???????ڣ?2007-09-25 15:08:45
?ռ??ˣ?Peer Chen
???ͣ?linux-kernel; linux-ide; akpm
???⣺Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Peer Chen wrote:
> Add the ahci controller legacy mode support to sata_nv.
> Move the DIDs of legacy mode from ahci.c to sata_nv.c
>
> The patch base on kernel 2.6.23-rc8
>
> Signed-off-by: Peer Chen <[email protected]>

I don't understand why these are being moved?

If an interface can be driven via the AHCI driver, that is greatly
preferred over the sata_nv driver.

Jeff




2007-09-25 08:14:08

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Peer Chen wrote:
> We have three mode for one controller - IDE/RAID/AHCI, we want sata_nv being load when user select the IDE mode in BIOS, load ahci driver if RAID/AHCI being selected, which will verify if our legacy mode work well and have additional option if there is any
> bug for the ahci mode.

I understand that logic, but look at what happens in practice:

1) User installs new OS in AHCI mode. Distro updates initramfs (loaded
at kernel boot time, with boot drivers) to include ahci driver.
2) User reboots into BIOS setup, and switches from AHCI mode to IDE mode.
3) BIOS setup reboots computer.
4) OS kernel and initramfs image are loaded. ahci driver load fails.
5) User is left without a bootable system.

The same situation happens in reverse, if you install in IDE mode
(sata_nv in initramfs), and then switch to AHCI/RAID mode.

Additionally, AHCI provides better performance and more direct exposure
to the SATA frames. This is key for supporting many modern SATA
features that cannot be accessed via IDE legacy mode. AHCI lacks
in-silicon simulation of an IDE interface, which time has shown is a
less stable, edge-case-prone approach to SATA.

I do not find the "verify nvidia's legacy mode works" argument
compelling; that is not the kernel's job, nor the user's. And if there
is an AHCI silicon bug, let us deal with that when such a bug appears.

Overall, AFAICS this patch -introduces- new ways for the user to easily
render their systems unbootable.

Jeff


2007-09-25 09:08:38

by peer chen

[permalink] [raw]
Subject: Re: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Yes,I hear what you are saying but user should know what they are setting in BIOS,there are lots of ways to change the BIOS setting result in unbootable system not only change AHCI/IDE mode. If they encounter booting failure after changing the BIOS setting,they should restore it.
Using legacy driver for legacy mode won't affect user to enjoy the feature of AHCI,just select AHCI/RAID mode will ok.
As I know, Intel did it in the same way,and I think it's reasonable.


------------------
Peer Chen
2007-09-25

-------------------------------------------------------------
?????ˣ?Jeff Garzik
???????ڣ?2007-09-25 16:13:52
?ռ??ˣ?Peer Chen
???ͣ?linux-kernel; linux-ide; akpm
???⣺Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Peer Chen wrote:
> We have three mode for one controller - IDE/RAID/AHCI, we want sata_nv being load when user select the IDE mode in BIOS, load ahci driver if RAID/AHCI being selected, which will verify if our legacy mode work well and have additional option if there is any
> bug for the ahci mode.

I understand that logic, but look at what happens in practice:

1) User installs new OS in AHCI mode. Distro updates initramfs (loaded
at kernel boot time, with boot drivers) to include ahci driver.
2) User reboots into BIOS setup, and switches from AHCI mode to IDE mode.
3) BIOS setup reboots computer.
4) OS kernel and initramfs image are loaded. ahci driver load fails.
5) User is left without a bootable system.

The same situation happens in reverse, if you install in IDE mode
(sata_nv in initramfs), and then switch to AHCI/RAID mode.

Additionally, AHCI provides better performance and more direct exposure
to the SATA frames. This is key for supporting many modern SATA
features that cannot be accessed via IDE legacy mode. AHCI lacks
in-silicon simulation of an IDE interface, which time has shown is a
less stable, edge-case-prone approach to SATA.

I do not find the "verify nvidia's legacy mode works" argument
compelling; that is not the kernel's job, nor the user's. And if there
is an AHCI silicon bug, let us deal with that when such a bug appears.

Overall, AFAICS this patch -introduces- new ways for the user to easily
render their systems unbootable.

Jeff



2007-10-12 21:18:03

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Peer Chen wrote:
> Add the ahci controller legacy mode support to sata_nv.
> Move the DIDs of legacy mode from ahci.c to sata_nv.c
>
> The patch base on kernel 2.6.23-rc8
>
> Signed-off-by: Peer Chen <[email protected]>

Would you mind checking libata-dev.git#upstream, and make sure it has
all the NV patches?

I'm thinking I should go ahead and push the 'nv-swncq' branch, which
contains the sata_nv updates for swncq. They have been in -mm for a
while. I am leaning towards leaving the default to 'off' for 2.6.24 though.

Comments welcome...

Jeff



2007-10-16 08:08:42

by peer chen

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

I hope one of the following patches can be merged to 2.6.24.
==========================
http://lkml.org/lkml/2007/10/8/93
http://lkml.org/lkml/2007/9/25/20

Yes, I agree to set the 'swncq' as default for 2.6.24, after all, for
our server customers, stability is far more important than the new
feature no matter the problem is caused by drive or controller.


2007/10/13, Jeff Garzik <[email protected]>:
> Peer Chen wrote:
> > Add the ahci controller legacy mode support to sata_nv.
> > Move the DIDs of legacy mode from ahci.c to sata_nv.c
> >
> > The patch base on kernel 2.6.23-rc8
> >
> > Signed-off-by: Peer Chen <[email protected]>
>
> Would you mind checking libata-dev.git#upstream, and make sure it has
> all the NV patches?
>
> I'm thinking I should go ahead and push the 'nv-swncq' branch, which
> contains the sata_nv updates for swncq. They have been in -mm for a
> while. I am leaning towards leaving the default to 'off' for 2.6.24 though.
>
> Comments welcome...
>
> Jeff
>
>
>
>

2007-10-19 02:56:46

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

peer chen wrote:
> I hope one of the following patches can be merged to 2.6.24.
> ==========================
> http://lkml.org/lkml/2007/10/8/93
> http://lkml.org/lkml/2007/9/25/20

Unfortunately I do not feel like this is the right course of action.

Experience from Intel platforms tells us that our users get very unhappy
when their silicon supports AHCI mode, but they are forced into using a
less-performant mode. A popular example is an <unnamed> OEM whose BIOS
had no method whatsoever for enabling AHCI -- didn't even program the
PCI BAR -- even though tests showed the AHCI mode worked just fine when
manually programmed.

AHCI is more likely to provide a /stable/ Serial ATA experience, because
the silicon deals primarily with sending and receiving FIS's, and not
much else. In constrast, experience has shown the legacy IDE interface
to be a less reliable method of SATA support. And certainly AHCI is
much, much faster with less per-command overhead.

Given that AHCI is both faster and more stable, I feel it is the best
policy to enable AHCI when the hardware supports it, regardless of PCI
class code (IDE, SATA, or RAID).


> Yes, I agree to set the 'swncq' as default for 2.6.24, after all, for
> our server customers, stability is far more important than the new
> feature no matter the problem is caused by drive or controller.

Agreed. Done!

Jeff




2007-10-19 06:58:32

by peer chen

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Ok,I agree to use AHCI driver for our AHCI controllers no matter their
class codes are IDE/RAID/AHCI. But for those new or upcoming AHCI
controller which DIDs are not included in ahci.c and also IDE/RAID
mode being set in BIOS, no driver will be loaded currently, so I hope
the first patch http://lkml.org/lkml/2007/10/8/93 can be appied for
this case. Any comments?

2007/10/19, Jeff Garzik <[email protected]>:
> peer chen wrote:
> > I hope one of the following patches can be merged to 2.6.24.
> > ==========================
> > http://lkml.org/lkml/2007/10/8/93
> > http://lkml.org/lkml/2007/9/25/20
>
> Unfortunately I do not feel like this is the right course of action.
>
> Experience from Intel platforms tells us that our users get very unhappy
> when their silicon supports AHCI mode, but they are forced into using a
> less-performant mode. A popular example is an <unnamed> OEM whose BIOS
> had no method whatsoever for enabling AHCI -- didn't even program the
> PCI BAR -- even though tests showed the AHCI mode worked just fine when
> manually programmed.
>
> AHCI is more likely to provide a /stable/ Serial ATA experience, because
> the silicon deals primarily with sending and receiving FIS's, and not
> much else. In constrast, experience has shown the legacy IDE interface
> to be a less reliable method of SATA support. And certainly AHCI is
> much, much faster with less per-command overhead.
>
> Given that AHCI is both faster and more stable, I feel it is the best
> policy to enable AHCI when the hardware supports it, regardless of PCI
> class code (IDE, SATA, or RAID).
>
>
> > Yes, I agree to set the 'swncq' as default for 2.6.24, after all, for
> > our server customers, stability is far more important than the new
> > feature no matter the problem is caused by drive or controller.
>
> Agreed. Done!
>
> Jeff
>
>
>
>
>

2007-10-19 07:26:13

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

peer chen wrote:
> Ok,I agree to use AHCI driver for our AHCI controllers no matter their
> class codes are IDE/RAID/AHCI. But for those new or upcoming AHCI
> controller which DIDs are not included in ahci.c and also IDE/RAID
> mode being set in BIOS, no driver will be loaded currently, so I hope
> the first patch http://lkml.org/lkml/2007/10/8/93 can be appied for
> this case. Any comments?

hmmm is that the correct URL? That one updates sata_nv to support AHCI
controllers?.

I would think you would want to add the RAID class code to ahci.c
instead? And just some regular PCI IDs?

Jeff


2007-10-22 01:56:17

by peer chen

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Yes, link - http://lkml.org/lkml/2007/10/8/93 add the AHCI legacy
support to sata_nv when IDE/RAID mode been set in SBIOS and Device IDs
are not in ahci.c at this moment. To do so, when a new chipset come
out and DIDs haven't been submited to LKML,user still can use ahci
driver to handle it when setting AHCI mode in BIOS, using sata_nv when
setting RAID/IDE in BIOS.
For example, ahci driver in Fedora 7 doesn't include the DIDs of
MCP78, so if you set the IDE/RAID mode in BIOS, os installation onto
SATA drive will fail. But if Fedora 7 include this patch, installation
will be successful.

2007/10/19, Jeff Garzik <[email protected]>:
> peer chen wrote:
> > Ok,I agree to use AHCI driver for our AHCI controllers no matter their
> > class codes are IDE/RAID/AHCI. But for those new or upcoming AHCI
> > controller which DIDs are not included in ahci.c and also IDE/RAID
> > mode being set in BIOS, no driver will be loaded currently, so I hope
> > the first patch http://lkml.org/lkml/2007/10/8/93 can be appied for
> > this case. Any comments?
>
> hmmm is that the correct URL? That one updates sata_nv to support AHCI
> controllers?.
>
> I would think you would want to add the RAID class code to ahci.c
> instead? And just some regular PCI IDs?
>
> Jeff
>
>
>

2007-11-10 04:04:55

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

peer chen wrote:
> Yes, link - http://lkml.org/lkml/2007/10/8/93 add the AHCI legacy
> support to sata_nv when IDE/RAID mode been set in SBIOS and Device IDs
> are not in ahci.c at this moment. To do so, when a new chipset come
> out and DIDs haven't been submited to LKML,user still can use ahci
> driver to handle it when setting AHCI mode in BIOS, using sata_nv when
> setting RAID/IDE in BIOS.
> For example, ahci driver in Fedora 7 doesn't include the DIDs of
> MCP78, so if you set the IDE/RAID mode in BIOS, os installation onto
> SATA drive will fail. But if Fedora 7 include this patch, installation
> will be successful.

The problem with this change is that sata_nv picks it up through the
default entry, and then later on, ahci picks it up as well.

Rather than the confusion of "which driver is the user running on this
hardware?" it is easier to take steps to ensure the user is always
running in AHCI mode.

Therefore, I think the best idea is to follow the same policy as
drivers/net/forcedeth.c (Ayaz @ NVIDIA) and Intel AHCI, and simply
provide the individual PCI IDs in advance of product release.

If you wish to avoid this, then take steps to make sure
drivers/ata/ahci.c has the appropriate wildcard entry in its
pci_device_id table.

Really, look at the sata_nv bug reports that keep popping up. We want
to move away from that driver long term, and always drive the SATA
interface in AHCI mode.

The proposed sata_nv patch does the opposite -- guarantees we must
support the continually problematic legacy IDE interface ad infinitum.
Such patches are OK for the test lab, but in this specific case users
/suffer/ when not running AHCI mode.

Jeff



2007-11-10 05:00:38

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Jeff Garzik wrote:
> The proposed sata_nv patch does the opposite -- guarantees we must
> support the continually problematic legacy IDE interface ad infinitum.
> Such patches are OK for the test lab, but in this specific case users
> /suffer/ when not running AHCI mode.

Just to reinforce...

sata_nv support and bug fixes are primarily done right now through the
valiant efforts of Robert Hancock (with assists from Alan, Tejun, and
others).

Robert's job is difficult, because he has no hardware documentation[1],
and NVIDIA does not seem to be helping out much with driver bug reports
on the lists or in bugzillas.

As far as I know, I am the only one in the universe outside of NVIDIA
with any SATA docs at all, and those docs _only_ cover ADMA registers
and DMA structures, no PCI config info, no errata, nothing on SWNCQ or
legacy IDE (well, half a page).

NVIDIA has indeed become more engaged in sata_nv in recent times, and
that's a positive sign. You, Kuon and Ayaz have all been noticeably
more responsive in email. Thanks. Users have definitely benefited,
particularly from your help addressing a couple SWNCQ issues.

But at this point in time, being asked to choose between sata_nv and
ahci is no choice at all. One has public documentation, wide industry
support and little-or-no bugs. The other has several open issues, no
documentation, and support obstacles.

Jeff


[1] Robert, please correct me if I'm wrong...

2007-11-10 19:01:01

by Robert Hancock

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,ahci: add the ahci legacy mode support to sata_nv

Jeff Garzik wrote:
> Jeff Garzik wrote:
>> The proposed sata_nv patch does the opposite -- guarantees we must
>> support the continually problematic legacy IDE interface ad infinitum.
>> Such patches are OK for the test lab, but in this specific case users
>> /suffer/ when not running AHCI mode.
>
> Just to reinforce...
>
> sata_nv support and bug fixes are primarily done right now through the
> valiant efforts of Robert Hancock (with assists from Alan, Tejun, and
> others).
>
> Robert's job is difficult, because he has no hardware documentation[1],
> and NVIDIA does not seem to be helping out much with driver bug reports
> on the lists or in bugzillas.

Right, I don't have anything. Unless the original incomplete ADMA driver
release from NVIDIA counts as documentation, lol.

And yes, I've CC'ed NVIDIA people about a few ADMA-related issues and
been met with silence. It would be nice if they were as responsive about
ADMA issues as I must say Kuan and Peer have been on the SWNCQ side of
things..

>
> As far as I know, I am the only one in the universe outside of NVIDIA
> with any SATA docs at all, and those docs _only_ cover ADMA registers
> and DMA structures, no PCI config info, no errata, nothing on SWNCQ or
> legacy IDE (well, half a page).
>
> NVIDIA has indeed become more engaged in sata_nv in recent times, and
> that's a positive sign. You, Kuon and Ayaz have all been noticeably
> more responsive in email. Thanks. Users have definitely benefited,
> particularly from your help addressing a couple SWNCQ issues.
>
> But at this point in time, being asked to choose between sata_nv and
> ahci is no choice at all. One has public documentation, wide industry
> support and little-or-no bugs. The other has several open issues, no
> documentation, and support obstacles.

They're not even equivalent interfaces in this case, in the proposed
AHCI legacy mode patch these controllers are supported in the default
SFF mode only, no ADMA or SWNCQ, so you don't get any NCQ support..

--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

2007-12-11 12:10:08

by Kuan Luo

[permalink] [raw]
Subject: RE: [PATCH] sata_nv,adma: fix error when rmmod sata_nv

hi,
The below error happens when i rmmod sata_nv in adma mode on ck804
chipset with 2.6.24 kernel.
I traced the code and found that the driver attempts to write device mem
that has been unmapped.

Only simply removing the code" writew(0, mmio + NV_ADMA_CTL);" in the
nv_adma_port_stop function or remove .port_stop field in nv_adma_ops,
rmmod is ok.

static void nv_adma_port_stop(struct ata_port *ap)
{
struct nv_adma_port_priv *pp = ap->private_data;
void __iomem *mmio = pp->ctl_block;

VPRINTK("ENTER\n");
- writew(0, mmio + NV_ADMA_CTL);
}

Or
Place pcim_iomap_regions before ata_pci_prepare_native_host in
nv_init_one function.
This can guarantee that the code "writew(0, mmio + NV_ADMA_CTL) " write
device mem before the device mem is unmapped.

messages:
Dec 19 02:23:24 localhost kernel: ata16.00: disabled
Dec 19 02:23:24 localhost kernel: sd 15:0:0:0: [sda] Synchronizing SCSI
cache
Dec 19 02:23:24 localhost kernel: sd 15:0:0:0: [sda] Result:
hostbyte=0x04 driverbyte=0x00
Dec 19 02:23:24 localhost kernel: sd 15:0:0:0: [sda] Stopping disk
Dec 19 02:23:24 localhost kernel: sd 15:0:0:0: [sda] START_STOP FAILED
Dec 19 02:23:24 localhost kernel: sd 15:0:0:0: [sda] Result:
hostbyte=0x04 driverbyte=0x00
Dec 19 02:23:24 localhost kernel: BUG: unable to handle kernel paging
request at virtual address e881a4c0
Dec 19 02:23:24 localhost kernel: printing eip: e88224e9 *pde = 0180c067
*pte = 00000000
Dec 19 02:23:24 localhost kernel: Oops: 0002 [#1] SMP
Dec 19 02:23:24 localhost kernel: Modules linked in: sata_nv libata
Dec 19 02:23:24 localhost kernel:
Dec 19 02:23:24 localhost kernel: Pid: 4915, comm: rmmod Not tainted
(2.6.24-rc1-ge2e031eb #2)
Dec 19 02:23:24 localhost kernel: EIP: 0060:[<e88224e9>] EFLAGS:
00210292 CPU: 1
Dec 19 02:23:24 localhost kernel: EIP is at nv_adma_port_stop+0x3d/0x53
[sata_nv]
Dec 19 02:23:24 localhost kernel: EAX: 00000015 EBX: e881a480 ECX:
00200046 EDX: 00200046
Dec 19 02:23:24 localhost kernel: ESI: c220b50c EDI: c1979154 EBP:
c197904c ESP: c2429ecc
Dec 19 02:23:24 localhost kernel: DS: 007b ES: 007b FS: 00d8 GS: 0033
SS: 0068
Dec 19 02:23:24 localhost kernel: Process rmmod (pid: 4915, ti=c2428000
task=c25e5a40 task.ti=c2428000)
Dec 19 02:23:24 localhost kernel: Stack: e8823e1b e881a480 00000000
e884398e c220b500 c18a3ec0 c1979154 00000000
Dec 19 02:23:24 localhost kernel: c02818be c1979154 c197904c
0000000d c18a3ec0 c18a3a40 c197904c e88259d0
Dec 19 02:23:24 localhost kernel: e88259d0 c2428000 c0281987
00200286 c197904c c027f8bb c1834600 c197904c
Dec 19 02:23:24 localhost kernel: Call Trace:
Dec 19 02:23:24 localhost kernel: [<e884398e>]
ata_host_release+0x2f/0x92 [libata]
Dec 19 02:23:24 localhost kernel: [<c02818be>]
release_nodes+0x10f/0x12f
Dec 19 02:23:24 localhost kernel: [<c0281987>]
devres_release_all+0x27/0x2a
Dec 19 02:23:24 localhost kernel: [<c027f8bb>]
__device_release_driver+0x72/0x88
Dec 19 02:23:24 localhost kernel: [<c027fccb>] driver_detach+0x76/0xb3
Dec 19 02:23:24 localhost kernel: [<c027f49f>]
bus_remove_driver+0x63/0x81
Dec 19 02:23:24 localhost kernel: [<c0228b5a>]
pci_unregister_driver+0xc/0x57
Dec 19 02:23:24 localhost kernel: [<c0143792>]
sys_delete_module+0x197/0x1bd
Dec 19 02:23:24 localhost kernel: [<c0417673>]
do_page_fault+0x202/0x581
Dec 19 02:23:24 localhost kernel: [<c01570ec>] do_munmap+0x193/0x1ac
Dec 19 02:23:24 localhost kernel: [<c0104e4e>]
sysenter_past_esp+0x5f/0x85
Dec 19 02:23:24 localhost kernel: =======================
Dec 19 02:23:24 localhost kernel: Code: 3e 82 e8 e8 b8 30 90 d7 89 5c 24
04 c7 04 24 0f 3e 82 e8 e8 a8 30 90 d7 8b 5b 10 c7 04 24 1b 3e 82 e8 89
5c 24 04 e8 95 30 90 d7 <66> c7 43 40 00 00 c7 04 24 29 3e 82 e8 e8 83
30 90 d7 5b 58 5b
Dec 19 02:23:24 localhost kernel: EIP: [<e88224e9>]
nv_adma_port_stop+0x3d/0x53 [sata_nv] SS:ESP 0068:c2429ecc
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

2007-12-12 08:18:48

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] sata_nv,adma: fix error when rmmod sata_nv

Kuan Luo wrote:
> hi,
> The below error happens when i rmmod sata_nv in adma mode on ck804
> chipset with 2.6.24 kernel.
> I traced the code and found that the driver attempts to write device mem
> that has been unmapped.
>
> Only simply removing the code" writew(0, mmio + NV_ADMA_CTL);" in the
> nv_adma_port_stop function or remove .port_stop field in nv_adma_ops,
> rmmod is ok.
>
> static void nv_adma_port_stop(struct ata_port *ap)
> {
> struct nv_adma_port_priv *pp = ap->private_data;
> void __iomem *mmio = pp->ctl_block;
>
> VPRINTK("ENTER\n");
> - writew(0, mmio + NV_ADMA_CTL);
> }
>
> Or
> Place pcim_iomap_regions before ata_pci_prepare_native_host in
> nv_init_one function.
> This can guarantee that the code "writew(0, mmio + NV_ADMA_CTL) " write
> device mem before the device mem is unmapped.

Which kernel version are you using? The following commit should have
fixed the problem. Please give a shot at 2.6.24-rc5. Thanks.

commit 32ebbc0c0d5d18c0135b55d1eb0029f48c54aff0
Author: Tejun Heo <[email protected]>
Date: Thu Nov 8 13:09:00 2007 +0900

libata: port and host should be stopped before hardware resources are
released

--
tejun

2007-12-13 03:10:32

by Kuan Luo

[permalink] [raw]
Subject: RE: [PATCH] sata_nv,adma: fix error when rmmod sata_nv

Tejun Heo wrote:
> Which kernel version are you using? The following commit should have
> fixed the problem. Please give a shot at 2.6.24-rc5. Thanks.
>
> commit 32ebbc0c0d5d18c0135b55d1eb0029f48c54aff0
> Author: Tejun Heo <[email protected]>
> Date: Thu Nov 8 13:09:00 2007 +0900
>
> libata: port and host should be stopped before hardware resources are
> released
>
> --
> tejun
>
I have seen your commit. And i apologize for not updating the newest
kernel.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------