On Wed, 2018-01-31 at 17:40 -0500, Douglas Gilbert wrote: > On 2018-01-31 05:05 PM, Bart Van Assche wrote: > > On Wed, 2018-01-31 at 15:26 -0500, Douglas Gilbert wrote: > > > On 2018-01-31 12:06 PM, Bart Van Assche wrote: > > > > On 01/29/18 21:54, Douglas Gilbert wrote: > > > > > +static const struct opcode_info_t sync_cache_iarr[] = { > > > > > + {0, 0x91, 0, F_LONG_DELAY | F_M_ACCESS, resp_sync_cache, NULL, > > > > > + {16, 0x7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > > > > > ^^^ > > > > Can you clarify the choice of "0x7" in the above? After having had a look at > > > > SBC-4 I was expecting 0x2 (the mask for the IMMED bit) since all other bits in > > > > that command byte are either reserved or obsolete. > > > > > > As a general rule when you see "obsolete" that means that field was used > > > in an earlier standard (bit 0 was RELADDR and bit 2 was SYNC_NV). So > > > application clients complying with earlier versions of SBC might set those > > > bits. If they are set and the mask is being enforced I choose to not fail > > > the command as an illegal request. Basically accept and ignore. > > > > I agree with setting bits for obsolete flags. The reason I was asking about that > > mask is because I found the following in SBC-4 for byte 1 of SYNCHRONIZE CACHE(16): > > * Bits 7..3: reserved. > > * Bit 2: obsolete. > > * Bit 1: IMMED. > > * Bit 0: reserved. > > I did see the discrepancy between byte 1 bit 0 in SYNCHRONIZE CACHE(10) > where it is "obsolete" and SYNCHRONIZE CACHE(16) where it is "reserved" > initially thinking it might be a typo. So I went for the more permissive. > I'm now guessing (because I don't have the drafts between SBC(-1) and > SBC-2 and for some reason T10 wants you to be a member to download older > drafts) that RELADDR was removed before SYNCHRONIZE CACHE(16) was added > (as there was no 64 LBA support in SBC-1). So if there never was a > publicly available draft where SYNCHRONIZE CACHE(16) had a RELADDR bit > defined, then why mark it as obsolete? Reserved means a later > draft/standard may use that bit/field. > > BTW There is no requirement for a device server to enforce the mask and > most don't (from my testing). The mask is provided as an aid to the > application client and is required by the implementation of REPORT > SUPPORTED OPERATION CODES which is why I have it in scsi_debug. By default > the scsi_debug driver does not enforce the mask (but does when strict=1). > > Anyway, aren't we splitting hairs here? Does this really justify a new > patch being rolled? People trust you as a SCSI expert and may consider to copy code from the scsi_debug.c driver in their own GPL drivers if they need similar functionality. Do you think that is sufficient as a motivation to edit and repost this patch? Thanks, Bart.