[PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Hannes Reinecke]

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).

This patch includes the modifications suggested by
Bart van Assche.

Cc: Bart van Assche <bvanassche@acm.org>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: James Bottomley <jbottomley@parallels.com>
---
 drivers/scsi/scsi_scan.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fa57a04..553e1c7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1263,14 +1263,15 @@ 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
+ *
+ *     This encoding will return a standard integer LUN for LUNs smaller
+ *     than 256, which typically use a single level LUN structure with
+ *     addressing method 0.
  **/
 u64 scsilun_to_int(struct scsi_lun *scsilun)
 {
@@ -1279,8 +1280,8 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
 
 	lun = 0;
 	for (i = 0; i < sizeof(lun); i += 2)
-		lun = lun | (((scsilun->scsi_lun[i] << 8) |
-			      scsilun->scsi_lun[i + 1]) << (i * 8));
+		lun = lun | (((u64)scsilun->scsi_lun[i] << ((i + 1) *8)) |
+			     ((u64)scsilun->scsi_lun[i + 1] << (i * 8)));
 	return lun;
 }
 EXPORT_SYMBOL(scsilun_to_int);
@@ -1294,13 +1295,10 @@ EXPORT_SYMBOL(scsilun_to_int);
  *     Reverts the functionality of the scsilun_to_int, which packed
  *     an 8-byte lun value into an int. This routine unpacks the int
  *     back into the lun value.
- *     Note: the scsilun_to_int() routine does not truly handle all
- *     8bytes of the lun value. This functions restores only as much
- *     as was set by the routine.
  *
  * Notes:
- *     Given an integer : 0x0b030a04,  this function returns a
- *     scsi_lun of : struct scsi_lun of: 0a 04 0b 03 00 00 00 00
+ *     Given an integer : 0x0b03d204,  this function returns a
+ *     struct scsi_lun of: d2 04 0b 03 00 00 00 00
  *
  **/
 void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
-- 
1.7.12.4

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Bart Van Assche]

On 06/10/14 17:01, James Bottomley wrote:
> On Tue, 2014-06-10 at 16:48 +0200, Bart Van Assche wrote:
>> On 06/10/14 16:06, James Bottomley wrote:
>>> On Tue, 2014-06-10 at 13:37 +0200, Bart Van Assche wrote:
>>>> On 06/03/14 10:58, Hannes Reinecke wrote:
>>>>> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
>>>>> + *     returns the integer: 0x0b03d204
>>>>> + *
>>>>> + *     This encoding will return a standard integer LUN for LUNs smaller
>>>>> + *     than 256, which typically use a single level LUN structure with
>>>>> + *     addressing method 0.
>>>>>   **/
>>>>>  u64 scsilun_to_int(struct scsi_lun *scsilun)
>>>>>  {
>>>>> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>>>>>  
>>>>>  	lun = 0;
>>>>>  	for (i = 0; i < sizeof(lun); i += 2)
>>>>> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
>>>>> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
>>>>>  			      scsilun->scsi_lun[i + 1]) << (i * 8));
>>>>>  	return lun;
>>>>>  }
>>>>
>>>> The above code doesn't match the comment header. Parentheses have been
>>>> placed such that each byte with an even index is shifted left (2*i+1)*8
>>>> bits instead of (i+1)*8.
>>>
>>> I don't see that in the code, which parentheses do you mean?
>>
>> This patch should change the code into what I think was intended by the
>> comment above scsilun_to_int():
>>
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1280,8 +1280,8 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>>  
>>  	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));
>> +		lun = lun | (((u64)scsilun->scsi_lun[i] << ((i + 1) *8)) |
>> +			     ((u64)scsilun->scsi_lun[i + 1] << (i * 8)));
>>  	return lun;
>>  }
>>  EXPORT_SYMBOL(scsilun_to_int);
> 
> So this is nothing to do with a wrong 2*i+1 step, which was your initial
> complaint?  Now it's an arithmetic conversion problem (which looks
> reasonable: on 32 bits, we'll do the shift at the natural size, which is
> u32, so we'll overshift for i>4.  If we're using sizeof(lun) in the for
> loop, the converter should probably be typeof(lun) for consistency).
> 
> I don't see your second set of brackets being necessary bitwise or is
> one of the lowest precedence non-logical operators; certainly it's lower
> than shift:
> 
> http://en.cppreference.com/w/c/language/operator_precedence

Hello James,

In case you would be wondering why I made the above comments about
scsilun_to_int(): both issues - the misplaced parentheses and the
missing casts - were discovered by testing Hannes' patch series with an
LLD driver that had been modified to support 64-bit LUNs and by running
a test with LUN numbers above 2**32.

Bart.

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[James Bottomley]

