From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [patch 05/25] ata: add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61 Date: Wed, 26 Sep 2007 01:38:39 -0400 Message-ID: <46F9F05F.40905@garzik.org> References: <200708102059.l7AKxYVQ008581@imap1.linux-foundation.org> <46F2F6E5.3010406@garzik.org> <46F35C7F.20400@dunaweb.hu> <46F9D6A8.8010809@garzik.org> <46F9EFC9.9030209@dunaweb.hu> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from srv5.dvmed.net ([207.36.208.214]:45180 "EHLO mail.dvmed.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752392AbXIZFip (ORCPT ); Wed, 26 Sep 2007 01:38:45 -0400 In-Reply-To: <46F9EFC9.9030209@dunaweb.hu> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Zoltan Boszormenyi Cc: akpm@linux-foundation.org, kluo@nvidia.com, pchen@nvidia.com, Robert Hancock , linux-ide@vger.kernel.org Zoltan Boszormenyi wrote: > 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 S= ATA >>>>> controller. NCQ function is disable by default, you can enable i= t=20 >>>>> with >>>>> 'swncq=3D1'. NCQ will be turned off if the drive is Maxtor on MC= P51=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.g= it >>>> >>>> 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 ver= sion >>> it seems both the change from 4 individual flags to a single intege= r=20 >>> and the >>> nv_swncq_bmdma_stop() -> __ata_bmdma_stop() transition are obviousl= y=20 >>> correct. >>> I attached a small cleanup patch which may make one check a bit mor= e=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_LEGA= CY, >>> - .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. >=20 > Thanks, but you took the wrong cleanup patch. > My original, i.e. this chunk below is wrong. >=20 > @@ -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 transactio= n"); > ehi->err_mask |=3D AC_ERR_HSM; > ehi->action |=3D ATA_EH_HARDRESET; >=20 > 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