Re: something strange in libata-core.c for kernel 2.6.22-rc3
Tejun Heo wrote: [EMAIL PROTECTED] wrote: Mybe I am wrong, but if you are detecting 40-wire cable to set them to DMA/33, why the check includes also 80-wire cables configuring them to DMA/33 too? With this patch my nvidia4 IDE controllers detects correctly and configure correctly DMA/100 for my HD and DMA/33 for my DVD (the first uses a 80-wire cable, the second a 40-wire cable). Am I wrong somewhere? That's the drive side verification of 80c cable check, so if the condition triggers we downgrade 80c or unknown to 40c. Cable detection on nvidia PATA is a disaster. You're supposed to do some ACPI dancing and drive side detection is completely bogus. Eeeek Alan, did you have a chance to test the ACPI cable detection? It just didn't work when I tried it. It always returned 80c on my machine. On a whim I started poking around in the disassembled ACPI DSDT code for my Asus A8N-SLI Deluxe board, which is one of these chipsets. The original thought was that the STM/GTM trick on these chipsets is supposed to allow us to determine what modes we should use based on what modes it sets up appropriately. Unfortunately, unless I'm missing something in the AML (which is possible) it doesn't seem like there is any validation being done on the settings passed in. The settings appear to essentially just get programmed into the controller when STM is called and read back on GTM. The only complication is some logic on the _PTS method (Prepare to Sleep) which stores the current settings into some variables, and in STM, if a flag was set by the _PTS method, the previous settings for all registers are stored back first before writing the requested values into the correct places. So in this case, obviously the approach used by pata_acpi, etc. won't work for cable detection. Whatever magic register on the chipset contains the cable detect value, the AML doesn't seem to be accessing it. The ACPI spec doesn't really give any guarantee that the try STM on all possible modes trick will work either, since there seems to be no mention of the AML being required to validate the mode and the STM function has no return value to indicate failure. I guess this means that what we have to do is trust that the BIOS set up a reasonable mode and base the cable detect on that (either by reading back the boot-up controller registers, or by calling GTM). I imagine this is what the Windows default IDE driver is doing (just using the boot-up mode and feeding it back using GTM/STM on suspend/resume cycles). -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: something strange in libata-core.c for kernel 2.6.22-rc3
Alan Cox wrote: Yeah, that's consistent to what I've seen on my machine which is a variant of A8N. No matter what value I through at _STM, _GTM just echoed the result thus always leading to 80c configuration. I guess this means that what we have to do is trust that the BIOS set up a reasonable mode and base the cable detect on that (either by reading back the boot-up controller registers, or by calling GTM). I imagine this is what the Windows default IDE driver is doing (just using the boot-up mode and feeding it back using GTM/STM on suspend/resume cycles). Alan, what do you think? Interesting, sounds like it is still useful rather than just reading the registers as the GTM/STM seem to survive resume cycles which drive config may not (eg if the driver is loaded after a s2ram/resume. I don't think that case is handled in this BIOS anyway - if you call GTM after resume without previously calling STM, it's just going to read whatever random values are in the controller and give you timings based on that, which presumably will be junk. It looks like the main purpose for what it's doing with saving those registers in the _PTS method is to save and restore a couple of controller registers called ID20 (PCI config space offset 0x50, 16 bits) and ID22 (PCI config space offset 0x5C, 32 bits) which aren't otherwise used in the AML. According to pata_amd, for the AMD IDE interface the former is some reserved bits as well as the cable detect bits, while the latter is the cycle time and address setup time register. Presumably those aren't really the cable detect bits though, since the detection based on those bits in pata_amd doesn't really work.. If it just echoes back we should also be able to detect this by using knowingly invalid values. Well, this implementation doesn't purely echo back the same values, it echoes back values derived from what the controller was actually set to, so I imagine if you put in something ridiculous it would come back with the closest possible mode that it was set to (PIO mode 0, etc.) I suspect the implementation we would need to use (which doesn't depend on anything not given in the spec) would be: -On driver load, execute _GTM to get the timing mode the BIOS had set. Assume this represents the fastest modes the controller supports, and set cable detect based on whether it includes UDMA modes 2. -If we decide to set a slower mode (speed down due to errors, etc.), set it using _STM and then read back the actual values that were set using _GTM (for possible use in suspend/resume). -On resume after suspend, re-set the last mode using _STM followed by executing _GTF and running those commands. This won't handle the case where the driver is loaded after the system was already suspended to RAM and resumed, however I don't know exactly how one could handle that in this situation.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Add Seagate STT20000A to DMA blacklist.
Dave Jones wrote: On Mon, May 21, 2007 at 05:15:51PM +0100, Alan Cox wrote: On Mon, 21 May 2007 10:50:42 -0400 Dave Jones [EMAIL PROTECTED] wrote: http://bugzilla.kernel.org/show_bug.cgi?id=1044 has been open for _four_ years with a patch available. Here's a rediffed version of the same. Please update libata as well when you udpate the blacklists. Sure, point me at the table(s) ? Dave ata_device_blacklist in drivers/ata/libata-core.c -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sd_resume redundant? [was: [PATCH] libata: implement ata_wait_after_reset()]
Randy Dunlap wrote: On Sun, 20 May 2007 11:45:03 -0600 Robert Hancock wrote: Indan Zupancic wrote: Everything seems to work fine without sd_resume(), so why is it needed? Because not all disks spin up without being told to do so and like it or not spinning disks up on resume is the default behavior. As I wrote in the other reply, it would be worthwhile to make it configurable. Not even after they receive a read command? Ugh. ATA disks are supposed to spin up, yes. SCSI disks require a command to tell them to spin up if they're in the stopped state. Good info, but linux-ide was dropped. Is that due to lack of reply-to-all or is it a newsgroup thing or what? That would be a newsgroup thing. It seems that sometimes CCs get dropped when the posts are forwarded to fa.linux.kernel where I normally read them. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sata_sil: Greatly improve DMA support
Jeff Garzik wrote: Since Alan expressed a desire to see Large Block Transfer (LBT) support in pata_sil680, I though I would re-post my patch for adding LBT support to sata_sil. Silicon Image's Large Block Transfer (LBT) support is a vendor-specific DMA scatter/gather engine, which enables 64-bit DMA addresses (where supported by platform) and eliminates the annoying 64k DMA boundary found in legacy PCI IDE BMDMA engines. Looks like it doesn't allow 64-bit DMA addresses, it only gets rid of the 64K boundary limitation. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Kuan Luo wrote:
Thanks for your comment, see the explaination inline.
We'll apply your advice in later patch.
...
Please don't duplicate this code in the driver, this is part of libata
core in libata-scsi.c. Add an export for these functions if you need to
use them in the driver.
[kuan]: These calls are declared in static type . I can't export them
and don't want to
modify libata code.
If you really need these functions then they should be made non-static
and exported. Duplicating the code is not a solution.
I'm not so sure you actually need all that, though. I suspect you can
likely handle the deferring of commands if you detect an FPDMA data
phase inside the qc_issue function only (like you already do in some
cases) instead of having to mess with deferring them at the SCSI layer.
I'm still puzzling out how this stuff all works, but it looks like this
code makes you stop sending new commands if:
-the port is in the FPDMA Data Phase (DMA Setup FIS received but the
transfer is not complete yet) - I assume the hardware doesn't handle
this itself, which seems rather unique
-we previously deferred a command inside of qc_issue because we were in
the FPDMA data phase
-we previously saw dhfis_flags not equal to qc_active, or we got a
BACKOUT interrupt (whatever exactly that means), both of which set some
value in the back_byte
[kuan]:
-If we got BACKOUT interrupt, it means that a command just sent by
driver
backed out.The driver should resend the command.So new commands should
be defered.
-If dhfis_flags != qc_active, it indicates that the last command doesn't
generate a device to host register FIS .
After sending some commands, I found that the last command sometimes has
this problem
but previous commands are normal.In this case, we need resend the last
command.
Both cases set back_byte.
The case where the command didn't generate a D2H FIS should likely be
investigated further, otherwise we don't necessarily know that this
workaround will work in all cases?
This code seems a bit odd. Isn't this tossing out a bunch of potential
error status, etc?
[kuan]:
If there are commands in queue, the driver can send a new command
only after receiving dhfis intr of previous command and before receiving
any dmasetup fis intr.
In this place, i do the last check before sending the command.
But the D2H FIS can contain an error indication, correct? If that
happens here it won't detect this. In this situation error handling
should be triggered.
+ done_mask = pp-qc_active ^ sactive;
+ if (unlikely(done_mask sactive)) {
+ ata_port_printk(ap, KERN_ERR, illegal qc_active
transition
+ (%08x-%08x)\n, ap-qc_active,
sactive);
+ return -EINVAL;
+ }
Shouldn't this trigger error handling if it happens instead of just
printing an error?
[kuan]:
I think the error handling can be triggered by timeout. In fact,
this case should seldom happen.
There have been reports of some drives with bad NCQ implementations that
return completion status for commands that were not issued. If we detect
this case we should raise an HSM violation which will disable NCQ on
this drive if it happens repeatedly. See the code in ahci.c in
ahci_host_intr.
This comment still applies:
Additional/general comments:
Think you need some code to handle suspend and resume (re-enable SATA
MMIO space, etc.)
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Peer Chen wrote:
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller.
This patch base on sata_nv.c file from kernel 2.6.22-rc1
See attachment for the patch.
Signed-off-by: Kuan Luo [EMAIL PROTECTED]
Signed-off-by: Peer Chen [EMAIL PROTECTED]
Good to finally see this come out. I've pasted the code below (indented)
in order to make some comments:
--- linux-2.6.22-rc1/drivers/ata/sata_nv.c.orig 2007-05-17
14:48:26.0 -0400
+++ linux-2.6.22-rc1/drivers/ata/sata_nv.c 2007-05-17
17:07:28.0 -0400
@@ -46,6 +46,8 @@
#include linux/device.h
#include scsi/scsi_host.h
#include scsi/scsi_device.h
+#include scsi/scsi.h
+#include scsi/scsi_cmnd.h
#include linux/libata.h
#define DRV_NAME sata_nv
@@ -169,6 +171,36 @@ enum {
NV_ADMA_PORT_REGISTER_MODE = (1 0),
NV_ADMA_ATAPI_SETUP_COMPLETE= (1 1),
+ /* MCP55 reg offset */
+ NV_CTL_MCP55= 0x400,
+ NV_INT_STATUS_MCP55 = 0x440,
+ NV_INT_ENABLE_MCP55 = 0x444,
+ NV_NCQ_REG_MCP55= 0x448,
+ NV_CH1_SACTIVE_MCP55= 0x0C,
+
+ /* MCP55 */
+ NV_INT_ALL_MCP55= 0x,
+ NV_INT_PORT_SHIFT_MCP55 = 16, /* each port occupies
16 bits */
+ NV_INT_MASK_MCP55 = NV_INT_ALL_MCP55 0xfffd,
+
+ /* NCQ ENABLE BITS*/
+ NV_CTL_PRI_SWNCQ= 0x02,
+ NV_CTL_SEC_SWNCQ= 0x04,
+
+ /* MCP55 status bits*/
+ NV_INT_DEV_MCP55= 0x01,
+ NV_INT_PM_MCP55 = 0x02,
+ NV_INT_ADDED_MCP55 = 0x04,
+ NV_INT_REMOVED_MCP55= 0x08,
+
+ NV_INT_BACKOUT_MCP55= 0x10,
+ NV_INT_SDBFIS_MCP55 = 0x20,
+ NV_INT_DHREGFIS_MCP55 = 0x40,
+ NV_INT_DMASETUP_MCP55 = 0x80,
+
+ NV_INT_HOTPLUG_MCP55= (NV_INT_ADDED_MCP55 |
+ NV_INT_REMOVED_MCP55),
+
};
/* ADMA Physical Region Descriptor - one SG segment */
@@ -264,13 +296,118 @@ static void nv_adma_host_stop(struct ata
static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile
*tf);
+static void ncq_error_handler(struct ata_port *ap);
+static void nv_mcp55_thaw(struct ata_port *ap);
+static void nv_mcp55_freeze(struct ata_port *ap);
+static void ncq_host_init(struct ata_host *host);
+static void nv_bmdma_stop(struct ata_port *ap);
+static int nv_std_qc_defer(struct ata_port *ap);
+static int nv_port_start(struct ata_port *ap);
+static void nv_port_stop(struct ata_port *ap);
+static void ncq_clear(struct ata_port *ap);
+static void nv_qc_prep(struct ata_queued_cmd *qc);
+static void nv_fill_sg(struct ata_queued_cmd *qc);
+static void ncq_sactive_start (struct ata_queued_cmd *qc);
+static u32 ncq_sactive_value (struct ata_port *ap);
+static unsigned int nv_qc_issue_prot(struct ata_queued_cmd *qc);
+static u32 ncq_tag_value(struct ata_port *ap);
+static int nv_ncqintr_sdbfis(struct ata_port *ap);
+static int nv_ncqintr_dmasetupfis(struct ata_port *ap);
+static void ncq_clear_singlefis(struct ata_port *ap, u32 val);
+static u32 ncq_ownfisintr_value (struct ata_port *ap);
+void ncq_hotplug(struct ata_port *ap, u32 fis);
+static irqreturn_t nv_mcp55_interrupt(int irq, void *dev_instance);
+static int ncq_interrupt(struct ata_port *ap, u32 fis);
+static int nv_scsi_queuecmd(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *));
These functions should use mcp51 or swncq or something in the name
instead of ncq, since the latter implies it may be related to ADMA as
well.
+
+#undef NCQ_DEBUG
+#undef NCQ_VERBOSE_DEBUG
+#ifdef NCQ_DEBUG
+#define NPRINTK(fmt, args...) printk(KERN_ERR %s: fmt,
__FUNCTION__, ## args)
+#ifdef NCQ_VERBOSE_DEBUG
+#define NVPRINTK(fmt, args...) printk(KERN_ERR %s: fmt,
__FUNCTION__, ## args)
+#else
+#define NVPRINTK(fmt, args...) do { } while(0)
+#endif /* NCQ_VERBOSE_DEBUG */
+#else
+#define NPRINTK(fmt, args...) do { } while(0)
+#define NVPRINTK(fmt, args...) do { } while(0)
+#endif
We don't need these private helper macros, just use
[PATCH] libata: add human-readable error value decoding (v2)
This adds human-readable decoding of the ATA status and error registers (similar
to what drivers/ide does) as well as the SATA Serror register to libata error
handling output. This prevents the need to pore through standards documents
to figure out the meaning of the bits in these registers when looking at error
reports. Some bits that drivers/ide decoded are not decoded here, since the bits
are either command-dependent or obsolete, and properly parsing them would add
too much complexity.
This version reduces the length of the SError parsed output strings relative to
the
previous version of this patch.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21.1/drivers/ata/libata-eh.c 2007-04-27 15:49:26.0
-0600
+++ linux-2.6.21.1edit/drivers/ata/libata-eh.c 2007-05-14 17:38:35.0
-0600
@@ -1523,6 +1523,27 @@ static void ata_eh_report(struct ata_por
ata_port_printk(ap, KERN_ERR, (%s)\n, desc);
}
+ if (ehc-i.serror)
+ ata_port_printk(ap, KERN_ERR,
+ SError: {%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s}\n,
+ ehc-i.serror SERR_DATA_RECOVERED ? RecovData : ,
+ ehc-i.serror SERR_COMM_RECOVERED ? RecovComm : ,
+ ehc-i.serror SERR_DATA ? UnrecovData : ,
+ ehc-i.serror SERR_PERSISTENT ? Persist : ,
+ ehc-i.serror SERR_PROTOCOL ? Proto : ,
+ ehc-i.serror SERR_INTERNAL ? HostInt : ,
+ ehc-i.serror SERR_PHYRDY_CHG ? PHYRdyChg : ,
+ ehc-i.serror SERR_PHY_INT_ERR ? PHYInt : ,
+ ehc-i.serror SERR_COMM_WAKE ? CommWake : ,
+ ehc-i.serror SERR_10B_8B_ERR ? 10B8B : ,
+ ehc-i.serror SERR_DISPARITY ? Dispar : ,
+ ehc-i.serror SERR_CRC ? BadCRC : ,
+ ehc-i.serror SERR_HANDSHAKE ? Handshk : ,
+ ehc-i.serror SERR_LINK_SEQ_ERR ? LinkSeq : ,
+ ehc-i.serror SERR_TRANS_ST_ERROR ? TrStaTrns : ,
+ ehc-i.serror SERR_UNRECOG_FIS ? UnrecFIS : ,
+ ehc-i.serror SERR_DEV_XCHG ? DevExch : );
+
for (tag = 0; tag ATA_MAX_QUEUE; tag++) {
static const char *dma_str[] = {
[DMA_BIDIRECTIONAL] = bidi,
@@ -1552,6 +1573,29 @@ static void ata_eh_report(struct ata_por
res-hob_feature, res-hob_nsect,
res-hob_lbal, res-hob_lbam, res-hob_lbah,
res-device, qc-err_mask,
ata_err_string(qc-err_mask));
+
+ if (res-command (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ |
+ ATA_ERR) ) {
+ if (res-command ATA_BUSY)
+ ata_dev_printk(qc-dev, KERN_ERR,
+ status: {Busy}\n );
+ else
+ ata_dev_printk(qc-dev, KERN_ERR,
+ status: {%s%s%s%s}\n,
+ res-command ATA_DRDY ? DRDY : ,
+ res-command ATA_DF ? DF : ,
+ res-command ATA_DRQ ? DRQ : ,
+ res-command ATA_ERR ? ERR : );
+ }
+
+ if (cmd-command != ATA_CMD_PACKET
+ (res-feature (ATA_ICRC | ATA_UNC | ATA_IDNF |
ATA_ABORTED)))
+ ata_dev_printk(qc-dev, KERN_ERR,
+ error: {%s%s%s%s}\n,
+ res-feature ATA_ICRC ? ICRC : ,
+ res-feature ATA_UNC ? UNC : ,
+ res-feature ATA_IDNF ? IDNF : ,
+ res-feature ATA_ABORTED ? ABRT : );
}
}
--- linux-2.6.21.1/include/linux/ata.h 2007-04-27 15:49:26.0 -0600
+++ linux-2.6.21.1edit/include/linux/ata.h 2007-05-09 19:25:54.0
-0600
@@ -223,6 +223,15 @@ enum {
SERR_PROTOCOL = (1 10), /* protocol violation */
SERR_INTERNAL = (1 11), /* host internal error */
SERR_PHYRDY_CHG = (1 16), /* PHY RDY changed */
+ SERR_PHY_INT_ERR= (1 17), /* PHY internal error */
+ SERR_COMM_WAKE = (1 18), /* Comm wake */
+ SERR_10B_8B_ERR = (1 19), /* 10b to 8b decode error */
+ SERR_DISPARITY = (1 20), /* Disparity */
+ SERR_CRC= (1 21), /* CRC error */
+ SERR_HANDSHAKE = (1 22), /* Handshake error */
+ SERR_LINK_SEQ_ERR = (1 23), /* Link sequence error */
+ SERR_TRANS_ST_ERROR = (1 24), /* Transport state transition
error */
+ SERR_UNRECOG_FIS= (1 25), /* Unrecognized FIS */
SERR_DEV_XCHG = (1 26), /* device exchanged */
/* struct ata_taskfile flags */
-
To unsubscribe from
Re: [2.6.21.1] SATA freeze
Fred Moyer wrote: I just joined the list today so apologies if this email breaks any email client post threading. I have been seeing similar errors on two different systems. I applied Robert's sata_nv patch posted to the list on May 5th, and approved today by Jeff Garzik. I've taken several steps to insure that this isn't a faulty cable or drive issue. This is running on a hp dl145g2. Here is my lspci, dmesg, and relevant kernel config sections: (snip) ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen ata1.00: cmd b0/d2:f1:00:4f:c2/00:00:00:00:00/00 tag 0 cdb 0x0 data 123392 in res 50/00:f1:00:4f:c2/00:00:00:00:00/00 Emask 0x202 (HSM violation) ata1: soft resetting port ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: configured for UDMA/100 ata1: EH complete This appears to be a different problem. Something is issuing SMART-related commands (smartd or smartctl perhaps) which the drive seems to be reacting strangely to. It apparently completed the command but never raised DRQ to request any data being transferred even though we expected it to. Maybe SMART is disabled on the drive and that's causing it to just toss these commands? CCing linux-ide in case anyone knows what would cause this. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6.21.1] SATA freeze
Fred Moyer wrote: This appears to be a different problem. Something is issuing SMART-related commands (smartd or smartctl perhaps) which the drive seems to be reacting strangely to. It apparently completed the command but never raised DRQ to request any data being transferred even though we expected it to. Maybe SMART is disabled on the drive and that's causing it to just toss these commands? CCing linux-ide in case anyone knows what would cause this. Here's smartctl -a for this drive - same output for both sda and sdb. Smartd is currently running. Any advice appreciated. Previously on 2.6.15 I was seeing sdb remount as readonly under heavy i/o. I have not seen that issue yet with 2.6.21 (with Robert's patch from May 5th for sata_nv), but that occurrence of remounts read-only was infrequently, so that issue may be solved. app2 ~ # smartctl -a /dev/sda smartctl version 5.36 [x86_64-pc-linux-gnu] Copyright (C) 2002-6 Bruce Allen Home page is http://smartmontools.sourceforge.net/ Device: ATA ST3808110AS Version: n/a Serial number: 5LR8895K Device type: disk Local Time is: Sat May 12 12:05:58 2007 PDT Device does not support SMART Error Counter logging not supported [GLTSD (Global Logging Target Save Disable) set. Enable Save with '-S on'] Device does not support Self Test logging Sounds like SMART is likely disabled on that drive. You can try doing smartctl -s on /dev/sda and see if that will turn it on. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: add human-readable error value decoding
Tejun Heo wrote:
+if (ehc-i.serror)
+ata_port_printk(ap, KERN_ERR,
+ SError: {%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s}\n,
+ ehc-i.serror SERR_DATA_RECOVERED ? RecovDataErr : ,
+ ehc-i.serror SERR_COMM_RECOVERED ? RecovCommErr : ,
+ ehc-i.serror SERR_DATA ? UnrecovDataErr : ,
+ ehc-i.serror SERR_PERSISTENT ? PersistErr : ,
+ ehc-i.serror SERR_PROTOCOL ? ProtocolErr : ,
+ ehc-i.serror SERR_INTERNAL ? HostInternalErr : ,
+ ehc-i.serror SERR_PHYRDY_CHG ? PHYRdyChg : ,
+ ehc-i.serror SERR_PHY_INT_ERR ? PHYInternalErr : ,
+ ehc-i.serror SERR_COMM_WAKE ? CommWake : ,
+ ehc-i.serror SERR_10B_8B_ERR ? 10B8BErr : ,
+ ehc-i.serror SERR_DISPARITY ? Disparity : ,
+ ehc-i.serror SERR_CRC ? CRCErr : ,
+ ehc-i.serror SERR_HANDSHAKE ? HandshakeErr : ,
+ ehc-i.serror SERR_LINK_SEQ_ERR ? LinkSeqErr : ,
+ ehc-i.serror SERR_TRANS_ST_ERROR ? TransStatTransErr : ,
+ ehc-i.serror SERR_UNRECOG_FIS ? UnrecogFIS : ,
+ ehc-i.serror SERR_DEV_XCHG ? DevExchanged : );
I'm not really convinced whether this is necessary. The human readable
form is also a bit cryptic and can get quite long. So, mild NACK from me.
It certainly seems useful when debugging hotplug issues or random SATA
problems which end up being caused by communication problems. Without
this output, Joe User stands no chance of figuring out what's going on,
and neither does Joe libata Developer unless they really care to dig
through the spec and count bits to figure out what they mean. At least
with this you can see that there was a CRC error, etc. and go from that..
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: add human-readable error value decoding
Jeff Garzik wrote: Mark Lord wrote: If we're compiling the messages into the kernel regardless, then it doesn't really make much sense to NOT show all of them on the error paths. Not true. Uncontrolled message spewage inevitably results in critical information scrolling off the screen, before a user can take a digital photo of the output... Or of users being confused by subsequent error fallout (i.e. multiple oopses reporting problem). Moderation and restraint still have roles to play... :) Jeff I don't think this is as big of a deal here as in other cases, like oops output. With libata errors, if they're at the console (which they'd have to be to see these messages), unless something has actually caused a panic the scrollback buffer should still be functional and they'd be able to see the entire output.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: add human-readable error value decoding
This adds human-readable decoding of the ATA status and error registers (similar
to what drivers/ide does) as well as the SATA Serror register to libata error
handling output. This prevents the need to pore through standards documents
to figure out the meaning of the bits in these registers when looking at error
reports. Some bits that drivers/ide decoded are not decoded here, since the bits
are either command-dependent or obsolete, and properly parsing them would add
too much complexity.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21.1/drivers/ata/libata-eh.c 2007-04-27 15:49:26.0
-0600
+++ linux-2.6.21.1edit/drivers/ata/libata-eh.c 2007-05-09 19:47:53.0
-0600
@@ -1523,6 +1523,27 @@ static void ata_eh_report(struct ata_por
ata_port_printk(ap, KERN_ERR, (%s)\n, desc);
}
+ if (ehc-i.serror)
+ ata_port_printk(ap, KERN_ERR,
+ SError: {%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s}\n,
+ ehc-i.serror SERR_DATA_RECOVERED ? RecovDataErr : ,
+ ehc-i.serror SERR_COMM_RECOVERED ? RecovCommErr : ,
+ ehc-i.serror SERR_DATA ? UnrecovDataErr : ,
+ ehc-i.serror SERR_PERSISTENT ? PersistErr : ,
+ ehc-i.serror SERR_PROTOCOL ? ProtocolErr : ,
+ ehc-i.serror SERR_INTERNAL ? HostInternalErr : ,
+ ehc-i.serror SERR_PHYRDY_CHG ? PHYRdyChg : ,
+ ehc-i.serror SERR_PHY_INT_ERR ? PHYInternalErr : ,
+ ehc-i.serror SERR_COMM_WAKE ? CommWake : ,
+ ehc-i.serror SERR_10B_8B_ERR ? 10B8BErr : ,
+ ehc-i.serror SERR_DISPARITY ? Disparity : ,
+ ehc-i.serror SERR_CRC ? CRCErr : ,
+ ehc-i.serror SERR_HANDSHAKE ? HandshakeErr : ,
+ ehc-i.serror SERR_LINK_SEQ_ERR ? LinkSeqErr : ,
+ ehc-i.serror SERR_TRANS_ST_ERROR ? TransStatTransErr :
,
+ ehc-i.serror SERR_UNRECOG_FIS ? UnrecogFIS : ,
+ ehc-i.serror SERR_DEV_XCHG ? DevExchanged : );
+
for (tag = 0; tag ATA_MAX_QUEUE; tag++) {
static const char *dma_str[] = {
[DMA_BIDIRECTIONAL] = bidi,
@@ -1552,6 +1573,29 @@ static void ata_eh_report(struct ata_por
res-hob_feature, res-hob_nsect,
res-hob_lbal, res-hob_lbam, res-hob_lbah,
res-device, qc-err_mask,
ata_err_string(qc-err_mask));
+
+ if (res-command (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ |
+ ATA_ERR) ) {
+ if (res-command ATA_BUSY)
+ ata_dev_printk(qc-dev, KERN_ERR,
+ status: {Busy}\n );
+ else
+ ata_dev_printk(qc-dev, KERN_ERR,
+ status: {%s%s%s%s}\n,
+ res-command ATA_DRDY ? DRDY : ,
+ res-command ATA_DF ? DF : ,
+ res-command ATA_DRQ ? DRQ : ,
+ res-command ATA_ERR ? ERR : );
+ }
+
+ if (cmd-command != ATA_CMD_PACKET
+ (res-feature (ATA_ICRC | ATA_UNC | ATA_IDNF |
ATA_ABORTED)))
+ ata_dev_printk(qc-dev, KERN_ERR,
+ error: {%s%s%s%s}\n,
+ res-feature ATA_ICRC ? ICRC : ,
+ res-feature ATA_UNC ? UNC : ,
+ res-feature ATA_IDNF ? IDNF : ,
+ res-feature ATA_ABORTED ? ABRT : );
}
}
--- linux-2.6.21.1/include/linux/ata.h 2007-04-27 15:49:26.0 -0600
+++ linux-2.6.21.1edit/include/linux/ata.h 2007-05-09 19:25:54.0
-0600
@@ -223,6 +223,15 @@ enum {
SERR_PROTOCOL = (1 10), /* protocol violation */
SERR_INTERNAL = (1 11), /* host internal error */
SERR_PHYRDY_CHG = (1 16), /* PHY RDY changed */
+ SERR_PHY_INT_ERR= (1 17), /* PHY internal error */
+ SERR_COMM_WAKE = (1 18), /* Comm wake */
+ SERR_10B_8B_ERR = (1 19), /* 10b to 8b decode error */
+ SERR_DISPARITY = (1 20), /* Disparity */
+ SERR_CRC= (1 21), /* CRC error */
+ SERR_HANDSHAKE = (1 22), /* Handshake error */
+ SERR_LINK_SEQ_ERR = (1 23), /* Link sequence error */
+ SERR_TRANS_ST_ERROR = (1 24), /* Transport state transition
error */
+ SERR_UNRECOG_FIS= (1 25), /* Unrecognized FIS */
SERR_DEV_XCHG = (1 26), /* device exchanged */
/* struct ata_taskfile flags */
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body
[PATCH] sata_nv: fix ADMA freeze/thaw/irq_clear issues
This patch fixes some problems with ADMA-capable controllers with regard to
freeze,
thaw and irq_clear libata callbacks. Freeze and thaw didn't switch the
ADMA-specific
interrupts on or off, and more critically the irq_clear function didn't respect
the restriction that the notifier clear registers for both ports have to be
written
at the same time even when only one port is being cleared. This could result in
timeouts on one port when error handling (i.e. as a result of hotplug)
occurred on the other port.
As well, this fixes some issues in the interrupt handler: we shouldn't check any
ADMA status if the port has ADMA switched off because of an ATAPI device, and
it also checks to see if any ADMA interrupt has been raised even when we are in
port-register mode.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21.1/drivers/ata/sata_nv.c2007-04-27 15:49:26.0
-0600
+++ linux-2.6.21.1edit/drivers/ata/sata_nv.c2007-05-05 15:25:04.0
-0600
@@ -257,6 +257,8 @@ static void nv_adma_port_stop(struct ata
static int nv_adma_port_suspend(struct ata_port *ap, pm_message_t mesg);
static int nv_adma_port_resume(struct ata_port *ap);
#endif
+static void nv_adma_freeze(struct ata_port *ap);
+static void nv_adma_thaw(struct ata_port *ap);
static void nv_adma_error_handler(struct ata_port *ap);
static void nv_adma_host_stop(struct ata_host *host);
static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
@@ -446,8 +448,8 @@ static const struct ata_port_operations
.bmdma_status = ata_bmdma_status,
.qc_prep= nv_adma_qc_prep,
.qc_issue = nv_adma_qc_issue,
- .freeze = nv_ck804_freeze,
- .thaw = nv_ck804_thaw,
+ .freeze = nv_adma_freeze,
+ .thaw = nv_adma_thaw,
.error_handler = nv_adma_error_handler,
.post_internal_cmd = nv_adma_post_internal_cmd,
.data_xfer = ata_data_xfer,
@@ -810,8 +812,16 @@ static irqreturn_t nv_adma_interrupt(int
u16 status;
u32 gen_ctl;
u32 notifier, notifier_error;
+
+ /* if ADMA is disabled, use standard ata interrupt
handler */
+ if (pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE) {
+ u8 irq_stat = readb(host-iomap[NV_MMIO_BAR] +
NV_INT_STATUS_CK804)
+(NV_INT_PORT_SHIFT * i);
+ handled += nv_host_intr(ap, irq_stat);
+ continue;
+ }
- /* if in ATA register mode, use standard ata interrupt
handler */
+ /* if in ATA register mode, check for standard
interrupts */
if (pp-flags NV_ADMA_PORT_REGISTER_MODE) {
u8 irq_stat = readb(host-iomap[NV_MMIO_BAR] +
NV_INT_STATUS_CK804)
(NV_INT_PORT_SHIFT * i);
@@ -821,7 +831,6 @@ static irqreturn_t nv_adma_interrupt(int
command is active, to prevent
losing interrupts. */
irq_stat |= NV_INT_DEV;
handled += nv_host_intr(ap, irq_stat);
- continue;
}
notifier = readl(mmio + NV_ADMA_NOTIFIER);
@@ -907,22 +916,77 @@ static irqreturn_t nv_adma_interrupt(int
return IRQ_RETVAL(handled);
}
+static void nv_adma_freeze(struct ata_port *ap)
+{
+ struct nv_adma_port_priv *pp = ap-private_data;
+ void __iomem *mmio = pp-ctl_block;
+ u16 tmp;
+
+ nv_ck804_freeze(ap);
+
+ if (pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE)
+ return;
+
+ /* clear any outstanding CK804 notifications */
+ writeb( NV_INT_ALL (ap-port_no * NV_INT_PORT_SHIFT),
+ ap-host-iomap[NV_MMIO_BAR] + NV_INT_STATUS_CK804);
+
+ /* Disable interrupt */
+ tmp = readw(mmio + NV_ADMA_CTL);
+ writew( tmp ~(NV_ADMA_CTL_AIEN | NV_ADMA_CTL_HOTPLUG_IEN),
+ mmio + NV_ADMA_CTL);
+ readw( mmio + NV_ADMA_CTL );/* flush posted write */
+}
+
+static void nv_adma_thaw(struct ata_port *ap)
+{
+ struct nv_adma_port_priv *pp = ap-private_data;
+ void __iomem *mmio = pp-ctl_block;
+ u16 tmp;
+
+ nv_ck804_thaw(ap);
+
+ if (pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE)
+ return;
+
+ /* Enable interrupt */
+ tmp = readw(mmio + NV_ADMA_CTL);
+ writew( tmp | (NV_ADMA_CTL_AIEN | NV_ADMA_CTL_HOTPLUG_IEN),
+ mmio + NV_ADMA_CTL);
+ readw( mmio + NV_ADMA_CTL );/* flush posted write */
+}
+
static void nv_adma_irq_clear(struct ata_port *ap)
{
struct nv_adma_port_priv *pp = ap-private_data
Re: System freezes with kernel 2.6.19 - sata_nv [added crash info kern 2.6.21.1]
Stefan wrote: First thing you should try is replacing the SATA cable to that drive. Yeap, please apply some hardware debugging techniques - replacing / reseating SATA cables and connecting it to different power connector. But it's disturbing to see machine lock up even if CRC error occurs. sata_nv non-adma interface locks the whole machine up too after certain error conditions but I thought adma was saner than that. I hope we can work around this somehow. Thanks. I have just replaced the cables with brand new ones, still no change the machine freezes ~10min after boot. Sometimes I even have to completely power down the machine to be able to boot again. Power problems would be another possibility.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: System freezes with kernel 2.6.19 - sata_nv [added crash info kern 2.6.21.1]
Stefan wrote: Okay, I had time to set this up. I'm attaching the log messages I got via netconsole. I tested about 20h with adma disabled, the crash won't occur. If I remove sata_nv.adma=0 from boot options again it doesn't take long until my machine locks up. [Attached dmesg output with 2.6.21.1 kernel + crash info I got via netconsole] I hope this is useful to you guys. It looks like you've got SError bits set from the controller, 0x20 means link layer CRC error (btw, we really should be decoding that error and printing it in human readable form rather than making people pore through the SATA spec and count bits). First thing you should try is replacing the SATA cable to that drive. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: System freezes with kernel 2.6.19 - sata_nv
Tejun Heo wrote: Stefan wrote: Hi folks, yesterday I upgraded kernel 2.6.19 to 2.6.20 (gentoo kernel). Now my box locks up about 10 min after boot. After that I tested with a vanilla 2.6.21.1 it shows the same behavior. I'm attaching a kern log file from the 2.6.20. The 2.6.21.1 locked up so hard, that there was no trace left in the log file. If I switch back to 2.6.19 everything is fine again. If necessary I will hook up a laptop to this box, so I can capture messages via netconsole. Yes please. This machine is running an AMD X2 64, NFORCE4 (ASUS A8N-E) I haven't been following the development of libata for a while, but from the 2.6.20 changelog It looks like there have been some major changes. Just let me know what kind of information you need in order to narrow it down. Does giving 'sata_nv.adma=0' kernel parameter make any difference? If adma=0 words, then that means it's either an ADMA related problem or that SAMSUNG HD401LJ drive has some problems with NCQ (since ADMA off means NCQ off as well). I would say the latter is more likely. We should really have some kind of noncq kernel parameter we can use to help debugging these problems. Though, later kernels are supposed to switch it off automatically after too many errors.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: skip FLUSH and STADNBYNOW1 during shutdown if device is already spun down
Tejun Heo wrote:
libata didn't use to spin down disks properly on shutdown and userland
shutdown(8) worked around it by synchronizing cache and spinning down
by itself before telling the kernel to shutdown. However, this
userland work around collides with libata shutdown because some drives
spin up if it receives FLUSH or STANDBYNOW1 while spun down. This
results in unpleasant spin down-up-down sequence.
This patch makes libata skip FLUSH and STANDBYNOW1 during shutdown if
the drive is already spun down. Note that whether FLUSH has been
performed is not checked. This is because some userland shutdown(8)'s
only do STANDBYNOW1. Transition to standby mode implies cache flush,
so this should be safe.
Are we sure this is true in all cases? The ATA spec doesn't explicitly
say that STANDBY IMMEDIATE implies a cache flush. Granted it would be
retarded for a drive to spin itself down with data still pending in the
write cache, but firmware people have done some strange things..
libata prints informational messages when skipping commands. This is
for debugging and to urge distributions to update shutdown(8) such
that it doesn't do superflous flush and spindown.
Signed-off-by: Tejun Heo [EMAIL PROTECTED]
---
drivers/ata/libata-scsi.c | 29 +
include/linux/libata.h|1 +
2 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8f80019..2a0717c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1309,6 +1309,7 @@ nothing_to_do:
static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc-ap;
+ struct ata_device *adev = qc-dev;
struct scsi_cmnd *cmd = qc-scsicmd;
u8 *cdb = cmd-cmnd;
int need_sense = (qc-err_mask != 0);
@@ -1349,6 +1350,13 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd
*qc)
}
}
+ /* Set spundown status. Some userland tools use STANDBY
+* instead of STANDBYNOW1. Take both into account.
+*/
+ if (unlikely(qc-tf.command == ATA_CMD_STANDBY ||
+qc-tf.command == ATA_CMD_STANDBYNOW1))
+ adev-flags |= ATA_DFLAG_SPUNDOWN;
Is checking for STANDBY really valid here? STANDBY does not do an
immediate entry to standby mode, it only sets the standby timer.
Therefore just because userspace issued a standby command, does not mean
the drive is really spun down currently.
Could we use CHECK POWER MODE to see if the drive is currently spun down
rather than tracking whether a standby was already issued? That would
handle the case above as well, we could tell if the drive had actually
spun down on a timer or not.
+
if (need_sense !ap-ops-error_handler)
ata_dump_status(ap-print_id, qc-result_tf);
@@ -1454,6 +1462,27 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
if (xlat_func(qc))
goto early_finish;
+ /* Some userland shutdown(8) spins down device to work around
+* previous kernel bugs. Issuing cache flush or spin down
+* again might spin up some drives. Skip cache flush and
+* spindown for -shutdown if it's already spun down.
+*/
+ switch (qc-tf.command) {
+ case ATA_CMD_FLUSH:
+ case ATA_CMD_FLUSH_EXT:
+ case ATA_CMD_STANDBYNOW1: /* -shutdown always uses STANDBYNOW1 */
+ if (unlikely((system_state SYSTEM_RUNNING)
+(dev-flags ATA_DFLAG_SPUNDOWN))) {
+ ata_dev_printk(dev, KERN_INFO, already spun down,
+ skipping cmd 0x%x\n, qc-tf.command);
+ cmd-result = SAM_STAT_GOOD;
+ goto early_finish;
+ }
+ break;
+ default:
+ dev-flags = ~ATA_DFLAG_SPUNDOWN;
+ }
+
/* select device, send command to hardware */
ata_qc_issue(qc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6ef4055..fa551fd 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
ATA_DFLAG_CDB_INTR = (1 2), /* device asserts INTRQ when ready
for CDB */
ATA_DFLAG_NCQ = (1 3), /* device supports NCQ */
ATA_DFLAG_FLUSH_EXT = (1 4), /* do FLUSH_EXT instead of FLUSH */
+ ATA_DFLAG_SPUNDOWN = (1 5), /* device is spun down by user */
ATA_DFLAG_CFG_MASK = (1 8) - 1,
ATA_DFLAG_PIO = (1 8), /* device limited to PIO mode */
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: add Samsung HD401LJ to the NCQ blacklist
Max Kellermann wrote: Yet another hard drive which doesn't seem to get NCQ right. ata1: EH in ADMA mode, notifier 0x0 notifier_error 0x0 gen_ctl 0x1501000 status 0x400 next cpb count 0xB next cpb idx 0x0 [...] ata1: timeout waiting for ADMA IDLE, stat=0x400 ata1: timeout waiting for ADMA LEGACY, stat=0x400 ata1.00: exception Emask 0x0 SAct 0x3fff SErr 0x20 action 0x2 frozen [...] ata1: soft resetting port Don't know if I noticed this before, but the controller seems to be reporting an SError, looks like a link layer CRC error. This could be a potential symptom of the drive and controller just not getting along, but you might want to try a different SATA cable as well. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: HPA support
- Original Message - From: Kyle McMartin [EMAIL PROTECTED] Date: Friday, April 13, 2007 10:47 am Subject: Re: [PATCH] libata: HPA support [Adding Robert to the CC incase he doesn't follow linux-ide] On Fri, Apr 13, 2007 at 12:33:41PM -0400, Kyle McMartin wrote: On Fri, Apr 13, 2007 at 12:24:34PM -0400, Jeff Garzik wrote: Kyle McMartin wrote: Oddly, the command at least executes and doesn't MCE (but it's not at all happy either) if I use ATA_PROT_PIO. I wonder if ATA_PROT_NODATA is buggered on this sata_nv chip (Asus A8N-E). Weird... Try turning off ADMA using the module parameter, and see if ATA_PROT_NODATA magically works. ADMA is an advanced command execution mode, and it may not be appropriate for certain non-data commands. Thanks so much, Jeff! This did it. Think we should drop ADMA by default? Do you know off-hand if there's any other drivers this might bite us on? Seems to have been commit 382a6652e91b34d5480cfc0ed840c196650493d4 thatcaused it (submitting NODATA commands using ADMA.) Reverting that commit (or booting with sata_nv.adma=0) fixes HPA for me here... Robert, is reverting that commit going to crush my little world, or is it a safe course of action? I'd rather not disable ADMA (which turns off NCQ, right?) wholesale, as the whizbang-gentoo crowd will hang me. There is already a patch in libata-dev that will fix this, assuming those commands are marked as requiring a result taskfile in the command flags, which they should be: http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=commit;h=eb20a5742d230c67b9af4efd71b8b6b680ca3a09 ADMA should only be used for NODATA commands which don't require any result taskfile, such as cache flushes. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sata_nv: Don't attempt using ADMA for (READ|SET)_MAX commands
Mark Lord wrote: Alan Cox wrote: On Fri, 13 Apr 2007 13:08:31 -0400 Kyle McMartin [EMAIL PROTECTED] wrote: READ_NATIVE_MAX and SET_MAX were causing timeouts on sata_nv controllers. Disabling ADMA helped, but that is quite a large hammer to use. Reverting 382a6652e91b34d5480cfc0ed840c196650493d4 also helped, but we might as well fix it right, instead of disabling the performance gain on cache flushes by using ADMA mode. Probably not going to make any performance difference to blacklist all non-data commands or all but a few like cache-flush. I agree. The Pacific Digital ADMA stuff had some quirks for various non-data commands as well, and the sensible thing was to just not use ADMA for anything other than READs/WRITEs and possible CACHE FLUSHes. I'm told by NVidia that non-data commands should be fine in ADMA mode, except for those that require reading back a result taskfile (which can't be done in ADMA mode). The patch in libata-dev which I mentioned in another email will ensure that we don't try to use ADMA for such commands, so this patch should not be needed. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: add NCQ blacklist entries from Silicon Image Windows driver (v2)
Jeff Garzik wrote:
Robert Hancock wrote:
This adds some NCQ blacklist entries taken from the Silicon Image
3124/3132
Windows driver .inf files. There are some confirming reports of problems
with these drives under Linux (for example
http://lkml.org/lkml/2007/3/4/178)
so let's disable NCQ on these drives.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21-rc5-git9/drivers/ata/libata-core.c2007-04-02
21:03:29.0 -0600
+++ linux-2.6.21-rc5-git9edit/drivers/ata/libata-core.c2007-04-02
21:26:23.0 -0600
@@ -3363,6 +3363,11 @@ static const struct ata_blacklist_entry {
Maxtor 6L250S0, BANC1G10, ATA_HORKAGE_NONCQ },
/* NCQ hard hangs device under heavier load, needs hard power
cycle */
{ Maxtor 6B250S0,BANC1B70,ATA_HORKAGE_NONCQ },
+/* Blacklist entries taken from Silicon Image 3124/3132
+ Windows driver .inf file - also several Linux problem reports */
+{ HTS541060G9SA00,MB3OC60D, ATA_HORKAGE_NONCQ, },
+{ HTS541080G9SA00,MB4OC60D, ATA_HORKAGE_NONCQ, },
+{ HTS541010G9SA00,MBZOC60D, ATA_HORKAGE_NONCQ, },
The thread you link to seems like an irq problem, especially because it
worked in 2.6.20 and prior?
Jeff
According to this post:
http://lkml.org/lkml/2007/3/9/475
with 2.6.21-rc3, it started working after the kernel disabled NCQ
because of too many errors. That seems to point away from it being an
IRQ problem, as you'd expect it to not work at all. I don't expect the
interrupts would be handled any differently between NCQ and non-NCQ
commands. However, apparently disabling ACPI also prevents the problem,
which does seem a bit odd.
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: add NCQ blacklist entries from Silicon Image Windows driver (v2)
This adds some NCQ blacklist entries taken from the Silicon Image 3124/3132
Windows driver .inf files. There are some confirming reports of problems
with these drives under Linux (for example http://lkml.org/lkml/2007/3/4/178)
so let's disable NCQ on these drives.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21-rc5-git9/drivers/ata/libata-core.c 2007-04-02
21:03:29.0 -0600
+++ linux-2.6.21-rc5-git9edit/drivers/ata/libata-core.c 2007-04-02
21:26:23.0 -0600
@@ -3363,6 +3363,11 @@ static const struct ata_blacklist_entry
{ Maxtor 6L250S0, BANC1G10, ATA_HORKAGE_NONCQ },
/* NCQ hard hangs device under heavier load, needs hard power cycle */
{ Maxtor 6B250S0, BANC1B70, ATA_HORKAGE_NONCQ },
+ /* Blacklist entries taken from Silicon Image 3124/3132
+ Windows driver .inf file - also several Linux problem reports */
+ { HTS541060G9SA00,MB3OC60D, ATA_HORKAGE_NONCQ, },
+ { HTS541080G9SA00,MB4OC60D, ATA_HORKAGE_NONCQ, },
+ { HTS541010G9SA00,MBZOC60D, ATA_HORKAGE_NONCQ, },
/* Devices with NCQ limits */
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sata_nv ADMA controller lockup investigation
Neil Schemenauer wrote: Not sure if this helps. I'm getting this reset with 2.6.21-rc4. After the reset the controller seems to work again. ... ata2.00: ATA-7: Maxtor 6V300F0, VA111630, max UDMA/133 ata2.00: 586114704 sectors, multi 16: LBA48 NCQ (depth 31/32) ... ata2: EH in ADMA mode, notifier 0x0 notifier_error 0x0 gen_ctl 0x1501000 status 0x400 next cpb count 0x0 next cpb idx 0x0 ata2: CPB 1: ctl_flags 0x1f, resp_flags 0x2 ata2: timeout waiting for ADMA IDLE, stat=0x400 ata2: timeout waiting for ADMA LEGACY, stat=0x400 ata2.00: exception Emask 0x0 SAct 0x2 SErr 0x0 action 0x2 frozen ata2.00: cmd 61/00:08:72:44:22/02:00:21:00:00/40 tag 1 cdb 0x0 data 262144 out res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout) ata2: soft resetting port ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 300) ata2.00: configured for UDMA/133 ata2: EH complete SCSI device sda: 586114704 512-byte hdwr sectors (300091 MB) sda: Write Protect is off sda: Mode Sense: 00 3a 00 00 SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA That one looks like a drive-side issue. CPB resp_flags 2 indicates the drive accepted the command and the controller is still waiting for a response. Could be that this is another drive that needs to be added to the NCQ blacklist, some similar Maxtor models seem to have issues.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode
Jeff Garzik wrote: [EMAIL PROTECTED] wrote: From: Robert Hancock [EMAIL PROTECTED] Reading from the ATA shadow registers while we are in ADMA mode may cause undefined behavior. Don't read the ATA status register when completing commands for this reason, it shouldn't be needed as the controller will notify us if the command failed. Also, don't allow commands with result taskfile requested to execute in ADMA mode, since that requires accessing the shadow registers. We also still need to override tf_read since libata will read the result taskfile on a command failure, and we need to go into port register mode before allowing this. Signed-off-by: Robert Hancock [EMAIL PROTECTED] Signed-off-by: Andrew Morton [EMAIL PROTECTED] --- drivers/ata/sata_nv.c | 33 - 1 file changed, 20 insertions(+), 13 deletions(-) Robert, do you think this should be pushed into #upstream-fixes (2.6.21-rc)? I lean towards yes, since it is a needed-by-hardware fix, but I also am interested in testing feedback since it is so late in the 2.6.21-rc game. I would lean toward that as well, but it would be good to get some testing from some sata_nv ADMA users to make sure it doesn't do anything funny for them.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode
Alistair John Strachan wrote: I lean towards yes, since it is a needed-by-hardware fix, but I also am interested in testing feedback since it is so late in the 2.6.21-rc game. I would lean toward that as well, but it would be good to get some testing from some sata_nv ADMA users to make sure it doesn't do anything funny for them.. Since I've been a bit of a problem case this time, I'd be happy to test it. Can I assume that I can apply the patch you sent to Jeff [PATCH] sata_nv: revert use of notifiers for now, and apply this one, to -rc3, and then be able to usefully test? Yes, you should be able to. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: revert use of notifiers for now
Commit 721449bf0d51213fe3abf0ac3e3561ef9ea7827a added support for using the
ADMA notifier bits to determine which commands to check for completion.
However there have been reports that this causes command timeouts in certain
cases. This is still being investigated. In addition, apparently the notifiers
won't work if ADMA is disabled on the other port as a result of an ATAPI device
being connected, and we don't handle this case properly.
For now, just restore the previous behavior of checking all active commands
to see if they are complete, without relying on the notifiers.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21-rc3-git3/drivers/ata/sata_nv.c 2007-03-08 17:15:25.0
-0600
+++ linux-2.6.21-rc3-git3edit/drivers/ata/sata_nv.c 2007-03-08
17:19:42.0 -0600
@@ -874,8 +874,14 @@
if (status (NV_ADMA_STAT_DONE |
NV_ADMA_STAT_CPBERR)) {
- u32 check_commands = notifier | notifier_error;
+ u32 check_commands;
int pos, error = 0;
+
+ if(ata_tag_valid(ap-active_tag))
+ check_commands = 1 ap-active_tag;
+ else
+ check_commands = ap-sactive;
+
/** Check CPBs for completed commands */
while ((pos = ffs(check_commands)) !error) {
pos--;
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: don't read shadow registers when in ADMA mode
Reading from the ATA shadow registers while we are in ADMA mode may cause
undefined behavior. Don't read the ATA status register when completing commands
for this reason, it shouldn't be needed as the controller will notify us if
the command failed. Also, don't allow commands with result taskfile requested
to execute in ADMA mode, since that requires accessing the shadow registers.
We also still need to override tf_read since libata will read the result
taskfile on a command failure, and we need to go into port register mode
before allowing this.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21-rc2-git3/drivers/ata/sata_nv.c 2007-03-04 14:44:05.0
-0600
+++ linux-2.6.21-rc2-git3edit/drivers/ata/sata_nv.c 2007-03-05
19:55:14.0 -0600
@@ -260,6 +260,7 @@ static int nv_adma_port_resume(struct at
static void nv_adma_error_handler(struct ata_port *ap);
static void nv_adma_host_stop(struct ata_host *host);
static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
+static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
enum nv_host_type
{
@@ -435,7 +436,7 @@ static const struct ata_port_operations
static const struct ata_port_operations nv_adma_ops = {
.port_disable = ata_port_disable,
.tf_load= ata_tf_load,
- .tf_read= ata_tf_read,
+ .tf_read= nv_adma_tf_read,
.check_atapi_dma= nv_adma_check_atapi_dma,
.exec_command = ata_exec_command,
.check_status = ata_check_status,
@@ -667,6 +668,18 @@ static int nv_adma_check_atapi_dma(struc
return !(pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE);
}
+static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
+{
+ /* Since commands where a result TF is requested are not
+ executed in ADMA mode, the only time this function will be called
+ in ADMA mode will be if a command fails. In this case we
+ don't care about going into register mode with ADMA commands
+ pending, as the commands will all shortly be aborted anyway. */
+ nv_adma_register_mode(ap);
+
+ ata_tf_read(ap, tf);
+}
+
static unsigned int nv_adma_tf_to_cpb(struct ata_taskfile *tf, __le16 *cpb)
{
unsigned int idx = 0;
@@ -738,19 +751,11 @@ static int nv_adma_check_cpb(struct ata_
return 1;
}
- if (flags NV_CPB_RESP_DONE) {
+ if (likely(flags NV_CPB_RESP_DONE)) {
struct ata_queued_cmd *qc = ata_qc_from_tag(ap, cpb_num);
VPRINTK(CPB flags done, flags=0x%x\n, flags);
if (likely(qc)) {
- /* Grab the ATA port status for non-NCQ commands.
- For NCQ commands the current status may have nothing
to do with
- the command just completed. */
- if (qc-tf.protocol != ATA_PROT_NCQ) {
- u8 ata_status = readb(pp-ctl_block +
(ATA_REG_STATUS * 4));
- qc-err_mask |= ac_err_mask(ata_status);
- }
- DPRINTK(Completing qc from tag %d with err_mask
%u\n,cpb_num,
- qc-err_mask);
+ DPRINTK(Completing qc from tag %d\n,cpb_num);
ata_qc_complete(qc);
} else {
struct ata_eh_info *ehi = ap-eh_info;
@@ -1161,9 +1166,11 @@ static int nv_adma_use_reg_mode(struct a
struct nv_adma_port_priv *pp = qc-ap-private_data;
/* ADMA engine can only be used for non-ATAPI DMA commands,
- or interrupt-driven no-data commands. */
+ or interrupt-driven no-data commands, where a result taskfile
+ is not required. */
if((pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE) ||
- (qc-tf.flags ATA_TFLAG_POLLING))
+ (qc-tf.flags ATA_TFLAG_POLLING) ||
+ (qc-flags ATA_QCFLAG_RESULT_TF))
return 1;
if((qc-flags ATA_QCFLAG_DMAMAP) ||
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata FUA revisited
Ric Wheeler wrote: I think that FUA was designed for a different use case than what Linux is using barriers for currently. The advantage with FUA is when you have before barrier, after barrier and don't care sets, where only the specific things you care about ordering are in the before/after barrier sets. Then you can do this: Issue all before barrier requests with FUA bit set Wait for all those to complete Issue all after barrier requests with FUA bit set Wait for all those to complete A couple of issues with this would be in how to support our current semantics of fsync(). Today, the flush behavior of the barrier/fsync combination means that applications can have a hard promise of data on platter for any file after a successful fsync command. If I understand correctly, to get a similar semantic from a pure FUA implementation would require us to tag all file IO as FUA. I suspect that this would actually be less efficient since it would not allow the drives to reorder IO's up to the point that we actually care (fsync time). I think for the fsync case a cache flush would likely still be needed, unless the app was only writing small amounts of data in between the syncs (it may be complicated to figure out when to do that, though). The other big user of barriers is the internal transaction of journaled file systems. It would seem that we would need to tag each write from the journal with a FUA IO as well. Again, we might actually go more slowly in some cases as you mention below. The limited queue depth of NCQ would seem to make it much harder to have a win in this case... I think the journal write case is less problematic as there are likely to be much fewer/smaller requests involved which would be more likely to fit inside the queue.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: add NCQ blacklist entries from Silicon Image Windows driver
Ask Bjørn Hansen wrote:
On Feb 21, 2007, at 10:57 PM, Jeff Garzik wrote:
+/* The following blacklist entries are taken from the Windows
+ driver .inf files for the Silicon Image 3124 and 3132. */
+{ Maxtor 7B250S0,BANC1B70,ATA_HORKAGE_NONCQ, },
[...]
Do we have information that these drives fail on non-SiI controllers?
At least tangentially related:
On one of my boxes (running 2.6.18-1.2869 from Fedora) I have a couple
of other Maxtor drives that didn't like NCQ. They are on a JMicron
20360/20363 (ahci driver). (There's also a Promise 300 TX4 card in the
box and an Intel ICH8 that shows up with ata_piix).
model and (partial) firmware revision of the drives:
Maxtor 7V300F0 VA11
Maxtor 7B300S0 BANC
Until I disabled NCQ I got gazillions of messages like the ones below
and absymal performance.
- ask
ata5: spurious interrupt (irq_stat 0x8 active_tag -84148995 sactive 0xf)
Sounds like those are some that we should be blacklisting as well,
unless Eric has a good reason why not (CCing). Can you provide the full
firmware revision strings from those drives, i.e. from hdparm -I?
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata FUA revisited
Tejun Heo wrote: Aside from the issue above, as I mentioned elsewhere, lots of NCQ drives don't support non-NCQ FUA writes.. To me, using the NCQ FUA bit on such drives doesn't seem to be a good idea. Maybe I'm just too chicken but it's not like we can gain a lot from doing FUA at this point. Are there a lot of drives which support NCQ but not FUA opcodes? Well, it's hard to say whether lots have this issue, but the ones I have in my machine, Seagate 7200.7 NCQ 160GB (ST3160827AS) and 7200.10 320GB (ST3320620AS), both support NCQ and don't support non-NCQ FUA, and those (especially the latter) seem to be very popular models. Likely Seagate didn't implement that command since they figured nobody would use that if they had NCQ.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: add NCQ blacklist entries from Silicon Image Windows driver
This patch adds in some NCQ blacklist entries taken from the Silicon
Image Windows drivers' .inf files for the 3124 and 3132 controllers.
These entries were marked as DisableSataQueueing. Assume these are
in their blacklist for a reason and disable NCQ on these drives.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21-rc1edit/drivers/ata/libata-core.c.prev 2007-02-21
22:23:05.0 -0600
+++ linux-2.6.21-rc1edit/drivers/ata/libata-core.c 2007-02-21
22:25:44.0 -0600
@@ -3269,6 +3269,13 @@ static const struct ata_blacklist_entry
/* Devices with NCQ limits */
+ /* The following blacklist entries are taken from the Windows
+ driver .inf files for the Silicon Image 3124 and 3132. */
+ { Maxtor 7B250S0, BANC1B70, ATA_HORKAGE_NONCQ, },
+ { HTS541060G9SA00, MB3OC60D, ATA_HORKAGE_NONCQ, },
+ { HTS541080G9SA00, MB4OC60D, ATA_HORKAGE_NONCQ, },
+ { HTS541010G9SA00, MBZOC60D, ATA_HORKAGE_NONCQ, },
+
/* End Marker */
{ }
};
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: complain on spurious completion notifiers
Recently Tejun wrote a patch to ahci.c to make it raise a HSM violation
if the drive attempted to complete a tag that wasn't outstanding. We could
run into the same problem with sata_nv ADMA. This adds code to raise a HSM
violation error if the controller gives us a notifier tag that isn't
outstanding, since the drive may be issuing spurious completions.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.21-rc1edit/drivers/ata/sata_nv.c.prev 2007-02-21
22:17:31.0 -0600
+++ linux-2.6.21-rc1edit/drivers/ata/sata_nv.c 2007-02-21 22:22:14.0
-0600
@@ -740,6 +740,17 @@ static int nv_adma_check_cpb(struct ata_
DPRINTK(Completing qc from tag %d with err_mask
%u\n,cpb_num,
qc-err_mask);
ata_qc_complete(qc);
+ } else {
+ struct ata_eh_info *ehi = ap-eh_info;
+ /* Notifier bits set without a command may indicate the
drive
+ is misbehaving. Raise host state machine violation
on this
+ condition. */
+ ata_port_printk(ap, KERN_ERR, notifier for tag %d with no
command?\n,
+ cpb_num);
+ ehi-err_mask |= AC_ERR_HSM;
+ ehi-action |= ATA_EH_SOFTRESET;
+ ata_port_freeze(ap);
+ return 1;
}
}
return 0;
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: kill old private BMDMA helper functions
sata_nv implemented its own copies of the BMDMA helper functions for ADMA,
since the ADMA BMDMA status registers are PIO while the other registers
are MMIO, and this was the only way to handle this previously. Now that
we have iomap support, the standard routines should just work, so use them.
The only thing we need to override as far as ADMA and BMDMA is the
post_internal_cmd callback, where we should only call ata_post_internal_cmd
if we are in port-register mode.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git14edit/drivers/ata/sata_nv.c.before_cleanup 2007-02-20
19:42:32.0 -0600
+++ linux-2.6.20-git14edit/drivers/ata/sata_nv.c2007-02-20
19:45:44.0 -0600
@@ -255,10 +255,7 @@
static int nv_adma_port_resume(struct ata_port *ap);
static void nv_adma_error_handler(struct ata_port *ap);
static void nv_adma_host_stop(struct ata_host *host);
-static void nv_adma_bmdma_setup(struct ata_queued_cmd *qc);
-static void nv_adma_bmdma_start(struct ata_queued_cmd *qc);
-static void nv_adma_bmdma_stop(struct ata_queued_cmd *qc);
-static u8 nv_adma_bmdma_status(struct ata_port *ap);
+static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
enum nv_host_type
{
@@ -433,16 +430,16 @@
.exec_command = ata_exec_command,
.check_status = ata_check_status,
.dev_select = ata_std_dev_select,
- .bmdma_setup= nv_adma_bmdma_setup,
- .bmdma_start= nv_adma_bmdma_start,
- .bmdma_stop = nv_adma_bmdma_stop,
- .bmdma_status = nv_adma_bmdma_status,
+ .bmdma_setup= ata_bmdma_setup,
+ .bmdma_start= ata_bmdma_start,
+ .bmdma_stop = ata_bmdma_stop,
+ .bmdma_status = ata_bmdma_status,
.qc_prep= nv_adma_qc_prep,
.qc_issue = nv_adma_qc_issue,
.freeze = nv_ck804_freeze,
.thaw = nv_ck804_thaw,
.error_handler = nv_adma_error_handler,
- .post_internal_cmd = nv_adma_bmdma_stop,
+ .post_internal_cmd = nv_adma_post_internal_cmd,
.data_xfer = ata_data_xfer,
.irq_handler= nv_adma_interrupt,
.irq_clear = nv_adma_irq_clear,
@@ -899,73 +896,12 @@
iowrite8(ioread8(dma_stat_addr), dma_stat_addr);
}
-static void nv_adma_bmdma_setup(struct ata_queued_cmd *qc)
-{
- struct ata_port *ap = qc-ap;
- unsigned int rw = (qc-tf.flags ATA_TFLAG_WRITE);
- struct nv_adma_port_priv *pp = ap-private_data;
- u8 dmactl;
-
- if(!(pp-flags NV_ADMA_PORT_REGISTER_MODE)) {
- WARN_ON(1);
- return;
- }
-
- /* load PRD table addr. */
- iowrite32(ap-prd_dma, ap-ioaddr.bmdma_addr + ATA_DMA_TABLE_OFS);
-
- /* specify data direction, triple-check start bit is clear */
- dmactl = ioread8(ap-ioaddr.bmdma_addr + ATA_DMA_CMD);
- dmactl = ~(ATA_DMA_WR | ATA_DMA_START);
- if (!rw)
- dmactl |= ATA_DMA_WR;
-
- iowrite8(dmactl, ap-ioaddr.bmdma_addr + ATA_DMA_CMD);
-
- /* issue r/w command */
- ata_exec_command(ap, qc-tf);
-}
-
-static void nv_adma_bmdma_start(struct ata_queued_cmd *qc)
-{
- struct ata_port *ap = qc-ap;
- struct nv_adma_port_priv *pp = ap-private_data;
- u8 dmactl;
-
- if(!(pp-flags NV_ADMA_PORT_REGISTER_MODE)) {
- WARN_ON(1);
- return;
- }
-
- /* start host DMA transaction */
- dmactl = ioread8(ap-ioaddr.bmdma_addr + ATA_DMA_CMD);
- iowrite8(dmactl | ATA_DMA_START,
-ap-ioaddr.bmdma_addr + ATA_DMA_CMD);
-}
-
-static void nv_adma_bmdma_stop(struct ata_queued_cmd *qc)
+static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc)
{
- struct ata_port *ap = qc-ap;
- struct nv_adma_port_priv *pp = ap-private_data;
-
- if(!(pp-flags NV_ADMA_PORT_REGISTER_MODE))
- return;
-
- /* clear start/stop bit */
- iowrite8(ioread8(ap-ioaddr.bmdma_addr + ATA_DMA_CMD) ~ATA_DMA_START,
-ap-ioaddr.bmdma_addr + ATA_DMA_CMD);
-
- /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
- ata_altstatus(ap);/* dummy read */
-}
-
-static u8 nv_adma_bmdma_status(struct ata_port *ap)
-{
- struct nv_adma_port_priv *pp = ap-private_data;
-
- WARN_ON(!(pp-flags NV_ADMA_PORT_REGISTER_MODE));
+ struct nv_adma_port_priv *pp = qc-ap-private_data;
- return ioread8(ap-ioaddr.bmdma_addr + ATA_DMA_STATUS);
+ if(pp-flags NV_ADMA_PORT_REGISTER_MODE)
+ ata_bmdma_post_internal_cmd(qc);
}
static int nv_adma_port_start(struct ata_port *ap)
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo
Re: libata FUA revisited
Jens Axboe wrote: But we can't really change that, since you need the cache flushed before issuing the FUA write. I've been advocating for an ordered bit for years, so that we could just do: 3. w/FUA+ORDERED normal operation - barrier issued - write barrier FUA+ORDERED - normal operation resumes So we don't have to serialize everything both at the block and device level. I would have made FUA imply this already, but apparently it's not what MS wanted FUA for, so... The current implementations take the FUA bit (or WRITE FUA) as a hint to boost it to head of queue, so you are almost certainly going to jump ahead of already queued writes. Which we of course really do not. I think that FUA was designed for a different use case than what Linux is using barriers for currently. The advantage with FUA is when you have before barrier, after barrier and don't care sets, where only the specific things you care about ordering are in the before/after barrier sets. Then you can do this: Issue all before barrier requests with FUA bit set Wait for all those to complete Issue all after barrier requests with FUA bit set Wait for all those to complete Meanwhile a bunch of don't care requests could be going through on the device in the background. If we could do this, then I think there would be an advantage. Right now, it just saves a command to the drive when we're flushing on the post-barrier writes. This would only be efficient with NCQ FUA, because regular FUA forces the requests to complete serially, whereas in this case we don't really care what order the individual requests finish, we just care about the ordering of the pre vs. post barrier requests. I'm not too nervous about the FUA write commands, I hope we can safely assume that if you set the FUA supported bit in the id AND the write fua command doesn't get aborted, that FUA must work. Anything else would just be an immensely stupid implementation. NCQ+FUA is more tricky, I agree that it being just a command bit does make it more likely that it could be ignored. And that is indeed a danger. Given state of NCQ in early firmware drives, I would not at all be surprised if the drive vendors screwed that up too. But, since we don't have the ordered bit for NCQ/FUA anyway, we do need to drain the drive queue before issuing the WRITE/FUA. And at that point we may as well not use the NCQ command, just go for the regular non-NCQ FUA write. I think that should be safe. Aside from the issue above, as I mentioned elsewhere, lots of NCQ drives don't support non-NCQ FUA writes.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: delay on switching between NCQ and non-NCQ commands
This patch appears to solve some problems with commands timing out in
cases where an NCQ command is immediately followed by a non-NCQ command
(or possibly vice versa). This is a rather ugly solution, but until we
know more about why this is needed, this is about all we can do.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6edit/drivers/ata/sata_nv.c.before_hacking 2007-02-15
18:19:13.0 -0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0
-0600
@@ -219,6 +219,7 @@
void __iomem * gen_block;
void __iomem * notifier_clear_block;
u8 flags;
+ int last_issue_ncq;
};
struct nv_host_priv {
@@ -1260,6 +1261,7 @@
{
struct nv_adma_port_priv *pp = qc-ap-private_data;
void __iomem *mmio = pp-ctl_block;
+ int curr_ncq = (qc-tf.protocol == ATA_PROT_NCQ);
VPRINTK(ENTER\n);
@@ -1274,6 +1276,14 @@
/* write append register, command tag in lower 8 bits
and (number of cpbs to append -1) in top 8 bits */
wmb();
+
+ if(curr_ncq != pp-last_issue_ncq) {
+ /* Seems to need some delay before switching between NCQ and
non-NCQ
+ commands, else we get command timeouts and such. */
+ udelay(20);
+ pp-last_issue_ncq = curr_ncq;
+ }
+
writew(qc-tag, mmio + NV_ADMA_APPEND);
DPRINTK(Issued tag %u\n,qc-tag);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: warn if speed limited due to 40-wire cable (v2)
Here's a revised version of my previous patch to warn the user if a
drive's transfer rate is limited because of a 40-wire cable detection.
This one hopefully addresses Alan's previous comments - we now do this
at the very end of the function, and the ugly if condition has been
cleaned up somewhat. Also, it's been inadvertently tested (it seems that
pata_amd Nvidia cable detection is broken in current -git..)
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6/drivers/ata/libata-core.c 2007-02-11 17:31:19.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/libata-core.c 2007-02-13
19:15:58.0 -0600
@@ -3305,19 +3305,7 @@ static void ata_dev_xfermask(struct ata_
xfer_mask = ata_pack_xfermask(ap-pio_mask,
ap-mwdma_mask, ap-udma_mask);
- /* Apply cable rule here. Don't apply it early because when
-* we handle hot plug the cable type can itself change.
-*/
- if (ap-cbl == ATA_CBL_PATA40)
- xfer_mask = ~(0xF8 ATA_SHIFT_UDMA);
- /* Apply drive side cable rule. Unknown or 80 pin cables reported
-* host side are checked drive side as well. Cases where we know a
-* 40wire cable is used safely for 80 are not checked here.
-*/
-if (ata_drive_40wire(dev-id) (ap-cbl == ATA_CBL_PATA_UNK ||
ap-cbl == ATA_CBL_PATA80))
- xfer_mask = ~(0xF8 ATA_SHIFT_UDMA);
-
-
+ /* drive modes available */
xfer_mask = ata_pack_xfermask(dev-pio_mask,
dev-mwdma_mask, dev-udma_mask);
xfer_mask = ata_id_xfermask(dev-id);
@@ -3348,6 +3336,25 @@ static void ata_dev_xfermask(struct ata_
if (ap-ops-mode_filter)
xfer_mask = ap-ops-mode_filter(ap, dev, xfer_mask);
+ /* Apply cable rule here. Don't apply it early because when
+* we handle hot plug the cable type can itself change.
+* Check this last so that we know if the transfer rate was
+* solely limited by the cable.
+* Unknown or 80 wire cables reported host side are checked
+* drive side as well. Cases where we know a 40wire cable
+* is used safely for 80 are not checked here.
+*/
+ if (xfer_mask (0xF8 ATA_SHIFT_UDMA))
+ /* UDMA/44 or higher would be available */
+ if((ap-cbl == ATA_CBL_PATA40) ||
+ (ata_drive_40wire(dev-id)
+(ap-cbl == ATA_CBL_PATA_UNK ||
+ ap-cbl == ATA_CBL_PATA80))) {
+ ata_dev_printk(dev, KERN_WARNING,
+limited to UDMA/33 due to 40-wire cable\n);
+ xfer_mask = ~(0xF8 ATA_SHIFT_UDMA);
+ }
+
ata_unpack_xfermask(xfer_mask, dev-pio_mask,
dev-mwdma_mask, dev-udma_mask);
}
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] sata_nv: various cleanups and fixes
This patch series contains several fixes for issues I noticed while debugging some command timeout problems. None of these proved to be related to that issue, but they fall into the category of things we should be doing anyway. Split into separate patches to ease bisecting in case of issues. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] sata_nv: cleanup CPB and APRD initialization
Clean up the initialization of the CPB and APRD structures so that we
strictly follow the rules for ordering of writes to the CPB flags and
response flags, and prevent duplicate initialization.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c.delayandfixes 2007-02-19
17:00:14.0 -0600
@@ -1164,11 +1159,7 @@ static void nv_adma_fill_aprd(struct ata
int idx,
struct nv_adma_prd *aprd)
{
- u8 flags;
-
- memset(aprd, 0, sizeof(struct nv_adma_prd));
-
- flags = 0;
+ u8 flags = 0;
if (qc-tf.flags ATA_TFLAG_WRITE)
flags |= NV_APRD_WRITE;
if (idx == qc-n_elem - 1)
@@ -1179,6 +1170,7 @@ static void nv_adma_fill_aprd(struct ata
aprd-addr = cpu_to_le64(((u64)sg_dma_address(sg)));
aprd-len = cpu_to_le32(((u32)sg_dma_len(sg))); /* len in bytes */
aprd-flags = flags;
+ aprd-packet_len = 0;
}
static void nv_adma_fill_sg(struct ata_queued_cmd *qc, struct nv_adma_cpb *cpb)
@@ -1199,6 +1191,8 @@ static void nv_adma_fill_sg(struct ata_q
}
if (idx 5)
cpb-next_aprd = cpu_to_le64(((u64)(pp-aprd_dma +
NV_ADMA_SGTBL_SZ * qc-tag)));
+ else
+ cpb-next_aprd = cpu_to_le64(0);
}
static int nv_adma_use_reg_mode(struct ata_queued_cmd *qc)
@@ -1231,7 +1225,10 @@ static void nv_adma_qc_prep(struct ata_q
return;
}
- memset(cpb, 0, sizeof(struct nv_adma_cpb));
+ cpb-resp_flags = NV_CPB_RESP_DONE;
+ wmb();
+ cpb-ctl_flags = 0;
+ wmb();
cpb-len = 3;
cpb-tag = qc-tag;
@@ -1255,6 +1252,8 @@ static void nv_adma_qc_prep(struct ata_q
finished filling in all of the contents */
wmb();
cpb-ctl_flags = ctl_flags;
+ wmb();
+ cpb-resp_flags = 0;
}
static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc)
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] sata_nv: Add CPB register info to error_handler output
When error handling occurs with pending commands, output the contents
of the next CPB count and next CPB index registers as well as the others,
since these may be useful for debugging.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c.delayandfixes 2007-02-19
17:00:14.0 -0600
@@ -1462,10 +1461,14 @@ static void nv_adma_error_handler(struct
u32 notifier_error = readl(mmio +
NV_ADMA_NOTIFIER_ERROR);
u32 gen_ctl = readl(pp-gen_block + NV_ADMA_GEN_CTL);
u32 status = readw(mmio + NV_ADMA_STAT);
+ u8 cpb_count = readb(mmio + NV_ADMA_CPB_COUNT);
+ u8 next_cpb_idx = readb(mmio + NV_ADMA_NEXT_CPB_IDX);
ata_port_printk(ap, KERN_ERR, EH in ADMA mode, notifier
0x%X
- notifier_error 0x%X gen_ctl 0x%X status
0x%X\n,
- notifier, notifier_error, gen_ctl, status);
+ notifier_error 0x%X gen_ctl 0x%X status 0x%X
+ next cpb count 0x%X next cpb idx 0x%x\n,
+ notifier, notifier_error, gen_ctl, status,
+ cpb_count, next_cpb_idx);
for( i=0;iNV_ADMA_MAX_CPBS;i++) {
struct nv_adma_cpb *cpb = pp-cpb[i];
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] sata_nv: enable hotplug interrupt and fix some readl/readw mismatches
We already have code that handles hotplug interrupt indications in ADMA mode, this turns on the control flag that actually enables these interrupts. Also fixes some cases in the same functions where a 16-bit register was read using a readl instead of a readw. Signed-off-by: Robert Hancock [EMAIL PROTECTED] --- linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0 -0600 +++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c.delayandfixes 2007-02-19 17:00:14.0 -0600 @@ -1041,14 +1034,15 @@ static int nv_adma_port_start(struct ata /* clear GO for register mode, enable interrupt */ tmp = readw(mmio + NV_ADMA_CTL); - writew( (tmp ~NV_ADMA_CTL_GO) | NV_ADMA_CTL_AIEN, mmio + NV_ADMA_CTL); + writew( (tmp ~NV_ADMA_CTL_GO) | NV_ADMA_CTL_AIEN | +NV_ADMA_CTL_HOTPLUG_IEN, mmio + NV_ADMA_CTL); tmp = readw(mmio + NV_ADMA_CTL); writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL); - readl( mmio + NV_ADMA_CTL );/* flush posted write */ + readw( mmio + NV_ADMA_CTL );/* flush posted write */ udelay(1); writew(tmp ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL); - readl( mmio + NV_ADMA_CTL );/* flush posted write */ + readw( mmio + NV_ADMA_CTL );/* flush posted write */ return 0; } @@ -1100,14 +1094,15 @@ static int nv_adma_port_resume(struct at /* clear GO for register mode, enable interrupt */ tmp = readw(mmio + NV_ADMA_CTL); - writew((tmp ~NV_ADMA_CTL_GO) | NV_ADMA_CTL_AIEN, mmio + NV_ADMA_CTL); + writew( (tmp ~NV_ADMA_CTL_GO) | NV_ADMA_CTL_AIEN | +NV_ADMA_CTL_HOTPLUG_IEN, mmio + NV_ADMA_CTL); tmp = readw(mmio + NV_ADMA_CTL); writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL); - readl( mmio + NV_ADMA_CTL );/* flush posted write */ + readw( mmio + NV_ADMA_CTL );/* flush posted write */ udelay(1); writew(tmp ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL); - readl( mmio + NV_ADMA_CTL );/* flush posted write */ + readw( mmio + NV_ADMA_CTL );/* flush posted write */ return 0; } @@ -1490,10 +1493,10 @@ static void nv_adma_error_handler(struct /* Reset channel */ tmp = readw(mmio + NV_ADMA_CTL); writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL); - readl( mmio + NV_ADMA_CTL );/* flush posted write */ + readw( mmio + NV_ADMA_CTL );/* flush posted write */ udelay(1); writew(tmp ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL); - readl( mmio + NV_ADMA_CTL );/* flush posted write */ + readw( mmio + NV_ADMA_CTL );/* flush posted write */ } ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] sata_nv: Cleanup taskfile setup
This edits the taskfile setup to more closely match the way that libata
sends the taskfile for other controllers. This avoids putting taskfile writes
into the CPB buffer that are not needed according to the taskfile flags.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c.delayandfixes 2007-02-19
17:00:14.0 -0600
@@ -662,29 +662,30 @@ static unsigned int nv_adma_tf_to_cpb(st
{
unsigned int idx = 0;
- cpb[idx++] = cpu_to_le16((ATA_REG_DEVICE 8) | tf-device | WNB);
-
- if ((tf-flags ATA_TFLAG_LBA48) == 0) {
- cpb[idx++] = cpu_to_le16(IGN);
- cpb[idx++] = cpu_to_le16(IGN);
- cpb[idx++] = cpu_to_le16(IGN);
- cpb[idx++] = cpu_to_le16(IGN);
- cpb[idx++] = cpu_to_le16(IGN);
- }
- else {
- cpb[idx++] = cpu_to_le16((ATA_REG_ERR8) |
tf-hob_feature);
- cpb[idx++] = cpu_to_le16((ATA_REG_NSECT 8) | tf-hob_nsect);
- cpb[idx++] = cpu_to_le16((ATA_REG_LBAL 8) | tf-hob_lbal);
- cpb[idx++] = cpu_to_le16((ATA_REG_LBAM 8) | tf-hob_lbam);
- cpb[idx++] = cpu_to_le16((ATA_REG_LBAH 8) | tf-hob_lbah);
- }
- cpb[idx++] = cpu_to_le16((ATA_REG_ERR 8) | tf-feature);
- cpb[idx++] = cpu_to_le16((ATA_REG_NSECT 8) | tf-nsect);
- cpb[idx++] = cpu_to_le16((ATA_REG_LBAL8) | tf-lbal);
- cpb[idx++] = cpu_to_le16((ATA_REG_LBAM8) | tf-lbam);
- cpb[idx++] = cpu_to_le16((ATA_REG_LBAH8) | tf-lbah);
+ if(tf-flags ATA_TFLAG_ISADDR) {
+ if (tf-flags ATA_TFLAG_LBA48) {
+ cpb[idx++] = cpu_to_le16((ATA_REG_ERR8) |
tf-hob_feature | WNB);
+ cpb[idx++] = cpu_to_le16((ATA_REG_NSECT 8) |
tf-hob_nsect);
+ cpb[idx++] = cpu_to_le16((ATA_REG_LBAL 8) |
tf-hob_lbal);
+ cpb[idx++] = cpu_to_le16((ATA_REG_LBAM 8) |
tf-hob_lbam);
+ cpb[idx++] = cpu_to_le16((ATA_REG_LBAH 8) |
tf-hob_lbah);
+ cpb[idx++] = cpu_to_le16((ATA_REG_ERR 8) |
tf-feature);
+ } else
+ cpb[idx++] = cpu_to_le16((ATA_REG_ERR 8) |
tf-feature | WNB);
+
+ cpb[idx++] = cpu_to_le16((ATA_REG_NSECT 8) | tf-nsect);
+ cpb[idx++] = cpu_to_le16((ATA_REG_LBAL8) | tf-lbal);
+ cpb[idx++] = cpu_to_le16((ATA_REG_LBAM8) | tf-lbam);
+ cpb[idx++] = cpu_to_le16((ATA_REG_LBAH8) | tf-lbah);
+ }
+
+ if(tf-flags ATA_TFLAG_DEVICE)
+ cpb[idx++] = cpu_to_le16((ATA_REG_DEVICE 8) | tf-device);
cpb[idx++] = cpu_to_le16((ATA_REG_CMD 8) | tf-command | CMDEND);
+
+ while(idx 12)
+ cpb[idx++] = cpu_to_le16(IGN);
return idx;
}
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] sata_nv: Use notifier for completion checks
The hardware provides us a notifier register that indicates what command
tags have completed. Use this to determine which CPBs to check, rather
than blindly checking all active CPBs. This should provide a minor
performance win, since if the controller has touched some of these
incomplete CPBs, accessing them will likely result in a cache miss.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c.delayandfixes 2007-02-19
17:00:14.0 -0600
@@ -853,22 +854,14 @@ static irqreturn_t nv_adma_interrupt(int
if (status (NV_ADMA_STAT_DONE |
NV_ADMA_STAT_CPBERR)) {
+ u32 check_commands = notifier | notifier_error;
+ int pos, error = 0;
/** Check CPBs for completed commands */
-
- if (ata_tag_valid(ap-active_tag)) {
- /* Non-NCQ command */
- nv_adma_check_cpb(ap, ap-active_tag,
- notifier_error (1
ap-active_tag));
- } else {
- int pos, error = 0;
- u32 active = ap-sactive;
-
- while ((pos = ffs(active)) !error) {
- pos--;
- error = nv_adma_check_cpb(ap,
pos,
- notifier_error (1
pos) );
- active = ~(1 pos );
- }
+ while ((pos = ffs(check_commands)) !error) {
+ pos--;
+ error = nv_adma_check_cpb(ap, pos,
+ notifier_error (1 pos) );
+ check_commands = ~(1 pos );
}
}
}
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC] libata: FUA updates
This updates libata FUA support to be more more in line with reality.
FUA support remains off by default.
Add a setting for the fua command-line parameter on libata which enables
FUA only on NCQ-supporting disks.
Update the ata_dev_supports_fua function to remove the blacklisting of
Maxtor BANC1G10 firmware, as there is no conclusive evidence it was ever
needed.
Centralize all of the FUA on/off decisions in this function.
Bypass the checks for FUA command support bit for NCQ disks, since that
bit only applies to non-NCQ FUA, and FUA is part of the spec for NCQ
commands.
When queue depth is set by the user resulting in enabling/disabling
NCQ, trigger a revalidate so that the FUA state will be rechecked,
since disabling/enabling NCQ may also affect FUA support on some disks.
Add a controller flag to indicate it doesn't support FUA commands, and
set this flag for Silicon Image 311x (since that one is known to have
problems) as well as for ITE821x when in smart mode.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
---
Any comments on this version? It's essentially what I posted previously
in libata FUA revisited, except that the default for FUA support remains
off, so this shouldn't break anything for anyone that doesn't explicitly
enable it.
diff -urp linux-2.6.20-git6/drivers/ata/libata-core.c
linux-2.6.20-git6edit/drivers/ata/libata-core.c
--- linux-2.6.20-git6/drivers/ata/libata-core.c 2007-02-11 17:31:19.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/libata-core.c 2007-02-19
16:55:49.0 -0600
@@ -87,7 +87,7 @@ MODULE_PARM_DESC(atapi_dmadir, Enable A
int libata_fua = 0;
module_param_named(fua, libata_fua, int, 0444);
-MODULE_PARM_DESC(fua, FUA support (0=off, 1=on));
+MODULE_PARM_DESC(fua, FUA support (0=off, 1=on for NCQ drives only, 2=on));
static int ata_probe_timeout = ATA_TMOUT_INTERNAL / HZ;
module_param(ata_probe_timeout, int, 0444);
diff -urp linux-2.6.20-git6/drivers/ata/libata-scsi.c
linux-2.6.20-git6edit/drivers/ata/libata-scsi.c
--- linux-2.6.20-git6/drivers/ata/libata-scsi.c 2007-02-11 17:31:19.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/libata-scsi.c 2007-02-19
16:47:46.0 -0600
@@ -986,7 +986,7 @@ int ata_scsi_change_queue_depth(struct s
struct ata_port *ap = ata_shost_to_port(sdev-host);
struct ata_device *dev;
unsigned long flags;
- int max_depth;
+ int max_depth, revalidate;
if (queue_depth 1)
return sdev-queue_depth;
@@ -1003,10 +1003,22 @@ int ata_scsi_change_queue_depth(struct s
scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, queue_depth);
spin_lock_irqsave(ap-lock, flags);
- if (queue_depth 1)
+ if (queue_depth 1 (dev-flags ATA_DFLAG_NCQ_OFF)) {
dev-flags = ~ATA_DFLAG_NCQ_OFF;
- else
+ revalidate = 1;
+ } else if(!(dev-flags ATA_DFLAG_NCQ_OFF)) {
dev-flags |= ATA_DFLAG_NCQ_OFF;
+ revalidate = 1;
+ }
+ /* Note: NCQ is switched off if queue depth is set to 1.
+ Thus changing the depth may also enable/disable FUA,
+ which the SCSI layer needs to know about, so we trigger
+ a revalidate. */
+ if(revalidate) {
+ ap-eh_info.action |= ATA_EH_REVALIDATE;
+ ata_port_schedule_eh(ap);
+ }
+
spin_unlock_irqrestore(ap-lock, flags);
return queue_depth;
@@ -1990,27 +2002,46 @@ static unsigned int ata_msense_rw_recove
}
/*
- * We can turn this into a real blacklist if it's needed, for now just
- * blacklist any Maxtor BANC1G10 revision firmware
+ * ata_dev_supports_fua - Determine if this device supports FUA.
+ * @dev: Device to check
+ *
+ * Determine if this device supports FUA based on drive and
+ * controller capabilities.
+ *
+ * LOCKING:
+ * None.
*/
-static int ata_dev_supports_fua(u16 *id)
+static int ata_dev_supports_fua(struct ata_device* dev)
{
- unsigned char model[ATA_ID_PROD_LEN + 1], fw[ATA_ID_FW_REV_LEN + 1];
-
+ /* Is FUA completely disabled? */
if (!libata_fua)
return 0;
- if (!ata_id_has_fua(id))
+
+ /* Does the drive support FUA?
+ NCQ-enabled drives always support FUA, otherwise
+ check if the drive indicates support for FUA commands. */
+ if((dev-flags (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
+ ATA_DFLAG_NCQ)) != ATA_DFLAG_NCQ) {
+ if(libata_fua == 1)
+ /* FUA enabled only for NCQ */
+ return 0;
+
+ /* Does the drive support FUA commands? */
+ if (!(dev-flags ATA_DFLAG_LBA48) ||
+ !ata_id_has_fua(dev-id))
+ return 0;
+
+ /* Can't use FUA if we can only use PIO and can't use
+ WRITE MULTIPLE FUA EXT */
+ if((dev-flags ATA_DFLAG_PIO) !dev
Re: pata_amd dropping to PIO on resume
Tobias Diedrich wrote: Possibly a known issue: After resume pata_amd drops from UDMA/33 to PIO on my system. Reloading the module puts both attached optical drives (master and slave) back to UDMA/33. AFAICS simplex DMA is claimed by other device, disabling DMA seems to be causing it to drop to PIO (but only after a suspend/resume cycle, not on boot or module load). Burning a DVD with 6x speed using PIO makes heavy use of burnproof and makes the whole system quite sluggish. :) Yes, the fact that it's going into simplex mode is the problem, it wasn't in simplex to start with. It looks like pata_amd does an ata_pci_clear_simplex only for certain chip models, maybe this model needs it as well? -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sata_nv ADMA controller lockup investigation
Jeff Garzik wrote:
Robert Hancock wrote:
It's curious that only the post-cache-flush command is having issues,
and normal NCQ operation seems fine. Maybe it's related to that tag 0
being reused repeatedly?
If you take cache flush out of the equation, what happens when NCQ is
enabled with a queue depth of 1 (to reproduce tag-0-used-repeatedly
condition)?
Jeff
I was able to reproduce it in my same test case with NCQ depth set to 1.
Of course, there were still some cache flushes going on there, so I'm not
certain that test really told us anything new. I'm rather doutbful that it's
related to reusing the same tag now, though, based on the tests I've been
doing. It may be something to do with switching between NCQ and non-NCQ
commands, maybe the controller isn't able to handle doing that too rapidly.
This patch seems to fix the problem - or at least it hasn't failed the tests
that
I've run so far. It's not really ideal though, so I'd like to do some more
investigation/testing before proclaiming it as a fix. Experimentally, it
appears that 10 microseconds is not enough delay, but 20 seems to work better.
Hints from the peanut gallery remain welcome.
--- linux-2.6.20-git6edit/drivers/ata/sata_nv.c.before_hacking 2007-02-15
18:19:13.0 -0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-15 22:36:02.0
-0600
@@ -219,6 +219,7 @@
void __iomem * gen_block;
void __iomem * notifier_clear_block;
u8 flags;
+ int last_issue_ncq;
};
struct nv_host_priv {
@@ -1260,6 +1261,7 @@
{
struct nv_adma_port_priv *pp = qc-ap-private_data;
void __iomem *mmio = pp-ctl_block;
+ int curr_ncq = (qc-tf.protocol == ATA_PROT_NCQ);
VPRINTK(ENTER\n);
@@ -1274,6 +1276,14 @@
/* write append register, command tag in lower 8 bits
and (number of cpbs to append -1) in top 8 bits */
wmb();
+
+ if(curr_ncq != pp-last_issue_ncq) {
+ /* Seems to need some delay before switching between NCQ and
non-NCQ
+ commands, else we get command timeouts and such. */
+ udelay(20);
+ pp-last_issue_ncq = curr_ncq;
+ }
+
writew(qc-tag, mmio + NV_ADMA_APPEND);
DPRINTK(Issued tag %u\n,qc-tag);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
sata_nv ADMA controller lockup investigation
action 0x2 frozen ata4.00: cmd 61/08:00:e0:a1:64/00:00:0a:00:00/40 tag 0 cdb 0x0 data 4096 out res 40/00:01:00:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout) ata4: soft resetting port ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata4.00: configured for UDMA/133 ata4: EH complete SCSI device sdb: 312581808 512-byte hdwr sectors (160042 MB) sdb: Write Protect is off sdb: Mode Sense: 00 3a 00 00 SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA The pattern of the hang always seems to be that it's an NCQ command that immediately follows a cache flush. When that NCQ command is issued, the IDLE bit clears as we would expect, but then it stays like that with just the STOPPED bit set. No interrupt is raised and the CPB response flags are not updated to indicate the command was released (i.e. received by the drive) or completed. It's like the controller accepted the command and then stalled before actually doing anything with it. My guess it's either a bug in the controller that needs to be somehow worked around, or we're somehow not operating the controller properly. The latter is very difficult for me to determine since the only documentation I have other than the original NVIDIA-provided driver for this controller is the ADMA standard, which this controller only vaguely follows (for example, not all of the status bits on this controller are in the ADMA standard and some of the meanings for those that are there seem to be different). My only real clue at this point is maybe there's something wrong with the way we are giving the commands to the controller, using the APPEND register. That's another thing that's not in the ADMA standard. It's curious that only the post-cache-flush command is having issues, and normal NCQ operation seems fine. Maybe it's related to that tag 0 being reused repeatedly? -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata FUA revisited
Tejun Heo wrote: On the NCQ side, I think it's pretty safe to assume that all controllers will handle it. Obviously I've verified it with sata_nv (at least that it doesn't blow up obviously), and the other two NCQ drivers we have, ahci and sata_sil24 just feed raw FIS data into the controller so there should be no issue with not supporting it. FWIW, ICH6/7/8 ahci's clear PMP field when transmitting FIS. The reason why I'm hesitant is because there is no way to tell whether the FUA bit got honored or ignored. With extra opcode, it's okay because barrier explicitly fails but if NCQ FUA is not supported, it will succeed silently as normal write. Everything will be okay generally but the barrier is done incorrectly and on a really bad day it will lead to journal corruption. Well, we should be able to determine that experimentally (at least on specific controllers) with a little test program that just writes little bits of data and fsyncs repeatedly (assuming that does in fact trigger FUAs currently..) If it runs faster than the drive could possibly be rewriting the physical disk then obviously the FUA bit is not getting through and/or not respected and we can blacklist FUA on that controller. Also, the FUA bit in the NCQ commands is in the device register, so it's not like the PMP fields where it's not used for anything else and so the controller messing with it wouldn't be otherwise noticed.. So, actually, I was thinking about *always* using the non-NCQ FUA opcode. As currently implemented, FUA request is always issued by itself, so NCQ doesn't make any difference there. So, I think it would be better to turn on FUA on driver-by-driver basis whether the controller supports NCQ or not. Unfortunately not all drives that support NCQ support the non-NCQ FUA commands (my Seagates are like this). There's definitely a potential advantage to FUA with NCQ - if you have non-synchronous accesses going on concurrently with synchronous ones, if you have to use non-NCQ FUA or flush cache commands, you have to wait for all the IOs of both types to drain out before you can issue the flush (since those can't be overlapped with the NCQ read/writes). And if you can only use flush cache, then you're forcing all the writes to be flushed including the non-synchronous ones you didn't care about. Whether or not the block layer currently exploits this I don't know, but it definitely could. Well, I might be being too paranoid but silent FUA failure would be really hard to diagnose if that ever happens (and I'm fairly certain that it will on some firmwares). Well, there are also probably drives that ignore flush cache commands or fail to do other things that they should. There's only so far we can go in coping if the firmware authors are being retarded. If any drive is broken like that we should likely just blacklist NCQ on it entirely as obviously little thought or testing went into the implementation.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata FUA revisited
Tejun Heo wrote: -Add a new port flag ATA_FLAG_NO_FUA to indicate that a controller can't handle FUA commands, and add that flag to sata_sil. Force FUA off on any drive connected to a controller with this bit set. There was some talk that sata_mv might have this problem, but I believe the conclusion was that it didn't. The only controllers that would are ones that actually try to interpret the ATA command codes and don't know about WRITE DMA FUA. I think it would be better to add ATA_FLAG_FUA instead of ATA_FLAG_NO_FUA. I'm not sure about that, it appears that the number of controllers that have problems is much lower than the number that don't, so this would just need to be added to almost every driver. Especially since the non-NCQ FUA which was problematic on SiI in the past isn't being switched on by default. -Change the fua module option to control FUA enable/disable to have a third value, enable for NCQ-supporting drives only, which would become the new default. That case seems less likely to cause problems since FUA on NCQ is just another bit in the command whereas FUA on non-NCQ is an entirely different, potentially unsupported command. Not enabling FUA doesn't result in huge performance hit. I'm not sure whether we should go such far. Just supporting FUA on known good controllers sounds good enough to me. On the NCQ side, I think it's pretty safe to assume that all controllers will handle it. Obviously I've verified it with sata_nv (at least that it doesn't blow up obviously), and the other two NCQ drivers we have, ahci and sata_sil24 just feed raw FIS data into the controller so there should be no issue with not supporting it. Assuming that we leave FUA off by default for non-NCQ for the foreseeable future, is there really much concern here? Any comments? As an aside, I came across a comment that the Silicon Image Windows drivers for NCQ-supporting controllers have some blacklist entries for drives with broken NCQ in their .inf files. We only seem to have one in the libata NCQ blacklist, we may want to add some more of these. The ones in the current SiI3124 and 3132 drivers' .inf files for DisableSataQueueing appear to be: ModelFirmware Maxtor 7B250S0BANC1B70 HTS541060G9SA00MB3OC60D HTS541080G9SA00MB4OC60D HTS541010G9SA00MBZOC60D (the latter 3 being Hitachi Travelstar drives) Yeah, I don't think we need to be too careful about blacklisting NCQ considering the sorry state of many early NCQ firmwares. Please submit a patch. It would be nice if you add a comment why the drives are added for documentation. Will do shortly. Eric, do you have any info on the blacklisting of that Maxtor 7B250S0 drive? I would hope that Silicon Image would have a good reason for blacklisting that one.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: handle SError status indication
ADMA-capable controllers provide a bit in the status register that appears
to indicate that the controller detected an SError condition. Update sata_nv
to detect this and trigger error handling in order to handle the fault.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- drivers/ata/sata_nv.c.before_serror 2007-02-11 17:49:27.0 -0600
+++ drivers/ata/sata_nv.c 2007-02-11 17:54:39.0 -0600
@@ -827,7 +827,8 @@ static irqreturn_t nv_adma_interrupt(int
/* freeze if hotplugged or controller error */
if (unlikely(status (NV_ADMA_STAT_HOTPLUG |
NV_ADMA_STAT_HOTUNPLUG |
- NV_ADMA_STAT_TIMEOUT))) {
+ NV_ADMA_STAT_TIMEOUT |
+ NV_ADMA_STAT_SERROR))) {
struct ata_eh_info *ehi = ap-eh_info;
ata_ehi_clear_desc(ehi);
@@ -841,6 +842,9 @@ static irqreturn_t nv_adma_interrupt(int
} else if (status NV_ADMA_STAT_HOTUNPLUG) {
ata_ehi_hotplugged(ehi);
ata_ehi_push_desc(ehi, : hot unplug);
+ } else if (status NV_ADMA_STAT_SERROR) {
+ /* let libata analyze SError and figure
out the cause */
+ ata_ehi_push_desc(ehi, : SError);
}
ata_port_freeze(ap);
continue;
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sata_nv: add back some verbosity into ADMA error_handler
Some debug output in the ADMA error_handler function was removed recently,
but it may be useful in certain cases, like NCQ commands timing out. Add it
back in, but make it a bit more intelligent so that it only prints if
command(s) are active and only prints the CPBs for those commands.
That way it won't spew at inappropriate times like suspend/resume.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-git6/drivers/ata/sata_nv.c 2007-02-11 17:31:19.0
-0600
+++ linux-2.6.20-git6edit/drivers/ata/sata_nv.c 2007-02-11 17:40:48.0
-0600
@@ -1442,6 +1442,26 @@ static void nv_adma_error_handler(struct
void __iomem *mmio = pp-ctl_block;
int i;
u16 tmp;
+
+ if(ata_tag_valid(ap-active_tag) || ap-sactive) {
+ u32 notifier = readl(mmio + NV_ADMA_NOTIFIER);
+ u32 notifier_error = readl(mmio +
NV_ADMA_NOTIFIER_ERROR);
+ u32 gen_ctl = readl(pp-gen_block + NV_ADMA_GEN_CTL);
+ u32 status = readw(mmio + NV_ADMA_STAT);
+
+ ata_port_printk(ap, KERN_ERR, EH in ADMA mode, notifier
0x%X
+ notifier_error 0x%X gen_ctl 0x%X status
0x%X\n,
+ notifier, notifier_error, gen_ctl, status);
+
+ for( i=0;iNV_ADMA_MAX_CPBS;i++) {
+ struct nv_adma_cpb *cpb = pp-cpb[i];
+ if( (ata_tag_valid(ap-active_tag) i ==
ap-active_tag) ||
+ ap-sactive (1 i) )
+ ata_port_printk(ap, KERN_ERR,
+ CPB %d: ctl_flags 0x%x, resp_flags
0x%x\n,
+ i, cpb-ctl_flags,
cpb-resp_flags);
+ }
+ }
/* Push us back into port register mode for error handling. */
nv_adma_register_mode(ap);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
libata FUA revisited
I've been looking at some list archives from about a year ago when there was a big hoohah about FUA in libata. To summarize what I've gotten from that discussion: Nicolas Mailhot ran into a problem with the first kernels that supported libata FUA, using a Silicon Image 3114 controller and a Maxtor 6L300S0 drive with BANC1G10 firmware. Essentially it would quickly corrupt the filesystem on bootup. After that: -A blacklist entry was added into libata disabling FUA on Maxtor drives with BANC1G10 firmware -Eric Mudama from Maxtor complained that there was nothing wrong with FUA on those drives and the blacklist should be taken out (though it never was) -It was also confirmed by Eric and others that Silicon Image 311x controllers go nuts if they're issued WRITE DMA FUA commands, at least without some driver improvements which I assume haven't happened. -Eventually FUA was disabled by default globally in libata. Given the above, what I'm proposing to do is: -Remove the blacklisting of Maxtor BANC1G10 firmware for FUA. If we need to FUA-blacklist any drives this should likely be added to the existing horkage mechanism we now have. However, at this point I don't think that's needed, considering that I've seen no conclusive evidence that any drive has ever been established to have broken FUA. -Add a new port flag ATA_FLAG_NO_FUA to indicate that a controller can't handle FUA commands, and add that flag to sata_sil. Force FUA off on any drive connected to a controller with this bit set. There was some talk that sata_mv might have this problem, but I believe the conclusion was that it didn't. The only controllers that would are ones that actually try to interpret the ATA command codes and don't know about WRITE DMA FUA. -Change the fua module option to control FUA enable/disable to have a third value, enable for NCQ-supporting drives only, which would become the new default. That case seems less likely to cause problems since FUA on NCQ is just another bit in the command whereas FUA on non-NCQ is an entirely different, potentially unsupported command. Any comments? As an aside, I came across a comment that the Silicon Image Windows drivers for NCQ-supporting controllers have some blacklist entries for drives with broken NCQ in their .inf files. We only seem to have one in the libata NCQ blacklist, we may want to add some more of these. The ones in the current SiI3124 and 3132 drivers' .inf files for DisableSataQueueing appear to be: Model Firmware Maxtor 7B250S0 BANC1B70 HTS541060G9SA00 MB3OC60D HTS541080G9SA00 MB4OC60D HTS541010G9SA00 MBZOC60D (the latter 3 being Hitachi Travelstar drives) -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH -mm] sata_nv: wait for response on entering/leaving ADMA mode
Update sata_nv to wait for the controller to indicate via the status register
that it has entered the requested state when switching between ADMA mode and
register mode. This issue came up recently when debugging some problems with
cache flush command timeouts and while it didn't appear to fix that problem,
this is something we should likely be doing in any case.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-rc6-mm3/drivers/ata/sata_nv.c 2007-02-04 21:48:25.0
-0600
+++ linux-2.6.20-rc6-mm3edit/drivers/ata/sata_nv.c 2007-02-04
22:13:36.0 -0600
@@ -507,14 +507,38 @@ static void nv_adma_register_mode(struct
{
struct nv_adma_port_priv *pp = ap-private_data;
void __iomem *mmio = pp-ctl_block;
- u16 tmp;
+ u16 tmp, status;
+ int count = 0;
if (pp-flags NV_ADMA_PORT_REGISTER_MODE)
return;
+ status = readw(mmio + NV_ADMA_STAT);
+ while(!(status NV_ADMA_STAT_IDLE) count 20) {
+ ndelay(50);
+ status = readw(mmio + NV_ADMA_STAT);
+ count++;
+ }
+ if(count == 20)
+ ata_port_printk(ap, KERN_WARNING,
+ timeout waiting for ADMA IDLE, stat=0x%hx\n,
+ status);
+
tmp = readw(mmio + NV_ADMA_CTL);
writew(tmp ~NV_ADMA_CTL_GO, mmio + NV_ADMA_CTL);
+ count = 0;
+ status = readw(mmio + NV_ADMA_STAT);
+ while(!(status NV_ADMA_STAT_LEGACY) count 20) {
+ ndelay(50);
+ status = readw(mmio + NV_ADMA_STAT);
+ count++;
+ }
+ if(count == 20)
+ ata_port_printk(ap, KERN_WARNING,
+timeout waiting for ADMA LEGACY, stat=0x%hx\n,
+status);
+
pp-flags |= NV_ADMA_PORT_REGISTER_MODE;
}
@@ -522,7 +546,8 @@ static void nv_adma_mode(struct ata_port
{
struct nv_adma_port_priv *pp = ap-private_data;
void __iomem *mmio = pp-ctl_block;
- u16 tmp;
+ u16 tmp, status;
+ int count = 0;
if (!(pp-flags NV_ADMA_PORT_REGISTER_MODE))
return;
@@ -532,6 +557,18 @@ static void nv_adma_mode(struct ata_port
tmp = readw(mmio + NV_ADMA_CTL);
writew(tmp | NV_ADMA_CTL_GO, mmio + NV_ADMA_CTL);
+ status = readw(mmio + NV_ADMA_STAT);
+ while(((status NV_ADMA_STAT_LEGACY) ||
+ !(status NV_ADMA_STAT_IDLE)) count 20) {
+ ndelay(50);
+ status = readw(mmio + NV_ADMA_STAT);
+ count++;
+ }
+ if(count == 20)
+ ata_port_printk(ap, KERN_WARNING,
+ timeout waiting for ADMA LEGACY clear and IDLE,
stat=0x%hx\n,
+ status);
+
pp-flags = ~NV_ADMA_PORT_REGISTER_MODE;
}
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH -mm] sata_nv: propagate ata_pci_device_do_resume return value
ata_pci_device_do_resume can fail if the PCI device couldn't be re-enabled.
Update sata_nv to propagate the return value from this call and to
not try to do any other resume activities if it fails. Fixes a compile
warning.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-rc6-mm3edit/drivers/ata/sata_nv.c.beforeresumechange
2007-02-04 22:18:49.0 -0600
+++ linux-2.6.20-rc6-mm3edit/drivers/ata/sata_nv.c 2007-02-04
22:20:06.0 -0600
@@ -1604,8 +1604,11 @@ static int nv_pci_device_resume(struct p
{
struct ata_host *host = dev_get_drvdata(pdev-dev);
struct nv_host_priv *hpriv = host-private_data;
+ int rc;
- ata_pci_device_do_resume(pdev);
+ rc = ata_pci_device_do_resume(pdev);
+ if(rc)
+ return rc;
if (pdev-dev.power.power_state.event == PM_EVENT_SUSPEND) {
if(hpriv-type = CK804) {
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] sd: spin down disks on removal or power-down
Stefan Richter wrote: Robert Hancock wrote: http://marc.theaimsgroup.com/?t=11692262122 It looks like Tejun's patch essentially does the same thing as mine with the addition of the control from userspace. There is one exception though, my patch also does the stop on removal of the SCSI disk (i.e. writing 1 to its delete file in sysfs, what scsiadd -r does). Isn't sd_shutdown called at this occasion? Ah, indeed it is. I was thinking of the way I had written my patch, where it only does the spindown if the system state is SYSTEM_POWER_OFF so I needed an extra call to force this on removal. Tejun's patch just checks for not SYSTEM_RESTART, so it should happen on removal as well. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: fix translation for START STOP UNIT
Jeff Garzik wrote: * Include the patch inline rather than as an attachment. Even a text/plain attachment is very difficult to review and quote in popular email programs. Jeff I'd love to, but unfortunately nobody seems to have come up with a way of doing this in Thunderbird that keeps it from mangling whitespace without a ton of hassle. I was able to get it to cooperate once (sort of, anyway, I think it may have still damaged something on the first try), but it required mangling a bunch of settings that made using it for normal mail impossible. The last time I looked, the main how-to page I found had an addendum that I gave up on this, it's too hard, I just attach the patches. If anyone has gotten any new insight.. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH -mm] sata_nv: use ADMA for NODATA commands
Patch is against 2.6.20-rc6-mm1, though will also apply to 2.6.20-rc6 if
sata_nv-cleanup-adma-error-handling-v2.patch and
sata_nv-cleanup-adma-error-handling-v2-cleanup.patch from -mm are
applied first. Testing from those who experienced the previous cache
flush timeout problem, in particular, would be appreciated.
---
Some problems showed up recently with cache flush commands timing out on
sata_nv. Previously these commands were always handled by transitioning
to legacy mode from ADMA mode first. The timeout problem was worked
around already by a change to the interrupt handling code for legacy
mode, but for non-data commands like these it appears we can handle them
in ADMA mode, so the switch to legacy mode is not needed.
This patch changes the behavior so that we use ADMA mode to submit
interrupt-driven commands with ATA_PROT_NODATA protocol. In addition to
avoiding the problem mentioned above entirely, this avoids the overhead
of switching to legacy mode and back to ADMA mode for handling cache
flushes. When handling non-DMA-mapped commands, we leave the APRD blank
and clear the NV_CPB_CTL_APRD_VALID field in the CPB so the controller
does not attempt to read it.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
--- linux-2.6.20-rc6nv/drivers/ata/sata_nv.c2007-01-28 17:05:27.0
-0600
+++ linux-2.6.20-rc6nvedit/drivers/ata/sata_nv.c2007-01-28
17:20:11.0 -0600
@@ -1172,16 +1172,31 @@ static void nv_adma_fill_sg(struct ata_q
cpb-next_aprd = cpu_to_le64(((u64)(pp-aprd_dma +
NV_ADMA_SGTBL_SZ * qc-tag)));
}
+static int nv_adma_use_reg_mode(struct ata_queued_cmd *qc)
+{
+ struct nv_adma_port_priv *pp = qc-ap-private_data;
+
+ /* ADMA engine can only be used for non-ATAPI DMA commands,
+ or interrupt-driven no-data commands. */
+ if((pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE) ||
+ (qc-tf.flags ATA_TFLAG_POLLING))
+ return 1;
+
+ if((qc-flags ATA_QCFLAG_DMAMAP) ||
+ (qc-tf.protocol == ATA_PROT_NODATA))
+ return 0;
+
+ return 1;
+}
+
static void nv_adma_qc_prep(struct ata_queued_cmd *qc)
{
struct nv_adma_port_priv *pp = qc-ap-private_data;
struct nv_adma_cpb *cpb = pp-cpb[qc-tag];
u8 ctl_flags = NV_CPB_CTL_CPB_VALID |
- NV_CPB_CTL_APRD_VALID |
NV_CPB_CTL_IEN;
- if (!(qc-flags ATA_QCFLAG_DMAMAP) ||
-(pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE)) {
+ if (nv_adma_use_reg_mode(qc)) {
nv_adma_register_mode(qc-ap);
ata_qc_prep(qc);
return;
@@ -1200,8 +1215,12 @@ static void nv_adma_qc_prep(struct ata_q
VPRINTK(qc-flags = 0x%lx\n, qc-flags);
nv_adma_tf_to_cpb(qc-tf, cpb-tf);
-
- nv_adma_fill_sg(qc, cpb);
+
+ if(qc-flags ATA_QCFLAG_DMAMAP) {
+ nv_adma_fill_sg(qc, cpb);
+ ctl_flags |= NV_CPB_CTL_APRD_VALID;
+ } else
+ memset(cpb-aprd[0], 0, sizeof(struct nv_adma_prd) * 5);
/* Be paranoid and don't let the device see NV_CPB_CTL_CPB_VALID until
we are
finished filling in all of the contents */
@@ -1216,10 +1235,9 @@ static unsigned int nv_adma_qc_issue(str
VPRINTK(ENTER\n);
- if (!(qc-flags ATA_QCFLAG_DMAMAP) ||
-(pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE)) {
+ if (nv_adma_use_reg_mode(qc)) {
/* use ATA register mode */
- VPRINTK(no dmamap or ATAPI, using ATA register mode: 0x%lx\n,
qc-flags);
+ VPRINTK(using ATA register mode: 0x%lx\n, qc-flags);
nv_adma_register_mode(qc-ap);
return ata_qc_issue_prot(qc);
} else
[PATCH] libata: fix translation for START STOP UNIT
Applies to 2.6.20-rc6.
---
libata's SCSI translation for the SCSI START STOP UNIT command with the
START bit clear (i.e. stopping the drive) appears to be incorrect. It
sends an ATA STANDBY command with the time period set to 0, which the
code comment says means now, but the ATA standard says this means
disable the standby timer, which effectively does nothing. Change this
to issue a STANDBY IMMEDIATE command which will actually spin the drive
down. The SAT (SCSI/ATA Translation) standard revision 9 concurs with
this choice.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
--- linux-2.6.20-rc6nv/drivers/ata/libata-scsi.c2007-01-28
16:59:58.0 -0600
+++ linux-2.6.20-rc6nvedit/drivers/ata/libata-scsi.c2007-01-28
17:30:12.0 -0600
@@ -983,11 +983,10 @@ static unsigned int ata_scsi_start_stop_
}
tf-command = ATA_CMD_VERIFY; /* READ VERIFY */
- } else {
- tf-nsect = 0; /* time period value (0 implies now) */
- tf-command = ATA_CMD_STANDBY;
- /* Consider: ATA STANDBY IMMEDIATE command */
- }
+ } else
+ /* Issue ATA STANDBY IMMEDIATE command */
+ tf-command = ATA_CMD_STANDBYNOW1;
+
/*
* Standby and Idle condition timers could be implemented but that
* would require libata to implement the Power condition mode page
[PATCH RFC] sd: spin down disks on removal or power-down
Here's a patch for sd.c I've cooked up which issues a START STOP UNIT
command to stop the drive when the SCSI disk is removed or the machine
is powered down. The rationale behind this is that apparently on many
drives, simply cutting power to the spinning disk forces it to do an
emergency head park/unload which creates more wear on the drive then a
controlled park/unload with power still applied. This change ensures
that the drive will be spun down if the user shuts down the machine or
if they are about to hot-unplug the drive and have done scsiadd -r first.
The main potential concern I have about this implementation is that if
the drive is used in a multi-initiator, iSCSI, etc. environment,
stopping the drive may be undesirable as another initiator may still be
accessing it. I'm not familiar enough with these setups to know if this
problem is likely to come up or not. For this and other reasons we may
want to make this behavior controllable - I'm open to suggestions on how
to do this or whether it's needed.
I've tested that this does work on SATA disks through libata (with my
patch libata: fix translation for START STOP UNIT applied). I also
tested with some external USB-to-IDE drive enclosures. The support for
START STOP UNIT on those seems rather poor though, on one enclosure with
a Genesys bridge chip it returned a check condition with Invalid field
in CDB, and on another with a JMicron chip the request succeeded but it
didn't actually spin the drive down.
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
--- linux-2.6.20-rc6nv/drivers/scsi/sd.c2007-01-28 17:00:00.0
-0600
+++ linux-2.6.20-rc6nvedit/drivers/scsi/sd.c2007-01-28 18:08:53.0
-0600
@@ -821,6 +821,39 @@ static int sd_sync_cache(struct scsi_dev
return res;
}
+static int sd_stop_unit(struct scsi_device *sdp, struct scsi_disk* sdkp)
+{
+ int res;
+ struct scsi_sense_hdr sshdr;
+ unsigned char cmd[10] = { 0 };
+
+ if (!scsi_device_online(sdp))
+ return -ENODEV;
+
+ cmd[0] = START_STOP;
+ /*
+* Leave the rest of the command zero to indicate
+* transition to stopped power condition and return
+* on completion.
+*/
+ printk(KERN_NOTICE Stopping SCSI disk %s\n,
+ sdkp-disk-disk_name);
+ res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, sshdr,
+ SD_TIMEOUT, SD_MAX_RETRIES);
+
+ if (res) {
+ printk(KERN_WARNING sd stop failed: status = %x, message =
%02x,
+ host = %d, driver = %02x\n,
+ status_byte(res), msg_byte(res),
+ host_byte(res), driver_byte(res));
+ if (driver_byte(res) DRIVER_SENSE)
+ scsi_print_sense_hdr(sd, sshdr);
+ }
+
+ return res;
+}
+
+
static int sd_issue_flush(struct device *dev, sector_t *error_sector)
{
int ret = 0;
@@ -1727,11 +1760,13 @@ static int sd_probe(struct device *dev)
**/
static int sd_remove(struct device *dev)
{
+ struct scsi_device *sdp = to_scsi_device(dev);
struct scsi_disk *sdkp = dev_get_drvdata(dev);
class_device_del(sdkp-cdev);
del_gendisk(sdkp-disk);
sd_shutdown(dev);
+ sd_stop_unit(sdp,sdkp);
mutex_lock(sd_ref_mutex);
dev_set_drvdata(dev, NULL);
@@ -1784,6 +1819,9 @@ static void sd_shutdown(struct device *d
sdkp-disk-disk_name);
sd_sync_cache(sdp);
}
+ if(system_state == SYSTEM_POWER_OFF)
+ sd_stop_unit(sdp,sdkp);
+
scsi_disk_put(sdkp);
}
[PATCH -mm] sata_nv: cleanup ADMA error handling v2
Should apply to -mm tree or current libata-dev git tree. This version
leaves out part of a change in the first version which in retrospect
should have been left alone.
---
This cleans up a few issues with the error handling in sata_nv in ADMA
mode to make it more consistent with other NCQ-capable drivers like ahci
and sata_sil24:
-When a command failed, we would effectively set AC_ERR_DEV on the
queued command always. In the case of NCQ commands this prevents libata
from doing a log page query to determine the details of the failed
command, since it thinks we've already analyzed. Just set flags in the
port ehi-err_mask, then freeze or abort and let libata figure out what
went wrong.
-The code handled NV_ADMA_STAT_CPBERR as a really bad error which
caused it to set error flags on every queued command. I don't know
exactly what this flag means (no docs, grr!) but from what I can guess
from the standard ADMA spec, it just means that one or more of the CPBs
had an error, so we just need to go through and do our normal checks in
this case.
-In the error_handler function the code would always dump the state of
all the CPBs. This output seems redundant at this point since libata
already dumps the state of all active commands on errors (and it also
triggers at times when it shouldn't, like when suspending). Take this
out.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.20-rc5/drivers/ata/sata_nv.c 2007-01-12 23:18:08.0
-0600
+++ linux-2.6.20-rc5edit/drivers/ata/sata_nv.c 2007-01-17 17:22:05.0
-0600
@@ -646,53 +646,64 @@ static unsigned int nv_adma_tf_to_cpb(st
return idx;
}
-static void nv_adma_check_cpb(struct ata_port *ap, int cpb_num, int force_err)
+static int nv_adma_check_cpb(struct ata_port *ap, int cpb_num, int force_err)
{
struct nv_adma_port_priv *pp = ap-private_data;
- int complete = 0, have_err = 0;
u8 flags = pp-cpb[cpb_num].resp_flags;
VPRINTK(CPB %d, flags=0x%x\n, cpb_num, flags);
- if (flags NV_CPB_RESP_DONE) {
- VPRINTK(CPB flags done, flags=0x%x\n, flags);
- complete = 1;
- }
- if (flags NV_CPB_RESP_ATA_ERR) {
- ata_port_printk(ap, KERN_ERR, CPB flags ATA err,
flags=0x%x\n, flags);
- have_err = 1;
- complete = 1;
- }
- if (flags NV_CPB_RESP_CMD_ERR) {
- ata_port_printk(ap, KERN_ERR, CPB flags CMD err,
flags=0x%x\n, flags);
- have_err = 1;
- complete = 1;
- }
- if (flags NV_CPB_RESP_CPB_ERR) {
- ata_port_printk(ap, KERN_ERR, CPB flags CPB err,
flags=0x%x\n, flags);
- have_err = 1;
- complete = 1;
+ if(unlikely((force_err ||
+flags (NV_CPB_RESP_ATA_ERR |
+ NV_CPB_RESP_CMD_ERR |
+ NV_CPB_RESP_CPB_ERR {
+ struct ata_eh_info *ehi = ap-eh_info;
+ int freeze = 0;
+ ata_ehi_clear_desc(ehi);
+ ata_ehi_push_desc(ehi, CPB resp_flags 0x%x, flags );
+ if(flags NV_CPB_RESP_ATA_ERR) {
+ ata_ehi_push_desc(ehi, : ATA error);
+ ehi-err_mask |= AC_ERR_DEV;
+ }
+ else if(flags NV_CPB_RESP_CMD_ERR) {
+ ata_ehi_push_desc(ehi, : CMD error);
+ ehi-err_mask |= AC_ERR_DEV;
+ }
+ else if(flags NV_CPB_RESP_CPB_ERR) {
+ ata_ehi_push_desc(ehi, : CPB error);
+ ehi-err_mask |= AC_ERR_SYSTEM;
+ freeze = 1;
+ }
+ else {
+ /* notifier error, but no error in CPB flags? */
+ ehi-err_mask |= AC_ERR_OTHER;
+ freeze = 1;
+ }
+ /* Kill all commands. EH will determine what actually failed. */
+ if(freeze)
+ ata_port_freeze(ap);
+ else
+ ata_port_abort(ap);
+ return 1;
}
- if(complete || force_err)
- {
+
+ if (flags NV_CPB_RESP_DONE) {
struct ata_queued_cmd *qc = ata_qc_from_tag(ap, cpb_num);
+ VPRINTK(CPB flags done, flags=0x%x\n, flags);
if(likely(qc)) {
- u8 ata_status = 0;
- /* Only use the ATA port status for non-NCQ commands.
+ /* Grab the ATA port status for non-NCQ commands.
For NCQ commands the current status may have nothing
to do with
the command just completed. */
- if(qc-tf.protocol != ATA_PROT_NCQ)
- ata_status = readb(pp-ctl_block +
(ATA_REG_STATUS * 4));
-
- if(have_err || force_err
Re: [PATCH -mm] sata_nv: cleanup ADMA error handling
Robert Hancock wrote: -In the error_handler function the code would always go through and do an ADMA channel reset and also dump out the state of all the CPBs. This reset seems heinous in this situation since we haven't even decided to reset anything yet. The output seems redundant at this point since libata already dumps the state of all active commands on errors (and it also triggers at times when it shouldn't, like when suspending). Do the ADMA reset only on hardreset and remove the output. Actually, upon further thought some of this stuff really should be done in the error_handler method, just maybe not the channel reset. I'll cut another patch shortly. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: Add the SGPIO support for sata_nv.c
Since I've been the one making the most changes in sata_nv lately I
figured I would make some more comments on this patch:
Subject:
RE: [PATCH] SCSI: Add the SGPIO support for sata_nv.c
From:
Peer Chen [EMAIL PROTECTED]
Date:
Tue, 7 Nov 2006 17:55:11 +0800
To:
Christoph Hellwig [EMAIL PROTECTED]
To:
Christoph Hellwig [EMAIL PROTECTED]
CC:
[EMAIL PROTECTED], linux-kernel@vger.kernel.org,
linux-ide@vger.kernel.org, Kuan Luo [EMAIL PROTECTED]
Modified and resent out the patch as attachment.
Description about the patch:
Add SGPIO support in sata_nv.c.
SGPIO (Serial General Purpose Input Output) is a sideband serial 4-wire
interface that a storage controller uses to communicate with a storage
enclosure management controller, primarily to control activity and
status LEDs that are located within drive bays or on a storage
backplane. SGPIO is defined by [SFF8485].
In this patch,we drive the LEDs to blink when read/write operation
happen on SATA drives connect the corresponding ports on MCP55 board.
==
The patch will be applied to kernel 2.6.19-rc4-git9.
Singed-off-by: Kuan Luo [EMAIL PROTECTED]
Singed-off-by: Peer Chen [EMAIL PROTECTED]
BRs
Peer Chen
---
--- linux-2.6.19-rc4-git9/drivers/ata/sata_nv.c.orig2006-11-06
08:47:49.0 +0800
+++ linux-2.6.19-rc4-git9/drivers/ata/sata_nv.c 2006-11-07 08:36:54.0
+0800
First off, can you rebase against the current libata-dev version of
sata_nv? There have been
a ton of changes since 2.6.19-rc4-git9.
Also, it would be best to make sure the code passes the checks from the
Sparse tool
using make C=1. sata_nv now passes this, it would be a shame to break that.
@@ -80,6 +80,176 @@
NV_MCP_SATA_CFG_20_SATA_SPACE_EN = 0x04,
};
+/* sgpio
+* Sgpio defines
+* SGPIO state defines
+*/
+#define NV_SGPIO_STATE_RESET 0
+#define NV_SGPIO_STATE_OPERATIONAL 1
+#define NV_SGPIO_STATE_ERROR 2
+
+/* SGPIO command opcodes */
+#define NV_SGPIO_CMD_RESET 0
+#define NV_SGPIO_CMD_READ_PARAMS 1
+#define NV_SGPIO_CMD_READ_DATA 2
+#define NV_SGPIO_CMD_WRITE_DATA3
+
+/* SGPIO command status defines */
+#define NV_SGPIO_CMD_OK0
+#define NV_SGPIO_CMD_ACTIVE1
+#define NV_SGPIO_CMD_ERR 2
+
+#define NV_SGPIO_UPDATE_TICK 90
+#define NV_SGPIO_MIN_UPDATE_DELTA 33
+#define NV_CNTRLR_SHARE_INIT 2
+#define NV_SGPIO_MAX_ACTIVITY_ON 20
+#define NV_SGPIO_MIN_FORCE_OFF 5
+#define NV_SGPIO_PCI_CSR_OFFSET0x58
+#define NV_SGPIO_PCI_CB_OFFSET 0x5C
+#define NV_SGPIO_DFLT_CB_SIZE 256
+#define NV_ON 1
+#define NV_OFF 0
+
+
+
+static inline u8 bf_extract(u8 value, u8 offset, u8 bit_count)
+{
+ return u8)(value)) (offset)) ((1 (bit_count)) - 1));
+}
+
+static inline u8 bf_ins(u8 value, u8 ins, u8 offset, u8 bit_count)
+{
+ return (((value) ~1 (bit_count)) - 1)) (offset))) |
+ (((u8)(ins)) (offset)));
+}
+
+static inline u32 bf_extract_u32(u32 value, u8 offset, u8 bit_count)
+{
+ return u32)(value)) (offset)) ((1 (bit_count)) - 1));
+
+}
+static inline u32 bf_ins_u32(u32 value, u32 ins, u8 offset, u8 bit_count)
+{
+ return (((value) ~1 (bit_count)) - 1)) (offset))) |
+ (((u32)(ins)) (offset)));
+}
Maybe some comment about what these functions do? It's not entirely obvious.
+
+#define GET_SGPIO_STATUS(v)bf_extract(v, 0, 2)
+#define GET_CMD_STATUS(v) bf_extract(v, 3, 2)
+#define GET_CMD(v) bf_extract(v, 5, 3)
+#define SET_CMD(v, cmd) bf_ins(v, cmd, 5, 3)
+
+#define GET_ENABLE(v) bf_extract(v, 23, 1)
+#define SET_ENABLE(v) bf_ins_u32(v, 1, 23, 1)
+
+/* Needs to have a u8 bit-field insert. */
+#define GET_ACTIVITY(v)bf_extract(v, 5, 3)
+#define SET_ACTIVITY(v, on_off)bf_ins(v, on_off, 5, 3)
+
+
+
+union nv_sgpio_nvcr
+{
+ struct {
+ u8 init_cnt;
+ u8 cb_size;
+ u8 cbver;
+ u8 rsvd;
+ } bit;
+ u32 all;
+};
+
+union nv_sgpio_tx
+{
+ u8 tx_port[4];
+ u32 all;
+};
+
+struct nv_sgpio_cb
+{
+ u64 scratch_space;
+ union nv_sgpio_nvcr nvcr;
+ u32 cr0;
+ u32 rsvd[4];
+ union nv_sgpio_tx tx[2];
+};
+
+struct nv_sgpio_host_share
+{
+ spinlock_t *plock;
+ unsigned long *ptstamp;
+};
+
+struct nv_sgpio_host_flags
+{
+ u8 sgpio_enabled:1;
+ u8 need_update:1;
+ u8 rsvd:6;
+};
+
+struct nv_host_sgpio
+{
+ struct nv_sgpio_host_flags flags;
+ u8 *pcsr;
+ struct
[PATCH] sata_nv: add suspend/resume support v3
This patch is against 2.6.19-rc6-mm2 and should apply to what is in
Linus' and Jeff's git trees currently. This includes a later fix to the
kfree ordering in the nv_remove_one function. Both the original patch
and the later fix are already in the -mm tree.
---
This patch adds the necessary callbacks to support suspend/resume
properly in sata_nv. Most of the controllers don't need any specific
handling but CK804/MCP04 controllers, whether ADMA is enabled or not,
need some additional setup on resume.
As well as the additional storage of the controller type needed for
proper resume handling, this also removes the inline helper functions
for getting ADMA register locations by storing the pointers so we don't
have to keep calculating them all the time.
This also includes a fix to a bug in the original version of this patch
where the hpriv structure was freed while it could potentially still be
used.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--- linux-2.6.19-rc6-mm2/drivers/ata/sata_nv.c 2006-12-12 17:49:02.0
-0600
+++ linux-2.6.19-rc6-mm2admafix/drivers/ata/sata_nv.c 2006-12-11
22:15:58.0 -0600
@@ -49,7 +49,7 @@
#include linux/libata.h
#define DRV_NAMEsata_nv
-#define DRV_VERSION3.2
+#define DRV_VERSION3.3
#define NV_ADMA_DMA_BOUNDARY0xUL
@@ -213,12 +213,21 @@ struct nv_adma_port_priv {
dma_addr_t cpb_dma;
struct nv_adma_prd *aprd;
dma_addr_t aprd_dma;
+ void __iomem * ctl_block;
+ void __iomem * gen_block;
+ void __iomem * notifier_clear_block;
u8 flags;
};
+struct nv_host_priv {
+ unsigned long type;
+};
+
#define NV_ADMA_CHECK_INTR(GCTL, PORT) ((GCTL) ( 1 (19 + (12 * (PORT)
static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static void nv_remove_one (struct pci_dev *pdev);
+static int nv_pci_device_resume(struct pci_dev *pdev);
static void nv_ck804_host_stop(struct ata_host *host);
static irqreturn_t nv_generic_interrupt(int irq, void *dev_instance);
static irqreturn_t nv_nf2_interrupt(int irq, void *dev_instance);
@@ -239,6 +248,8 @@ static irqreturn_t nv_adma_interrupt(int
static void nv_adma_irq_clear(struct ata_port *ap);
static int nv_adma_port_start(struct ata_port *ap);
static void nv_adma_port_stop(struct ata_port *ap);
+static int nv_adma_port_suspend(struct ata_port *ap, pm_message_t mesg);
+static int nv_adma_port_resume(struct ata_port *ap);
static void nv_adma_error_handler(struct ata_port *ap);
static void nv_adma_host_stop(struct ata_host *host);
static void nv_adma_bmdma_setup(struct ata_queued_cmd *qc);
@@ -292,7 +303,9 @@ static struct pci_driver nv_pci_driver =
.name = DRV_NAME,
.id_table = nv_pci_tbl,
.probe = nv_init_one,
- .remove = ata_pci_remove_one,
+ .suspend= ata_pci_device_suspend,
+ .resume = nv_pci_device_resume,
+ .remove = nv_remove_one,
};
static struct scsi_host_template nv_sht = {
@@ -311,6 +324,8 @@ static struct scsi_host_template nv_sht
.slave_configure = ata_scsi_slave_config,
.slave_destroy = ata_scsi_slave_destroy,
.bios_param = ata_std_bios_param,
+ .suspend= ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
static struct scsi_host_template nv_adma_sht = {
@@ -330,6 +345,8 @@ static struct scsi_host_template nv_adma
.slave_configure= nv_adma_slave_config,
.slave_destroy = ata_scsi_slave_destroy,
.bios_param = ata_std_bios_param,
+ .suspend= ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
static const struct ata_port_operations nv_generic_ops = {
@@ -438,6 +455,8 @@ static const struct ata_port_operations
.scr_write = nv_scr_write,
.port_start = nv_adma_port_start,
.port_stop = nv_adma_port_stop,
+ .port_suspend = nv_adma_port_suspend,
+ .port_resume= nv_adma_port_resume,
.host_stop = nv_adma_host_stop,
};
@@ -476,6 +495,7 @@ static struct ata_port_info nv_port_info
{
.sht= nv_adma_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+ ATA_FLAG_HRST_TO_RESUME |
ATA_FLAG_MMIO | ATA_FLAG_NCQ,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
@@ -492,32 +512,10 @@ MODULE_VERSION(DRV_VERSION);
static int adma_enabled = 1;
-static inline void __iomem *__nv_adma_ctl_block(void __iomem *mmio,
- unsigned int port_no)
-{
- mmio
[PATCH -mm] sata_nv: fix kfree ordering in remove
Jeff Garzik wrote: It is unwise to free the struct before the ports are even detached. Right, theoretically something bad could happen here (though not likely). Here's a fix. Sorry for attaching with something so trivial, but Thunderbird isn't very cooperative.. --- The suspend/resume change for sata_nv introduced a potential bug where the hpriv structure could be used after it was freed in nv_remove_one. Fix that. Signed-off-by: Robert Hancock [EMAIL PROTECTED] --- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ --- linux-2.6.19-rc6-mm2/drivers/ata/sata_nv.c 2006-12-11 22:13:26.0 -0600 +++ linux-2.6.19-rc6-mm2admafix/drivers/ata/sata_nv.c 2006-12-11 22:15:58.0 -0600 @@ -1555,8 +1555,8 @@ static void nv_remove_one (struct pci_de struct ata_host *host = dev_get_drvdata(pdev-dev); struct nv_host_priv *hpriv = host-private_data; - kfree(hpriv); ata_pci_remove_one(pdev); + kfree(hpriv); } static int nv_pci_device_resume(struct pci_dev *pdev)
[PATCH -mm] sata_nv: add suspend/resume support
The attached patch is against 2.6.18-rc6-mm1, to be applied on top of
the patch sata_nv: fix ATAPI in ADMA mode which Andrew and Jeff
already have in their trees. I've only been able to test this myself by
doing an aborted suspend and immediate resume and verifying it doesn't
blow up in that case (suspend-to-RAM is broken on my box and something
isn't configured properly for suspend-to-disk to work). However, since
resume will definitely not work on some of these controllers without
this patch, I think it's an improvement in any case..
---
This patch adds the necessary callbacks to support suspend/resume
properly in sata_nv. Most of the controllers don't need any specific
handling but CK804/MCP04 controllers, whether ADMA is enabled or not,
need some additional setup on resume.
As well as the additional storage of the controller type needed for
proper resume handling, this also removes the inline helper functions
for getting ADMA register locations by storing the pointers so we don't
have to keep calculating them all the time.
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
---
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
--- linux-2.6.19-rc6-mm1-admafixnoresume/drivers/ata/sata_nv.c 2006-11-26
00:53:44.0 -0600
+++ linux-2.6.19-rc6-mm1-admafix/drivers/ata/sata_nv.c 2006-11-29
18:42:17.0 -0600
@@ -49,7 +49,7 @@
#include linux/libata.h
#define DRV_NAME sata_nv
-#define DRV_VERSION3.2
+#define DRV_VERSION3.3
#define NV_ADMA_DMA_BOUNDARY 0xUL
@@ -213,12 +213,21 @@ struct nv_adma_port_priv {
dma_addr_t cpb_dma;
struct nv_adma_prd *aprd;
dma_addr_t aprd_dma;
+ void __iomem * ctl_block;
+ void __iomem * gen_block;
+ void __iomem * notifier_clear_block;
u8 flags;
};
+struct nv_host_priv {
+ unsigned long type;
+};
+
#define NV_ADMA_CHECK_INTR(GCTL, PORT) ((GCTL) ( 1 (19 + (12 * (PORT)
static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static void nv_remove_one (struct pci_dev *pdev);
+static int nv_pci_device_resume(struct pci_dev *pdev);
static void nv_ck804_host_stop(struct ata_host *host);
static irqreturn_t nv_generic_interrupt(int irq, void *dev_instance);
static irqreturn_t nv_nf2_interrupt(int irq, void *dev_instance);
@@ -239,6 +248,8 @@ static irqreturn_t nv_adma_interrupt(int
static void nv_adma_irq_clear(struct ata_port *ap);
static int nv_adma_port_start(struct ata_port *ap);
static void nv_adma_port_stop(struct ata_port *ap);
+static int nv_adma_port_suspend(struct ata_port *ap, pm_message_t mesg);
+static int nv_adma_port_resume(struct ata_port *ap);
static void nv_adma_error_handler(struct ata_port *ap);
static void nv_adma_host_stop(struct ata_host *host);
static void nv_adma_bmdma_setup(struct ata_queued_cmd *qc);
@@ -292,7 +303,9 @@ static struct pci_driver nv_pci_driver =
.name = DRV_NAME,
.id_table = nv_pci_tbl,
.probe = nv_init_one,
- .remove = ata_pci_remove_one,
+ .suspend= ata_pci_device_suspend,
+ .resume = nv_pci_device_resume,
+ .remove = nv_remove_one,
};
static struct scsi_host_template nv_sht = {
@@ -311,6 +324,8 @@ static struct scsi_host_template nv_sht
.slave_configure= ata_scsi_slave_config,
.slave_destroy = ata_scsi_slave_destroy,
.bios_param = ata_std_bios_param,
+ .suspend= ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
static struct scsi_host_template nv_adma_sht = {
@@ -330,6 +345,8 @@ static struct scsi_host_template nv_adma
.slave_configure= nv_adma_slave_config,
.slave_destroy = ata_scsi_slave_destroy,
.bios_param = ata_std_bios_param,
+ .suspend= ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
static const struct ata_port_operations nv_generic_ops = {
@@ -438,6 +455,8 @@ static const struct ata_port_operations
.scr_write = nv_scr_write,
.port_start = nv_adma_port_start,
.port_stop = nv_adma_port_stop,
+ .port_suspend = nv_adma_port_suspend,
+ .port_resume= nv_adma_port_resume,
.host_stop = nv_adma_host_stop,
};
@@ -476,6 +495,7 @@ static struct ata_port_info nv_port_info
{
.sht= nv_adma_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+ ATA_FLAG_HRST_TO_RESUME
[PATCH -mm] sata_nv: fix ATAPI in ADMA mode
The attached patch against 2.6.19-rc6-mm1 fixes some problems in sata_nv
with ATAPI devices on controllers running in ADMA mode. Some of the
logic in the nv_adma_bmdma_* functions was inverted causing a bunch of
warnings and caused those functions not to work properly. Also, when an
ATAPI device is connected, we need to use the legacy DMA engine. The
code now disables the PCI configuration register bits for ADMA so that
this works, and ensures that no ATAPI DMA commands go through until this
is done.
Fixes Bugzilla http://bugzilla.kernel.org/show_bug.cgi?id=7538
Signed-off-by: Robert Hancock [EMAIL PROTECTED]
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
--- linux-2.6.19-rc6-mm1/drivers/ata/sata_nv.c 2006-11-25 10:04:14.0
-0600
+++ linux-2.6.19-rc6-mm1-admafix/drivers/ata/sata_nv.c 2006-11-25
21:06:24.0 -0600
@@ -49,7 +49,7 @@
#include linux/libata.h
#define DRV_NAME sata_nv
-#define DRV_VERSION3.1
+#define DRV_VERSION3.2
#define NV_ADMA_DMA_BOUNDARY 0xUL
@@ -165,6 +165,7 @@ enum {
/* port flags */
NV_ADMA_PORT_REGISTER_MODE = (1 0),
+ NV_ADMA_ATAPI_SETUP_COMPLETE= (1 1),
};
@@ -231,6 +232,7 @@ static void nv_ck804_freeze(struct ata_p
static void nv_ck804_thaw(struct ata_port *ap);
static void nv_error_handler(struct ata_port *ap);
static int nv_adma_slave_config(struct scsi_device *sdev);
+static int nv_adma_check_atapi_dma(struct ata_queued_cmd *qc);
static void nv_adma_qc_prep(struct ata_queued_cmd *qc);
static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc);
static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance);
@@ -415,6 +417,7 @@ static const struct ata_port_operations
.port_disable = ata_port_disable,
.tf_load= ata_tf_load,
.tf_read= ata_tf_read,
+ .check_atapi_dma= nv_adma_check_atapi_dma,
.exec_command = ata_exec_command,
.check_status = ata_check_status,
.dev_select = ata_std_dev_select,
@@ -489,13 +492,71 @@ MODULE_VERSION(DRV_VERSION);
static int adma_enabled = 1;
+static inline void __iomem *__nv_adma_ctl_block(void __iomem *mmio,
+ unsigned int port_no)
+{
+ mmio += NV_ADMA_PORT + port_no * NV_ADMA_PORT_SIZE;
+ return mmio;
+}
+
+static inline void __iomem *nv_adma_ctl_block(struct ata_port *ap)
+{
+ return __nv_adma_ctl_block(ap-host-mmio_base, ap-port_no);
+}
+
+static inline void __iomem *nv_adma_gen_block(struct ata_port *ap)
+{
+ return (ap-host-mmio_base + NV_ADMA_GEN);
+}
+
+static inline void __iomem *nv_adma_notifier_clear_block(struct ata_port *ap)
+{
+ return (nv_adma_gen_block(ap) + NV_ADMA_NOTIFIER_CLEAR + (4 *
ap-port_no));
+}
+
+static void nv_adma_register_mode(struct ata_port *ap)
+{
+ void __iomem *mmio = nv_adma_ctl_block(ap);
+ struct nv_adma_port_priv *pp = ap-private_data;
+ u16 tmp;
+
+ if (pp-flags NV_ADMA_PORT_REGISTER_MODE)
+ return;
+
+ tmp = readw(mmio + NV_ADMA_CTL);
+ writew(tmp ~NV_ADMA_CTL_GO, mmio + NV_ADMA_CTL);
+
+ pp-flags |= NV_ADMA_PORT_REGISTER_MODE;
+}
+
+static void nv_adma_mode(struct ata_port *ap)
+{
+ void __iomem *mmio = nv_adma_ctl_block(ap);
+ struct nv_adma_port_priv *pp = ap-private_data;
+ u16 tmp;
+
+ if (!(pp-flags NV_ADMA_PORT_REGISTER_MODE))
+ return;
+
+ WARN_ON(pp-flags NV_ADMA_ATAPI_SETUP_COMPLETE);
+
+ tmp = readw(mmio + NV_ADMA_CTL);
+ writew(tmp | NV_ADMA_CTL_GO, mmio + NV_ADMA_CTL);
+
+ pp-flags = ~NV_ADMA_PORT_REGISTER_MODE;
+}
+
static int nv_adma_slave_config(struct scsi_device *sdev)
{
struct ata_port *ap = ata_shost_to_port(sdev-host);
+ struct nv_adma_port_priv *pp = ap-private_data;
+ struct pci_dev *pdev = to_pci_dev(ap-host-dev);
u64 bounce_limit;
unsigned long segment_boundary;
unsigned short sg_tablesize;
int rc;
+ int adma_enable;
+ u32 current_reg, new_reg, config_mask;
rc = ata_scsi_slave_config(sdev);
@@ -516,13 +577,40 @@ static int nv_adma_slave_config(struct s
/* Subtract 1 since an extra entry may be needed for padding,
see
libata-scsi.c */
sg_tablesize = LIBATA_MAX_PRD - 1;
+
+ /* Since the legacy DMA engine is in use, we need to disable
ADMA
+ on the port. */
+ adma_enable = 0;
+ nv_adma_register_mode(ap);
}
else {
bounce_limit = *ap-dev-dma_mask;
segment_boundary = NV_ADMA_DMA_BOUNDARY;
sg_tablesize = NV_ADMA_SGTBL_TOTAL_LEN