On Tue, 2014-06-10 at 16:48 +0200, Bart Van Assche wrote:
> On 06/10/14 16:06, James Bottomley wrote:
> > On Tue, 2014-06-10 at 13:37 +0200, Bart Van Assche wrote:
> >> On 06/03/14 10:58, Hannes Reinecke wrote:
> >>> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
> >>> + *     returns the integer: 0x0b03d204
> >>> + *
> >>> + *     This encoding will return a standard integer LUN for LUNs smaller
> >>> + *     than 256, which typically use a single level LUN structure with
> >>> + *     addressing method 0.
> >>>   **/
> >>>  u64 scsilun_to_int(struct scsi_lun *scsilun)
> >>>  {
> >>> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
> >>>  
> >>>  	lun = 0;
> >>>  	for (i = 0; i < sizeof(lun); i += 2)
> >>> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
> >>> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
> >>>  			      scsilun->scsi_lun[i + 1]) << (i * 8));
> >>>  	return lun;
> >>>  }
> >>
> >> The above code doesn't match the comment header. Parentheses have been
> >> placed such that each byte with an even index is shifted left (2*i+1)*8
> >> bits instead of (i+1)*8.
> > 
> > I don't see that in the code, which parentheses do you mean?
> 
> This patch should change the code into what I think was intended by the
> comment above scsilun_to_int():
> 
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1280,8 +1280,8 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>  
>  	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));
> +		lun = lun | (((u64)scsilun->scsi_lun[i] << ((i + 1) *8)) |
> +			     ((u64)scsilun->scsi_lun[i + 1] << (i * 8)));
>  	return lun;
>  }
>  EXPORT_SYMBOL(scsilun_to_int);

So this is nothing to do with a wrong 2*i+1 step, which was your initial
complaint?  Now it's an arithmetic conversion problem (which looks
reasonable: on 32 bits, we'll do the shift at the natural size, which is
u32, so we'll overshift for i>4.  If we're using sizeof(lun) in the for
loop, the converter should probably be typeof(lun) for consistency).

I don't see your second set of brackets being necessary bitwise or is
one of the lowest precedence non-logical operators; certainly it's lower
than shift:

http://en.cppreference.com/w/c/language/operator_precedence

James

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Douglas Gilbert]

On 14-06-10 10:06 AM, James Bottomley wrote:
> On Tue, 2014-06-10 at 13:37 +0200, Bart Van Assche wrote:
>> On 06/03/14 10:58, Hannes Reinecke wrote:
>>> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
>>> + *     returns the integer: 0x0b03d204
>>> + *
>>> + *     This encoding will return a standard integer LUN for LUNs smaller
>>> + *     than 256, which typically use a single level LUN structure with
>>> + *     addressing method 0.
>>>    **/
>>>   u64 scsilun_to_int(struct scsi_lun *scsilun)
>>>   {
>>> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>>>
>>>   	lun = 0;
>>>   	for (i = 0; i < sizeof(lun); i += 2)
>>> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
>>> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
>>>   			      scsilun->scsi_lun[i + 1]) << (i * 8));
>>>   	return lun;
>>>   }
>>
>> The above code doesn't match the comment header. Parentheses have been
>> placed such that each byte with an even index is shifted left (2*i+1)*8
>> bits instead of (i+1)*8.
>
> I don't see that in the code, which parentheses do you mean?

For sizeof(int)==4 then
   unsigned char << 32
   unsigned char << 40
<etc>
does _not_ give a 64 bit quantity. It is undefined but seems to
wrap on a 32 bit unsigned int (i.e. 32 bits). One solution: the
left argument to "<<" needs to be a 64 bit quantity
(e.g. uint64_t).

Get sg_luns (from sg3_utils version 1.36 or later) and try this
hierarchical LUN:
   sg_luns -t 0105020603070408
Decoded LUN:
   Peripheral device addressing: bus_id=1, target=5
   >>Second level addressing:
     Peripheral device addressing: bus_id=2, target=6
   >>Third level addressing:
     Peripheral device addressing: bus_id=3, target=7
   >>Fourth level addressing:
     Peripheral device addressing: bus_id=4, target=8

Now ask for a Linux integer translation (in hex) using the
first function I showed in my previous post:
   sg_luns -t 0105020603070408L -H
Linux 'word flipped' integer LUN representation: 0x408030702060105
Decoded LUN:
<same as before>

As expected.

Now ask for a Linux integer translation (in hex) using the
second function (that Bart is objecting to):
   $ sg_luns -t 0105020603070408W -H
64 bit LUN in T10 preferred (hex) format:  01 05 02 06 03 07 04 08
Linux internal 64 bit LUN representation: 0x60e0307
Decoded LUN:
<same as before>

