diff mbox series

[1/2] scsi: sd: Use revalidate buffer for VPD pages

Message ID 20190228035733.19793-2-martin.petersen@oracle.com (mailing list archive)
State Changes Requested
Headers show
Series [1/2] scsi: sd: Use revalidate buffer for VPD pages | expand

Commit Message

Martin K. Petersen Feb. 28, 2019, 3:57 a.m. UTC 
No point in allocating separate memory for the VPD pages we're
querying. Use the buffer we already allocated.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi.c |  1 +
 drivers/scsi/sd.c   | 48 ++++++++++++++-------------------------------
 2 files changed, 16 insertions(+), 33 deletions(-)

Comments

Bart Van Assche Feb. 28, 2019, 4:12 p.m. UTC | #1 
On Wed, 2019-02-27 at 22:57 -0500, Martin K. Petersen wrote:
> No point in allocating separate memory for the VPD pages we're
> querying. Use the buffer we already allocated.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi.c |  1 +
>  drivers/scsi/sd.c   | 48 ++++++++++++++-------------------------------
>  2 files changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 7675ff0ca2ea..1d58e466722f 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -341,6 +341,7 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>  	cmd[3] = len >> 8;
>  	cmd[4] = len & 0xff;
>  	cmd[5] = 0;		/* Control byte */
> +	memset(buffer, 0x0, len);

Why has this memset() call been added? Please explain this change in the
patch description if you want to keep this change.

>  	/*
>  	 * I'm not convinced we need to try quite this hard to get VPD, but
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index cc175b78b366..df9538e57dcb 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2879,16 +2879,14 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
>   * sd_read_block_limits - Query disk device for preferred I/O sizes.
>   * @sdkp: disk to query
>   */
> -static void sd_read_block_limits(struct scsi_disk *sdkp)
> +static void sd_read_block_limits(struct scsi_disk *sdkp, unsigned char *buffer)
>  {
>  	unsigned int sector_sz = sdkp->device->sector_size;
>  	const int vpd_len = 64;
> -	unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
>  
> -	if (!buffer ||
> -	    /* Block Limits VPD */
> -	    scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
> -		goto out;
> +	/* Block Limits VPD */
> +	if (scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
> +		return;
>  
>  	blk_queue_io_min(sdkp->disk->queue,
>  			 get_unaligned_be16(&buffer[6]) * sector_sz);
> @@ -2902,7 +2900,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
>  		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
>  
>  		if (!sdkp->lbpme)
> -			goto out;
> +			return;
>  
>  		lba_count = get_unaligned_be32(&buffer[20]);
>  		desc_count = get_unaligned_be32(&buffer[24]);
> @@ -2934,28 +2932,21 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
>  				sd_config_discard(sdkp, SD_LBP_DISABLE);
>  		}
>  	}
> -
> - out:
> -	kfree(buffer);
>  }

I would appreciate it if the buffer length would be passed as an argument.
That would make it easier to verify sd_read_block_limits() and its callers.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 7675ff0ca2ea..1d58e466722f 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -341,6 +341,7 @@  static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	cmd[3] = len >> 8;
 	cmd[4] = len & 0xff;
 	cmd[5] = 0;		/* Control byte */
+	memset(buffer, 0x0, len);
 
 	/*
 	 * I'm not convinced we need to try quite this hard to get VPD, but
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cc175b78b366..df9538e57dcb 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2879,16 +2879,14 @@  static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
  * sd_read_block_limits - Query disk device for preferred I/O sizes.
  * @sdkp: disk to query
  */
-static void sd_read_block_limits(struct scsi_disk *sdkp)
+static void sd_read_block_limits(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	unsigned int sector_sz = sdkp->device->sector_size;
 	const int vpd_len = 64;
-	unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
 
-	if (!buffer ||
-	    /* Block Limits VPD */
-	    scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
-		goto out;
+	/* Block Limits VPD */
+	if (scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
+		return;
 
 	blk_queue_io_min(sdkp->disk->queue,
 			 get_unaligned_be16(&buffer[6]) * sector_sz);
@@ -2902,7 +2900,7 @@  static void sd_read_block_limits(struct scsi_disk *sdkp)
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
 
 		if (!sdkp->lbpme)
-			goto out;
+			return;
 
 		lba_count = get_unaligned_be32(&buffer[20]);
 		desc_count = get_unaligned_be32(&buffer[24]);
@@ -2934,28 +2932,21 @@  static void sd_read_block_limits(struct scsi_disk *sdkp)
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
 		}
 	}
-
- out:
-	kfree(buffer);
 }
 
 /**
  * sd_read_block_characteristics - Query block dev. characteristics
  * @sdkp: disk to query
  */
-static void sd_read_block_characteristics(struct scsi_disk *sdkp)
+static void sd_read_block_characteristics(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	struct request_queue *q = sdkp->disk->queue;
-	unsigned char *buffer;
 	u16 rot;
 	const int vpd_len = 64;
 
-	buffer = kmalloc(vpd_len, GFP_KERNEL);
-
-	if (!buffer ||
-	    /* Block Device Characteristics VPD */
-	    scsi_get_vpd_page(sdkp->device, 0xb1, buffer, vpd_len))
-		goto out;
+	/* Block Device Characteristics VPD */
+	if (scsi_get_vpd_page(sdkp->device, 0xb1, buffer, vpd_len))
+		return;
 
 	rot = get_unaligned_be16(&buffer[4]);
 
@@ -2985,35 +2976,26 @@  static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 	if (blk_queue_is_zoned(q) && sdkp->first_scan)
 		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
 		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
-
- out:
-	kfree(buffer);
 }
 
 /**
  * sd_read_block_provisioning - Query provisioning VPD page
  * @sdkp: disk to query
  */
-static void sd_read_block_provisioning(struct scsi_disk *sdkp)
+static void sd_read_block_provisioning(struct scsi_disk *sdkp, unsigned char *buffer)
 {
-	unsigned char *buffer;
 	const int vpd_len = 8;
 
 	if (sdkp->lbpme == 0)
 		return;
 
-	buffer = kmalloc(vpd_len, GFP_KERNEL);
-
-	if (!buffer || scsi_get_vpd_page(sdkp->device, 0xb2, buffer, vpd_len))
-		goto out;
+	if (scsi_get_vpd_page(sdkp->device, 0xb2, buffer, vpd_len))
+		return;
 
 	sdkp->lbpvpd	= 1;
 	sdkp->lbpu	= (buffer[5] >> 7) & 1;	/* UNMAP */
 	sdkp->lbpws	= (buffer[5] >> 6) & 1;	/* WRITE SAME(16) with UNMAP */
 	sdkp->lbpws10	= (buffer[5] >> 5) & 1;	/* WRITE SAME(10) with UNMAP */
-
- out:
-	kfree(buffer);
 }
 
 static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
@@ -3102,9 +3084,9 @@  static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_capacity(sdkp, buffer);
 
 		if (scsi_device_supports_vpd(sdp)) {
-			sd_read_block_provisioning(sdkp);
-			sd_read_block_limits(sdkp);
-			sd_read_block_characteristics(sdkp);
+			sd_read_block_provisioning(sdkp, buffer);
+			sd_read_block_limits(sdkp, buffer);
+			sd_read_block_characteristics(sdkp, buffer);
 			sd_zbc_read_zones(sdkp, buffer);
 		}