ipr SATA problems in 2.6.20

ipr SATA problems in 2.6.20

[Brian King]

I'm currently debugging some problems with SATA on ipr and figured I would
give a heads up. The problems include overlength errors and data corruption.
Basically SATA on ipr is completely broken in its current state.
>From what I have found so far, it looks like due to some libata changes
made recently, qc->nbytes is no longer getting setup for ATAPI devices
all the time.

Jeff - should qc->nbytes be used a libata user? If not, what is the proper
way to determine the total command length in bytes? Summing all the sg entries?
I was using nbytes + pad_len.

The other oddity I've been seeing is that I am getting zero length commands,
such as TEST_UNIT_READY with a dma_dir of DMA_FROM_DEVICE. Shouldn't this be
DMA_NONE? I'm still tracking this down.


Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: ipr SATA problems in 2.6.20

[Alan]

> The other oddity I've been seeing is that I am getting zero length commands,
> such as TEST_UNIT_READY with a dma_dir of DMA_FROM_DEVICE. Shouldn't this be
> DMA_NONE? I'm still tracking this down.

I was looking at a PATA trace that was looking the same 2 days ago and
couldn't figure what was going on. 

Alan

Re: ipr SATA problems in 2.6.20

[Jeff Garzik]

Alan wrote:
>> The other oddity I've been seeing is that I am getting zero length commands,
>> such as TEST_UNIT_READY with a dma_dir of DMA_FROM_DEVICE. Shouldn't this be
>> DMA_NONE? I'm still tracking this down.
> 
> I was looking at a PATA trace that was looking the same 2 days ago and
> couldn't figure what was going on. 

Tejun recently updated the CDB length areas of the code.  I bet it's 
either a bug somewhere in there, or the SCSI layer isn't passing us 
proper command lengths in a case or two.

Additional traces (starting with SCSI command, before it hits libata) 
would be helpful.

	Jeff

Re: ipr SATA problems in 2.6.20

[James Bottomley]

On Tue, 2007-01-16 at 17:45 -0500, Jeff Garzik wrote:
> Tejun recently updated the CDB length areas of the code.  I bet it's 
> either a bug somewhere in there, or the SCSI layer isn't passing us 
> proper command lengths in a case or two.
> 
> Additional traces (starting with SCSI command, before it hits libata) 
> would be helpful.

Actually, this looks like a potential bug in atapi_xlat():  it doesn't
set qc->dma_dir.  Shouldn't it be setting it from
qc->scsicmd->sc_data_direction like ata_scsi_translate() does?

James

Re: ipr SATA problems in 2.6.20

[Brian King]

James Bottomley wrote:
> On Tue, 2007-01-16 at 17:45 -0500, Jeff Garzik wrote:
>> Tejun recently updated the CDB length areas of the code.  I bet it's
>> either a bug somewhere in there, or the SCSI layer isn't passing us
>> proper command lengths in a case or two.
>>
>> Additional traces (starting with SCSI command, before it hits libata)
>> would be helpful.
> 
> Actually, this looks like a potential bug in atapi_xlat():  it doesn't
> set qc->dma_dir.  Shouldn't it be setting it from
> qc->scsicmd->sc_data_direction like ata_scsi_translate() does?

I think we are OK here since atapi_xlate is only ever called by
ata_scsi_translate.

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: ipr SATA problems in 2.6.20

[Alan]

On Tue, 16 Jan 2007 17:45:56 -0500
Jeff Garzik <jgarzik@pobox.com> wrote:

> Alan wrote:
> >> The other oddity I've been seeing is that I am getting zero length commands,
> >> such as TEST_UNIT_READY with a dma_dir of DMA_FROM_DEVICE. Shouldn't this be
> >> DMA_NONE? I'm still tracking this down.
> > 
> > I was looking at a PATA trace that was looking the same 2 days ago and
> > couldn't figure what was going on. 
> 
> Tejun recently updated the CDB length areas of the code.  I bet it's 
> either a bug somewhere in there, or the SCSI layer isn't passing us 
> proper command lengths in a case or two.

That might explain a few things - the qemu PIIX3 emulation bug possibly
and also perhaps the recent NVidia iommu fun.

Alan

Re: ipr SATA problems in 2.6.20

[James Bottomley]

