diff mbox

[#upstream] sata_sil24: always set protocol override for non-ATAPI data commands

Message ID 4A71FE71.7040002@gmail.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Robert Hancock July 30, 2009, 8:11 p.m. UTC 
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 <hancockrwd@gmail.com>
Tested-by: Mark Lord <liml@rtr.ca>

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark Lord July 30, 2009, 8:18 p.m. UTC | #1 
Robert Hancock wrote:
> 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 <hancockrwd@gmail.com>
> Tested-by: Mark Lord <liml@rtr.ca>
> 
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 77aa8d7..e6946fc 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -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;


Peachy!
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik July 30, 2009, 8:53 p.m. UTC | #2 
Robert Hancock wrote:
> 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 <hancockrwd@gmail.com>
> Tested-by: Mark Lord <liml@rtr.ca>
> 
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 77aa8d7..e6946fc 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -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);
> +		}

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



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo July 31, 2009, 1:20 a.m. UTC | #3 
Jeff Garzik wrote:
>> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
>> index 77aa8d7..e6946fc 100644
>> --- a/drivers/ata/sata_sil24.c
>> +++ b/drivers/ata/sata_sil24.c
>> @@ -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);
>> +        }
> 
> 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.  :-)

> 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.
Robert Hancock Aug. 22, 2009, 5:57 a.m. UTC | #4 
On 07/30/2009 02:11 PM, Robert Hancock wrote:
> 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<hancockrwd@gmail.com>
> Tested-by: Mark Lord<liml@rtr.ca>

Any more comments on this one? Thought I heard an ack from Tejun on it..

>
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 77aa8d7..e6946fc 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -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;

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Lord Aug. 22, 2009, 2:03 p.m. UTC | #5 
Robert Hancock wrote:
> On 07/30/2009 02:11 PM, Robert Hancock wrote:
>> 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<hancockrwd@gmail.com>
>> Tested-by: Mark Lord<liml@rtr.ca>
> 
> Any more comments on this one? Thought I heard an ack from Tejun on it..
> 
>>
>> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
>> index 77aa8d7..e6946fc 100644
>> --- a/drivers/ata/sata_sil24.c
>> +++ b/drivers/ata/sata_sil24.c
>> @@ -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;
> 
..

Hasn't this gone upstream yet??
Heck, it should even go out for -stable, too.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik Aug. 22, 2009, 2:11 p.m. UTC | #6 
On 08/22/2009 10:03 AM, Mark Lord wrote:
> Robert Hancock wrote:
>> On 07/30/2009 02:11 PM, Robert Hancock wrote:
>>> 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<hancockrwd@gmail.com>
>>> Tested-by: Mark Lord<liml@rtr.ca>
>>
>> Any more comments on this one? Thought I heard an ack from Tejun on it..
>>
>>>
>>> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
>>> index 77aa8d7..e6946fc 100644
>>> --- a/drivers/ata/sata_sil24.c
>>> +++ b/drivers/ata/sata_sil24.c
>>> @@ -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;
>>
> ..
>
> 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



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Lord Aug. 22, 2009, 3:25 p.m. UTC | #7 
Jeff Garzik wrote:
> On 08/22/2009 10:03 AM, Mark Lord wrote:
>> Robert Hancock wrote:
>>> On 07/30/2009 02:11 PM, Robert Hancock wrote:
>>>> 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<hancockrwd@gmail.com>
>>>> Tested-by: Mark Lord<liml@rtr.ca>
>>>
>>> Any more comments on this one? Thought I heard an ack from Tejun on it..
>>>
>>>>
>>>> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
>>>> index 77aa8d7..e6946fc 100644
>>>> --- a/drivers/ata/sata_sil24.c
>>>> +++ b/drivers/ata/sata_sil24.c
>>>> @@ -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;
>>>
>> ..
>>
>> 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.
..

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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Lord Aug. 22, 2009, 3:28 p.m. UTC | #8 
Mark Lord wrote:
> Jeff Garzik wrote:
>> On 08/22/2009 10:03 AM, Mark Lord wrote:
..
>>> 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.
> ..
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik Aug. 22, 2009, 4:02 p.m. UTC | #9 
On 08/22/2009 11:25 AM, Mark Lord wrote:
> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik Aug. 22, 2009, 4:10 p.m. UTC | #10 
On 08/22/2009 11:28 AM, Mark Lord wrote:
> Mmm.. I wonder if something like this patch might also be applicable
> to the sata_sil.c driver 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Hancock Aug. 22, 2009, 4:27 p.m. UTC | #11 
On Sat, Aug 22, 2009 at 10:10 AM, Jeff Garzik<jeff@garzik.org> wrote:
> On 08/22/2009 11:28 AM, Mark Lord wrote:
>>
>> Mmm.. I wonder if something like this patch might also be applicable
>> to the sata_sil.c driver 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Hancock Aug. 22, 2009, 4:30 p.m. UTC | #12 
On Sat, Aug 22, 2009 at 8:11 AM, Jeff Garzik<jeff@garzik.org> wrote:
>> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 77aa8d7..e6946fc 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -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;