Re: [PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

On Thu, 2012-09-06 at 22:51 +0200, Paolo Bonzini wrote:
> Il 06/09/2012 20:58, Nicholas A. Bellinger ha scritto:
> > On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
> >> The purpose of this function is to clear a LUN set in the CDB, in case
> >> the initiator talking to us is speaking an old standards version.
> >> However, as things stand, pscsi_clear_cdb_lun has two problems.  It
> >> will "deceive" the guest by clearing the LUN bits on initial
> >> commands (INQUIRY, TEST UNIT READY, etc.); but then it will let the
> >> LUN bits through on reads, which will likely fail due to protection not
> >> being enabled.  Second, not all commands are properly filtered, in
> >> particular WRITE's WRPROTECT bits are cleared.
> >>
> >> This should be done by the fabric module rather than by PSCSI, if it
> >> knows such initiators may be lying around _and_ it can assume that its
> >> initiators do not really care about protection information.  Nuke it
> >> from PSCSI.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> > 
> > NAK.  This code was originally added to prevent certain pSCSI HBAs from
> > going bonkers when they got a legacy SCSI LUN ID encoded within the CDB
> > (eg: the fabric LUN ID) that's different from the physical SCSI LUN ID
> > on an Parallel SCSI bus.
> 
> I understood, but what's good in making INQUIRY work, if the HBA will
> equally go bonkers on the first actual I/O (READ/WRITE/VERIFY are all
> affected)?
> 

The original purpose of pscsi_clear_cdb_lun() is to clear byte 1 bit 0-5
for all incoming CDBs in SCSI-2 that did encode LUN ID into the CDB as a
work-around for (some) Parallel SCSI hardware that depends on this value
to function.

As modern SCSI-3 + SAS fabrics, et al have moved away from doing this
completely, thinking about this some more I would be OK with disabling
this by default for modern SCSI HW if the pSCSI device is reporting >=
SCSI-3 from se_subsystem_api->get_device_rev().

> This is something the LLDs or the fabric module should be doing.  But I
> guess it could be an attribute too.
> 

I think it's safe to disable this for modern SCSI hw, but lets keep it
around for pSCSI backend devices that are reporting SCSI-2 support and
below.

Care to respin..?  ;)

Thanks Paolo!