From owner-freebsd-current@FreeBSD.ORG Mon Nov 8 17:07:12 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B4601106566C; Mon, 8 Nov 2010 17:07:12 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-gy0-f182.google.com (mail-gy0-f182.google.com [209.85.160.182]) by mx1.freebsd.org (Postfix) with ESMTP id 3E1878FC17; Mon, 8 Nov 2010 17:07:11 +0000 (UTC) Received: by gya6 with SMTP id 6so3656542gya.13 for ; Mon, 08 Nov 2010 09:07:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:message-id:date:from :user-agent:mime-version:to:cc:subject:references:in-reply-to :x-enigmail-version:content-type:content-transfer-encoding; bh=mKLa3Rhl/UcBVIRyAaMGm5Q/nCbrZQiygnc0ugu78rw=; b=A3XV+P5mrQmXOKz9L7KZfb7bYXFwW//qua9paFfJh1RJ6uig2tmHkl2Zv2mTfmFgiP t/n/dfyIJ2FVIYlvUDIebhzg5SVI4kzY5ieHUOp/GTuo+rxcPRYOCZjx8hXr42wIaVLl YcNr9r+uQkRRsud/nAT0eZbADQ57nno8EXYrQ= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:x-enigmail-version:content-type :content-transfer-encoding; b=aet/aXVnQY3M3DJkssNRvfM2Xjne+7wCV/FN6RXf1dQFrB/XhLCSUFk7jI2sEDQ27y zM4d2HkmByjxQ0nEq2FktO+YOZcqOj0Uwkq0SLryFbbdcam2tCETyvxwWGV2NoA/redd M6AHDjyYel3npX1w0VzzcG6WcM+qiLWfOjG/Q= Received: by 10.204.83.215 with SMTP id g23mr4999124bkl.211.1289236030919; Mon, 08 Nov 2010 09:07:10 -0800 (PST) Received: from mavbook2.mavhome.dp.ua (pc.mavhome.dp.ua [212.86.226.226]) by mx.google.com with ESMTPS id p34sm103871bkf.3.2010.11.08.09.07.07 (version=SSLv3 cipher=RC4-MD5); Mon, 08 Nov 2010 09:07:08 -0800 (PST) Sender: Alexander Motin Message-ID: <4CD82E2A.3070407@FreeBSD.org> Date: Mon, 08 Nov 2010 19:06:50 +0200 From: Alexander Motin User-Agent: Thunderbird 2.0.0.23 (X11/20091212) MIME-Version: 1.0 To: Joerg Schilling References: <4CD45209.5010607@FreeBSD.org> <20101105192028.GA68728@alchemy.franken.de> <4cd822df.o/wBtwsNCXiy8xZn%Joerg.Schilling@fokus.fraunhofer.de> In-Reply-To: <4cd822df.o/wBtwsNCXiy8xZn%Joerg.Schilling@fokus.fraunhofer.de> X-Enigmail-Version: 0.96.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: freebsd-scsi@freebsd.org, freebsd-current@freebsd.org, freebsd-stable@freebsd.org, marius@alchemy.franken.de Subject: Re: Sense fetching [Was: cdrtools /devel ...] X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Nov 2010 17:07:12 -0000 Joerg Schilling wrote: > Marius Strobl wrote: >> On Fri, Nov 05, 2010 at 08:50:49PM +0200, Alexander Motin wrote: >>> I've reviewed tests that scgcheck does to SCSI subsystem. It shown >>> combination of several issues in both CAM, ahci(4) and cdrtools itself. >>> Several small patches allow us to pass most of that tests: >>> http://people.freebsd.org/~mav/sense/ >>> >>> ahci_resid.patch: Add support for reporting residual length on data >>> underrun. SCSI commands often returns results shorter then expected. >>> Returned value allows application to know/check how much data it really >>> has. It is also important for sense fetching, as ATAPI and USB devices >>> return sense as data in response to REQUEST_SENSE command. >>> >>> sense_resid.patch: When manually requesting sense data (ATAPI or USB), >>> request only as much data as user requested (not the fixed structure >>> size), and return respective sense residual length. > > Your patch to libscg looks definitely OK if we only look at the new corrected > kernel driver behavior. > > There is a problem: > > In case that there is a sense data residual > 0, libscg will asume that there > is less sense data that really present in case that a "new" libscg is runnung > on an old kernel. > > Given the fact that many drives will probably only return 18 bytes of sense > data, this will happen every time libscg is told to fetch more sense than the > drive is willing to return. > > Is there a way to distinct an old kernel from a new one? I don't see the problem. Previous kernel in most cases reported sesnse_resid == 0, lying that there is more sense data then really is. New one should report real (often positive) value. In both cases sesnse_resid value measured from the value submitted to the kernel. >>> pass_autosence.patch: Unless CAM_DIS_AUTOSENSE is set, always fetch >>> sense if not done by SIM, independently of CAM_PASS_ERR_RECOVER. As soon >>> as device freeze released before returning to user-level, user-level >>> application by definition can't reliably fetch sense data if some other >>> application (like hald) tries to access device same time. > > This is what we need! Agree. >>> cdrtools.patch: Make libscg (part of cdrtools) on FreeBSD to submit >>> wanted sense length to CAM and do not clear sense return buffer. It is >>> mostly cosmetics, important probably only for scgcheck. > > I see no advantage in removing the call to fillbytes(). scgcheck tests how much sense data received by counting 0x00 and 0xff bytes. Zero-filling response buffer breaks that check. Though I have no idea if other crdtools' applications depend on these zeros. There could be some internal inconsistency. >> Please don't commit this to the port directly but let it loop back >> via upstream (CC'ed) instead, otherwise we would need to obey the >> following, which is undesirable, especially if these really are >> mostly cosmetic issues: >> /* >> * Warning: you may change this source, but if you do that >> * you need to change the _scg_version and _scg_auth* string below. >> * You may not return "schily" for an SCG_AUTHOR request anymore. >> * Choose your name instead of "schily" and make clear that the version >> * string is related to a modified source. >> */ >> >>> Testers and reviewers welcome. I am especially interested in opinion >>> about pass_autosence.patch -- may be we should lower sense fetching even >>> deeper, to make it work for all cam_periph_runccb() consumers. > > Did you test a modified libscg on an unmodified kernel? Unmodified kernel by default doesn't return any sense data at all for new CAM-based ATA -- this changes should be invariant. New scgcheck runs same bad as old. It just can't become worse. :) Legacy atapicam wrapper ignores sense_len on input and doesn't fill sense_resig on output -- I haven't tested, but it also should be invariant. -- Alexander Motin