The undocumented "W" suffix calls sg_luns' t10_2linux_lun64bitBR()
function. That function never sets any bits between bit 32 and 63.

Doug Gilbert

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Bart Van Assche]

On 06/10/14 16:06, James Bottomley wrote:
> On Tue, 2014-06-10 at 13:37 +0200, Bart Van Assche wrote:
>> On 06/03/14 10:58, Hannes Reinecke wrote:
>>> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
>>> + *     returns the integer: 0x0b03d204
>>> + *
>>> + *     This encoding will return a standard integer LUN for LUNs smaller
>>> + *     than 256, which typically use a single level LUN structure with
>>> + *     addressing method 0.
>>>   **/
>>>  u64 scsilun_to_int(struct scsi_lun *scsilun)
>>>  {
>>> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>>>  
>>>  	lun = 0;
>>>  	for (i = 0; i < sizeof(lun); i += 2)
>>> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
>>> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
>>>  			      scsilun->scsi_lun[i + 1]) << (i * 8));
>>>  	return lun;
>>>  }
>>
>> The above code doesn't match the comment header. Parentheses have been
>> placed such that each byte with an even index is shifted left (2*i+1)*8
>> bits instead of (i+1)*8.
> 
> I don't see that in the code, which parentheses do you mean?

This patch should change the code into what I think was intended by the
comment above scsilun_to_int():

--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1280,8 +1280,8 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
 
 	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));
+		lun = lun | (((u64)scsilun->scsi_lun[i] << ((i + 1) *8)) |
+			     ((u64)scsilun->scsi_lun[i + 1] << (i * 8)));
 	return lun;
 }
 EXPORT_SYMBOL(scsilun_to_int);

Bart.

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[James Bottomley]

On Tue, 2014-06-10 at 13:37 +0200, Bart Van Assche wrote:
> On 06/03/14 10:58, Hannes Reinecke wrote:
> > + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
> > + *     returns the integer: 0x0b03d204
> > + *
> > + *     This encoding will return a standard integer LUN for LUNs smaller
> > + *     than 256, which typically use a single level LUN structure with
> > + *     addressing method 0.
> >   **/
> >  u64 scsilun_to_int(struct scsi_lun *scsilun)
> >  {
> > @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
> >  
> >  	lun = 0;
> >  	for (i = 0; i < sizeof(lun); i += 2)
> > -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
> > +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
> >  			      scsilun->scsi_lun[i + 1]) << (i * 8));
> >  	return lun;
> >  }
> 
> The above code doesn't match the comment header. Parentheses have been
> placed such that each byte with an even index is shifted left (2*i+1)*8
> bits instead of (i+1)*8.

I don't see that in the code, which parentheses do you mean?

James

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Douglas Gilbert]

On 14-06-10 07:37 AM, Bart Van Assche wrote:
> On 06/03/14 10:58, Hannes Reinecke wrote:
>> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
>> + *     returns the integer: 0x0b03d204
>> + *
>> + *     This encoding will return a standard integer LUN for LUNs smaller
>> + *     than 256, which typically use a single level LUN structure with
>> + *     addressing method 0.
>>    **/
>>   u64 scsilun_to_int(struct scsi_lun *scsilun)
>>   {
>> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>>
>>   	lun = 0;
>>   	for (i = 0; i < sizeof(lun); i += 2)
>> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
>> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
>>   			      scsilun->scsi_lun[i + 1]) << (i * 8));
>>   	return lun;
>>   }
>
> The above code doesn't match the comment header. Parentheses have been
> placed such that each byte with an even index is shifted left (2*i+1)*8
> bits instead of (i+1)*8. I assume this means the parentheses have been
> misplaced ? Another bug in this code is that a cast from
> scsilun->scsi_lun[i] from u8 to u64 is missing. I think the shift
> operations in the above code trigger what is called undefined behavior
> in the C standard if i >= 4.

Hmm, we have been over this ground before. In sg_luns,
to support translating between the T10 and Linux
representation of 64 bit LUNs, there are these two
versions:


static uint64_t
t10_2linux_lun(const unsigned char t10_lun[])
{
     const unsigned char * cp;
     uint64_t res;

      res = (t10_lun[6] << 8) + t10_lun[7];
      for (cp = t10_lun + 4; cp >= t10_lun; cp -= 2) {
         res <<= 16;
         res += (*cp << 8) + *(cp + 1);
     }
     return res;
}

/* Copy of t10_lun --> Linux unsigned int (i.e. 32 bit ) present in Linux
  * kernel, up to least lk 3.8.0, extended to 64 bits.
  * BEWARE: for sizeof(int==4) this function is BROKEN and is left here as
  * as example and may soon be removed. */
