[PATCH 0/2] small cleanups

[PATCH 0/2] small cleanups

[Damien Le Moal]

Martin,

Here is a couple of patches ifor some light cleanup in scsi_lib.
No functional changes are introduced.

Damien Le Moal (2):
  scsi: simplify scsi_get_vpd_page()
  scsi: fix scsi_mode_select() interface

 drivers/scsi/scsi.c        | 31 ++++++++++++++-----------------
 drivers/scsi/scsi_lib.c    |  8 +++-----
 drivers/scsi/sd.c          |  2 +-
 include/scsi/scsi_device.h |  5 ++---
 4 files changed, 20 insertions(+), 26 deletions(-)

-- 
2.31.1

[PATCH 1/2] scsi: simplify scsi_get_vpd_page()

[Damien Le Moal]

Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
readability and use memchr() instead of an open coded search loop.
Also update the outdated kernel doc comment for this function.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/scsi.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b241f9e3885c..6be68b3427a0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -339,47 +339,44 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
  *
  * SCSI devices may optionally supply Vital Product Data.  Each 'page'
  * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
- * If the device supports this VPD page, this routine returns a pointer
- * to a buffer containing the data from that page.  The caller is
- * responsible for calling kfree() on this pointer when it is no longer
- * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
+ * If the device supports this VPD page, this routine fills @buf
+ * with the data from that page and return 0. If the VPD page is not
+ * supported or its content cannot be retrieved, -EINVAL is returned.
  */
 int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 		      int buf_len)
 {
-	int i, result;
+	int result, len;
 
 	if (sdev->skip_vpd_pages)
-		goto fail;
+		return -EINVAL;
 
 	/* Ask for all the pages supported by this device */
 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
 	if (result < 4)
-		goto fail;
+		return -EINVAL;
 
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
 		return 0;
 
-	for (i = 4; i < min(result, buf_len); i++)
-		if (buf[i] == page)
-			goto found;
+	len = min(result, buf_len);
+	if (len > 4 && memchr(&buf[4], page, len - 4))
+		goto found;
 
-	if (i < result && i >= buf_len)
-		/* ran off the end of the buffer, give us benefit of doubt */
+	/* If we ran off the end of the buffer, give us benefit of doubt */
+	if (result > buf_len)
 		goto found;
+
 	/* The device claims it doesn't support the requested page */
-	goto fail;
+	return -EINVAL;
 
  found:
 	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
 	if (result < 0)
-		goto fail;
+		return -EINVAL;
 
 	return 0;
-
- fail:
-	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
-- 
2.31.1

[PATCH 2/2] scsi: fix scsi_mode_select() interface

[Damien Le Moal]

The modepage argument is unused. Remove it.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/scsi_lib.c    | 8 +++-----
 drivers/scsi/sd.c          | 2 +-
 include/scsi/scsi_device.h | 5 ++---
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index dcf105287a76..f1fe5803d7ec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2001,7 +2001,6 @@ void scsi_exit_queue(void)
  *	@sdev:	SCSI device to be queried
  *	@pf:	Page format bit (1 == standard, 0 == vendor specific)
  *	@sp:	Save page bit (0 == don't save, 1 == save)
- *	@modepage: mode page being requested
  *	@buffer: request buffer (may not be smaller than eight bytes)
  *	@len:	length of request buffer.
  *	@timeout: command timeout
@@ -2014,10 +2013,9 @@ void scsi_exit_queue(void)
  *	status on error
  *
  */
-int
-scsi_mode_select(struct scsi_device *sdev, int pf, int sp, int modepage,
-		 unsigned char *buffer, int len, int timeout, int retries,
-		 struct scsi_mode_data *data, struct scsi_sense_hdr *sshdr)
+int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
+		     unsigned char *buffer, int len, int timeout, int retries,
+		     struct scsi_mode_data *data, struct scsi_sense_hdr *sshdr)
 {
 	unsigned char cmd[10];
 	unsigned char *real_buffer;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 71fa70b42c2b..89b5eea0ea0c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -209,7 +209,7 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
 	 */
 	data.device_specific = 0;
 
-	if (scsi_mode_select(sdp, 1, sp, 8, buffer_data, len, SD_TIMEOUT,
+	if (scsi_mode_select(sdp, 1, sp, buffer_data, len, SD_TIMEOUT,
 			     sdkp->max_retries, &data, &sshdr)) {
 		if (scsi_sense_valid(&sshdr))
 			sd_print_sense_hdr(sdkp, &sshdr);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 09a17f6e93a7..1a9d2fe6aa02 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -415,9 +415,8 @@ extern int scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 			   int retries, struct scsi_mode_data *data,
 			   struct scsi_sense_hdr *);
 extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
-			    int modepage, unsigned char *buffer, int len,
-			    int timeout, int retries,
-			    struct scsi_mode_data *data,
+			    unsigned char *buffer, int len, int timeout,
+			    int retries, struct scsi_mode_data *data,
 			    struct scsi_sense_hdr *);
 extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
 				int retries, struct scsi_sense_hdr *sshdr);
-- 
2.31.1

Re: [PATCH 1/2] scsi: simplify scsi_get_vpd_page()

[Himanshu Madhani]

> On Sep 29, 2021, at 4:17 AM, Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> 
> Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
> readability and use memchr() instead of an open coded search loop.
> Also update the outdated kernel doc comment for this function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> drivers/scsi/scsi.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b241f9e3885c..6be68b3427a0 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -339,47 +339,44 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>  *
>  * SCSI devices may optionally supply Vital Product Data.  Each 'page'
>  * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
> - * If the device supports this VPD page, this routine returns a pointer
> - * to a buffer containing the data from that page.  The caller is
> - * responsible for calling kfree() on this pointer when it is no longer
> - * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
> + * If the device supports this VPD page, this routine fills @buf
> + * with the data from that page and return 0. If the VPD page is not
> + * supported or its content cannot be retrieved, -EINVAL is returned.
>  */
> int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
> 		      int buf_len)
> {
> -	int i, result;
> +	int result, len;
> 
> 	if (sdev->skip_vpd_pages)
> -		goto fail;
> +		return -EINVAL;
> 
> 	/* Ask for all the pages supported by this device */
> 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
> 	if (result < 4)
> -		goto fail;
> +		return -EINVAL;
> 
> 	/* If the user actually wanted this page, we can skip the rest */
> 	if (page == 0)
> 		return 0;
> 
> -	for (i = 4; i < min(result, buf_len); i++)
> -		if (buf[i] == page)
> -			goto found;
> +	len = min(result, buf_len);
> +	if (len > 4 && memchr(&buf[4], page, len - 4))
> +		goto found;
> 
> -	if (i < result && i >= buf_len)
> -		/* ran off the end of the buffer, give us benefit of doubt */
> +	/* If we ran off the end of the buffer, give us benefit of doubt */
> +	if (result > buf_len)
> 		goto found;
> +
> 	/* The device claims it doesn't support the requested page */
> -	goto fail;
> +	return -EINVAL;
> 
>  found:
> 	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
> 	if (result < 0)
> -		goto fail;
> +		return -EINVAL;
> 
> 	return 0;
> -
> - fail:
> -	return -EINVAL;
> }
> EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> 
> -- 
> 2.31.1
> 

Looks fine. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

Re: [PATCH 2/2] scsi: fix scsi_mode_select() interface

[Himanshu Madhani]

> On Sep 29, 2021, at 4:17 AM, Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> 
> The modepage argument is unused. Remove it.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> drivers/scsi/scsi_lib.c    | 8 +++-----
> drivers/scsi/sd.c          | 2 +-
> include/scsi/scsi_device.h | 5 ++---
> 3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index dcf105287a76..f1fe5803d7ec 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2001,7 +2001,6 @@ void scsi_exit_queue(void)
>  *	@sdev:	SCSI device to be queried
>  *	@pf:	Page format bit (1 == standard, 0 == vendor specific)
>  *	@sp:	Save page bit (0 == don't save, 1 == save)
> - *	@modepage: mode page being requested
>  *	@buffer: request buffer (may not be smaller than eight bytes)
>  *	@len:	length of request buffer.
>  *	@timeout: command timeout
> @@ -2014,10 +2013,9 @@ void scsi_exit_queue(void)
>  *	status on error
>  *
>  */
> -int
> -scsi_mode_select(struct scsi_device *sdev, int pf, int sp, int modepage,
> -		 unsigned char *buffer, int len, int timeout, int retries,
> -		 struct scsi_mode_data *data, struct scsi_sense_hdr *sshdr)
> +int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
> +		     unsigned char *buffer, int len, int timeout, int retries,
> +		     struct scsi_mode_data *data, struct scsi_sense_hdr *sshdr)
> {
> 	unsigned char cmd[10];
> 	unsigned char *real_buffer;
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 71fa70b42c2b..89b5eea0ea0c 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -209,7 +209,7 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
> 	 */
> 	data.device_specific = 0;
> 
> -	if (scsi_mode_select(sdp, 1, sp, 8, buffer_data, len, SD_TIMEOUT,
> +	if (scsi_mode_select(sdp, 1, sp, buffer_data, len, SD_TIMEOUT,
> 			     sdkp->max_retries, &data, &sshdr)) {
> 		if (scsi_sense_valid(&sshdr))
> 			sd_print_sense_hdr(sdkp, &sshdr);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 09a17f6e93a7..1a9d2fe6aa02 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -415,9 +415,8 @@ extern int scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
> 			   int retries, struct scsi_mode_data *data,
> 			   struct scsi_sense_hdr *);
> extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
> -			    int modepage, unsigned char *buffer, int len,
> -			    int timeout, int retries,
> -			    struct scsi_mode_data *data,
> +			    unsigned char *buffer, int len, int timeout,
> +			    int retries, struct scsi_mode_data *data,
> 			    struct scsi_sense_hdr *);
> extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
> 				int retries, struct scsi_sense_hdr *sshdr);
> -- 
> 2.31.1
> 

Looks Good.

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

Re: [PATCH 0/2] small cleanups

[Martin K. Petersen]

On Wed, 29 Sep 2021 18:17:42 +0900, Damien Le Moal wrote:

> Martin,
> 
> Here is a couple of patches ifor some light cleanup in scsi_lib.
> No functional changes are introduced.
> 
> Damien Le Moal (2):
>   scsi: simplify scsi_get_vpd_page()
>   scsi: fix scsi_mode_select() interface
> 
> [...]

Applied to 5.17/scsi-queue, thanks!

[2/2] scsi: fix scsi_mode_select() interface
      https://git.kernel.org/mkp/scsi/c/81d3f500ee98

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 0/2] Small cleanups

[Damien Le Moal]

This is series is a couple of cleanup patches. The first one cleans up
the helper function nvme_block_nr() using SECTOR_SHIFT instead of the
hard coded value 9. The second patch introduces the nvme_block_sect()
helper to convert a device block number into a 512B sector value.

Please consider this series for kernel 5.5.

Damien Le Moal (2):
  nvme: Cleanup nvme_block_nr()
  nvme: Introduce nvme_block_sect()

 drivers/nvme/host/core.c | 14 +++++++-------
 drivers/nvme/host/nvme.h | 13 ++++++++++++-
 2 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.21.0

[PATCH 0/2] Small cleanups

[Damien Le Moal]

This is series is a couple of cleanup patches. The first one cleans up
the helper function nvme_block_nr() using SECTOR_SHIFT instead of the
hard coded value 9. The second patch introduces the nvme_block_sect()
helper to convert a device block number into a 512B sector value.

Please consider this series for kernel 5.5.

Damien Le Moal (2):
  nvme: Cleanup nvme_block_nr()
  nvme: Introduce nvme_block_sect()

 drivers/nvme/host/core.c | 14 +++++++-------
 drivers/nvme/host/nvme.h | 13 ++++++++++++-
 2 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.21.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme