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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
