| Message ID | 20181012100850.23316-4-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Zoned block device support improvements | expand |
On 10/12/18 12:08 PM, Damien Le Moal wrote: > The unsigned 32 bits overflow check for the zone size value is already > done within sd_zbc_check_zones() with the test: > > } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { > > so there is no need to check again for an out of range value in > sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() > error return to -EFBIG instead of -ENODEV if the zone size is too large. > Change the return type of sd_zbc_check_zones() to an int for the error > code and return the zone size (zone_blocks) through a u32 pointer to > avoid overflowing the signed 32 return value. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/scsi/sd_zbc.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c > index ca73c46931c0..0678e1e108b0 100644 > --- a/drivers/scsi/sd_zbc.c > +++ b/drivers/scsi/sd_zbc.c > @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp, > * Returns the zone size in number of blocks upon success or an error code > * upon failure. > */ > -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) > +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks) > { > u64 zone_blocks = 0; > sector_t max_lba, block = 0; Actually I thought to just change the 's32' to 'int', and not adding another parameter; but anyway. Reviewed-by: Hannes Reinecke <hare@suse.com> Hannes
On 2018/10/12 19:23, Hannes Reinecke wrote: > On 10/12/18 12:08 PM, Damien Le Moal wrote: >> The unsigned 32 bits overflow check for the zone size value is already >> done within sd_zbc_check_zones() with the test: >> >> } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { >> >> so there is no need to check again for an out of range value in >> sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() >> error return to -EFBIG instead of -ENODEV if the zone size is too large. >> Change the return type of sd_zbc_check_zones() to an int for the error >> code and return the zone size (zone_blocks) through a u32 pointer to >> avoid overflowing the signed 32 return value. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/scsi/sd_zbc.c | 19 ++++++++----------- >> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >> index ca73c46931c0..0678e1e108b0 100644 >> --- a/drivers/scsi/sd_zbc.c >> +++ b/drivers/scsi/sd_zbc.c >> @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp, >> * Returns the zone size in number of blocks upon success or an error code >> * upon failure. >> */ >> -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) >> +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks) >> { >> u64 zone_blocks = 0; >> sector_t max_lba, block = 0; > > Actually I thought to just change the 's32' to 'int', and not adding > another parameter; but anyway. Yes, I understood that. But since chunk_sectors is unsigned int, zone_blocks has to be too and so returning that through an int would be asking for troubles. If we ever have see a drive with a 2G LBA zone size that is :) I thought it was cleaner this way. > > Reviewed-by: Hannes Reinecke <hare@suse.com> Thanks ! > > Hannes >
Damien, > The unsigned 32 bits overflow check for the zone size value is already > done within sd_zbc_check_zones() with the test: > > } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { > > so there is no need to check again for an out of range value in > sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() > error return to -EFBIG instead of -ENODEV if the zone size is too large. > Change the return type of sd_zbc_check_zones() to an int for the error > code and return the zone size (zone_blocks) through a u32 pointer to > avoid overflowing the signed 32 return value. Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> @@ -687,12 +688,8 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf) > * Check zone size: only devices with a constant zone size (except > * an eventual last runt zone) that is a power of 2 are supported. > */ > - zone_blocks = sd_zbc_check_zones(sdkp); > - ret = -EFBIG; > - if (zone_blocks != (u32)zone_blocks) > - goto err; > - ret = zone_blocks; > - if (ret < 0) > + ret = sd_zbc_check_zones(sdkp, &zone_blocks); > + if (ret != 0) This coud be simplified to if (ret) or just keep the old if (ret < 0) which would still work. Otherwise looks fine: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index ca73c46931c0..0678e1e108b0 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp, * Returns the zone size in number of blocks upon success or an error code * upon failure. */ -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks) { u64 zone_blocks = 0; sector_t max_lba, block = 0; @@ -381,7 +381,7 @@ static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) unsigned char *rec; unsigned int buf_len; unsigned int list_length; - s64 ret; + int ret; u8 same; /* Get a buffer */ @@ -472,9 +472,10 @@ static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) if (sdkp->first_scan) sd_printk(KERN_NOTICE, sdkp, "Zone size too large\n"); - ret = -ENODEV; + ret = -EFBIG; } else { - ret = zone_blocks; + *zblocks = zone_blocks; + ret = 0; } out_free: @@ -668,7 +669,7 @@ static int sd_zbc_setup(struct scsi_disk *sdkp, u32 zone_blocks) int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf) { - int64_t zone_blocks; + u32 zone_blocks; int ret; if (!sd_is_zoned(sdkp)) @@ -687,12 +688,8 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf) * Check zone size: only devices with a constant zone size (except * an eventual last runt zone) that is a power of 2 are supported. */ - zone_blocks = sd_zbc_check_zones(sdkp); - ret = -EFBIG; - if (zone_blocks != (u32)zone_blocks) - goto err; - ret = zone_blocks; - if (ret < 0) + ret = sd_zbc_check_zones(sdkp, &zone_blocks); + if (ret != 0) goto err; /* The drive satisfies the kernel restrictions: set it up */
The unsigned 32 bits overflow check for the zone size value is already done within sd_zbc_check_zones() with the test: } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { so there is no need to check again for an out of range value in sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() error return to -EFBIG instead of -ENODEV if the zone size is too large. Change the return type of sd_zbc_check_zones() to an int for the error code and return the zone size (zone_blocks) through a u32 pointer to avoid overflowing the signed 32 return value. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/scsi/sd_zbc.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)