Re: [PATCH] scsi: stop judging after finding a VPD page expected to be processed.

[Bart Van Assche <bvanassche@acm.org>]

On 2/25/25 10:58 PM, Chaohai Chen wrote:
> When the vpd_buf->data[i] is expected to be processed, stop other
> judgments.
> 
> Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>
> ---
>   drivers/scsi/scsi.c | 28 ++++++++++++++++++++--------
>   1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index a77e0499b738..53daf923ad8e 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -510,22 +510,34 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>   		return;
>   
>   	for (i = 4; i < vpd_buf->len; i++) {
> -		if (vpd_buf->data[i] == 0x0)
> +		switch (vpd_buf->data[i]) {
> +		case 0x0:
>   			scsi_update_vpd_page(sdev, 0x0, &sdev->vpd_pg0);
> -		if (vpd_buf->data[i] == 0x80)
> +			break;
> +		case 0x80:
>   			scsi_update_vpd_page(sdev, 0x80, &sdev->vpd_pg80);
> -		if (vpd_buf->data[i] == 0x83)
> +			break;
> +		case 0x83:
>   			scsi_update_vpd_page(sdev, 0x83, &sdev->vpd_pg83);
> -		if (vpd_buf->data[i] == 0x89)
> +			break;
> +		case 0x89:
>   			scsi_update_vpd_page(sdev, 0x89, &sdev->vpd_pg89);
> -		if (vpd_buf->data[i] == 0xb0)
> +			break;
> +		case 0xb0:
>   			scsi_update_vpd_page(sdev, 0xb0, &sdev->vpd_pgb0);
> -		if (vpd_buf->data[i] == 0xb1)
> +			break;
> +		case 0xb1:
>   			scsi_update_vpd_page(sdev, 0xb1, &sdev->vpd_pgb1);
> -		if (vpd_buf->data[i] == 0xb2)
> +			break;
> +		case 0xb2:
>   			scsi_update_vpd_page(sdev, 0xb2, &sdev->vpd_pgb2);
> -		if (vpd_buf->data[i] == 0xb7)
> +			break;
> +		case 0xb7:
>   			scsi_update_vpd_page(sdev, 0xb7, &sdev->vpd_pgb7);
> +			break;
> +		default:
> +			break;
> +		}
>   	}
>   	kfree(vpd_buf);
>   }

Instead of preserving the code duplication, please change this function
such that no code is duplicated. This will make it easier to cache more
VPD pages in the future. Here is an example of how this could be done
(entirely untested):

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index a77e0499b738..1365168941ed 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -487,6 +487,19 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  		kfree_rcu(vpd_buf, rcu);
  }

+struct vpd_page_info {
+	u8 page_code;
+	u16 offset; /* offset in struct scsi_device of vpd_pg... member */
+};
+
+#define SCSI_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) - 
sizeof(char))
+
+#define VPD_PAGE_INFO(vpd_page)						\
+	{ 0x##vpd_page, offsetof(struct scsi_device, vpd_pg##vpd_page) +\
+		SCSI_BUILD_BUG_ON(					\
+		!__same_type(&((struct scsi_device *)NULL)->vpd_pg##vpd_page, \
+			     struct scsi_vpd __rcu **))}
+
  /**
   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
   * @sdev: The device to ask
@@ -498,7 +511,17 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
   */
  void scsi_attach_vpd(struct scsi_device *sdev)
  {
-	int i;
+	static const struct vpd_page_info cached_page[] = {
+		VPD_PAGE_INFO(0),
+		VPD_PAGE_INFO(80),
+		VPD_PAGE_INFO(83),
+		VPD_PAGE_INFO(89),
+		VPD_PAGE_INFO(b0),
+		VPD_PAGE_INFO(b1),
+		VPD_PAGE_INFO(b2),
+		VPD_PAGE_INFO(b7),
+	};
+	int i, j;
  	struct scsi_vpd *vpd_buf;

  	if (!scsi_device_supports_vpd(sdev))
@@ -510,22 +533,17 @@ void scsi_attach_vpd(struct scsi_device *sdev)
  		return;

  	for (i = 4; i < vpd_buf->len; i++) {
-		if (vpd_buf->data[i] == 0x0)
-			scsi_update_vpd_page(sdev, 0x0, &sdev->vpd_pg0);
-		if (vpd_buf->data[i] == 0x80)
-			scsi_update_vpd_page(sdev, 0x80, &sdev->vpd_pg80);
-		if (vpd_buf->data[i] == 0x83)
-			scsi_update_vpd_page(sdev, 0x83, &sdev->vpd_pg83);
-		if (vpd_buf->data[i] == 0x89)
-			scsi_update_vpd_page(sdev, 0x89, &sdev->vpd_pg89);
-		if (vpd_buf->data[i] == 0xb0)
-			scsi_update_vpd_page(sdev, 0xb0, &sdev->vpd_pgb0);
-		if (vpd_buf->data[i] == 0xb1)
-			scsi_update_vpd_page(sdev, 0xb1, &sdev->vpd_pgb1);
-		if (vpd_buf->data[i] == 0xb2)
-			scsi_update_vpd_page(sdev, 0xb2, &sdev->vpd_pgb2);
-		if (vpd_buf->data[i] == 0xb7)
-			scsi_update_vpd_page(sdev, 0xb7, &sdev->vpd_pgb7);
+		for (j = 0; j < ARRAY_SIZE(cached_page); j++) {
+			const u8 page_code = cached_page[j].page_code;
+			const u16 offset = cached_page[j].offset;
+			struct scsi_vpd __rcu **vpd_data =
+				(void *)&sdev + offset;
+
+			if (vpd_buf->data[i] == page_code) {
+				scsi_update_vpd_page(sdev, page_code, vpd_data);
+				break;
+			}
+		}
  	}
  	kfree(vpd_buf);
  }