sata_nv: fix for completion handling

PATCH Applied Robert Hancock 3 participants 4 messages Jan 30, 2008 – Feb 1, 2008 libata

Share & tools

Robert Hancock

author

2008-01-30 01:54 UTC #0 lore raw

PATCH 1/1 sata_nv: fix for completion handling

This patch is based on an original patch from Kuan Luo of NVIDIA, posted under subject "fixed a bug of adma in rhel4u5 with HDS7250SASUN500G". His description follows. I've reworked it a bit to avoid some unnecessary repeated checks but it should be functionally identical.

"The patch is to solve the error message "ata1: CPB flags CMD err, flags=0x11" when testing HDS7250SASUN500G in rhel4u5.
I tested this hd in 2.6.24-rc7 which needed to remove the mask in blacklist to run the ncq and the same error also showed up.

I traced the bug and found that the interrupt finished a command (for example, tag=0) when the driver got that adma status is NV_ADMA_STAT_DONE and cpb->resp_flags is NV_CPB_RESP_DONE. However, For this hd, the drive maybe didn't clear bit 0 at this moment. It meaned the hardware had not completely finished the command. If at the same time the driver freed the command(tag 0) and sended another command (tag 0), the error happened.

The notifier register is 32-bit register containing notifier value. Value is bit vector containing one bit per tag number (0-31) in corresponding bit positions (bit 0 is for tag 0, etc). When bit is set then ADMA indicates that command with corresponding tag number completed execution.

So i added the check notifier code. Sometimes i saw that the notifier reg set some bits , but the adma status set NV_ADMA_STAT_CMD_COMPLETE ,not NV_ADMA_STAT_DONE. So i added the NV_ADMA_STAT_CMD_COMPLETE check code."

Signed-off-by Robert Hancock <[email protected]>

drivers/ata/sata_nv.c +12 -6