On Tue, 2007-01-16 at 17:17 -0600, Brian King wrote:
> I think we are OK here since atapi_xlate is only ever called by
> ata_scsi_translate.

True.  But can you tell me where it gets set in ata_scsi_translate if
sc_data_direction is DMA_NONE ...

James

Re: ipr SATA problems in 2.6.20

[Brian King]

Brian King wrote:
> James Bottomley wrote:
>> On Tue, 2007-01-16 at 17:45 -0500, Jeff Garzik wrote:
>>> Tejun recently updated the CDB length areas of the code.  I bet it's
>>> either a bug somewhere in there, or the SCSI layer isn't passing us
>>> proper command lengths in a case or two.
>>>
>>> Additional traces (starting with SCSI command, before it hits libata)
>>> would be helpful.
>> Actually, this looks like a potential bug in atapi_xlat():  it doesn't
>> set qc->dma_dir.  Shouldn't it be setting it from
>> qc->scsicmd->sc_data_direction like ata_scsi_translate() does?
> 
> I think we are OK here since atapi_xlate is only ever called by
> ata_scsi_translate.

I spoke too soon... ata_scsi_translate only sets up dma_dir if its
a read or write, which means it never gets initialized if the direction
is DMA_NONE. 

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Tejun Heo]

libata didn't used to init qc->dma_dir to any specific value on qc
initialization and command translation path didn't set qc->dma_dir if
the command doesn't need data transfer.  This made non-data commands
to have random qc->dma_dir.

This usually doesn't cause problem because LLDs usually check
qc->protocol first and look at qc->dma_dir iff the command needs data
transfer but this doesn't hold for all LLDs.

It might be worthwhile to rename qc->dma_dir to qc->data_dir as we use
the field to tag data direction for both PIO and DMA protocols.

This problem has been spotted by James Bottomley.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: James Bottomley <James.Bottomley@SteelEye.com>

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7cfc18f..925ad7f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1139,6 +1139,7 @@ static inline void ata_tf_init(struct ata_device *dev, struct ata_taskfile *tf)
 
 static inline void ata_qc_reinit(struct ata_queued_cmd *qc)
 {
+	qc->dma_dir = DMA_NONE;
 	qc->__sg = NULL;
 	qc->flags = 0;
 	qc->cursect = qc->cursg = qc->cursg_ofs = 0;

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Brian King]

ACK

Tejun Heo wrote:
> libata didn't used to init qc->dma_dir to any specific value on qc
> initialization and command translation path didn't set qc->dma_dir if
> the command doesn't need data transfer.  This made non-data commands
> to have random qc->dma_dir.
> 
> This usually doesn't cause problem because LLDs usually check
> qc->protocol first and look at qc->dma_dir iff the command needs data
> transfer but this doesn't hold for all LLDs.
> 
> It might be worthwhile to rename qc->dma_dir to qc->data_dir as we use
> the field to tag data direction for both PIO and DMA protocols.
> 
> This problem has been spotted by James Bottomley.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: James Bottomley <James.Bottomley@SteelEye.com>
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 7cfc18f..925ad7f 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1139,6 +1139,7 @@ static inline void ata_tf_init(struct ata_device *dev, struct ata_taskfile *tf)
> 
>  static inline void ata_qc_reinit(struct ata_queued_cmd *qc)
>  {
> +	qc->dma_dir = DMA_NONE;
>  	qc->__sg = NULL;
>  	qc->flags = 0;
>  	qc->cursect = qc->cursg = qc->cursg_ofs = 0;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Jeff Garzik]

Brian King wrote:
> ACK

Does this response mean that you've tested it, and successfully verified 
your problem is gone?

	Jeff

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Brian King]

Jeff Garzik wrote:
> Brian King wrote:
>> ACK
> 
> Does this response mean that you've tested it, and successfully verified
> your problem is gone?

Yes, I have tested it, but all my problems are not gone with this
one patch. This fixes the problem I was seeing where the data direction
was set incorrectly, but I still have some other issues I am
working through. I'll be sending out additional patches shortly.

Brian


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[James Bottomley]

