On Sat, 2014-05-31 at 11:01 +0200, Hannes Reinecke wrote:
> scsilun_to_int() has an error which prevents it from generating
> correct LUN numbers for 64bit values.
> Also we should remove the misleading comment about portions of
> the LUN being ignored; the initiator should treat the LUN as
> an opaque value.
> And, finally, the example given should use the correct
> prefix (here: extended flat space addressing scheme).
>
> Suggested-by: Doug Gilbert <dgilbert@xxxxxxxxxxxx>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> ---
> drivers/scsi/scsi_scan.c | 29 ++++++++++++-----------------
> 1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index fa57a04..42843ec 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1263,25 +1263,23 @@ static void scsi_sequential_lun_scan(struct scsi_target *starget,
> * truncation before using this function.
> *
> * Notes:
> - * The struct scsi_lun is assumed to be four levels, with each level
> - * effectively containing a SCSI byte-ordered (big endian) short; the
> - * addressing bits of each level are ignored (the highest two bits).
> * For a description of the LUN format, post SCSI-3 see the SCSI
> * Architecture Model, for SCSI-3 see the SCSI Controller Commands.
> *
> - * Given a struct scsi_lun of: 0a 04 0b 03 00 00 00 00, this function returns
> - * the integer: 0x0b030a04
> + * Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
> + * returns the integer: 0x0b03d204
> **/
> u64 scsilun_to_int(struct scsi_lun *scsilun)
> {
> - int i;
> - u64 lun;
> + const unsigned char * cp;
> + uint64_t res;
u64 please; uint64_t is for shared headers with userspace only.
> - lun = 0;
> - for (i = 0; i < sizeof(lun); i += 2)
> - lun = lun | (((scsilun->scsi_lun[i] << 8) |
> - scsilun->scsi_lun[i + 1]) << (i * 8));
> - return lun;
> + res = (scsilun->scsi_lun[6] << 8) + scsilun->scsi_lun[7];
> + for (cp = scsilun->scsi_lun + 4; cp >= scsilun->scsi_lun; cp -= 2) {
> + res <<= 16;
> + res += (*cp << 8) + *(cp + 1);
> + }
> + return res;
This makes the code horrible to read because it implies wrongly that the
setup isn't the same code as the loop. If we really have to have it
done this way, then do
u64 res = 0
for (cp = scsilun->scsi_lun + 6; cp >= scsilun->scsi_lun; cp -= 2) {
res <<= 16;
res += (*cp << 8) + *(cp + 1);
}
return res;
But really, I don't see what's wrong with
u64 lun = 0;
for (i = 0; i < sizeof(lun); i += 2)
lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8) |
scsilun->scsi_lun[i + 1]) << (i * 8));
return lun;
To my mind, the latter is clearer.
Changing the comment to match a realistic LUN is great. However,
somewhere we should say that the reason for this transformation is that
a single level lun with address method zero matches the standard integer
luns for lun < 256.
After this is sorted out, the patch looks ready to go to me (you can add
my reviewed by).
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
