ipr SATA problems in 2.6.20

PATCH Applied Brian King 6 participants 17 messages Jan 16, 2007 – Jan 20, 2007

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Brian King

author

2007-01-16 22:24 UTC #0 lore raw

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.

Quoted 1 line

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

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Alan

2007-01-16 22:37 UTC #1 lore raw

Quoted 3 lines

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

Jeff Garzik

2007-01-16 22:45 UTC #2 lore raw

Quoted 6 lines · Alan

Quoted 3 lines
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

James Bottomley

2007-01-16 23:11 UTC #3 lore raw

Quoted 6 lines · Jeff Garzik

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

Brian King

author

2007-01-16 23:17 UTC #4 lore raw

Quoted 11 lines · James Bottomley

Quoted 6 lines · Jeff Garzik
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

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

James Bottomley

2007-01-16 23:30 UTC #5 lore raw

Quoted 2 lines · Brian King

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

Brian King

author

2007-01-16 23:33 UTC #6 lore raw

In reply to Brian King

Quoted 6 lines · Brian King

Quoted 5 lines · James Bottomley
Quoted 4 lines · Jeff Garzik
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Tejun Heo

2007-01-17 02:34 UTC #7 lore raw

PATCH 1/1 libata: initialize qc->dma_dir to DMA_NONE

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 <[email protected]>

Cc James Bottomley <[email protected]>

include/linux/libata.h +1 -0

index 7cfc18f..925ad7f 100644
@@ -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;

Brian King

2007-01-17 03:43 UTC #8 lore raw

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

ACK

Quoted 33 lines · 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 <[email protected]>
Cc James Bottomley <[email protected]>
include/linux/libata.h +1 -1
index 7cfc18f..925ad7f 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Jeff Garzik

2007-01-17 03:59 UTC #9 lore raw

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

Quoted 1 line · Brian King

ACK

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

Jeff

Brian King

author

2007-01-17 14:37 UTC #10 lore raw

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

Quoted 5 lines · Jeff Garzik

Quoted 1 line · Brian King
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

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Brian King

author

2007-01-17 18:35 UTC #11 lore raw

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

Quoted 10 lines · Brian King

Quoted 4 lines · Jeff Garzik
Quoted 1 line · Brian King
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

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

James Bottomley

2007-01-17 15:19 UTC #12 lore raw

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

In reply to Tejun Heo

Quoted 9 lines · 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.

Quoted 7 lines

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 <[email protected]>
Cc James Bottomley <[email protected]>

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

Tejun Heo

2007-01-17 15:49 UTC #13 lore raw

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

Quoted 2 lines · James Bottomley

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.

Quoted 3 lines

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.

Signature · 1 line

tejun

James Bottomley

2007-01-17 16:07 UTC #14 lore raw

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

Quoted 9 lines · Tejun Heo

Quoted 2 lines · James Bottomley
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.

Quoted 6 lines

Quoted 3 lines
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

Jeff Garzik

Applied

2007-01-20 00:19 UTC #15 lore raw

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

In reply to Tejun Heo

Quoted 9 lines · 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.

Quoted 7 lines

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 <[email protected]>
Cc James Bottomley <[email protected]>

applied to #upstream-fixes

Alan

2007-01-16 23:28 UTC #16 lore raw

In reply to Jeff Garzik

Quoted 11 lines · On Tue, 16 Jan 2007 17:45:56 -0500 Jeff Garzik <[email protected]> wrote:

Quoted 6 lines · Alan
Quoted 3 lines
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