Re: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]
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]
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 <martin.peter...@oracle.com>; James E . J .
> Bottomley <j...@linux.vnet.ibm.com>
> Cc: linux-scsi@vger.kernel.org; Bart Van Assche <bart.vanass...@wdc.com>;
> Christoph Hellwig <h...@lst.de>; Hannes Reinecke <h...@suse.de>;
> Johannes Thumshirn <jthumsh...@suse.de>; Seymour, Shane M
> <shane.seym...@hpe.com>
> 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 <bart.vanass...@wdc.com>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: Johannes Thumshirn <jthumsh...@suse.de>
> Cc: Shane Seymour <shane.seym...@hpe.com>
> ---
> 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(sd
[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 = scsi_get_vpd_buf(sdev, 0x83);
+
mutex_lock(>inquiry_mutex);
- orig_vpd_buf = sdev->vpd_pg83;
- sdev->vpd_pg83_len = vpd_len;
+ orig_vpd_buf = rcu_dereference_protected(sdev->vpd_pg83,
+ lockdep_is_held(>inquiry_mutex));
rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
mutex_unlock(>inquiry_mutex);
- synchronize_rcu();
+
if (orig_vpd_buf)
- kfree(orig_vpd_buf);
+ kfree_rcu(orig_vpd_buf, rcu);
}
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 813d2456ae93..3d6e50087600
