Date: Mon, 08 Nov 2010 19:06:50 +0200 From: Alexander Motin <mav@FreeBSD.org> To: Joerg Schilling <Joerg.Schilling@fokus.fraunhofer.de> Cc: freebsd-scsi@freebsd.org, freebsd-current@freebsd.org, freebsd-stable@freebsd.org, marius@alchemy.franken.de Subject: Re: Sense fetching [Was: cdrtools /devel ...] Message-ID: <4CD82E2A.3070407@FreeBSD.org> In-Reply-To: <4cd822df.o/wBtwsNCXiy8xZn%Joerg.Schilling@fokus.fraunhofer.de> References: <4CD45209.5010607@FreeBSD.org> <20101105192028.GA68728@alchemy.franken.de> <4cd822df.o/wBtwsNCXiy8xZn%Joerg.Schilling@fokus.fraunhofer.de>
next in thread | previous in thread | raw e-mail | index | archive | help
Joerg Schilling wrote: > Marius Strobl <marius@alchemy.franken.de> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4CD82E2A.3070407>