Re: [patch 05/25] ata: add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Zoltan Boszormenyi wrote:
Jeff Garzik írta:
Zoltan Boszormenyi wrote:
Hi,

Jeff Garzik írta:
akpm@xxxxxxxxxxxxxxxxxxxx wrote:
From: Kuan Luo <kluo@xxxxxxxxxx>

Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller. NCQ function is disable by default, you can enable it with 'swncq=1'. NCQ will be turned off if the drive is Maxtor on MCP51 or MCP55
rev 0xa2 platform.

[akpm@xxxxxxxxxxxxxxxxxxxx: build fix]
Signed-off-by: Kuan Luo <kluo@xxxxxxxxxx>
Signed-off-by: Peer Chen <pchen@xxxxxxxxxx>
Cc: Zoltan Boszormenyi <zboszor@xxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---

 drivers/ata/sata_nv.c |  860 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 851 insertions(+), 9 deletions(-)

I finally gave this a thorough review.

Overall, good work. The state transitions all seem solid. I made several minor changes and cleanups, and checked it into the 'nv-swncq' branch of git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git

Two hurdles before I'm ready to push upstream:

* someone please verify my minor changes did not break anything; I don't have real hardware

After reading the diff between the original and your cleaned up version
it seems both the change from 4 individual flags to a single integer and the nv_swncq_bmdma_stop() -> __ata_bmdma_stop() transition are obviously correct. I attached a small cleanup patch which may make one check a bit more readable.

However, can you explain this chunk below? Why isn't it needed?

@@ -615,7 +622,6 @@ static const struct ata_port_info nv_por
       {
               .sht            = &nv_swncq_sht,
               .flags          = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
-               .link_flags     = ATA_LFLAG_HRST_TO_RESUME,
               .pio_mask       = NV_PIO_MASK,
               .mwdma_mask     = NV_MWDMA_MASK,
               .udma_mask      = NV_UDMA_MASK,

that's a bug.  fixed.

I also applied your cleanup.

Thanks, but you took the wrong cleanup patch.
My original, i.e. this chunk below is wrong.

@@ -2291,8 <http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=blob;f=drivers/ata/sata_nv.c;h=540f218e10e56c8a85a937bc920a5d96434bcc8f;hb=41f0f8af3edfccd58b5c9d714928d773a072b693#l2291> +2283,7 <http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=blob;f=drivers/ata/sata_nv.c;h=ffd00f5c533d7b58dde67cfef7d2191ca15ea140;hb=5a5a9e1890b8260686218a68862d880daee1a817#l2283> @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
                */
               pp->dhfis_bits |= (0x1 << pp->last_issue_tag);
               pp->ncq_flags |= ncq_saw_d2h;
-               if ((pp->ncq_flags & ncq_saw_sdb) ||
-                   (pp->ncq_flags & ncq_saw_backout)) {
+               if (pp->ncq_flags & (ncq_saw_sdb || ncq_saw_backout)) {
                       ata_ehi_push_desc(ehi, "illegal fis transaction");
                       ehi->err_mask |= AC_ERR_HSM;
                       ehi->action |= ATA_EH_HARDRESET;

It should be a binary OR between the flags. This caused immediate
NCQ errors upon boot and lead to disabling NCQ.
My second patch has it corrected.

fixed

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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux