| Message ID | 20250612060211.1970933-2-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve optimal IO size initialization | expand |
On 6/11/25 11:02 PM, Damien Le Moal wrote: > Make sure that logical_to_bytes() does not return an overflowed value > by changing its return type from unsigned int (32-bits) to size_t > (64-bits). size_t is only 64 bits on 64-bit systems. Shouldn't size_t be changed into u64? See also https://en.wikipedia.org/wiki/64-bit_computing. > -static inline unsigned int logical_to_bytes(struct scsi_device *sdev, sector_t blocks) > +static inline size_t logical_to_bytes(struct scsi_device *sdev, sector_t blocks) > { > return blocks * sdev->sector_size; > } Since 'blocks' represents an LBA instead of a byte offset divided by 512, please consider changing "sector_t blocks" into "u64 logical_blocks". Independent of this patch, "sector_size" probably should be renamed into "logical_block_size" since the word "sector" is only used in references to "physical sector" in the SBC specification. Otherwise this patch looks good to me. Thanks, Bart.
On 6/13/25 00:53, Bart Van Assche wrote: > On 6/11/25 11:02 PM, Damien Le Moal wrote: >> Make sure that logical_to_bytes() does not return an overflowed value >> by changing its return type from unsigned int (32-bits) to size_t >> (64-bits). > > size_t is only 64 bits on 64-bit systems. Shouldn't size_t be changed > into u64? See also https://en.wikipedia.org/wiki/64-bit_computing. > >> -static inline unsigned int logical_to_bytes(struct scsi_device *sdev, sector_t blocks) >> +static inline size_t logical_to_bytes(struct scsi_device *sdev, sector_t blocks) >> { >> return blocks * sdev->sector_size; >> } > > Since 'blocks' represents an LBA instead of a byte offset divided by > 512, please consider changing "sector_t blocks" into > "u64 logical_blocks". Independent of this patch, "sector_size" probably > should be renamed into "logical_block_size" since the word "sector" is > only used in references to "physical sector" in the SBC specification. Well, one could argue that struct scsi_device is SPC territory, not SBC, and that "sector_size" or block size has no business being in struct scsi_device. But changing that would not make access to that value easy when all we have is a scsi command :) > > Otherwise this patch looks good to me. > > Thanks, > > Bart.
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h index 36382eca941c..3803eb8cb532 100644 --- a/drivers/scsi/sd.h +++ b/drivers/scsi/sd.h @@ -213,7 +213,7 @@ static inline sector_t logical_to_sectors(struct scsi_device *sdev, sector_t blo return blocks << (ilog2(sdev->sector_size) - 9); } -static inline unsigned int logical_to_bytes(struct scsi_device *sdev, sector_t blocks) +static inline size_t logical_to_bytes(struct scsi_device *sdev, sector_t blocks) { return blocks * sdev->sector_size; }
Make sure that logical_to_bytes() does not return an overflowed value by changing its return type from unsigned int (32-bits) to size_t (64-bits). Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/scsi/sd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)