* NV SATA breakage: jgarzik/libata-dev#upstream etc
@ 2006-09-24 18:57 Krzysztof Halasa
2006-09-25 0:25 ` Jeff Garzik
0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Halasa @ 2006-09-24 18:57 UTC (permalink / raw)
To: Jeff Garzik, Tejun Heo; +Cc: linux-kernel
Hi,
The following commit in libata-dev breaks NV SATA init - I don't
have a dump handy, but the problem is a NULL ptr dereference here:
libata-core.c
int ata_device_add(const struct ata_probe_ent *ent)
{
...
/* register each port bound to this device */
for (i = 0; i < host->n_ports; i++) {
...
/* start port */
rc = ap->ops->port_start(ap);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The problematic commit is fea63e38013ec628ab3f7fddc4c2148064b7910a:
"[PATCH] libata: fix non-uniform ports handling
Non-uniform ports handling got broken while updating libata to handle
those in the same host. Only separate irq for the non-uniform
secondary port was implemented while all other fields (host flags,
transfer mode...) of the secondary port simply shared those of the
first.
For ata_piix combined mode, which ATM is the only user of non-uniform
ports, this causes the secondary port assume the wrong type. This can
cause PATA port to use SATA ops, which results in bogus check on PCS
and detection failure.
This patch adds ata_probe_ent->pinfo2 which points to optional
port_info for the secondary port. For the time being, this seems to
be the simplest solution. This workaround will be removed together
with ata_probe_ent itself after init model is updated to allow more
flexibility."
All details etc. are, as always, available on request.
00:05.0 IDE interface: nVidia Corporation MCP55 SATA Controller (rev a2)
(prog-if 85 [Master SecO PriO])
Subsystem: Micro-Star International Co., Ltd. Unknown device 7250
Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 225
I/O ports at c800 [size=8]
I/O ports at c480 [size=4]
I/O ports at c400 [size=8]
I/O ports at c080 [size=4]
I/O ports at c000 [size=16]
Memory at feaf9000 (32-bit, non-prefetchable) [size=4K]
--
Krzysztof Halasa
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-24 18:57 NV SATA breakage: jgarzik/libata-dev#upstream etc Krzysztof Halasa @ 2006-09-25 0:25 ` Jeff Garzik 2006-09-25 12:51 ` Krzysztof Halasa 0 siblings, 1 reply; 9+ messages in thread From: Jeff Garzik @ 2006-09-25 0:25 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Tejun Heo, linux-kernel Krzysztof Halasa wrote: > Hi, > > The following commit in libata-dev breaks NV SATA init - I don't > have a dump handy, but the problem is a NULL ptr dereference here: > > libata-core.c > int ata_device_add(const struct ata_probe_ent *ent) > { > ... > /* register each port bound to this device */ > for (i = 0; i < host->n_ports; i++) { > ... > /* start port */ > rc = ap->ops->port_start(ap); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > The problematic commit is fea63e38013ec628ab3f7fddc4c2148064b7910a: > "[PATCH] libata: fix non-uniform ports handling > > Non-uniform ports handling got broken while updating libata to handle > those in the same host. Only separate irq for the non-uniform > secondary port was implemented while all other fields (host flags, > transfer mode...) of the secondary port simply shared those of the > first. What's broken, and how does it affect NV sata? That's the chipset on my main dev workstation, and there are no problems here... Jeff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-25 0:25 ` Jeff Garzik @ 2006-09-25 12:51 ` Krzysztof Halasa 2006-09-25 17:15 ` Krzysztof Halasa 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Halasa @ 2006-09-25 12:51 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, linux-kernel Jeff Garzik <jgarzik@pobox.com> writes: >> libata-core.c >> int ata_device_add(const struct ata_probe_ent *ent) >> { >> ... >> /* register each port bound to this device */ >> for (i = 0; i < host->n_ports; i++) { >> ... >> /* start port */ >> rc = ap->ops->port_start(ap); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> The problematic commit is fea63e38013ec628ab3f7fddc4c2148064b7910a: >> "[PATCH] libata: fix non-uniform ports handling >> Non-uniform ports handling got broken while updating libata to handle >> those in the same host. Only separate irq for the non-uniform >> secondary port was implemented while all other fields (host flags, >> transfer mode...) of the secondary port simply shared those of the >> first. > > What's broken, and how does it affect NV sata? NV SATA initialization fails with NULL pointer dereference in ata_device_add (= kernel panic). > That's the chipset on my main dev workstation, and there are no > problems here... I'm a bit surprised... I'm using x86_64 with only one SATA "controller" enabled (ports 0 and 1 only, ports 2-5 are disabled in BIOS). Only port 0 is in use. Gcc 4.1.1. With a64f97f2c351410dfb3099c2369eacf7154b5532 (2.6.18-rc7+, "Merge branch 'tmp' into upstream", just before the commit in question) it works fine: libata version 2.00 loaded. sata_nv 0000:00:05.0: version 2.0 ACPI: PCI Interrupt Link [LSA0] enabled at IRQ 23 GSI 16 sharing vector 0xE1 and IRQ 16 ACPI: PCI Interrupt 0000:00:05.0[A] -> Link [LSA0] -> GSI 23 (level, low) -> IRQ 225 PCI: Setting latency timer of device 0000:00:05.0 to 64 ata1: SATA max UDMA/133 cmd 0xC800 ctl 0xC482 bmdma 0xC000 irq 225 ata2: SATA max UDMA/133 cmd 0xC400 ctl 0xC082 bmdma 0xC008 irq 225 scsi0 : sata_nv ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: ATA-7, max UDMA/133, 488397168 sectors: LBA48 NCQ (depth 0/32) ata1.00: ata1: dev 0 multi count 16 ata1.00: configured for UDMA/133 scsi1 : sata_nv ata2: SATA link down (SStatus 0 SControl 300) ATA: abnormal status 0x7F on port 0xC407 Vendor: ATA Model: ST3250823AS Rev: 3.03 Type: Direct-Access ANSI SCSI revision: 05 -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-25 12:51 ` Krzysztof Halasa @ 2006-09-25 17:15 ` Krzysztof Halasa 2006-09-25 22:20 ` Jeff Garzik [not found] ` <1327be9e0609251112j34ccc787wbcc4886623af2714@mail.gmail.com> 0 siblings, 2 replies; 9+ messages in thread From: Krzysztof Halasa @ 2006-09-25 17:15 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, linux-kernel Ok, rebooted it. After that commit (fea63e38013ec628ab3f7fddc4c2148064b7910a), it does: ata1: SATA max UDMA/133 cmd 0xC800 ctl 0xC482 bmdma 0xC000 irq 225 GPF: 0 [1] RIP: ata_device_add+0x19a/0x530 RAX: 48000002d0b38d48 R14: ffff81003f7cc7e0 Call trace: nv_init_one+0x190/0x1f0 pci_match_device pci_device_probe driver_probe_device __driver_attach __driver_attach bus_for_each_dev driver_register __pci_register_driver nv_init etc. ata_device_add: ... 193: 49 8b 46 08 mov 0x8(%r14),%rax <<<< ap->ops = invalid 197: 4c 89 f7 mov %r14,%rdi <<<< ap 19a: ff 90 f8 00 00 00 callq *0xf8(%rax) <<<< ap->ops->port_start ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ And it GPFs here (ap->ops->port_start(ap)). Actually it seems the ap->ops = RAX is invalid but not exactly a NULL ptr. Now, sata_nv.c: nv_init_one(): struct ata_port_info *ppi; ppi = &nv_port_info[ent->driver_data]; probe_ent = ata_pci_init_native_mode(pdev, &ppi, ATA_PORT_PRIMARY | ATA_ PORT_SECONDARY); while /** * ata_pci_init_native_mode - Initialize native-mode driver * @pdev: pci device to be initialized * @port: array[2] of pointers to port info structures. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Not sure... should the nv_init_one() just read: struct ata_port_info *ppi[2]; ppi[0] = ppi[1] = &nv_port_info[ent->driver_data]; probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_ PORT_SECONDARY); or should ppi[1] = NULL? -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-25 17:15 ` Krzysztof Halasa @ 2006-09-25 22:20 ` Jeff Garzik 2006-09-25 23:50 ` Krzysztof Halasa [not found] ` <1327be9e0609251112j34ccc787wbcc4886623af2714@mail.gmail.com> 1 sibling, 1 reply; 9+ messages in thread From: Jeff Garzik @ 2006-09-25 22:20 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Tejun Heo, linux-kernel Krzysztof Halasa wrote: > ppi[0] = ppi[1] = &nv_port_info[ent->driver_data]; That's probably the best solution. Jeff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-25 22:20 ` Jeff Garzik @ 2006-09-25 23:50 ` Krzysztof Halasa 2006-09-25 23:52 ` Jeff Garzik 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Halasa @ 2006-09-25 23:50 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, linux-kernel Jeff Garzik <jgarzik@pobox.com> writes: > Krzysztof Halasa wrote: >> ppi[0] = ppi[1] = &nv_port_info[ent->driver_data]; > > That's probably the best solution. It fixes the SATA NV problem (I'm not attaching the patch as you already posted similar one). -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-25 23:50 ` Krzysztof Halasa @ 2006-09-25 23:52 ` Jeff Garzik 2006-09-26 0:29 ` Krzysztof Halasa 0 siblings, 1 reply; 9+ messages in thread From: Jeff Garzik @ 2006-09-25 23:52 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Tejun Heo, linux-kernel Krzysztof Halasa wrote: > Jeff Garzik <jgarzik@pobox.com> writes: > >> Krzysztof Halasa wrote: >>> ppi[0] = ppi[1] = &nv_port_info[ent->driver_data]; >> That's probably the best solution. > > It fixes the SATA NV problem (I'm not attaching the patch as you already > posted similar one). The argument to ata_pci_init_native_mode() was also updated. Just for sanity's sake, could you test linux-2.5.git + my patch? Jeff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NV SATA breakage: jgarzik/libata-dev#upstream etc 2006-09-25 23:52 ` Jeff Garzik @ 2006-09-26 0:29 ` Krzysztof Halasa 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Halasa @ 2006-09-26 0:29 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, linux-kernel Jeff Garzik <jgarzik@pobox.com> writes: > The argument to ata_pci_init_native_mode() was also updated. > > Just for sanity's sake, could you test linux-2.5.git + my patch? I've already tested nearly identical patch against your latest tree (currently equal to Linus' - a6d967a485c67ec8a1276261f39d81ace6a3e308) and it works fine, so Acked-By: Krzysztof Halasa <khc@pm.waw.pl> -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <1327be9e0609251112j34ccc787wbcc4886623af2714@mail.gmail.com>]
* [PATCH v1] Re: NV SATA breakage: jgarzik/libata-dev#upstream etc [not found] ` <1327be9e0609251112j34ccc787wbcc4886623af2714@mail.gmail.com> @ 2006-09-25 22:39 ` Jeff Garzik 0 siblings, 0 replies; 9+ messages in thread From: Jeff Garzik @ 2006-09-25 22:39 UTC (permalink / raw) To: Diego Rosario Brogna Cc: linux-kernel@vger.kernel.org Krzysztof Halasa, Tejun Heo, linux-ide@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 53 bytes --] Does the attached patch fix the SATA oops? Jeff [-- Attachment #2: patch --] [-- Type: text/plain, Size: 3143 bytes --] diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c index 27c22fe..8cd730f 100644 --- a/drivers/ata/sata_nv.c +++ b/drivers/ata/sata_nv.c @@ -484,7 +484,7 @@ static void nv_error_handler(struct ata_ static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent) { static int printed_version = 0; - struct ata_port_info *ppi; + struct ata_port_info *ppi[2]; struct ata_probe_ent *probe_ent; int pci_dev_busy = 0; int rc; @@ -520,8 +520,8 @@ static int nv_init_one (struct pci_dev * rc = -ENOMEM; - ppi = &nv_port_info[ent->driver_data]; - probe_ent = ata_pci_init_native_mode(pdev, &ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); + ppi[0] = ppi[1] = &nv_port_info[ent->driver_data]; + probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); if (!probe_ent) goto err_out_regions; diff --git a/drivers/ata/sata_sis.c b/drivers/ata/sata_sis.c index 9b17375..18d49ff 100644 --- a/drivers/ata/sata_sis.c +++ b/drivers/ata/sata_sis.c @@ -240,7 +240,7 @@ static int sis_init_one (struct pci_dev struct ata_probe_ent *probe_ent = NULL; int rc; u32 genctl; - struct ata_port_info *ppi; + struct ata_port_info *ppi[2]; int pci_dev_busy = 0; u8 pmr; u8 port2_start; @@ -265,8 +265,8 @@ static int sis_init_one (struct pci_dev if (rc) goto err_out_regions; - ppi = &sis_port_info; - probe_ent = ata_pci_init_native_mode(pdev, &ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); + ppi[0] = ppi[1] = &sis_port_info; + probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); if (!probe_ent) { rc = -ENOMEM; goto err_out_regions; diff --git a/drivers/ata/sata_uli.c b/drivers/ata/sata_uli.c index 8fc6e80..dd76f37 100644 --- a/drivers/ata/sata_uli.c +++ b/drivers/ata/sata_uli.c @@ -185,7 +185,7 @@ static int uli_init_one (struct pci_dev { static int printed_version; struct ata_probe_ent *probe_ent; - struct ata_port_info *ppi; + struct ata_port_info *ppi[2]; int rc; unsigned int board_idx = (unsigned int) ent->driver_data; int pci_dev_busy = 0; @@ -211,8 +211,8 @@ static int uli_init_one (struct pci_dev if (rc) goto err_out_regions; - ppi = &uli_port_info; - probe_ent = ata_pci_init_native_mode(pdev, &ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); + ppi[0] = ppi[1] = &uli_port_info; + probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); if (!probe_ent) { rc = -ENOMEM; goto err_out_regions; diff --git a/drivers/ata/sata_via.c b/drivers/ata/sata_via.c index 7f087ae..a72a238 100644 --- a/drivers/ata/sata_via.c +++ b/drivers/ata/sata_via.c @@ -318,9 +318,10 @@ static void vt6421_init_addrs(struct ata static struct ata_probe_ent *vt6420_init_probe_ent(struct pci_dev *pdev) { struct ata_probe_ent *probe_ent; - struct ata_port_info *ppi = &vt6420_port_info; - - probe_ent = ata_pci_init_native_mode(pdev, &ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); + struct ata_port_info *ppi[2]; + + ppi[0] = ppi[1] = &vt6420_port_info; + probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY); if (!probe_ent) return NULL; ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-09-26 0:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-24 18:57 NV SATA breakage: jgarzik/libata-dev#upstream etc Krzysztof Halasa
2006-09-25 0:25 ` Jeff Garzik
2006-09-25 12:51 ` Krzysztof Halasa
2006-09-25 17:15 ` Krzysztof Halasa
2006-09-25 22:20 ` Jeff Garzik
2006-09-25 23:50 ` Krzysztof Halasa
2006-09-25 23:52 ` Jeff Garzik
2006-09-26 0:29 ` Krzysztof Halasa
[not found] ` <1327be9e0609251112j34ccc787wbcc4886623af2714@mail.gmail.com>
2006-09-25 22:39 ` [PATCH v1] " Jeff Garzik
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.