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.
