Re: [PATCH] qlogicpti: fixup qlogicpti_reset() definition

[Martin K. Petersen]

Hannes,

> A merge error crept in when formatting commit af167bc ("scsi:
> qlogicpti: move bus reset to host reset")

Applied to 4.14/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: qedi: off by one in qedi_get_cmd_from_tid()

[Martin K. Petersen]

Dan,

> The > here should be >= or we end up reading one element beyond the
> end of the qedi->itt_map[] array.  The qedi->itt_map[] array is
> allocated in qedi_alloc_itt().

Applied to 4.13/scsi-fixes. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

[PATCH] storvsc: fix memory leak on ring buffer busy

[Long Li]

From: Long Li 

When storvsc is sending I/O to Hyper-v, it may allocate a bigger buffer
descriptor for large data payload that can't fit into a pre-allocated
buffer descriptor. This bigger buffer is freed on return path.

If I/O request to Hyper-v fails due to ring buffer busy, the storvsc allocated
buffer descriptor should also be freed.

Signed-off-by: Long Li 
---
 drivers/scsi/storvsc_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 009adb0..db52882 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1657,6 +1657,8 @@ static int storvsc_queuecommand(struct Scsi_Host *host, 
struct scsi_cmnd *scmnd)
ret = storvsc_do_io(dev, cmd_request, smp_processor_id());
 
if (ret == -EAGAIN) {
+   if (payload_sz > sizeof(cmd_request->mpb))
+   kfree(payload);
/* no more space */
return SCSI_MLQUEUE_DEVICE_BUSY;
}
-- 
2.7.4

Re: [PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Paul E. McKenney]

On Mon, Aug 28, 2017 at 09:39:18PM +, Bart Van Assche wrote:
> On Mon, 2017-08-28 at 14:26 -0700, Paul E. McKenney wrote:
> > On Mon, Aug 28, 2017 at 01:46:13PM -0700, Bart Van Assche wrote:
> > > A common pattern in RCU code is to assign a new value to an RCU
> > > pointer after having read and stored the old value. Introduce a
> > > macro for this pattern.
> > > 
> > > Signed-off-by: Bart Van Assche 
> > > Cc: Paul E. McKenney 
> > > Cc: Ingo Molnar 
> > > Cc: Christoph Hellwig 
> > > Cc: Hannes Reinecke 
> > > Cc: Johannes Thumshirn 
> > > Cc: Shane M Seymour 
> > > ---
> > >  include/linux/rcupdate.h | 20 
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index f816fc72b51e..555815ce2e57 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
> > >   */
> > >  #define rcu_pointer_handoff(p) (p)
> > > 
> > > +/**
> > > + * rcu_swap_protected() - swap an RCU and a regular pointer
> > > + * @rcu_ptr: RCU pointer
> > > + * @ptr: regular pointer
> > > + * @c: the conditions under which the dereference will take place
> > > + *
> > > + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated 
> > > pointer and
> > > + * @c is the argument that is passed to the rcu_dereference_protected() 
> > > call
> > > + * used to read that pointer.
> > > + */
> > > +#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
> > > + typeof(ptr) __tmp;  \
> > > + \
> > > + BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
> > > + BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
> > 
> > Hmmm...
> > 
> > What kinds of bugs have these two BUILD_BUG_ON() instances have caught
> > that would not be caught by the assignments below?
> 
> Hello Paul,
> 
> These two BUILD_BUG_ON() statements can be left out. The purpose of these
> statements is to complain as early as possible if the type of rcu_ptr and/or
> ptr is incorrect. As we all know error messages that are triggered by macros
> used inside a macro definition can be hard to read. My hope is that these
> two BUILD_BUG_ON() macros will cause the compiler to report easier to read
> diagnostic messages.
> 
> > > + __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
> > > + rcu_assign_pointer((rcu_ptr), (ptr));   \
> > > + (ptr) = __tmp;  \
> > > +} while (0)
> > > +
> > 
> > Could you please put this after rcu_assign_pointer() and before
> > rcu_access_pointer()?  That way the things that assign to RCU-protected
> > pointers are together.
> 
> Something like the patch below (compile-tested only)?

Looks good!

I suspect that you would like to push this with your changes, so,
assuming 0day test robot and -next are OK with it:

Acked-by: Paul E. McKenney 

I am not necessarily inalterably opposed to the extra BUILD_BUG_ON()
statements, and in fact I do like improved diagnostics, but those need to
go up via my tree as a separate patch.  That way, any unexpected gotchas
can be handled without risking your rcu_swap_protected() functionality.

Thanx, Paul

> Thanks,
> 
> Bart.
> 
> 
> [PATCH] rcu: Introduce rcu_swap_protected()
> ---
>  include/linux/rcupdate.h | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index f816fc72b51e..8e920f0ecb07 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -407,6 +407,22 @@ static inline void rcu_preempt_sleep_check(void) { }
>   _r_a_p__v;\
>  })
>  
> +/**
> + * rcu_swap_protected() - swap an RCU and a regular pointer
> + * @rcu_ptr: RCU pointer
> + * @ptr: regular pointer
> + * @c: the conditions under which the dereference will take place
> + *
> + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer 
> and
> + * @c is the argument that is passed to the rcu_dereference_protected() call
> + * used to read that pointer.
> + */
> +#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
> + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
> + rcu_assign_pointer((rcu_ptr), (ptr));   \
> + (ptr) = __tmp;  \
> +} while (0)
> +
>  /**
>   * rcu_access_pointer() - fetch RCU pointer with no dereferencing
>   * @p: The pointer to read

Re: [PATCH v2 17/30] scsi: Define usercopy region in scsi_sense_cache slab cache

[Kees Cook]

On Mon, Aug 28, 2017 at 2:42 PM, Bart Van Assche  wrote:
> On Mon, 2017-08-28 at 14:34 -0700, Kees Cook wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f6097b89d5d3..f1c6bd56dd5b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
>>   if (shost->unchecked_isa_dma) {
>>   scsi_sense_isadma_cache =
>>   kmem_cache_create("scsi_sense_cache(DMA)",
>> - SCSI_SENSE_BUFFERSIZE, 0,
>> - SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>> + SCSI_SENSE_BUFFERSIZE, 0,
>> + SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>>   if (!scsi_sense_isadma_cache)
>>   ret = -ENOMEM;
>
> All this part of this patch does is to change source code indentation. Should
> these changes really be included in this patch?

I can certainly drop that hunk, but the existing alignment is really
ugly. :) Happy to do whatever.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 17/30] scsi: Define usercopy region in scsi_sense_cache slab cache

[Bart Van Assche]

On Mon, 2017-08-28 at 14:34 -0700, Kees Cook wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f6097b89d5d3..f1c6bd56dd5b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
>   if (shost->unchecked_isa_dma) {
>   scsi_sense_isadma_cache =
>   kmem_cache_create("scsi_sense_cache(DMA)",
> - SCSI_SENSE_BUFFERSIZE, 0,
> - SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
> + SCSI_SENSE_BUFFERSIZE, 0,
> + SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>   if (!scsi_sense_isadma_cache)
>   ret = -ENOMEM;

All this part of this patch does is to change source code indentation. Should
these changes really be included in this patch?

Thanks,

Bart.

Re: [PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

On Mon, 2017-08-28 at 14:26 -0700, Paul E. McKenney wrote:
> On Mon, Aug 28, 2017 at 01:46:13PM -0700, Bart Van Assche wrote:
> > A common pattern in RCU code is to assign a new value to an RCU
> > pointer after having read and stored the old value. Introduce a
> > macro for this pattern.
> > 
> > Signed-off-by: Bart Van Assche 
> > Cc: Paul E. McKenney 
> > Cc: Ingo Molnar 
> > Cc: Christoph Hellwig 
> > Cc: Hannes Reinecke 
> > Cc: Johannes Thumshirn 
> > Cc: Shane M Seymour 
> > ---
> >  include/linux/rcupdate.h | 20 
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index f816fc72b51e..555815ce2e57 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
> >   */
> >  #define rcu_pointer_handoff(p) (p)
> > 
> > +/**
> > + * rcu_swap_protected() - swap an RCU and a regular pointer
> > + * @rcu_ptr: RCU pointer
> > + * @ptr: regular pointer
> > + * @c: the conditions under which the dereference will take place
> > + *
> > + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer 
> > and
> > + * @c is the argument that is passed to the rcu_dereference_protected() 
> > call
> > + * used to read that pointer.
> > + */
> > +#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
> > +   typeof(ptr) __tmp;  \
> > +   \
> > +   BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
> > +   BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
> 
> Hmmm...
> 
> What kinds of bugs have these two BUILD_BUG_ON() instances have caught
> that would not be caught by the assignments below?

Hello Paul,

These two BUILD_BUG_ON() statements can be left out. The purpose of these
statements is to complain as early as possible if the type of rcu_ptr and/or
ptr is incorrect. As we all know error messages that are triggered by macros
used inside a macro definition can be hard to read. My hope is that these
two BUILD_BUG_ON() macros will cause the compiler to report easier to read
diagnostic messages.

> > +   __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
> > +   rcu_assign_pointer((rcu_ptr), (ptr));   \
> > +   (ptr) = __tmp;  \
> > +} while (0)
> > +
> 
> Could you please put this after rcu_assign_pointer() and before
> rcu_access_pointer()?  That way the things that assign to RCU-protected
> pointers are together.

Something like the patch below (compile-tested only)?

Thanks,

Bart.


[PATCH] rcu: Introduce rcu_swap_protected()
---
 include/linux/rcupdate.h | 16 
 1 file changed, 16 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..8e920f0ecb07 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -407,6 +407,22 @@ static inline void rcu_preempt_sleep_check(void) { }
_r_a_p__v;\
 })
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
+   typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
+   rcu_assign_pointer((rcu_ptr), (ptr));   \
+   (ptr) = __tmp;  \
+} while (0)
+
 /**
  * rcu_access_pointer() - fetch RCU pointer with no dereferencing
  * @p: The pointer to read

[PATCH v2 17/30] scsi: Define usercopy region in scsi_sense_cache slab cache

[Kees Cook]

From: David Windsor 

SCSI sense buffers, stored in struct scsi_cmnd.sense and therefore
contained in the scsi_sense_cache slab cache, need to be copied to/from
userspace.

cache object allocation:
drivers/scsi/scsi_lib.c:
scsi_select_sense_cache(...):
return ... ? scsi_sense_isadma_cache : scsi_sense_cache

scsi_alloc_sense_buffer(...):
return kmem_cache_alloc_node(scsi_select_sense_cache(), ...);

scsi_init_request(...):
...
cmd->sense_buffer = scsi_alloc_sense_buffer(...);
...
cmd->req.sense = cmd->sense_buffer

example usage trace:

block/scsi_ioctl.c:
(inline from sg_io)
blk_complete_sghdr_rq(...):
struct scsi_request *req = scsi_req(rq);
...
copy_to_user(..., req->sense, len)

scsi_cmd_ioctl(...):
sg_io(...);

In support of usercopy hardening, this patch defines a region in
the scsi_sense_cache slab cache in which userspace copy operations
are allowed.

This region is known as the slab cache's usercopy region.  Slab
caches can now check that each copy operation involving cache-managed
memory falls entirely within the slab's usercopy region.

Signed-off-by: David Windsor 
[kees: adjust commit log, provide usage trace]
Cc: "James E.J. Bottomley" 
Cc: "Martin K. Petersen" 
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook 
---
 drivers/scsi/scsi_lib.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6097b89d5d3..f1c6bd56dd5b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
if (shost->unchecked_isa_dma) {
scsi_sense_isadma_cache =
kmem_cache_create("scsi_sense_cache(DMA)",
-   SCSI_SENSE_BUFFERSIZE, 0,
-   SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
+   SCSI_SENSE_BUFFERSIZE, 0,
+   SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
if (!scsi_sense_isadma_cache)
ret = -ENOMEM;
} else {
scsi_sense_cache =
-   kmem_cache_create("scsi_sense_cache",
-   SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN, NULL);
+   kmem_cache_create_usercopy("scsi_sense_cache",
+   SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
+   0, SCSI_SENSE_BUFFERSIZE, NULL);
if (!scsi_sense_cache)
ret = -ENOMEM;
}
-- 
2.7.4

Re: [PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Paul E. McKenney]

On Mon, Aug 28, 2017 at 01:46:13PM -0700, Bart Van Assche wrote:
> A common pattern in RCU code is to assign a new value to an RCU
> pointer after having read and stored the old value. Introduce a
> macro for this pattern.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Paul E. McKenney 
> Cc: Ingo Molnar 
> Cc: Christoph Hellwig 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: Shane M Seymour 
> ---
>  include/linux/rcupdate.h | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index f816fc72b51e..555815ce2e57 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
>   */
>  #define rcu_pointer_handoff(p) (p)
> 
> +/**
> + * rcu_swap_protected() - swap an RCU and a regular pointer
> + * @rcu_ptr: RCU pointer
> + * @ptr: regular pointer
> + * @c: the conditions under which the dereference will take place
> + *
> + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer 
> and
> + * @c is the argument that is passed to the rcu_dereference_protected() call
> + * used to read that pointer.
> + */
> +#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
> + typeof(ptr) __tmp;  \
> + \
> + BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
> + BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \

Hmmm...

What kinds of bugs have these two BUILD_BUG_ON() instances have caught
that would not be caught by the assignments below?

> + __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
> + rcu_assign_pointer((rcu_ptr), (ptr));   \
> + (ptr) = __tmp;  \
> +} while (0)
> +

Could you please put this after rcu_assign_pointer() and before
rcu_access_pointer()?  That way the things that assign to RCU-protected
pointers are together.

Thanx, Paul

