OK, comparing the two ...
On Thu, Apr 23, 2009 at 01:29:35AM -0400, Martin K. Petersen wrote:
> static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
> {
> + struct ata_device *dev = args->dev;
> u64 last_lba = args->dev->n_sectors - 1; /* LBA of the last block */
You can drop the 'args->' here and save a pointer dereference.
> + u8 log_per_phys = 0;
> + u16 lowest_aligned_lba = 0;
> + u16 word_106 = dev->id[106];
> + u16 word_209 = dev->id[209];
> +
> + if ((word_106 & 0xc000) == 0x4000) {
> + /* Number and offset of logical sectors per physical sector */
> + if (word_106 & (1 << 13))
> + log_per_phys = word_106 & 0xf;
> + if ((word_209 & 0xc000) == 0x4000)
> + lowest_aligned_lba =
> + ((1 << log_per_phys) - (word_209 & 0x3fff))
> + % (1 << log_per_phys);
I found it clearer to do:
u16 first = word_209 & 0x3fff;
if (first)
lowest_aligned = (1 << log_per_phys) - first;
but you may disagree.
> + rbuf[14] = (lowest_aligned_lba >> 8) & 0x3f;
Hm, yes, it could overflow ... 1 << 0xf -1 is 32767 (0x7fff) which would
be larger than could fit in SCSI's RC16 and ends up inadvertently setting
Thin Provisioning Read Zeroes, which we really don't want. I suppose
reporting 0x3fff is better than reporting anything else in this field.
All highly theoretical, since you'd have to have 16MB physical sectors
with 512 byte logical sectors to get to this situation.
So I have no objections to this one going in, and please add:
Reviewed-by: Matthew Wilcox <willy@xxxxxxxxxxxxxxx>
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
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
