Skip to contentSkip to conversation

sata_mv: enable async_notify for 60x1 Rev.C0 and higher

PATCH In review Mark Lord 3 participants 11 messages Jun 18, 2008 – Jul 4, 2008 libata

lore
Share & tools
lore

Mark Lord

author
#0 lore raw

PATCH 1/2 sata_mv: enable async_notify for 60x1 Rev.C0 and higher

The early chipsets cannot safely handle Async Notification (AN), but 6041/6081 chip revision "C0" (and newer) can handle it.

So allow AN for "C0" and higher.

This enables use of hotplug on PMP ports for the 6041/6081 PCI Rev.9 chips.

drivers/ata/sata_mv.c +0 -0
@@ -1322,6 +1322,9 @@
		goto out_port_free_dma_mem;
	memset(pp->crpb, 0, MV_CRPB_Q_SZ);

+ /* 6041/6081 Rev. "C0" (and newer) are okay with async notify */
+ if (hpriv->hp_flags & MV_HP_ERRATA_60X1C0)
+ ap->flags |= ATA_FLAG_AN;
/*
* For GEN_I, there's no NCQ, so we only allocate a single sg_tbl.
* For later hardware, we need one unique sg_tbl per NCQ tag.

author
#1 lore raw

PATCH 2/2 sata_mv: warn on PIO with multiple DRQs

Chip errata sometimes prevents reliable use of PIO commands which involve more than a single DRQ (data request). In normal operation, libata should not generate such PIO commands (uses DMA instead), but they could be sent in via SG_IO from userspace.

A full workaround might be to break up such commands into sequences of single DRQ ones, but that's just way too complex for something that doesn't normally happen in real life.

So, allow the attempt (it often works, despite the errata), but log the event for reference when somebody screams.

---

Tejun/Alan/Jeff:

We may also want a patch to prevent libata-eh from downshifting all the way to PIO on these chipsets, but I am not sure how to accomplish that from inside sata_mv. Any suggestions?

drivers/ata/sata_mv.c +0 -0
@@ -1595,6 +1595,24 @@
 
	if ((qc->tf.protocol != ATA_PROT_DMA) &&
	    (qc->tf.protocol != ATA_PROT_NCQ)) {

+ static int limit_warnings = 10;
+ /*
+ * Errata SATA#16, SATA#24: warn if multiple DRQs expected.
+ *
+ * Someday, we might implement special polling workarounds
+ * for these, but it all seems rather unnecessary since we
+ * normally use only DMA for commands which transfer more
+ * than a single block of data.
+ *
+ * Much of the time, this could just work regardless.
+ * So for now, just log the incident, and allow the attempt.
+ */
+ if (limit_warnings && (qc->nbytes / qc->sect_size) > 1) {
+ --limit_warnings;
+ ata_link_printk(qc->dev->link, KERN_WARNING, DRV_NAME
+ ": attempting PIO w/multiple DRQ: "
+ "this may fail due to h/w errata\n");
+ }
/*
* We're about to send a non-EDMA capable command to the
* port. Turn off EDMA so there won't be problems accessing

#2 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

Quoted 5 lines · Mark Lord

Chip errata sometimes prevents reliable use of PIO commands which involve more than a single DRQ (data request). In normal operation, libata should not generate such PIO commands (uses DMA instead), but they could be sent in via SG_IO from userspace.

Quoted 8 lines

A full workaround might be to break up such commands into sequences of single DRQ ones, but that's just way too complex for something that doesn't normally happen in real life.

So, allow the attempt (it often works, despite the errata), but log the event for reference when somebody screams.

applied patches 1-2

Quoted 5 lines

Tejun/Alan/Jeff:

We may also want a patch to prevent libata-eh from downshifting all the way to PIO on these chipsets, but I am not sure how to accomplish that from inside sata_mv. Any suggestions?

Do we really want that? Seems to me we need that for older bridged PATA devices attached to a SATA bridge.

Jeff

author
#3 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

Quoted 20 lines · Jeff Garzik
Quoted 5 lines · Mark Lord

Chip errata sometimes prevents reliable use of PIO commands which involve more than a single DRQ (data request). In normal operation, libata should not generate such PIO commands (uses DMA instead), but they could be sent in via SG_IO from userspace.

Quoted 8 lines

A full workaround might be to break up such commands into sequences of single DRQ ones, but that's just way too complex for something that doesn't normally happen in real life.

So, allow the attempt (it often works, despite the errata), but log the event for reference when somebody screams.

applied patches 1-2

Quoted 7 lines
Quoted 5 lines

Tejun/Alan/Jeff:

We may also want a patch to prevent libata-eh from downshifting all the way to PIO on these chipsets, but I am not sure how to accomplish that from inside sata_mv. Any suggestions?

Do we really want that? Seems to me we need that for older bridged PATA devices attached to a SATA bridge.

..

Just about everything can do some form of DMA nowadays, but regardless of that the sata_mv chipsets have errata for PIO of anything more than a single sector.

In practice, I haven't managed to trigger the issue here, but the errata descriptions indicate that it should be a major issue for any volume of multi-sector PIO commands.

Cheers

#4 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

Quoted 8 lines · Mark Lord
Quoted 6 lines
Quoted 3 lines

We may also want a patch to prevent libata-eh from downshifting all the way to PIO on these chipsets, but I am not sure how to accomplish that from inside sata_mv. Any suggestions?

Do we really want that? Seems to me we need that for older bridged PATA devices attached to a SATA bridge.

..

Quoted 7 lines

Just about everything can do some form of DMA nowadays, but regardless of that the sata_mv chipsets have errata for PIO of anything more than a single sector.

In practice, I haven't managed to trigger the issue here, but the errata descriptions indicate that it should be a major issue for any volume of multi-sector PIO commands.

The following nack'd patch has something similar if it's really necessary.

http://article.gmane.org/gmane.linux.ide/32282

Signature · 1 line
tejun
#5 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

In reply to Mark Lord
Quoted 7 lines · Mark Lord

Just about everything can do some form of DMA nowadays, but regardless of that the sata_mv chipsets have errata for PIO of anything more than a single sector.

In practice, I haven't managed to trigger the issue here, but the errata descriptions indicate that it should be a major issue for any volume of multi-sector PIO commands.

I need to review the state of PIO-multi, but IIRC it's not used by default.

We should be able to handle this errata without banning all PIO...

Jeff

author
#6 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Mark Lord

Just about everything can do some form of DMA nowadays, but regardless of that the sata_mv chipsets have errata for PIO of anything more than a single sector.

Quoted 8 lines
Quoted 3 lines

In practice, I haven't managed to trigger the issue here, but the errata descriptions indicate that it should be a major issue for any volume of multi-sector PIO commands.

I need to review the state of PIO-multi, but IIRC it's not used by default.

We should be able to handle this errata without banning all PIO...

..

I'm not really all that worried about it.
But it affects *non multi* PIO as well -- anything that can generate more than a single DRQ per command-issue.

So a READ_SECTORS or WRITE_SECTORS (pio) for anything more than a single sector could do it, in theory.

If we *really* care, then I'll dig into it deeper in the next kernel. It can be worked around in the IRQ handler, by taking great care in the exact sequence of IRQ-arrival and DRQ-status checks.

Heck.. we might even have it right (by accident) today.

Cheers

#7 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

In reply to Mark Lord
Quoted 3 lines · Mark Lord

In practice, I haven't managed to trigger the issue here, but the errata descriptions indicate that it should be a major issue for any volume of multi-sector PIO commands.

To be specific: multi-sector or multi-DRQ?

Jeff

author
#8 lore raw

Re: [PATCH 02/02] sata_mv: warn on PIO with multiple DRQs

Quoted 7 lines · Jeff Garzik
Quoted 3 lines · Mark Lord

In practice, I haven't managed to trigger the issue here, but the errata descriptions indicate that it should be a major issue for any volume of multi-sector PIO commands.

To be specific: multi-sector or multi-DRQ?

..

Multi-DRQ. There's also an 8-sector blocksize limit on READ/WRITE_MULTI.

Cheers

author
#9 lore raw

PATCH 1/1 sata_mv: safer logic for limit_warnings

In reply to Mark Lord

There is a miniscule chance that two separate host controllers might be in sata_mv at the same time and manage to decrement the static limit_warnings variable below zero.

Fix the comparison to deal with it.

---

(blush) How did I miss that ???

/drivers/ata/sata_mv.c renamed from drivers/ata/sata_mv.c +0 -0
@@ -1607,7 +1607,7 @@

* Much of the time, this could just work regardless.
* So for now, just log the incident, and allow the attempt. */
- if (limit_warnings && (qc->nbytes / qc->sect_size) > 1) {

+		if (limit_warnings > 0 && (qc->nbytes / qc->sect_size) > 1) {
			--limit_warnings;
			ata_link_printk(qc->dev->link, KERN_WARNING, DRV_NAME
					": attempting PIO w/multiple DRQ: "
Applied
#10 lore raw

Re: [PATCH] sata_mv: safer logic for limit_warnings

Quoted 14 lines · Mark Lord

There is a miniscule chance that two separate host controllers might be in sata_mv at the same time and manage to decrement the static limit_warnings variable below zero.

Fix the comparison to deal with it.

---

(blush) How did I miss that ???

/drivers/ata/sata_mv.c 2008-06-18 21:52:19.000000000 -0400 line 1612

Quoted 7 lines
/drivers/ata/sata_mv.c 2008-06-18 21:52:19.000000000 -0400 renamed from drivers/ata/sata_mv.c 2008-06-17 19:22:15.000000000 -0400 +1 -1
@@ -1607,7 +1607,7 @@
         * Much of the time, this could just work regardless.
         * So for now, just log the incident, and allow the attempt.
         */
-        if (limit_warnings && (qc->nbytes / qc->sect_size) > 1) {
+        if (limit_warnings > 0 && (qc->nbytes / qc->sect_size) > 1) {
            --limit_warnings;
            ata_link_printk(qc->dev->link, KERN_WARNING, DRV_NAME

applied. sorry, this got buried, should have gone in sooner.