>  /**
>   * rcu_read_lock() - mark the beginning of an RCU read-side critical section
>   *
> -- 
> 2.14.1
> 

Re: [PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

On Mon, 2017-08-28 at 13:45 -0700, Bart Van Assche wrote:
> The conclusion of a recent discussion about the handling of SCSI device VPD
> [ ... ]

Please ignore this series too. I think I finally found and fixed the bug in
the script I use to post patches ...

Bart.

[PATCH v3 3/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

[PATCH v3 2/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 2/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche 
Cc: Paul E. McKenney 
Cc: Ingo Molnar 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 include/linux/rcupdate.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
+   typeof(ptr) __tmp;  \
+   \
+   BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+   BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
+   __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
+   rcu_assign_pointer((rcu_ptr), (ptr));   \
+   (ptr) = __tmp;  \
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 3/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

Re: [PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

On Mon, 2017-08-28 at 13:44 -0700, Bart Van Assche wrote:
> The conclusion of a recent discussion about the handling of SCSI device VPD
> [ ... ]

Please ignore this patch series too - same problem as last time.

Bart.

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche 
Cc: Paul E. McKenney 
Cc: Ingo Molnar 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 include/linux/rcupdate.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
+   typeof(ptr) __tmp;  \
+   \
+   BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+   BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
+   __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
+   rcu_assign_pointer((rcu_ptr), (ptr));   \
+   (ptr) = __tmp;  \
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 2/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 3/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

[PATCH v3 2/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 3/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche 
Cc: Paul E. McKenney 
Cc: Ingo Molnar 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 include/linux/rcupdate.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
+   typeof(ptr) __tmp;  \
+   \
+   BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+   BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
+   __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
+   rcu_assign_pointer((rcu_ptr), (ptr));   \
+   (ptr) = __tmp;  \
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche 
Cc: Paul E. McKenney 
Cc: Ingo Molnar 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 include/linux/rcupdate.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
+   typeof(ptr) __tmp;  \
+   \
+   BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+   BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
+   __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
+   rcu_assign_pointer((rcu_ptr), (ptr));   \
+   (ptr) = __tmp;  \
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 3/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche 
Cc: Paul E. McKenney 
Cc: Ingo Molnar 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 include/linux/rcupdate.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {   \
+   typeof(ptr) __tmp;  \
+   \
+   BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+   BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));  \
+   __tmp = rcu_dereference_protected((rcu_ptr), (c));  \
+   rcu_assign_pointer((rcu_ptr), (ptr));   \
+   (ptr) = __tmp;  \
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 2/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

Re: [PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

On Mon, 2017-08-28 at 13:41 -0700, Bart Van Assche wrote:
> The conclusion of a recent discussion about the handling of SCSI device VPD
> [ ... ]

Please ignore this patch series - my working directory was not clean when I ran
the command to post these patches.

Bart.

[PATCH v3 3/3] scsi_transport_sas: Fix error handling in sas_smp_request()

[Bart Van Assche]

sas_function_template.smp_handler implementations either return
0 or a Unix error code. Convert that error code into a SCSI
result. This patch is what I came up with after having analyzed
the following sparse warnings:

drivers/scsi/scsi_transport_sas.c:187:21: warning: incorrect type in assignment 
(different base types)
drivers/scsi/scsi_transport_sas.c:187:21:expected restricted blk_status_t 
[usertype] ret
drivers/scsi/scsi_transport_sas.c:187:21:got int
drivers/scsi/scsi_transport_sas.c:188:39: warning: incorrect type in assignment 
(different base types)
drivers/scsi/scsi_transport_sas.c:188:39:expected int [signed] result
drivers/scsi/scsi_transport_sas.c:188:39:got restricted blk_status_t 
[usertype] ret

Fixes: commit 17d5363b83f8 ("scsi: introduce a result field in struct 
scsi_request")
Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: 
---
 drivers/scsi/scsi_transport_sas.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_sas.c 
b/drivers/scsi/scsi_transport_sas.c
index e2e948f1ce28..6c05fd9a0be5 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -173,7 +173,7 @@ static void sas_smp_request(struct request_queue *q, struct 
Scsi_Host *shost,
struct sas_rphy *rphy)
 {
struct request *req;
-   blk_status_t ret;
+   int ret;
int (*handler)(struct Scsi_Host *, struct sas_rphy *, struct request *);
 
while ((req = blk_fetch_request(q)) != NULL) {
@@ -185,7 +185,9 @@ static void sas_smp_request(struct request_queue *q, struct 
Scsi_Host *shost,
blk_rq_bytes(req->next_rq);
handler = to_sas_internal(shost->transportt)->f->smp_handler;
ret = handler(shost, rphy, req);
-   scsi_req(req)->result = ret;
+   WARN_ONCE(ret != 0 && !IS_ERR_VALUE((uintptr_t)ret),
+ "%s: ret = %d\n", __func__, ret);
+   scsi_req(req)->result = ret ? DID_ERROR << 16 : 0;
 
blk_end_request_all(req, 0);
 
-- 
2.14.1

[PATCH v3 2/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 3/3] scsi_transport_sas: Fix error handling in sas_smp_request()

[Bart Van Assche]

sas_function_template.smp_handler implementations either return
0 or a Unix error code. Convert that error code into a SCSI
result. This patch is what I came up with after having analyzed
the following sparse warnings:

drivers/scsi/scsi_transport_sas.c:187:21: warning: incorrect type in assignment 
(different base types)
drivers/scsi/scsi_transport_sas.c:187:21:expected restricted blk_status_t 
[usertype] ret
drivers/scsi/scsi_transport_sas.c:187:21:got int
drivers/scsi/scsi_transport_sas.c:188:39: warning: incorrect type in assignment 
(different base types)
drivers/scsi/scsi_transport_sas.c:188:39:expected int [signed] result
drivers/scsi/scsi_transport_sas.c:188:39:got restricted blk_status_t 
[usertype] ret

Fixes: commit 17d5363b83f8 ("scsi: introduce a result field in struct 
scsi_request")
Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: 
---
 drivers/scsi/scsi_transport_sas.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_sas.c 
b/drivers/scsi/scsi_transport_sas.c
index e2e948f1ce28..6c05fd9a0be5 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -173,7 +173,7 @@ static void sas_smp_request(struct request_queue *q, struct 
Scsi_Host *shost,
struct sas_rphy *rphy)
 {
struct request *req;
-   blk_status_t ret;
+   int ret;
int (*handler)(struct Scsi_Host *, struct sas_rphy *, struct request *);
 
while ((req = blk_fetch_request(q)) != NULL) {
@@ -185,7 +185,9 @@ static void sas_smp_request(struct request_queue *q, struct 
Scsi_Host *shost,
blk_rq_bytes(req->next_rq);
handler = to_sas_internal(shost->transportt)->f->smp_handler;
ret = handler(shost, rphy, req);
-   scsi_req(req)->result = ret;
+   WARN_ONCE(ret != 0 && !IS_ERR_VALUE((uintptr_t)ret),
+ "%s: ret = %d\n", __func__, ret);
+   scsi_req(req)->result = ret ? DID_ERROR << 16 : 0;
 
blk_end_request_all(req, 0);
 
-- 
2.14.1

[PATCH v3 1/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Shane M Seymour 
---
 drivers/scsi/scsi.c | 144 
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, 
unsigned char *buf,
 }
 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)
+{
+   unsigned char *vpd_buf;
+   int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+   if (!vpd_buf)
+   return NULL;
+
+   result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+   if (result < 0) {
+   kfree(vpd_buf);
+   return NULL;
+   }
+   if (result > vpd_len) {
+   vpd_len = result;
+   kfree(vpd_buf);
+   goto retry_pg;
+   }
+
+   *len = result;
+
+   return vpd_buf;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+unsigned char __rcu **sdev_vpd_buf,
+int *sdev_vpd_len)
+{
+   unsigned char *vpd_buf;
+   int len;
+
+   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   if (!vpd_buf)
+   return;
+
+   mutex_lock(>inquiry_mutex);
+   rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+  lockdep_is_held(>inquiry_mutex));
+   *sdev_vpd_len = len;
+   mutex_unlock(>inquiry_mutex);
+
+   synchronize_rcu();
+
+   kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int result, i;
-   int vpd_len = SCSI_VPD_PG_LEN;
-   int pg80_supported = 0;
-   int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   int i, vpd_len;
+   unsigned char *vpd_buf;
 
if (!scsi_device_supports_vpd(sdev))
return;
 
-retry_pg0:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
/* Ask for all the pages supported by this device */
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
+   vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
+   if (!vpd_buf)
return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg0;
-   }
 
-   for (i = 4; i < result; i++) {
+   for (i = 4; i < vpd_len; i++) {
if (vpd_buf[i] == 0x80)
-   pg80_supported = 1;
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
+>vpd_pg80_len);
if (vpd_buf[i] == 0x83)
-   pg83_supported = 1;
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
+>vpd_pg83_len);
}
kfree(vpd_buf);
-   vpd_len = SCSI_VPD_PG_LEN;
-
-   if (pg80_supported) {
-retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
-   return;
-
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-   if (result < 0) {
-   kfree(vpd_buf);
-   return;
-   }
-   if (result > vpd_len) {
-   vpd_len = result;
-   kfree(vpd_buf);
-   goto retry_pg80;
-   }
-   mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   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;
-   }
- 

[PATCH v3 2/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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| 44 ++--
 drivers/scsi/scsi_lib.c| 16 
 drivers/scsi/scsi_sysfs.c  | 29 -
 include/scsi/scsi_device.h | 18 ++
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-unsigned char __rcu **sdev_vpd_buf,
-int *sdev_vpd_len)
+struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-   unsigned char *vpd_buf;
-   int len;
+   struct scsi_vpd *vpd_buf;
 
-   vpd_buf = scsi_get_vpd_buf(sdev, page, );
+   vpd_buf = scsi_get_vpd_buf(sdev, page);
if (!vpd_buf)
return;
 
mutex_lock(>inquiry_mutex);
rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
   lockdep_is_held(>inquiry_mutex));
-   *sdev_vpd_len = len;
mutex_unlock(>inquiry_mutex);
 
-   synchronize_rcu();
-
-   kfree(vpd_buf);
+   if (vpd_buf)
+   kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device 
*sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-   int i, vpd_len;
-   unsigned char *vpd_buf;
+   int i;
+   struct scsi_vpd *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)
-   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80,
->vpd_pg80_len);
-   if (vpd_buf[i] == 0x83)
-   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83,
->vpd_pg83_len);
+   for (i = 4; i < vpd_buf->len; i++) {
+   if (vpd_buf->data[i] == 0x80)
+   scsi_update_vpd_page(sdev, 0x80, >vpd_pg80);
+   if (vpd_buf->data[i] == 0x83)
+   scsi_update_vpd_page(sdev, 0x83, >vpd_pg83);
}
kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,8 @@ int scsi_vpd_lun_id(struct scsi_device 

[PATCH v3 0/3] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c   | 140 --
 drivers/scsi/scsi_lib.c   |  16 ++---
 drivers/scsi/scsi_sysfs.c |  29 +---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h|  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

Re: linux-next: manual merge of the scsi tree with the staging tree

[g...@kroah.com]

On Mon, Aug 28, 2017 at 06:51:59PM +0100, James Bottomley wrote:
> On Mon, 2017-08-28 at 18:44 +0200, g...@kroah.com wrote:
> > On Mon, Aug 28, 2017 at 04:36:06PM +, Bart Van Assche wrote:
> > > 
> > > On Mon, 2017-08-28 at 18:05 +0200, g...@kroah.com wrote:
> > > > 
> > > > On Mon, Aug 28, 2017 at 03:41:28PM +, Bart Van Assche wrote:
> > > > > 
> > > > > * Most SCSI drivers exist under drivers/scsi, including the
> > > > > virtio-scsi and   xen-scsifront drivers. So why has the
> > > > > visorhba driver been added under unisys/visorhba?
> > > > 
> > > > That's because right now it's still a staging driver.  Also,
> > > > there are other scsi drivers in other portions of the kernel tree
> > > > (like the USB driver), so there's no hard rule that all scsi
> > > > drivers have to be under drivers/scsi/
> > > > 
> > > > 
> > > > 
> > > > Please provide this review to them, on the properly mailing list,
> > > > I'm sure they would be glad to get it.
> > > 
> > > OK, I will do that. BTW, is there a written down version of the
> > > rules for adding a driver under drivers/staging available
> > > somewhere? 
> > 
> > The only 2 rules for adding a new drivers/staging driver is:
> > - has to compile
> > - correct license
> > and sometimes we let code in if the first one isn't true :)
> > 
> > > 
> > > As far as I can see the visorhba driver went in without the linux-
> > > scsi mailing list having been CC-ed. See also Benjamin Romer,
> > > [PATCH] staging: unisys: Add s-Par visorhba, linux-driver-devel
> > > mailing list, July 2015
> > > (https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-
> > > 3Fl-3Dlinux-2Ddriver-2Ddevel-26m-
> > > 3D143681271902628=DwIBAg=jf_iaSHvJObTbx-
> > > siA1ZOg=lxuMXTRCffN3cvb-
> > > aRKL97jBhWZUIH_7zc3AgXUz9Mw=muMI5n73yuzpaRBjZnIrYLfhcO8lIP0JZSjBG
> > > D1LC5I=76_tO8QEuhUmhJPijDkweBOfh6DLPRILWj9Rhphb8j0= ).
> > 
> > That's totally normal, why would the scsi developers care about a
> > staging driver in such a rough state.  Only when it looks "good
> > enough" would we ask for a scsi developer review to move it out of
> > staging.
> 
> I think I've said before, we'd really rather a SCSI driver went via the
> SCSI tree because most of what's wrong with it will be the mechanics of
> the driver and its interaction with the SCSI and block subsystems which
> simply don't get looked at in the staging tree.  The aesthetics and the
> formatting issues are mostly fixed by a quick trip through lindent or a
> brief education of the submitters on Linux style.

This driver has been in the tree for almost 2 years now, with no
problems in this area before.  It, and the infrastructure around it, has
needed hundreds, if not thousands, of patches and changes and reworks to
get it to the almost readable state it is in today.

It was anything but a "quick trip through lindent".

And we don't have code in the "real" part of the kernel depend on code
in drivers/staging/, as that would not work.

So let's just keep things as they are here, there's been no issues with
this for 2 years, and for times when there is, great, let me know and we
can work to fix things up.  You are always free to break staging drivers
with api changes, it's up to the owner of the staging driver to fix the
issues in that case.

thanks,

greg k-h

RE: [PATCH 2/4] hpsa: remove the smp_handler stub

[Don Brace]

From: linux-scsi-ow...@vger.kernel.org [linux-scsi-ow...@vger.kernel.org] on 
behalf of Christoph Hellwig [h...@lst.de]
Sent: Friday, August 25, 2017 8:37 AM
To: Chaitra Basappa; linux-scsi@vger.kernel.org
Cc: Bart Van Assche
Subject: [PATCH 2/4] hpsa: remove the smp_handler stub

EXTERNAL EMAIL


The SAS transport class will do the right thing and not register the BSG
node if now smp_handler method is present.

Signed-off-by: Christoph Hellwig 

Acked-by: Don Brace 

Re: linux-next: manual merge of the scsi tree with the staging tree

[James Bottomley]

On Mon, 2017-08-28 at 18:44 +0200, g...@kroah.com wrote:
> On Mon, Aug 28, 2017 at 04:36:06PM +, Bart Van Assche wrote:
> > 
> > On Mon, 2017-08-28 at 18:05 +0200, g...@kroah.com wrote:
> > > 
> > > On Mon, Aug 28, 2017 at 03:41:28PM +, Bart Van Assche wrote:
> > > > 
> > > > * Most SCSI drivers exist under drivers/scsi, including the
> > > > virtio-scsi and   xen-scsifront drivers. So why has the
> > > > visorhba driver been added under unisys/visorhba?
> > > 
> > > That's because right now it's still a staging driver.  Also,
> > > there are other scsi drivers in other portions of the kernel tree
> > > (like the USB driver), so there's no hard rule that all scsi
> > > drivers have to be under drivers/scsi/
> > > 
> > > 
> > > 
> > > Please provide this review to them, on the properly mailing list,
> > > I'm sure they would be glad to get it.
> > 
> > OK, I will do that. BTW, is there a written down version of the
> > rules for adding a driver under drivers/staging available
> > somewhere? 
> 
> The only 2 rules for adding a new drivers/staging driver is:
>   - has to compile
>   - correct license
> and sometimes we let code in if the first one isn't true :)
> 
> > 
> > As far as I can see the visorhba driver went in without the linux-
> > scsi mailing list having been CC-ed. See also Benjamin Romer,
> > [PATCH] staging: unisys: Add s-Par visorhba, linux-driver-devel
> > mailing list, July 2015
> > (https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-
> > 3Fl-3Dlinux-2Ddriver-2Ddevel-26m-
> > 3D143681271902628=DwIBAg=jf_iaSHvJObTbx-
> > siA1ZOg=lxuMXTRCffN3cvb-
> > aRKL97jBhWZUIH_7zc3AgXUz9Mw=muMI5n73yuzpaRBjZnIrYLfhcO8lIP0JZSjBG
> > D1LC5I=76_tO8QEuhUmhJPijDkweBOfh6DLPRILWj9Rhphb8j0= ).
> 
> That's totally normal, why would the scsi developers care about a
> staging driver in such a rough state.  Only when it looks "good
> enough" would we ask for a scsi developer review to move it out of
> staging.

I think I've said before, we'd really rather a SCSI driver went via the
SCSI tree because most of what's wrong with it will be the mechanics of
the driver and its interaction with the SCSI and block subsystems which
simply don't get looked at in the staging tree.  The aesthetics and the
formatting issues are mostly fixed by a quick trip through lindent or a
brief education of the submitters on Linux style.

James

ALPSS: reminder and submission deadline

[Christoph Hellwig]

Just one more month to go until the Alpine Linux Persistence and Storage
Summit (ALPSS) will be held from September 27-29 at the Lizumerhuette
in Austria!

If you want to submit a 30-minute talk please do so until Sep 1st, as we
plan to finalize our schedule.  BOFs and team meetings will be scheduled
ad-hoc in the available meeting rooms or outside with a beautiful mountain
panorama.

If you only want to attend you can do so until last minute as long as
space doesn't run.

To submit a talk or request attendance please send a mail to:

alpss...@lists.infradead.org

More details are available on our website:

http://www.alpss.at/

Thank you on behalf of the program committee:

Stephen Bates
Sagi Grimberg
Christoph Hellwig
Johannes Thumshirn
Richard Weinberger

Re: [PATCH] scsi: ufs: Make use of UFS_BIT macro wherever possible

[Bart Van Assche]

On Mon, 2017-08-28 at 17:49 +0530, Alim Akhtar wrote:
> This entire file uses UFS_BIT macro for bits definition, expect for few
> places. This patch convert those defines to use UFS_BIT macro to be aligned
> with reset of the file.

This is the definition of the UFS_BIT() macro I found in
drivers/scsi/ufs/ufshci.h:

#define UFS_BIT(x)  (1L << (x))

Using this macro makes code longer instead of shorter and does not improve
code readability. Is this macro really useful? Wouldn't it be better to
remove the UFS_BIT() macro instead of introducing more uses of it?

Thanks,

Bart.

[PATCH] lpfc: Don't return internal MBXERR_ERROR code from probe function

[Stefano Brivio]

Internal error codes happen to be positive, thus the PCI driver
core won't treat them as failure, but we do. This would cause a
crash later on as lpfc_pci_remove_one() is called (e.g. as
shutdown function).

Fixes: 6d368e532168 ("[SCSI] lpfc 8.3.24: Add resource extent support")
Signed-off-by: Stefano Brivio 
---
 drivers/scsi/lpfc/lpfc_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 491aa95eb0f6..38cc2b5bb5a2 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -6118,6 +6118,7 @@ lpfc_sli4_driver_resource_setup(struct lpfc_hba *phba)
"Extents and RPI headers enabled.\n");
}
mempool_free(mboxq, phba->mbox_mem_pool);
+   rc = -EIO;
goto out_free_bsmbx;
}
 
-- 
2.9.4

[PATCH] scsi: ufs: Make use of UFS_BIT macro wherever possible

[Alim Akhtar]

This entire file uses UFS_BIT macro for bits definition, expect for few
places. This patch convert those defines to use UFS_BIT macro to be aligned
with reset of the file.

Signed-off-by: Alim Akhtar 
---
 drivers/scsi/ufs/ufshci.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index f60145d..588094a 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -186,9 +186,9 @@ enum {
 /* UECDL - Host UIC Error Code Data Link Layer 3Ch */
 #define UIC_DATA_LINK_LAYER_ERROR  UFS_BIT(31)
 #define UIC_DATA_LINK_LAYER_ERROR_CODE_MASK0x7FFF
-#define UIC_DATA_LINK_LAYER_ERROR_PA_INIT  0x2000
-#define UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED 0x0001
-#define UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT 0x0002
+#define UIC_DATA_LINK_LAYER_ERROR_PA_INIT  UFS_BIT(13)
+#define UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED UFS_BIT(1)
+#define UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT UFS_BIT(2)
 
 /* UECN - Host UIC Error Code Network Layer 40h */
 #define UIC_NETWORK_LAYER_ERRORUFS_BIT(31)
-- 
2.7.4

Re: [PATCH v3 15/20] lpfc: Fix nvme target failure after 2nd adapter reset

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v3 13/20] lpfc: Fix MRQ > 1 context list handling

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v3 11/20] lpfc: Correct issues with FAWWN and FDISCs

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v3 12/20] lpfc: Limit amount of work processed in IRQ

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v3 04/20] lpfc: Fix oops when NVME Target is discovered in a nonNVME environment

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH] qlogicpti: fixup qlogicpti_reset() definition

[Hannes Reinecke]

A merge error crept in when formatting commit
af167bc ("scsi: qlogicpti: move bus reset to host reset")

Fixes: af167bc ("scsi: qlogicpti: move bus reset to host reset")
Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/qlogicpti.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qlogicpti.c b/drivers/scsi/qlogicpti.c
index 274ee1ccfea8..cec9a14982e6 100644
--- a/drivers/scsi/qlogicpti.c
+++ b/drivers/scsi/qlogicpti.c
@@ -1250,9 +1250,10 @@ static int qlogicpti_abort(struct scsi_cmnd *Cmnd)
return return_status;
 }
 
-static int qlogicpti_reset(struct Scsi_Host *host)
+static int qlogicpti_reset(struct scsi_cmnd *Cmnd)
 {
u_short param[6];
+   struct Scsi_Host *host = Cmnd->device->host;
struct qlogicpti *qpti = (struct qlogicpti *) host->hostdata;
int return_status = SUCCESS;
 
-- 
2.12.3

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 1/2] Introduce scsi_get_vpd_buf()

[Christoph Hellwig]

On Fri, Aug 25, 2017 at 01:36:00PM -0700, Bart Van Assche wrote:
>  
>   if (pg80_supported) {
> -retry_pg80:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> - if (!vpd_buf)
> - return;
> -
> - result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> - if (result < 0) {
> - kfree(vpd_buf);
> - return;
> - }
> - if (result > vpd_len) {
> - vpd_len = result;
> - kfree(vpd_buf);
> - goto retry_pg80;
> - }
> + vpd_buf = scsi_get_vpd_buf(sdev, 0x80, _len);
> +

The old code above did an early return when scsi_vpd_inquiry, so
contrary to the old description it does change behavior.

I think that needs to be fixed up.

Except for that this look slike a good cleanup.

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 = 

RE: [PATCH 1/2] Introduce scsi_get_vpd_buf()

[Seymour, Shane M]

Reviewed-by: Shane Seymour 

> -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 1/2] Introduce scsi_get_vpd_buf()
> 
> This patch does not change any functionality.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Christoph Hellwig 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: Shane M Seymour 
> ---
>  drivers/scsi/scsi.c | 96 +--
> --
>  1 file changed, 45 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3d38c6d463b8..775f609f89e2 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -411,6 +411,41 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8
> page, unsigned char *buf,
>  }
>  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)
> +{
> + unsigned char *vpd_buf;
> + int vpd_len = SCSI_VPD_PG_LEN, result;
> +
> +retry_pg:
> + vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> + if (!vpd_buf)
> + return NULL;
> +
> + result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
> + if (result < 0) {
> + kfree(vpd_buf);
> + return NULL;
> + }
> + if (result > vpd_len) {
> + vpd_len = result;
> + kfree(vpd_buf);
> + goto retry_pg;
> + }
> +
> + *len = result;
> +
> + return vpd_buf;
> +}
> +
>  /**
>   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
>   * @sdev: The device to ask
> @@ -422,8 +457,8 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>   */
>  void scsi_attach_vpd(struct scsi_device *sdev)
>  {
> - int result, i;
> - int vpd_len = SCSI_VPD_PG_LEN;
> + int i;
> + int vpd_len;
>   int pg80_supported = 0;
>   int pg83_supported = 0;
>   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> @@ -431,51 +466,25 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>   if (!scsi_device_supports_vpd(sdev))
>   return;
> 
> -retry_pg0:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> + /* Ask for all the pages supported by this device */
> + vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
>   if (!vpd_buf)
>   return;
> 
> - /* Ask for all the pages supported by this device */
> - result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
> - if (result < 0) {
> - kfree(vpd_buf);
> - return;
> - }
> - if (result > vpd_len) {
> - vpd_len = result;
> - kfree(vpd_buf);
> - goto retry_pg0;
> - }
> -
> - for (i = 4; i < result; i++) {
> + for (i = 4; i < vpd_len; i++) {
>   if (vpd_buf[i] == 0x80)
>   pg80_supported = 1;
>   if (vpd_buf[i] == 0x83)
>   pg83_supported = 1;
>   }
>   kfree(vpd_buf);
> - vpd_len = SCSI_VPD_PG_LEN;
> 
>   if (pg80_supported) {
> -retry_pg80:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> - if (!vpd_buf)
> - return;
> -
> - result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> - if (result < 0) {
> - kfree(vpd_buf);
> - return;
> - }
> - if (result > vpd_len) {
> - vpd_len = result;
> - kfree(vpd_buf);
> - goto retry_pg80;
> - }
> + vpd_buf = scsi_get_vpd_buf(sdev, 0x80, _len);
> +
>   mutex_lock(>inquiry_mutex);
>   orig_vpd_buf = sdev->vpd_pg80;
> - sdev->vpd_pg80_len = result;
> + sdev->vpd_pg80_len = vpd_len;
>   rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
>   mutex_unlock(>inquiry_mutex);
>   synchronize_rcu();
> @@ -483,28 +492,13 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>   kfree(orig_vpd_buf);
>   orig_vpd_buf = NULL;
>   }
> - vpd_len = SCSI_VPD_PG_LEN;
>   }
> 
>   if (pg83_supported) {
> -retry_pg83:
> -