Re: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]

[Christoph Hellwig]

This looks generally good.  But I wonder if there is a way to
factor the assining/clearing sequence into little helpers instead
of duplicating it?

RE: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

Hi Bart,

Comments inline below about the show_vpd_##_page macro.

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@wdc.com]
> Sent: Saturday, August 26, 2017 6:36 AM
> To: Martin K . Petersen ; James E . J .
> Bottomley 
> Cc: linux-scsi@vger.kernel.org; Bart Van Assche ;
> Christoph Hellwig ; Hannes Reinecke ;
> Johannes Thumshirn ; Seymour, Shane M
> 
> Subject: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]
> 
> Introduce struct scsi_vpd for the VPD page length, data and the
> RCU head that will be used to free the VPD data. Use kfree_rcu()
> instead of kfree() to free VPD data. Only annotate pointers that
> are shared across threads with __rcu. Use rcu_dereference() when
> dereferencing an RCU pointer. This patch suppresses about twenty
> sparse complaints about the vpd_pg8[03] pointers.
> 
> Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Bart Van Assche 
> Cc: Christoph Hellwig 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: Shane Seymour 
> ---
>  drivers/scsi/scsi.c| 48 
> +-
>  drivers/scsi/scsi_lib.c| 16 
>  drivers/scsi/scsi_sysfs.c  | 26 +++--
>  include/scsi/scsi_device.h | 18 +
>  4 files changed, 64 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 775f609f89e2..1cf3aef0de8a 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -415,22 +415,20 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>   * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
>   * @sdev: The device to ask
>   * @page: Which Vital Product Data to return
> - * @len: Upon success, the VPD length will be stored in *@len.
>   *
>   * Returns %NULL upon failure.
>   */
> -static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
> -int *len)
> +static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
>  {
> - unsigned char *vpd_buf;
> + struct scsi_vpd *vpd_buf;
>   int vpd_len = SCSI_VPD_PG_LEN, result;
> 
>  retry_pg:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> + vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL);
>   if (!vpd_buf)
>   return NULL;
> 
> - result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
> + result = scsi_vpd_inquiry(sdev, vpd_buf->data, page, vpd_len);
>   if (result < 0) {
>   kfree(vpd_buf);
>   return NULL;
> @@ -441,7 +439,7 @@ static unsigned char *scsi_get_vpd_buf(struct
> scsi_device *sdev, u8 page,
>   goto retry_pg;
>   }
> 
> - *len = result;
> + vpd_buf->len = result;
> 
>   return vpd_buf;
>  }
> @@ -458,52 +456,50 @@ static unsigned char *scsi_get_vpd_buf(struct
> scsi_device *sdev, u8 page,
>  void scsi_attach_vpd(struct scsi_device *sdev)
>  {
>   int i;
> - int vpd_len;
>   int pg80_supported = 0;
>   int pg83_supported = 0;
> - unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> + struct scsi_vpd *vpd_buf, *orig_vpd_buf;
> 
>   if (!scsi_device_supports_vpd(sdev))
>   return;
> 
>   /* Ask for all the pages supported by this device */
> - vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
> + vpd_buf = scsi_get_vpd_buf(sdev, 0);
>   if (!vpd_buf)
>   return;
> 
> - for (i = 4; i < vpd_len; i++) {
> - if (vpd_buf[i] == 0x80)
> + for (i = 4; i < vpd_buf->len; i++) {
> + if (vpd_buf->data[i] == 0x80)
>   pg80_supported = 1;
> - if (vpd_buf[i] == 0x83)
> + if (vpd_buf->data[i] == 0x83)
>   pg83_supported = 1;
>   }
>   kfree(vpd_buf);
> 
>   if (pg80_supported) {
> - vpd_buf = scsi_get_vpd_buf(sdev, 0x80, _len);
> + vpd_buf = scsi_get_vpd_buf(sdev, 0x80);
> 
>   mutex_lock(>inquiry_mutex);
> - orig_vpd_buf = sdev->vpd_pg80;
> - sdev->vpd_pg80_len = vpd_len;
> + orig_vpd_buf = rcu_dereference_protected(sdev-
> >vpd_pg80,
> + lockdep_is_held(
> >inquiry_mutex));
>   rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
>   mutex_unlock(>inquiry_mutex);
> - synchronize_rcu();
> - if (orig_vpd_buf) {
> - kfree(orig_vpd_buf);
> - orig_vpd_buf = NULL;
> - }
> +
> + if (orig_vpd_buf)
> + kfree_rcu(orig_vpd_buf, rcu);
>   }
> 
>   if (pg83_supported) {
> - vpd_buf = scsi_get_vpd_buf(sdev, 0x83, _len);
> + vpd_buf =