Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S261801AbVCAImx (ORCPT ); Tue, 1 Mar 2005 03:42:53 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261833AbVCAImo (ORCPT ); Tue, 1 Mar 2005 03:42:44 -0500 Received: from wproxy.gmail.com ([64.233.184.203]:863 "EHLO wproxy.gmail.com") by vger.kernel.org with ESMTP id S261767AbVCAImT (ORCPT ); Tue, 1 Mar 2005 03:42:19 -0500 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:reply-to:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:references; b=UztMHQOOPsIGZu20OG3h0eQNc1cSN+oBL85kDxyqJJNut3EzrcDI6EodoQF3Y5CyO809dCYMVpUbvY9TOW0oMAqAvdjgsBEIF5HrMHaBhcOid9gQf8QysSKOkIDZLNAFXP22SERu1C+2UjotAPLkQUqTC3jGGHynlI9m3rcxYYI= Message-ID: <58cb370e05030100424d98c85c@mail.gmail.com> Date: Tue, 1 Mar 2005 09:42:18 +0100 From: Bartlomiej Zolnierkiewicz Reply-To: Bartlomiej Zolnierkiewicz To: Tejun Heo Subject: Re: [patch ide-dev 8/9] make ide_task_ioctl() use REQ_DRIVE_TASKFILE Cc: Jeff Garzik , linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org In-Reply-To: <20050301042116.GA9001@htj.dyndns.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit References: <200502271731.29448.bzolnier@elka.pw.edu.pl> <422337A1.4060806@gmail.com> <200502281714.55960.bzolnier@elka.pw.edu.pl> <20050301042116.GA9001@htj.dyndns.org> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6026 Lines: 148 Hello, On Tue, 1 Mar 2005 13:21:16 +0900, Tejun Heo wrote: > Hello, Bartlomiej. > Hello, Jeff. > > On Mon, Feb 28, 2005 at 05:14:55PM +0100, Bartlomiej Zolnierkiewicz wrote: > > On Monday 28 February 2005 16:24, Tejun Heo wrote: > > > Bartlomiej Zolnierkiewicz wrote: > > > > > > > > Nope, it works just fine because REQ_DRIVE_TASK used only > > > > no-data protocol, please check task_no_data_intr(). > > > > > > > > > > Sorry, I missed that. IDE really has a lot of ways to finish a > > > command, doesn't it? hdio.txt is gonna look ugly. :-) > > > > Yep but it was a lot more "fun" when there were three PIO codepaths. ;-) > > > > hdio.txt doesn't need to know about driver internals so no problem here. > > > > I was talking about the TASKFILE ioctl IN register result. > > > > > > > > >> IMHO, this flag-to-get-result-registers thing is way too subtle. How > > > >>about keeping old behavior by just not copying out register outputs in > > > >>ide_taskfile_ioctl() in applicable cases instead of not reading > > > >>registers when ending commands? That is, unless there's some > > > >>noticeable performance impacts I'm not aware of. > > > > > > > > > > > > This would miss whole point of not _reading_ these registers (IO is slow). > > > > IMHO new flags denoting {in,out} registers should be added (to > > > > to share them with libata) so new code can be sane and old flags would map > > > > on new flags when needed. > > > > > > Please do it. > > > > > > Or, let me know what you have in mind (added fields, flag names, > > > etc...); then, I'll do it. I think we also need to hear Jeff's opinion > > > as things need to be added to ata.h. > > > > I was thinking about: > > > > * adding ATA_TFLAG_{IN,OUT}_xxx flags (there is enough free > > place for all flags in ->flags field of struct ata_taskfile) > > * teaching flagged_taskfile() about these flags and mapping ->tf_out_flags > > onto ATA_TFLAG_OUT_xxx (simple mapping no need to move ->tf_out_flags > > to ide_taskfile_ioctl() etc. - no risk of breaking something) > > * moving flagged taskfile writing to ide_tf_load_discrete() helper > > * adding ide_tf_read_discrete() helper for use by ide_end_drive_cmd() > > and mapping ->tf_in_flags onto ATA_TFLAG_IN_xxx (ditto) > > > > If you like this plan feel free to implement it. > > I'm also open for better ideas, comments etc. > > So, how do you like the following set of TFLAG's? > > /* struct ata_taskfile flags */ > > /* The following six flags are used by IDE driver to control register IO. */ > ATA_TFLAG_OUT_LBA48 = (1 << 0), /* enable 48-bit LBA and HOB out */ aggregate ATA_TFLAG_OUT_HOB_LBA{L,M,H}? > ATA_TFLAG_OUT_ADDR = (1 << 1), /* enable out to nsect/lba regs */ not needed currently, add it {when,if} it is needed > ATA_TFLAG_OUT_DEVICE = (1 << 2), /* enable out to device reg */ > ATA_TFLAG_IN_LBA48 = (1 << 3), /* enable 48-bit LBA and HOB in */ aggregate ATA_TFLAG_IN_HOB_LBA_{L,M,H}? > ATA_TFLAG_IN_ADDR = (1 << 4), /* enable in from nsect/lba regs */ not needed currently, add it {when,if} it is needed > ATA_TFLAG_IN_DEVICE = (1 << 5), /* enable in from device reg */ > > /* These three aggregate flags are used by libata, as it doesn't > really need to optimize register INs */ > ATA_TFLAG_LBA48 = (ATA_TFLAG_OUT_LBA48 | ATA_TFLAG_IN_LBA48), > ATA_TFLAG_ISADDR = (ATA_TFLAG_OUT_ADDR | ATA_TFLAG_IN_ADDR), > ATA_TFLAG_DEVICE = (ATA_TFLAG_OUT_DEVICE | ATA_TFLAG_IN_DEVICE), > > ATA_TFLAG_WRITE = (1 << 6), /* data dir */ > > /* The rest of TFLAGs are only for implementing ioctl direct drive > commands in the IDE driver. DO NOT use in other places. */ > ATA_TFLAG_IO_16BIT = (1 << 11), > > ATA_TFLAG_OUT_FEATURE = (1 << 12), > ATA_TFLAG_OUT_NSECT = (1 << 13), > ATA_TFLAG_OUT_LBAL = (1 << 14), > ATA_TFLAG_OUT_LBAM = (1 << 15), > ATA_TFLAG_OUT_LBAH = (1 << 16), > ATA_TFLAG_OUT_HOB_FEATURE = (1 << 17), > ATA_TFLAG_OUT_HOB_NSECT = (1 << 18), > ATA_TFLAG_OUT_HOB_LBAL = (1 << 19), > ATA_TFLAG_OUT_HOB_LBAM = (1 << 20), > ATA_TFLAG_OUT_HOB_LBAH = (1 << 21), > > ATA_TFLAG_IN_FEATURE = (1 << 22), > ATA_TFLAG_IN_NSECT = (1 << 23), > ATA_TFLAG_IN_LBAL = (1 << 24), > ATA_TFLAG_IN_LBAM = (1 << 25), > ATA_TFLAG_IN_LBAH = (1 << 26), > ATA_TFLAG_IN_HOB_FEATURE = (1 << 27), > ATA_TFLAG_IN_HOB_NSECT = (1 << 28), > ATA_TFLAG_IN_HOB_LBAL = (1 << 29), > ATA_TFLAG_IN_HOB_LBAM = (1 << 30), > ATA_TFLAG_IN_HOB_LBAH = (1 << 31), proposed changes will get rid of 4 bits > ATA_TFLAG_OUT_MASK = 0x007ff000, > ATA_TFLAG_IN_MASK = 0xffc00000, > ATA_TFLAG_OUT_IN_MASK = (ATA_TFLAG_OUT_MASK | ATA_TFLAG_IN_MASK), > > ATA_TFLAG_{OUT|IN}_{LBA48|ADDR|DEVICE} should provide enough > granuality for fs/internal requests without much hassle, and > individual IO/OUT flags will only be used to implement ioctls in > separate IN/OUT functions, something like ide_{load|read}_ioctl_task(). They would be later used by IDE driver itself so names like ide_discrete_tf_{load,read}() suits it better IMHO. > Would using more specific prefix for ioctl flags - like > ATA_TFLAG_IOC_{IN|OUT}_* - be better? Nope, they are not limited to ioctls by design. > libata will work as it works currently, but if optimizing out > register INs can help, converting to use IN/OUT flags should be easy. > > Please let me know what you guys think. It is fine with me. Thanks, Bartlomiej - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/