From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zoltan Boszormenyi Subject: Re: [patch 05/25] ata: add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61 Date: Wed, 26 Sep 2007 07:36:09 +0200 Message-ID: <46F9EFC9.9030209@dunaweb.hu> References: <200708102059.l7AKxYVQ008581@imap1.linux-foundation.org> <46F2F6E5.3010406@garzik.org> <46F35C7F.20400@dunaweb.hu> <46F9D6A8.8010809@garzik.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from linux.dunaweb.hu ([62.77.196.1]:50052 "EHLO linux.dunaweb.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751375AbXIZFgR convert rfc822-to-8bit (ORCPT ); Wed, 26 Sep 2007 01:36:17 -0400 In-Reply-To: <46F9D6A8.8010809@garzik.org> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Jeff Garzik Cc: akpm@linux-foundation.org, kluo@nvidia.com, pchen@nvidia.com, Robert Hancock , linux-ide@vger.kernel.org Jeff Garzik =EDrta: > Zoltan Boszormenyi wrote: >> Hi, >> >> Jeff Garzik =EDrta: >>> akpm@linux-foundation.org wrote: >>>> From: Kuan Luo >>>> >>>> Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SA= TA >>>> controller. NCQ function is disable by default, you can enable it= =20 >>>> with >>>> 'swncq=3D1'. NCQ will be turned off if the drive is Maxtor on MCP= 51=20 >>>> or MCP55 >>>> rev 0xa2 platform. >>>> >>>> [akpm@linux-foundation.org: build fix] >>>> Signed-off-by: Kuan Luo >>>> Signed-off-by: Peer Chen >>>> Cc: Zoltan Boszormenyi >>>> Signed-off-by: Andrew Morton >>>> --- >>>> >>>> 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=20 >>> several minor changes and cleanups, and checked it into the=20 >>> 'nv-swncq' branch of=20 >>> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.gi= t >>> >>> Two hurdles before I'm ready to push upstream: >>> >>> * someone please verify my minor changes did not break anything; I=20 >>> don't have real hardware >> >> After reading the diff between the original and your cleaned up vers= ion >> it seems both the change from 4 individual flags to a single integer= =20 >> and the >> nv_swncq_bmdma_stop() -> __ata_bmdma_stop() transition are obviously= =20 >> correct. >> I attached a small cleanup patch which may make one check a bit more= =20 >> 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 =3D &nv_swncq_sht, >> .flags =3D ATA_FLAG_SATA | ATA_FLAG_NO_LEGAC= Y, >> - .link_flags =3D ATA_LFLAG_HRST_TO_RESUME, >> .pio_mask =3D NV_PIO_MASK, >> .mwdma_mask =3D NV_MWDMA_MASK, >> .udma_mask =3D 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=20 =20 +2283,7=20 =20 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis) */ pp->dhfis_bits |=3D (0x1 << pp->last_issue_tag); pp->ncq_flags |=3D 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 |=3D AC_ERR_HSM; ehi->action |=3D 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. Best regards, Zolt=E1n B=F6sz=F6rm=E9nyi