On Wed, 2007-01-17 at 11:34 +0900, Tejun Heo wrote:
> libata didn't used to init qc->dma_dir to any specific value on qc
> initialization and command translation path didn't set qc->dma_dir if
> the command doesn't need data transfer.  This made non-data commands
> to have random qc->dma_dir.
> 
> This usually doesn't cause problem because LLDs usually check
> qc->protocol first and look at qc->dma_dir iff the command needs data
> transfer but this doesn't hold for all LLDs.
> 
> It might be worthwhile to rename qc->dma_dir to qc->data_dir as we use
> the field to tag data direction for both PIO and DMA protocols.
> 
> This problem has been spotted by James Bottomley.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: James Bottomley <James.Bottomley@SteelEye.com>

This looks perfectly fine as a possible solution.  Is there any reason
not to initialise qc->dma_dir unconditionally from the SCSI command?
The only potential problem is DMA_BIDIRECTIONAL, which we don't use
(yet) ... but if it ever did come down libata will do the wrong thing
anyway.

James

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Tejun Heo]

James Bottomley wrote:
> This looks perfectly fine as a possible solution.  Is there any reason
> not to initialise qc->dma_dir unconditionally from the SCSI command?

That should work too.  I did what I did because it was more in line with
what the current code assumed and initializing the field on qc alloc
seemed like a good idea with or without unconditional qc->dma_dir =
scmd->sc_data_direction because not all commands are translated from
scsi command.

> The only potential problem is DMA_BIDIRECTIONAL, which we don't use
> (yet) ... but if it ever did come down libata will do the wrong thing
> anyway.

If that ever happens, libata probably should emulate it using multiple
commands, I guess.

Thanks.

-- 
tejun

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[James Bottomley]

On Thu, 2007-01-18 at 00:49 +0900, Tejun Heo wrote:
> James Bottomley wrote:
> > This looks perfectly fine as a possible solution.  Is there any reason
> > not to initialise qc->dma_dir unconditionally from the SCSI command?
> 
> That should work too.  I did what I did because it was more in line with
> what the current code assumed and initializing the field on qc alloc
> seemed like a good idea with or without unconditional qc->dma_dir =
> scmd->sc_data_direction because not all commands are translated from
> scsi command.

Sure, I agree with the initialisation (although it should perhaps be
initialised to an error value we can detect just in case there are other
paths where it would leak through uninitialised ... we'll still get
cockups if a command with data ever goes through as DMA_NONE.

> > The only potential problem is DMA_BIDIRECTIONAL, which we don't use
> > (yet) ... but if it ever did come down libata will do the wrong thing
> > anyway.
> 
> If that ever happens, libata probably should emulate it using multiple
> commands, I guess.

This really depends on where SATA is going.  Current SCSI bidirectional
commands are the preserve of the complex command sets I don't believe
even a SATA device would ever emulate.  On the other hand, we're
planning to use bidirectional in SGv4 for frame in/frame out type
packets that are used to control expanders.

So the interesting question is whether SATA would ever want to drive
expanders?  For the low end, given the lack of success of the port
multipliers which are effectively a poor man's expander, I guess not ...
but, since most SAS/SATA cages seem to be coming with built in
expanders, I can see that changing.

James

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Brian King]

Brian King wrote:
> Jeff Garzik wrote:
>> Brian King wrote:
>>> ACK
>> Does this response mean that you've tested it, and successfully verified
>> your problem is gone?
> 
> Yes, I have tested it, but all my problems are not gone with this
> one patch. This fixes the problem I was seeing where the data direction
> was set incorrectly, but I still have some other issues I am
> working through. I'll be sending out additional patches shortly.

I just sent three small libata patches to linux-ide which fix the problems
I was seeing. I am still running additional tests to make sure there are
not any other issues, but everything looks pretty good so far.

Thanks,

Brian


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] libata: initialize qc->dma_dir to DMA_NONE

[Jeff Garzik]

Tejun Heo wrote:
> libata didn't used to init qc->dma_dir to any specific value on qc
> initialization and command translation path didn't set qc->dma_dir if
> the command doesn't need data transfer.  This made non-data commands
> to have random qc->dma_dir.
> 
> This usually doesn't cause problem because LLDs usually check
> qc->protocol first and look at qc->dma_dir iff the command needs data
> transfer but this doesn't hold for all LLDs.
> 
> It might be worthwhile to rename qc->dma_dir to qc->data_dir as we use
> the field to tag data direction for both PIO and DMA protocols.
> 
> This problem has been spotted by James Bottomley.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: James Bottomley <James.Bottomley@SteelEye.com>

applied to #upstream-fixes