Re: Sense fetching [Was: cdrtools /devel ...]

Joerg Schilling wrote:
> Marius Strobl <marius_at_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