From owner-cvs-all@FreeBSD.ORG Fri Nov 16 17:12:14 2007 Return-Path: Delivered-To: cvs-all@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 587A116A419; Fri, 16 Nov 2007 17:12:14 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.freebsd.org (Postfix) with ESMTP id CBC0213C478; Fri, 16 Nov 2007 17:12:08 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from phobos.samsco.home (phobos.samsco.home [192.168.254.11]) (authenticated bits=0) by pooker.samsco.org (8.13.8/8.13.8) with ESMTP id lAGHC2PG034289; Fri, 16 Nov 2007 10:12:02 -0700 (MST) (envelope-from scottl@samsco.org) Message-ID: <473DCF61.6010107@samsco.org> Date: Fri, 16 Nov 2007 10:12:01 -0700 From: Scott Long User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.6) Gecko/20070802 SeaMonkey/1.1.4 MIME-Version: 1.0 To: =?ISO-8859-1?Q?S=F8ren_Schmidt?= References: <200710260859.l9Q8xPdP099307@repoman.freebsd.org> <473DC220.6030809@samsco.org> <473DC93D.5060205@deepcore.dk> <473DCAA7.3090408@root.org> <473DCBDA.1080104@deepcore.dk> In-Reply-To: <473DCBDA.1080104@deepcore.dk> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (pooker.samsco.org [168.103.85.57]); Fri, 16 Nov 2007 10:12:03 -0700 (MST) X-Spam-Status: No, score=-1.4 required=5.5 tests=ALL_TRUSTED autolearn=failed version=3.1.8 X-Spam-Checker-Version: SpamAssassin 3.1.8 (2007-02-13) on pooker.samsco.org Cc: cvs-src@FreeBSD.ORG, src-committers@FreeBSD.ORG, cvs-all@FreeBSD.ORG, Nate Lawson Subject: Re: cvs commit: src/sys/dev/ata atapi-cd.c atapi-cd.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Nov 2007 17:12:14 -0000 Søren Schmidt wrote: > Nate Lawson wrote: >> Søren Schmidt wrote: >> >>> Scott Long wrote: >>> >>>> SXren Schmidt wrote: >>>> >>>>> sos 2007-10-26 08:59:24 UTC >>>>> >>>>> FreeBSD src repository >>>>> >>>>> Modified files: >>>>> sys/dev/ata atapi-cd.c atapi-cd.h Log: >>>>> Update the way we get the mode pages on probe. >>>>> Revision Changes Path >>>>> 1.194 +22 -25 src/sys/dev/ata/atapi-cd.c >>>>> 1.47 +1 -0 src/sys/dev/ata/atapi-cd.h >>>>> >>>> Just curious, what was the motivation for changing from doing a >>>> TUR to doing a MODE_SENSE in g_access? Your new code now relies >>>> on what I'd consider is a fairly obscure feature of the SFF-8020i >>>> spec, and one that contradicts the MMC and SPC specs that are now >>>> used as the normative references for packet commands. There's at >>>> least once case, the virtual CDROM emulator in Parallels, that >>>> appears not to support this feature, and I'd bet pretty heavily >>>> that there are a number of real devices that won't support it >>>> either. >>>> >>>> Without reverting to the old TUR code, an easy way forward is to >>>> not fail the g_access command for the MST_FMT_NONE case. This is >>>> generally incorrect anyways as it just means that the device can't >>>> specifically identify the media though it knows that the media is >>>> inserted and valid. Since this constant evaluates to 0x00, ignoring >>>> it will also allow devices that don't support it to still work. The >>>> only side effect is that devices that don't support it will also not >>>> be able to report MST_NO_DISC. These devices will get handled later >>>> as a side effect of reporting a bogus media size. >>>> >>>> Ultimately, I do think that the code needs to go back to using a TUR >>>> and interpreting sense, asc, and ascq codes correctly. The code prior >>>> to 10/26 looks like it does this, so again I'm curious as to what , >>>> motivated the change. >>>> >>> The code is only intended to work around the verbose error chatter from >>> GEOM as it will read from a nonexisting media (if the drive is empty) >>> during boot, and clutter the console needlessly. There is currently no >>> way to prevent this, but I've talked to phk about it lately so this can >>> be left out alltogether. >>> Anyhow the only failure I've seen so far is Parallels when using a CD >>> image as a virtual CDROM drive, it does a poor HW emu in that case if >>> you ask me :) >>> >>> Anyhow you are free to do what you want with the code until I get a real >>> fix via GEOM done, but mind you the old code will not work on SATA ATAPI >>> drives so a simple rollback of the commit is not really a fix... >>> >> >> While there may be a way to solve this automatically for ATA, this is >> also an issue for floppy drives. GEOM tries to read the floppy on boot >> to see if there's a label, even if no media is present. I think GEOM >> should have a way of asking drives if they can tell if they have media >> present and not probe them if they can't. A separate env flag could be >> set to say "always probe removable media" if users want the previous >> behavior. For most of us, this just causes boot delay. >> > What I've sortof talked phk into is that if I return the "right" error > code in the "start" function, it will back off and not retry nor chatter. > We still need the "access" function to return OK if HW present or we > cannot send ioctl calls to an empty drive. > IMHO, overloading g_start just adds more cruft to the problem. I'm torn between saying that g_access should be extended to give more than just a simple yes/no error return, and saying that g_access is fundamentally flawed and that this kind of info should be relayed via GEOM attributes. As for rolling back the change or not, I don't see anywhere in the SFF-8020i spec where it says that the CAP_PAGE command is guaranteed to complete successfully and not return a CHECKCOND. It seems perfectly valid for it to fail if there is no media. But even disregarding that, the current code is still intended return a failure to g_access if there's no media; doesn't that still preclude you from being able to send ioctls to the device? Scott