@@ -1011,14 +1011,20 @@
 			}
 
 			if (status & (NV_ADMA_STAT_DONE |
-				      NV_ADMA_STAT_CPBERR)) {
-				u32 check_commands;
+				      NV_ADMA_STAT_CPBERR |
+				      NV_ADMA_STAT_CMD_COMPLETE)) {
+				u32 check_commands = notifier_clears[i];
 				int pos, error = 0;
 
-				if (ata_tag_valid(ap->link.active_tag))
-					check_commands = 1 << ap->link.active_tag;
-				else
-					check_commands = ap->link.sactive;
+				if (status & NV_ADMA_STAT_CPBERR) {
+					/* Check all active commands */
+					if (ata_tag_valid(ap->link.active_tag))
+						check_commands = 1 <<
+							ap->link.active_tag;
+					else
+						check_commands = ap->
+							link.sactive;
+				}
 
 				/** Check CPBs for completed commands */
 				while ((pos = ffs(check_commands)) && !error) {

Tejun Heo

2008-01-30 02:28 UTC #1 lore raw

Quoted 24 lines · Robert Hancock

This patch is based on an original patch from Kuan Luo of NVIDIA, posted under subject "fixed a bug of adma in rhel4u5 with HDS7250SASUN500G". His description follows. I've reworked it a bit to avoid some unnecessary repeated checks but it should be functionally identical.
"The patch is to solve the error message "ata1: CPB flags CMD err, flags=0x11" when testing HDS7250SASUN500G in rhel4u5.
I tested this hd in 2.6.24-rc7 which needed to remove the mask in blacklist to run the ncq and the same error also showed up.
I traced the bug and found that the interrupt finished a command (for example, tag=0) when the driver got that adma status is NV_ADMA_STAT_DONE and cpb->resp_flags is NV_CPB_RESP_DONE. However, For this hd, the drive maybe didn't clear bit 0 at this moment. It meaned the hardware had not completely finished the command. If at the same time the driver freed the command(tag 0) and sended another command (tag 0), the error happened.
The notifier register is 32-bit register containing notifier value. Value is bit vector containing one bit per tag number (0-31) in corresponding bit positions (bit 0 is for tag 0, etc). When bit is set then ADMA indicates that command with corresponding tag number completed execution.

Quoted 6 lines

So i added the check notifier code. Sometimes i saw that the notifier reg set some bits , but the adma status set NV_ADMA_STAT_CMD_COMPLETE ,not NV_ADMA_STAT_DONE. So i added the NV_ADMA_STAT_CMD_COMPLETE check code."
Signed-off-by Robert Hancock <[email protected]>

Any chance this fixes the FLUSH problem?

Signature · 1 line

tejun

Robert Hancock

author

2008-01-30 02:54 UTC #2 lore raw

Quoted 25 lines · Tejun Heo

Quoted 24 lines · Robert Hancock
This patch is based on an original patch from Kuan Luo of NVIDIA, posted under subject "fixed a bug of adma in rhel4u5 with HDS7250SASUN500G". His description follows. I've reworked it a bit to avoid some unnecessary repeated checks but it should be functionally identical.
"The patch is to solve the error message "ata1: CPB flags CMD err, flags=0x11" when testing HDS7250SASUN500G in rhel4u5.
I tested this hd in 2.6.24-rc7 which needed to remove the mask in blacklist to run the ncq and the same error also showed up.
I traced the bug and found that the interrupt finished a command (for example, tag=0) when the driver got that adma status is NV_ADMA_STAT_DONE and cpb->resp_flags is NV_CPB_RESP_DONE. However, For this hd, the drive maybe didn't clear bit 0 at this moment. It meaned the hardware had not completely finished the command. If at the same time the driver freed the command(tag 0) and sended another command (tag 0), the error happened.
The notifier register is 32-bit register containing notifier value. Value is bit vector containing one bit per tag number (0-31) in corresponding bit positions (bit 0 is for tag 0, etc). When bit is set then ADMA indicates that command with corresponding tag number completed execution.

Quoted 8 lines

Quoted 6 lines
So i added the check notifier code. Sometimes i saw that the notifier reg set some bits , but the adma status set NV_ADMA_STAT_CMD_COMPLETE ,not NV_ADMA_STAT_DONE. So i added the NV_ADMA_STAT_CMD_COMPLETE check code."
Signed-off-by Robert Hancock <[email protected]>
Any chance this fixes the FLUSH problem?

I could still reproduce that issue when I took the udelay(20) out of the driver. Others have seen that without taking it out, so I suspect some systems/drives are more sensitive to that for some reason. However, who knows, it may help some people with that problem.

The symptoms of the problem dealt with here are different, not a command timeout it appears, but the controller reporting an error.

Jeff Garzik

Applied

2008-02-01 16:50 UTC #3 lore raw

In reply to Robert Hancock

Quoted 24 lines · Robert Hancock

This patch is based on an original patch from Kuan Luo of NVIDIA, posted under subject "fixed a bug of adma in rhel4u5 with HDS7250SASUN500G". His description follows. I've reworked it a bit to avoid some unnecessary repeated checks but it should be functionally identical.
"The patch is to solve the error message "ata1: CPB flags CMD err, flags=0x11" when testing HDS7250SASUN500G in rhel4u5.
I tested this hd in 2.6.24-rc7 which needed to remove the mask in blacklist to run the ncq and the same error also showed up.
I traced the bug and found that the interrupt finished a command (for example, tag=0) when the driver got that adma status is NV_ADMA_STAT_DONE and cpb->resp_flags is NV_CPB_RESP_DONE. However, For this hd, the drive maybe didn't clear bit 0 at this moment. It meaned the hardware had not completely finished the command. If at the same time the driver freed the command(tag 0) and sended another command (tag 0), the error happened.
The notifier register is 32-bit register containing notifier value. Value is bit vector containing one bit per tag number (0-31) in corresponding bit positions (bit 0 is for tag 0, etc). When bit is set then ADMA indicates that command with corresponding tag number completed execution.

Quoted 6 lines

So i added the check notifier code. Sometimes i saw that the notifier reg set some bits , but the adma status set NV_ADMA_STAT_CMD_COMPLETE ,not NV_ADMA_STAT_DONE. So i added the NV_ADMA_STAT_CMD_COMPLETE check code."
Signed-off-by Robert Hancock <[email protected]>

applied, thanks all for investigating this stuff