static uint64_t
t10_2linux_lun64bitBR(const unsigned char t10_lun[])
{
     int i;
     uint64_t lun;

     lun = 0;
     for (i = 0; i < (int)sizeof(lun); i += 2)
         lun = lun | (((t10_lun[i] << 8) | t10_lun[i + 1]) << (i * 8));
     return lun;
}

The second one (with "BR" (for broken) appended) is for testing.
And that second one looks very similar to the code you are
objecting to.

Doug Gilbert

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Bart Van Assche]

On 06/03/14 10:58, Hannes Reinecke wrote:
> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
> + *     returns the integer: 0x0b03d204
> + *
> + *     This encoding will return a standard integer LUN for LUNs smaller
> + *     than 256, which typically use a single level LUN structure with
> + *     addressing method 0.
>   **/
>  u64 scsilun_to_int(struct scsi_lun *scsilun)
>  {
> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>  
>  	lun = 0;
>  	for (i = 0; i < sizeof(lun); i += 2)
> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
>  			      scsilun->scsi_lun[i + 1]) << (i * 8));
>  	return lun;
>  }

The above code doesn't match the comment header. Parentheses have been
placed such that each byte with an even index is shifted left (2*i+1)*8
bits instead of (i+1)*8. I assume this means the parentheses have been
misplaced ? Another bug in this code is that a cast from
scsilun->scsi_lun[i] from u8 to u64 is missing. I think the shift
operations in the above code trigger what is called undefined behavior
in the C standard if i >= 4.

Bart.

[PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Hannes Reinecke]

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).

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: James Bottomley <jbottomley@parallels.com>
---
 drivers/scsi/scsi_scan.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fa57a04..27d7956 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1263,14 +1263,15 @@ 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
+ *
+ *     This encoding will return a standard integer LUN for LUNs smaller
+ *     than 256, which typically use a single level LUN structure with
+ *     addressing method 0.
  **/
 u64 scsilun_to_int(struct scsi_lun *scsilun)
 {
@@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
 
 	lun = 0;
 	for (i = 0; i < sizeof(lun); i += 2)
-		lun = lun | (((scsilun->scsi_lun[i] << 8) |
+		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
 			      scsilun->scsi_lun[i + 1]) << (i * 8));
 	return lun;
 }
@@ -1294,13 +1295,10 @@ EXPORT_SYMBOL(scsilun_to_int);
  *     Reverts the functionality of the scsilun_to_int, which packed
  *     an 8-byte lun value into an int. This routine unpacks the int
  *     back into the lun value.
- *     Note: the scsilun_to_int() routine does not truly handle all
- *     8bytes of the lun value. This functions restores only as much
- *     as was set by the routine.
  *
  * Notes:
- *     Given an integer : 0x0b030a04,  this function returns a
- *     scsi_lun of : struct scsi_lun of: 0a 04 0b 03 00 00 00 00
+ *     Given an integer : 0x0b03d204,  this function returns a
+ *     struct scsi_lun of: d2 04 0b 03 00 00 00 00
  *
  **/
 void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
-- 
1.7.12.4

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Bart Van Assche]

On 05/31/14 11:01, Hannes Reinecke wrote:
>  u64 scsilun_to_int(struct scsi_lun *scsilun)
>  {
> -	int i;
> -	u64 lun;
> +	const unsigned char * cp;
> +	uint64_t res;
>  
> -	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;
>  }
>  EXPORT_SYMBOL(scsilun_to_int);

Has it been considered to use get_unaligned_be16(cp) instead of (*cp <<
8) + *(cp + 1) ? At least in my opinion the former is easier to read.

Bart.

Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[James Bottomley]

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@interlog.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  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

[PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Hannes Reinecke]

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@interlog.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 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;
 
-	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;
 }
 EXPORT_SYMBOL(scsilun_to_int);
 
@@ -1294,13 +1292,10 @@ EXPORT_SYMBOL(scsilun_to_int);
  *     Reverts the functionality of the scsilun_to_int, which packed
  *     an 8-byte lun value into an int. This routine unpacks the int
  *     back into the lun value.
- *     Note: the scsilun_to_int() routine does not truly handle all
- *     8bytes of the lun value. This functions restores only as much
- *     as was set by the routine.
  *
  * Notes:
- *     Given an integer : 0x0b030a04,  this function returns a
- *     scsi_lun of : struct scsi_lun of: 0a 04 0b 03 00 00 00 00
+ *     Given an integer : 0x0b03d204,  this function returns a
+ *     scsi_lun of : struct scsi_lun of: d2 04 0b 03 00 00 00 00
  *
  **/
 void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
-- 
1.7.12.4