sata_sil24: always set protocol override for non-ATAPI data commands
PATCH 1/1 sata_sil24: always set protocol override for non-ATAPI data commands
The sil24 hardware has a built-in list of commands and associated protocols that gets used by default to decide how to handle a given command. However, if the command is not known to the controller then it presumably assumes it to be a non-data command which then causes protocol mismatch errors if the device ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command causes this issue since it's a DMA data-out command.
Since we should always know best what protocol the command should be using, let's just set the override flag to inform the controller what protocol to use for all non-ATAPI commands with data transfer.
drivers/ata/sata_sil24.c +11 -0
@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb = &cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags &ATA_TFLAG_WRITE ) + prot |=PRB_PROT_WRITE ; +else + prot |=PRB_PROT_READ ; + prb->prot =cpu_to_le16 (prot); + } }else { prb = &cb->atapi.prb; sge = cb->atapi.sge;
Quoted 26 lines · Robert Hancock
The sil24 hardware has a built-in list of commands and associated protocols that gets used by default to decide how to handle a given command. However, if the command is not known to the controller then it presumably assumes it to be a non-data command which then causes protocol mismatch errors if the device ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command causes this issue since it's a DMA data-out command.
Since we should always know best what protocol the command should be using, let's just set the override flag to inform the controller what protocol to use for all non-ATAPI commands with data transfer.
Signed-off-by Robert Hancock <[email protected]>Tested-by Mark Lord <[email protected]>drivers/ata/sata_sil24.c +4
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb = &cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol))
drivers/ata/sata_sil24.c static void sil24_qc_prep(struct ata_queued_cmd *qc) · line 862
Quoted 10 lines
drivers/ata/sata_sil24.c +7
-0+ prot |=PRB_PROT_NCQ ; +if (qc->tf.flags &ATA_TFLAG_WRITE ) + prot |=PRB_PROT_WRITE ; +else + prot |=PRB_PROT_READ ; + prb->prot =cpu_to_le16 (prot); + } }else { prb = &cb->atapi.prb; sge = cb->atapi.sge;
Peachy!
Quoted 23 lines · Robert Hancock
The sil24 hardware has a built-in list of commands and associated protocols that gets used by default to decide how to handle a given command. However, if the command is not known to the controller then it presumably assumes it to be a non-data command which then causes protocol mismatch errors if the device ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command causes this issue since it's a DMA data-out command.
Since we should always know best what protocol the command should be using, let's just set the override flag to inform the controller what protocol to use for all non-ATAPI commands with data transfer.
Signed-off-by Robert Hancock <[email protected]>Tested-by Mark Lord <[email protected]>drivers/ata/sata_sil24.c +1
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb = &cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) {
drivers/ata/sata_sil24.c static void sil24_qc_prep(struct ata_queued_cmd *qc) · line 859
Quoted 10 lines
drivers/ata/sata_sil24.c +10
-0+u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags &ATA_TFLAG_WRITE ) + prot |=PRB_PROT_WRITE ; +else + prot |=PRB_PROT_READ ; + prb->prot =cpu_to_le16 (prot); + }
I'm trying to remember why we did not do this originally -- Tejun, do you recall?
I do not see any prohibition in the docs, so I am inclined to apply this.
Jeff
Quoted 14 lines · Jeff Garzik
Quoted 14 lines
drivers/ata/sata_sil24.c +6
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb = &cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags &ATA_TFLAG_WRITE )
Quoted 8 lines
drivers/ata/sata_sil24.c static void sil24_qc_prep(struct ata_queued_cmd *qc) · line 859
Quoted 5 lines
drivers/ata/sata_sil24.c +5
-0+ prot |=PRB_PROT_WRITE ; + else + prot |= PRB_PROT_READ; + prb->prot =cpu_to_le16 (prot); + }I'm trying to remember why we did not do this originally -- Tejun, do you recall?
Because that was how the example driver from SIMG worked. :-)
Quoted 2 lines
I do not see any prohibition in the docs, so I am inclined to apply this.
Eh... My original thought was to set the protocol only for new cmds for the sake of least surprise but it does make sense to set the protocol always. Going through the doc... Yeap, it should just be fine.
Thanks.
Signature · 1 line
tejun
Quoted 7 lines · Robert Hancock
The sil24 hardware has a built-in list of commands and associated protocols that gets used by default to decide how to handle a given command. However, if the command is not known to the controller then it presumably assumes it to be a non-data command which then causes protocol mismatch errors if the device ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command causes this issue since it's a DMA data-out command.
Quoted 6 lines
Since we should always know best what protocol the command should be using, let's just set the override flag to inform the controller what protocol to use for all non-ATAPI commands with data transfer.
Signed-off-by Robert Hancock<[email protected]>Tested-by Mark Lord<[email protected]>
Any more comments on this one? Thought I heard an ack from Tejun on it..
Quoted 23 lines
drivers/ata/sata_sil24.c +11
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb =&cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags&ATA_TFLAG_WRITE ) + prot |=PRB_PROT_WRITE ; +else + prot |=PRB_PROT_READ ; + prb->prot =cpu_to_le16 (prot); + } }else { prb =&cb->atapi.prb; sge = cb->atapi.sge;
Quoted 32 lines · Robert Hancock
Quoted 7 lines · Robert Hancock
The sil24 hardware has a built-in list of commands and associated protocols that gets used by default to decide how to handle a given command. However, if the command is not known to the controller then it presumably assumes it to be a non-data command which then causes protocol mismatch errors if the device ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command causes this issue since it's a DMA data-out command.
Quoted 6 lines
Since we should always know best what protocol the command should be using, let's just set the override flag to inform the controller what protocol to use for all non-ATAPI commands with data transfer.
Signed-off-by Robert Hancock<[email protected]>Tested-by Mark Lord<[email protected]>Any more comments on this one? Thought I heard an ack from Tejun on it..
Quoted 15 lines
drivers/ata/sata_sil24.c +6
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb =&cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags&ATA_TFLAG_WRITE )
Quoted 9 lines
drivers/ata/sata_sil24.c static void sil24_qc_prep(struct ata_queued_cmd *qc) · line 862
Quoted 8 lines
drivers/ata/sata_sil24.c +5
-0+ prot |=PRB_PROT_WRITE ; + else + prot |= PRB_PROT_READ; + prb->prot =cpu_to_le16 (prot); + } }else { prb =&cb->atapi.prb; sge = cb->atapi.sge;
..
Hasn't this gone upstream yet??
Heck, it should even go out for -stable, too.
Quoted 50 lines · Mark Lord
Quoted 40 lines · Robert Hancock
Quoted 12 lines · Robert Hancock
The sil24 hardware has a built-in list of commands and associated protocols
that gets used by default to decide how to handle a given command. However,
if the command is not known to the controller then it presumably assumes it to
be a non-data command which then causes protocol mismatch errors if the device
ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command
causes this issue since it's a DMA data-out command.Quoted 8 lines
Since we should always know best what protocol the command should be using,
let's just set the override flag to inform the controller what protocol to use
for all non-ATAPI commands with data transfer.Signed-off-by Robert Hancock<[email protected]>Tested-by Mark Lord<[email protected]>Any more comments on this one? Thought I heard an ack from Tejun on it..
Quoted 16 lines
drivers/ata/sata_sil24.c +6
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb =&cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags&ATA_TFLAG_WRITE )Quoted 9 lines
Quoted 8 lines
drivers/ata/sata_sil24.c +5
-0+ prot |=PRB_PROT_WRITE ; + else + prot |= PRB_PROT_READ; + prb->prot =cpu_to_le16 (prot); + } }else { prb =&cb->atapi.prb;sge =
cb->atapi.sge;
Quoted 4 lines
..
Hasn't this gone upstream yet??
Heck, it should even go out for -stable, too.
It's queued for 2.6.32... I'd rather be more conservative and get wider testing before pushing such a fundamental change to sata_sil24 data xfer path upon everyone.
Jeff
Quoted 51 lines · Jeff Garzik
Quoted 50 lines · Mark Lord
Quoted 49 lines · Robert Hancock
Quoted 12 lines · Robert Hancock
The sil24 hardware has a built-in list of commands and associated protocols
that gets used by default to decide how to handle a given command. However,
if the command is not known to the controller then it presumably assumes it to
be a non-data command which then causes protocol mismatch errors if the device
ends up requesting data transfer. The new DATA SET MANAGEMENT - Trim command
causes this issue since it's a DMA data-out command.Quoted 8 lines
Since we should always know best what protocol the command should be using,
let's just set the override flag to inform the controller what protocol to use
for all non-ATAPI commands with data transfer.Signed-off-by Robert Hancock<[email protected]>Tested-by Mark Lord<[email protected]>Any more comments on this one? Thought I heard an ack from Tejun on it..
Quoted 24 lines
drivers/ata/sata_sil24.c +11
-0@@ -846,6 +846,17 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)if (!ata_is_atapi (qc->tf.protocol)) { prb =&cb->ata.prb; sge = cb->ata.sge; +if (ata_is_data (qc->tf.protocol)) { +u16 prot =0 ; + ctrl =PRB_CTRL_PROTOCOL ; +if (ata_is_ncq (qc->tf.protocol)) + prot |=PRB_PROT_NCQ ; +if (qc->tf.flags&ATA_TFLAG_WRITE ) + prot |=PRB_PROT_WRITE ; +else + prot |=PRB_PROT_READ ; + prb->prot =cpu_to_le16 (prot); + } }else { prb =&cb->atapi.prb;sge =
cb->atapi.sge;
Quoted 6 lines
Quoted 4 lines
..
Hasn't this gone upstream yet??
Heck, it should even go out for -stable, too.It's queued for 2.6.32... I'd rather be more conservative and get wider testing before pushing such a fundamental change to
sata_sil24data xfer path upon everyone.
..
Whatever. It's very well tested (and in continuous use) here on two systems with siI-3132 controllers. So we'll want to hear from users of the 3124 and 3131/3531 chips for completeness, then.
Cheers
Quoted 2 lines · Mark Lord
Quoted 1 line · Jeff Garzik
On 08/22/2009 10:03 AM, Mark Lord wrote:
..
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
Hasn't this gone upstream yet??
Heck, it should even go out for -stable, too.It's queued for 2.6.32... I'd rather be more conservative and get wider testing before pushing such a fundamental change to
sata_sil24data xfer path upon everyone...
Whatever. It's very well tested (and in continuous use) here on two systems with siI-3132 controllers. So we'll want to hear from users of the 3124 and 3131/3531 chips for completeness, then.
..
Mmm.. I wonder if something like this patch might also be applicable to the sata_sil.c driver too?
I've got a couple of those chips around here someplace, so maybe I'll give that a spin.
Quoted 2 lines · Mark Lord
Mmm.. I wonder if something like this patch might also be applicable to the
sata_sil.cdriver too?
Not AFAICT. The 3112 has a Command Protocol Table inside the chip. Each command reads the ATA command protocol from that table. The table is programmable (see section 10.3 of [1]), but not on a per-command basis. The per-command PRD format is pretty much legacy IDE[2], and nothing like the modern FIS-based controllers.
Jeff
[1] http://gkernel.sourceforge.net/specs/sii/3112A_SiI-DS-0095-B2.pdf.bz2
[2] bmdma2 modifies the PRD format a bit, but it is not relevant to this thread
Quoted 11 lines · Jeff Garzik
Quoted 3 lines · Mark Lord
Mmm.. I wonder if something like this patch might also be applicable to the
sata_sil.cdriver too?Not AFAICT. The 3112 has a Command Protocol Table inside the chip. Each command reads the ATA command protocol from that table. The table is programmable (see section 10.3 of [1]), but not on a per-command basis. The per-command PRD format is pretty much legacy IDE[2], and nothing like the modern FIS-based controllers.
I think it should be possible to send trim commands through the 311x, but the protocol override is done differently, you'd have to send a VS Set Command Protocol command in order to set the protocol code for that command/feature combination to the correct DMA protcol code. It's not quite as general a solution as with the 3124/3132 controllers (you have to tell it specifically about each command it doesn't know about). Also the set protocols get erased on COMINIT/COMRESET so you'd have to redo the override post-reset.
Quoted 3 lines · Mark Lord
Whatever. It's very well tested (and in continuous use) here on two systems with siI-3132 controllers. So we'll want to hear from users of the 3124 and 3131/3531 chips for completeness, then.
Given that (a) Silicon Image's driver and the Other OS do not default to this data path, and (b) your mix of opcodes and workloads is inevitably different from others in the field, it is more than just completeness.
It is entirely possible that this sata_sil24 core data path change has never seen mass testing, ever. So, the validation level needs to be higher than "it works for Mark" :)
Jeff
Quoted 6 lines · Jeff Garzik
Quoted 2 lines
Hasn't this gone upstream yet??
Heck, it should even go out for -stable, too.It's queued for 2.6.32... I'd rather be more conservative and get wider testing before pushing such a fundamental change to
sata_sil24data xfer path upon everyone.
I think .32 is fair, it's not impossible it could break something.. just making sure it didn't get dropped on the floor. If it works out well it likely could be pushed to stable later though.