[PATCH 00/19] SCSI patches for kernel v4.14

[PATCH 00/19] SCSI patches for kernel v4.14

[Bart Van Assche]

Hello Martin,

The patches in this series fall into two categories:
- Two patches are related to using blk_mq_rq_to_pdu() instead of
  struct request.special.
- Seventeen patches suppress warnings reported by static analysis
  tools. These tools are very useful but unfortunately the current
  code base makes these tools report a significant number of false
  positives.

Please consider these patches for kernel v4.14.

Thanks,

Bart.

Bart Van Assche (19):
  Remove an obsolete function declaration
  Avoid sign extension of scsi_device.type
  Suppress gcc 7 fall-through warnings reported with W=1
  Convert a strncmp() call into a strcmp() call
  scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it
  Document which queue type a function is intended for
  Fix RCU handling of scsi_device.vpd_pg8[03]
  Use blk_mq_rq_to_pdu() to convert a request to a SCSI command pointer
  sd, sr: Convert two assignments into warning statements
  sd: Fix indentation
  sd: Remove a useless comparison
  sg: Fix type of last blk_trace_setup() argument
  libiscsi: Fix indentation
  libsas: Remove a set-but-not-used variable
  libsas: Annotate fall-through in a switch statement
  scsi_transport_sas, sas_tlr_supported(): Check kzalloc() return value
  scsi_transport_srp: Suppress a W=1 compiler warning
  scsi_debug: Remove a set-but-not-used variable
  iscsi_tcp: Remove a set-but-not-used variable

 drivers/scsi/iscsi_tcp.c            |  2 --
 drivers/scsi/libiscsi.c             |  2 +-
 drivers/scsi/libsas/sas_ata.c       |  1 +
 drivers/scsi/libsas/sas_scsi_host.c |  3 ---
 drivers/scsi/scsi.c                 |  6 +++---
 drivers/scsi/scsi_debug.c           |  3 +--
 drivers/scsi/scsi_error.c           | 10 ++++++++--
 drivers/scsi/scsi_ioctl.c           |  4 +++-
 drivers/scsi/scsi_lib.c             | 36 +++++++++++++++++++++---------------
 drivers/scsi/scsi_sysfs.c           |  9 ++++++---
 drivers/scsi/scsi_transport_sas.c   |  3 +++
 drivers/scsi/scsi_transport_srp.c   |  2 +-
 drivers/scsi/sd.c                   |  6 +++---
 drivers/scsi/sg.c                   |  3 +--
 drivers/scsi/sr.c                   |  2 +-
 include/scsi/scsi_cmnd.h            |  1 -
 include/scsi/scsi_device.h          |  2 +-
 include/scsi/scsi_tcq.h             |  2 +-
 18 files changed, 55 insertions(+), 42 deletions(-)

-- 
2.14.0

[PATCH 01/19] Remove an obsolete function declaration

[Bart Van Assche]

Commit e9c787e65c0c ("scsi: allocate scsi_cmnd structures as part of
struct request") removed the scsi_get_command() function. Hence also
remove the declaration of that function.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 include/scsi/scsi_cmnd.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a1266d318c85..f5afcff8d76f 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -158,7 +158,6 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
-extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
 extern void scsi_put_command(struct scsi_cmnd *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
-- 
2.14.0

[PATCH 02/19] Avoid sign extension of scsi_device.type

[Bart Van Assche]

This patch avoids that smatch reports the following:

drivers/scsi/scsi_sysfs.c:506 scsi_bus_uevent() warn: argument 3 to %02x specifier has type 'char'
drivers/scsi/scsi_sysfs.c:872 sdev_show_modalias() warn: argument 4 to %02x specifier has type 'char'

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 include/scsi/scsi_device.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 0979a5f3b69a..f054f3f43c75 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -111,7 +111,7 @@ struct scsi_device {
 	unsigned sector_size;	/* size in bytes */
 
 	void *hostdata;		/* available to low-level driver */
-	char type;
+	unsigned char type;
 	char scsi_level;
 	char inq_periph_qual;	/* PQ from INQUIRY data */	
 	struct mutex inquiry_mutex;
-- 
2.14.0

[PATCH 03/19] Suppress gcc 7 fall-through warnings reported with W=1

[Bart Van Assche]

The conclusion of a recent discussion about the new warnings
reported by gcc 7 is that the new warnings reported when building
with W=1 should be suppressed. However, gcc 7 still warns about
fall-through in switch statements when building with W=1. Suppress
these warnings by annotating the SCSI core properly.

See also Linus Torvalds, Lots of new warnings with gcc-7.1.1, 11
July 2017 (https://www.mail-archive.com/linux-media@vger.kernel.org/msg115428.html).

References: commit bd664f6b3e37 ("disable new gcc-7.1.1 warnings for now")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_error.c | 8 +++++++-
 drivers/scsi/scsi_ioctl.c | 4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ea9f40e51f68..01b2d2055edf 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -552,6 +552,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
 			set_host_byte(scmd, DID_ALLOC_FAILURE);
 			return SUCCESS;
 		}
+		/* FALLTHROUGH */
 	case COPY_ABORTED:
 	case VOLUME_OVERFLOW:
 	case MISCOMPARE:
@@ -573,6 +574,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
 			return ADD_TO_MLQUEUE;
 		else
 			set_host_byte(scmd, DID_TARGET_FAILURE);
+		/* FALLTHROUGH */
 
 	case ILLEGAL_REQUEST:
 		if (sshdr.asc == 0x20 || /* Invalid command operation code */
@@ -683,6 +685,7 @@ static int scsi_eh_completed_normally(struct scsi_cmnd *scmd)
 	switch (status_byte(scmd->result)) {
 	case GOOD:
 		scsi_handle_queue_ramp_up(scmd->device);
+		/* FALLTHROUGH */
 	case COMMAND_TERMINATED:
 		return SUCCESS;
 	case CHECK_CONDITION:
@@ -1734,6 +1737,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 			set_host_byte(scmd, DID_TIME_OUT);
 			return SUCCESS;
 		}
+		/* FALLTHROUGH */
 	case DID_NO_CONNECT:
 	case DID_BAD_TARGET:
 		/*
@@ -1819,6 +1823,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 		 * the case of trying to send too many commands to a
 		 * tagged queueing device.
 		 */
+		/* FALLTHROUGH */
 	case BUSY:
 		/*
 		 * device can't talk to us at the moment.  Should only
@@ -1831,6 +1836,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 		if (scmd->cmnd[0] == REPORT_LUNS)
 			scmd->device->sdev_target->expecting_lun_change = 0;
 		scsi_handle_queue_ramp_up(scmd->device);
+		/* FALLTHROUGH */
 	case COMMAND_TERMINATED:
 		return SUCCESS;
 	case TASK_ABORTED:
@@ -2320,8 +2326,8 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 		rtn = scsi_try_host_reset(scmd);
 		if (rtn == SUCCESS)
 			break;
-	default:
 		/* FALLTHROUGH */
+	default:
 		rtn = FAILED;
 		break;
 	}
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index b6bf3f29a12a..0a875491f5a7 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -116,13 +116,15 @@ static int ioctl_internal_command(struct scsi_device *sdev, char *cmd,
 		case NOT_READY:	/* This happens if there is no disc in drive */
 			if (sdev->removable)
 				break;
+			/* FALLTHROUGH */
 		case UNIT_ATTENTION:
 			if (sdev->removable) {
 				sdev->changed = 1;
 				result = 0;	/* This is no longer considered an error */
 				break;
 			}
-		default:	/* Fall through for non-removable media */
+			/* FALLTHROUGH -- for non-removable media */
+		default:
 			sdev_printk(KERN_INFO, sdev,
 				    "ioctl_internal_command return code = %x\n",
 				    result);
-- 
2.14.0

[PATCH 04/19] Convert a strncmp() call into a strcmp() call

[Bart Van Assche]

This patch avoids that smatch reports the following warning:

drivers/scsi/scsi_sysfs.c:117: check_set() error: strncmp() '"-"' too small (2 vs 20)

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 41891db20108..5ed473a87589 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -114,7 +114,7 @@ static int check_set(unsigned long long *val, char *src)
 {
 	char *last;
 
-	if (strncmp(src, "-", 20) == 0) {
+	if (strcmp(src, "-") == 0) {
 		*val = SCAN_WILD_CARD;
 	} else {
 		/*
-- 
2.14.0

[PATCH 05/19] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

[Bart Van Assche]

The functional changes in this patch for scsi_setup_fs_cmnd() are:
- scsi_request.sense_len is cleared. This is OK since it is the
  responsibility of the LLD to set .sense_len before calling
  .scsi_done().
- scsi_request.cmd_len is changed to BLK_MAX_CDB. This is fine
  since scsi_driver.init_command() must overwrite this member for
  FS commands.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index efdcd9e79404..f9a0d5b13707 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1215,8 +1215,8 @@ static int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 			return ret;
 	}
 
-	cmd->cmnd = scsi_req(req)->cmd = scsi_req(req)->__cmd;
-	memset(cmd->cmnd, 0, BLK_MAX_CDB);
+	scsi_req_init(&cmd->req);
+	cmd->cmnd = cmd->req.cmd;
 	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
 
-- 
2.14.0

[PATCH 06/19] Document which queue type a function is intended for

[Bart Van Assche]

Document which queue type a function is intended for if this is not
easy to derive from the function name.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_lib.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f9a0d5b13707..1905962fb992 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2006,6 +2006,7 @@ static enum blk_eh_timer_return scsi_timeout(struct request *req,
 	return scsi_times_out(req);
 }
 
+/* scsi-mq */
 static int scsi_init_request(struct blk_mq_tag_set *set, struct request *rq,
 		unsigned int hctx_idx, unsigned int numa_node)
 {
@@ -2031,6 +2032,7 @@ static int scsi_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	return 0;
 }
 
+/* scsi-mq */
 static void scsi_exit_request(struct blk_mq_tag_set *set, struct request *rq,
 		unsigned int hctx_idx)
 {
@@ -2070,6 +2072,7 @@ static u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
 	return bounce_limit;
 }
 
+/* scsi-sq and scsi-mq */
 void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 {
 	struct device *dev = shost->dma_dev;
@@ -2109,6 +2112,7 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(__scsi_init_queue);
 
+/* scsi-sq */
 static int scsi_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
 {
 	struct Scsi_Host *shost = q->rq_alloc_data;
@@ -2139,6 +2143,7 @@ static int scsi_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
 	return -ENOMEM;
 }
 
+/* scsi-sq */
 static void scsi_exit_rq(struct request_queue *q, struct request *rq)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
@@ -2149,6 +2154,7 @@ static void scsi_exit_rq(struct request_queue *q, struct request *rq)
 			       cmd->sense_buffer);
 }
 
+/* scsi-sq */
 struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 {
 	struct Scsi_Host *shost = sdev->host;
-- 
2.14.0

[PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Only annotate pointers that are shared across threads with __rcu.
Use rcu_dereference() when dereferencing an RCU pointer. Protect
also the RCU pointer dereferences when freeing RCU pointers. This
patch suppresses about twenty sparse complaints about the vpd_pg8[03]
pointers.

Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Shane Seymour <shane.seymour@hpe.com>
---
 drivers/scsi/scsi.c       | 6 +++---
 drivers/scsi/scsi_lib.c   | 8 ++++----
 drivers/scsi/scsi_sysfs.c | 7 +++++--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..5bb15e698969 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -426,7 +426,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int vpd_len = SCSI_VPD_PG_LEN;
 	int pg80_supported = 0;
 	int pg83_supported = 0;
-	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+	unsigned char *vpd_buf, *orig_vpd_buf = NULL;
 
 	if (!scsi_device_supports_vpd(sdev))
 		return;
@@ -474,7 +474,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 			goto retry_pg80;
 		}
 		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg80;
+		orig_vpd_buf = rcu_dereference(sdev->vpd_pg80);
 		sdev->vpd_pg80_len = result;
 		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
@@ -503,7 +503,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 			goto retry_pg83;
 		}
 		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg83;
+		orig_vpd_buf = rcu_dereference(sdev->vpd_pg83);
 		sdev->vpd_pg83_len = result;
 		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1905962fb992..2ca91d251c5f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3282,7 +3282,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 	u8 cur_id_type = 0xff;
 	u8 cur_id_size = 0;
 	unsigned char *d, *cur_id_str;
-	unsigned char __rcu *vpd_pg83;
+	unsigned char *vpd_pg83;
 	int id_size = -EINVAL;
 
 	rcu_read_lock();
@@ -3431,7 +3431,7 @@ EXPORT_SYMBOL(scsi_vpd_lun_id);
 int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 {
 	unsigned char *d;
-	unsigned char __rcu *vpd_pg83;
+	unsigned char *vpd_pg83;
 	int group_id = -EAGAIN, rel_port = -1;
 
 	rcu_read_lock();
@@ -3441,8 +3441,8 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 		return -ENXIO;
 	}
 
-	d = sdev->vpd_pg83 + 4;
-	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+	d = vpd_pg83 + 4;
+	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5ed473a87589..cf8a2088a9ba 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -456,8 +456,11 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	/* NULL queue means the device can't be used */
 	sdev->request_queue = NULL;
 
-	kfree(sdev->vpd_pg83);
-	kfree(sdev->vpd_pg80);
+	mutex_lock(&sdev->inquiry_mutex);
+	kfree(rcu_dereference(sdev->vpd_pg83));
+	kfree(rcu_dereference(sdev->vpd_pg80));
+	mutex_unlock(&sdev->inquiry_mutex);
+
 	kfree(sdev->inquiry);
 	kfree(sdev);
 
-- 
2.14.0

[PATCH 08/19] Use blk_mq_rq_to_pdu() to convert a request to a SCSI command pointer

[Bart Van Assche]

Since commit e9c787e65c0c ("scsi: allocate scsi_cmnd structures as
part of struct request") struct request and struct scsi_cmnd are
adjacent. This means that there is now an alternative to reading
req->special to convert a pointer to a prepared request into a
SCSI command pointer, namely by using blk_mq_rq_to_pdu(). Make
this change where appropriate. Although this patch does not
change any functionality, it slightly improves performance and
slightly improves readability.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_error.c |  2 +-
 drivers/scsi/scsi_lib.c   | 18 +++++++++---------
 include/scsi/scsi_tcq.h   |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 01b2d2055edf..38942050b265 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -259,7 +259,7 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
  */
 enum blk_eh_timer_return scsi_times_out(struct request *req)
 {
-	struct scsi_cmnd *scmd = req->special;
+	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
 	enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
 	struct Scsi_Host *host = scmd->device->host;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2ca91d251c5f..e4966f8edbf9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -627,7 +627,7 @@ static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
 static bool scsi_end_request(struct request *req, blk_status_t error,
 		unsigned int bytes, unsigned int bidi_bytes)
 {
-	struct scsi_cmnd *cmd = req->special;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 	struct scsi_device *sdev = cmd->device;
 	struct request_queue *q = sdev->request_queue;
 
@@ -1176,7 +1176,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 
 static int scsi_setup_scsi_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd = req->special;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 
 	/*
 	 * Passthrough requests may transfer data, in which case they must
@@ -1207,7 +1207,7 @@ static int scsi_setup_scsi_cmnd(struct scsi_device *sdev, struct request *req)
  */
 static int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd = req->special;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 
 	if (unlikely(sdev->handler && sdev->handler->prep_fn)) {
 		int ret = sdev->handler->prep_fn(sdev, req);
@@ -1222,7 +1222,7 @@ static int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 
 static int scsi_setup_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd = req->special;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 
 	if (!blk_rq_bytes(req))
 		cmd->sc_data_direction = DMA_NONE;
@@ -1359,7 +1359,7 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
 
 static void scsi_unprep_fn(struct request_queue *q, struct request *req)
 {
-	scsi_uninit_cmd(req->special);
+	scsi_uninit_cmd(blk_mq_rq_to_pdu(req));
 }
 
 /*
@@ -1550,7 +1550,7 @@ static int scsi_lld_busy(struct request_queue *q)
  */
 static void scsi_kill_request(struct request *req, struct request_queue *q)
 {
-	struct scsi_cmnd *cmd = req->special;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 	struct scsi_device *sdev;
 	struct scsi_target *starget;
 	struct Scsi_Host *shost;
@@ -1581,7 +1581,7 @@ static void scsi_kill_request(struct request *req, struct request_queue *q)
 
 static void scsi_softirq_done(struct request *rq)
 {
-	struct scsi_cmnd *cmd = rq->special;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 	unsigned long wait_for = (cmd->allowed + 1) * rq->timeout;
 	int disposition;
 
@@ -1769,8 +1769,8 @@ static void scsi_request_fn(struct request_queue *q)
 			blk_start_request(req);
 
 		spin_unlock_irq(q->queue_lock);
-		cmd = req->special;
-		if (unlikely(cmd == NULL)) {
+		cmd = blk_mq_rq_to_pdu(req);
+		if (cmd != req->special) {
 			printk(KERN_CRIT "impossible request in %s.\n"
 					 "please mail a stack trace to "
 					 "linux-scsi@vger.kernel.org\n",
diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
index 4416b1026189..5b416debf101 100644
--- a/include/scsi/scsi_tcq.h
+++ b/include/scsi/scsi_tcq.h
@@ -39,7 +39,7 @@ static inline struct scsi_cmnd *scsi_host_find_tag(struct Scsi_Host *shost,
 
 	if (!req)
 		return NULL;
-	return req->special;
+	return blk_mq_rq_to_pdu(req);
 }
 
 #endif /* CONFIG_BLOCK */
-- 
2.14.0

[PATCH 09/19] sd, sr: Convert two assignments into warning statements

[Bart Van Assche]

Before scsi_prep_fn() calls the ULP .init_command() callback
function it stores the SCSI command pointer in request.special.
This means that the SCpnt = rq->special assignments in the sd
and sr drivers assign a pointer to itself. Hence convert these
two assignment statements into warning statements.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c | 2 +-
 drivers/scsi/sr.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bea36adeee17..a88639fbedb3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1013,7 +1013,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	ret = scsi_init_io(SCpnt);
 	if (ret != BLKPREP_OK)
 		goto out;
-	SCpnt = rq->special;
+	WARN_ON_ONCE(SCpnt != rq->special);
 
 	/* from here on until we're complete, any goto out
 	 * is used for a killable error condition */
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index a8f630213a1a..9be34d37c356 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -393,7 +393,7 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
 	ret = scsi_init_io(SCpnt);
 	if (ret != BLKPREP_OK)
 		goto out;
-	SCpnt = rq->special;
+	WARN_ON_ONCE(SCpnt != rq->special);
 	cd = scsi_cd(rq->rq_disk);
 
 	/* from here on until we're complete, any goto out
-- 
2.14.0

[PATCH 10/19] sd: Fix indentation

[Bart Van Assche]

This patch avoids that smatch reports the following:

drivers/scsi/sd.c:3540: sd_suspend_common() warn: inconsistent indenting

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a88639fbedb3..8b3d7994e182 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3537,7 +3537,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 			 * doesn't support sync. There's not much to do and
 			 * suspend shouldn't fail.
 			 */
-			 ret = 0;
+			ret = 0;
 		}
 	}
 
-- 
2.14.0

[PATCH 11/19] sd: Remove a useless comparison

[Bart Van Assche]

This patch avoids that gcc reports the following warning when
building with W=1:

drivers/scsi/sd.c:315:10: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
  if (val >= 0 && val <= T10_PI_TYPE3_PROTECTION)

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/scsi/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8b3d7994e182..7c0f9eb5a5fd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -312,7 +312,7 @@ protection_type_store(struct device *dev, struct device_attribute *attr,
 	if (err)
 		return err;
 
-	if (val >= 0 && val <= T10_PI_TYPE3_PROTECTION)
+	if (val <= T10_PI_TYPE3_PROTECTION)
 		sdkp->protection_type = val;
 
 	return count;
-- 
2.14.0

[PATCH 12/19] sg: Fix type of last blk_trace_setup() argument

[Bart Van Assche]

Avoid that sparse reports the following:

drivers/scsi/sg.c:1114:41: warning: incorrect type in argument 5 (different address spaces)
drivers/scsi/sg.c:1114:41:    expected char [noderef] <asn:1>*arg
drivers/scsi/sg.c:1114:41:    got char *<noident>

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/sg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f5705a95319f..03194c4b6744 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1110,8 +1110,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 		return blk_trace_setup(sdp->device->request_queue,
 				       sdp->disk->disk_name,
 				       MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
-				       NULL,
-				       (char *)arg);
+				       NULL, p);
 	case BLKTRACESTART:
 		return blk_trace_startstop(sdp->device->request_queue, 1);
 	case BLKTRACESTOP:
-- 
2.14.0

[PATCH 13/19] libiscsi: Fix indentation

[Bart Van Assche]

This patch avoids that smatch reports the following:

drivers/scsi/libiscsi.c:1081: iscsi_handle_reject() warn: inconsistent indenting

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/libiscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 42381adf0769..bd4605a34f54 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1078,7 +1078,7 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 		if (opcode != ISCSI_OP_NOOP_OUT)
 			return 0;
 
-		 if (rejected_pdu.itt == cpu_to_be32(ISCSI_RESERVED_TAG)) {
+		if (rejected_pdu.itt == cpu_to_be32(ISCSI_RESERVED_TAG)) {
 			/*
 			 * nop-out in response to target's nop-out rejected.
 			 * Just resend.
-- 
2.14.0

[PATCH 14/19] libsas: Remove a set-but-not-used variable

[Bart Van Assche]

This was detected by building with W=1.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/libsas/sas_scsi_host.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 87e5079d816b..fc90b8c65860 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -855,7 +855,6 @@ int sas_target_alloc(struct scsi_target *starget)
 int sas_slave_configure(struct scsi_device *scsi_dev)
 {
 	struct domain_device *dev = sdev_to_domain_dev(scsi_dev);
-	struct sas_ha_struct *sas_ha;
 
 	BUG_ON(dev->rphy->identify.device_type != SAS_END_DEVICE);
 
@@ -864,8 +863,6 @@ int sas_slave_configure(struct scsi_device *scsi_dev)
 		return 0;
 	}
 
-	sas_ha = dev->port->ha;
-
 	sas_read_port_mode_page(scsi_dev);
 
 	if (scsi_dev->tagged_supported) {
-- 
2.14.0

[PATCH 15/19] libsas: Annotate fall-through in a switch statement

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/libsas/sas_ata.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 87f5e694dbed..70be4425ae0b 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -343,6 +343,7 @@ static int smp_ata_check_ready(struct ata_link *link)
 	case SAS_END_DEVICE:
 		if (ex_phy->attached_sata_dev)
 			return sas_ata_clear_pending(dev, ex_phy);
+		/* fall through */
 	default:
 		return -ENODEV;
 	}
-- 
2.14.0

[PATCH 16/19] scsi_transport_sas, sas_tlr_supported(): Check kzalloc() return value

[Bart Van Assche]

Check whether memory allocation succeeded before dereferencing
the pointer to the allocated memory.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_transport_sas.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index a318c46db7cc..1a0fa79fd49a 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -423,6 +423,9 @@ sas_tlr_supported(struct scsi_device *sdev)
 	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
 	int ret = 0;
 
+	if (!buffer)
+		goto out;
+
 	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
 		goto out;
 
-- 
2.14.0

[PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning

[Bart Van Assche]

Avoid that the following compiler warning is reported when building
with W=1:

drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always false due to limited range of data type [-Wtype-limits]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_transport_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 698cc4681706..b8f5e4c47579 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo)
 	if (fast_io_fail_tmo < 0 &&
 	    dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
 		return -EINVAL;
-	if (dev_loss_tmo >= LONG_MAX / HZ)
+	if (dev_loss_tmo + 0UL >= LONG_MAX / HZ)
 		return -EINVAL;
 	if (fast_io_fail_tmo >= 0 && dev_loss_tmo >= 0 &&
 	    fast_io_fail_tmo >= dev_loss_tmo)
-- 
2.14.0

[PATCH 18/19] scsi_debug: Remove a set-but-not-used variable

[Bart Van Assche]

This patch avoids that gcc reports the following warning when
building with W=1:

drivers/scsi/scsi_debug.c:2264:15: warning: variable ?pcontrol? set but not used [-Wunused-but-set-variable]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_debug.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 3be980d47268..77a0335eb757 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2261,7 +2261,7 @@ static int resp_ie_l_pg(unsigned char * arr)
 static int resp_log_sense(struct scsi_cmnd * scp,
                           struct sdebug_dev_info * devip)
 {
-	int ppc, sp, pcontrol, pcode, subpcode, alloc_len, len, n;
+	int ppc, sp, pcode, subpcode, alloc_len, len, n;
 	unsigned char arr[SDEBUG_MAX_LSENSE_SZ];
 	unsigned char *cmd = scp->cmnd;
 
@@ -2272,7 +2272,6 @@ static int resp_log_sense(struct scsi_cmnd * scp,
 		mk_sense_invalid_fld(scp, SDEB_IN_CDB, 1, ppc ? 1 : 0);
 		return check_condition_result;
 	}
-	pcontrol = (cmd[2] & 0xc0) >> 6;
 	pcode = cmd[2] & 0x3f;
 	subpcode = cmd[3] & 0xff;
 	alloc_len = get_unaligned_be16(cmd + 7);
-- 
2.14.0

[PATCH 19/19] iscsi_tcp: Remove a set-but-not-used variable

[Bart Van Assche]

This patch avoids that gcc reports the following warning when
building with W=1:

drivers/scsi/iscsi_tcp.c:166:24: warning: variable ?session? set but not used [-Wunused-but-set-variable]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Lee Duncan <lduncan@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/iscsi_tcp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 4842fc0e809d..4d934d6c3e13 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -163,7 +163,6 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
 	struct iscsi_tcp_conn *tcp_conn;
 	struct iscsi_sw_tcp_conn *tcp_sw_conn;
 	struct iscsi_conn *conn;
-	struct iscsi_session *session;
 	void (*old_state_change)(struct sock *);
 
 	read_lock_bh(&sk->sk_callback_lock);
@@ -172,7 +171,6 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
 		read_unlock_bh(&sk->sk_callback_lock);
 		return;
 	}
-	session = conn->session;
 
 	iscsi_sw_sk_state_check(sk);
 
-- 
2.14.0

Re: [PATCH 02/19] Avoid sign extension of scsi_device.type

[Hannes Reinecke]

On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> This patch avoids that smatch reports the following:
> 
> drivers/scsi/scsi_sysfs.c:506 scsi_bus_uevent() warn: argument 3 to %02x specifier has type 'char'
> drivers/scsi/scsi_sysfs.c:872 sdev_show_modalias() warn: argument 4 to %02x specifier has type 'char'
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  include/scsi/scsi_device.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 0979a5f3b69a..f054f3f43c75 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -111,7 +111,7 @@ struct scsi_device {
>  	unsigned sector_size;	/* size in bytes */
>  
>  	void *hostdata;		/* available to low-level driver */
> -	char type;
> +	unsigned char type;
>  	char scsi_level;
>  	char inq_periph_qual;	/* PQ from INQUIRY data */	
>  	struct mutex inquiry_mutex;
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 01/19] Remove an obsolete function declaration

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 02/19] Avoid sign extension of scsi_device.type

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 03/19] Suppress gcc 7 fall-through warnings reported with W=1

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 04/19] Convert a strncmp() call into a strcmp() call

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 05/19] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

[Christoph Hellwig]

I don't really see the point of doing more work in exactly the same
amount of code here, especially given that this is the fast path.

Do you need this for any future patches?

Re: [PATCH 06/19] Document which queue type a function is intended for

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 02:39:56PM -0700, Bart Van Assche wrote:
> Document which queue type a function is intended for if this is not
> easy to derive from the function name.

How about adding mq and legacy to the function names instead?

Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote:
> Only annotate pointers that are shared across threads with __rcu.
> Use rcu_dereference() when dereferencing an RCU pointer. Protect
> also the RCU pointer dereferences when freeing RCU pointers. This
> patch suppresses about twenty sparse complaints about the vpd_pg8[03]
> pointers.

Shouldn't the kfrees be kfree_rcu?  or where else is the rcu protection
for them?

Re: [PATCH 08/19] Use blk_mq_rq_to_pdu() to convert a request to a SCSI command pointer

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 09/19] sd, sr: Convert two assignments into warning statements

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 02:39:59PM -0700, Bart Van Assche wrote:
> Before scsi_prep_fn() calls the ULP .init_command() callback
> function it stores the SCSI command pointer in request.special.
> This means that the SCpnt = rq->special assignments in the sd
> and sr drivers assign a pointer to itself. Hence convert these
> two assignment statements into warning statements.

Looks good.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Btw, in the longer run we should aim to kill the special pointer
in struct request entirely.

Re: [PATCH 10/19] sd: Fix indentation

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 12/19] sg: Fix type of last blk_trace_setup() argument

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 13/19] libiscsi: Fix indentation

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 14/19] libsas: Remove a set-but-not-used variable

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 15/19] libsas: Annotate fall-through in a switch statement

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 16/19] scsi_transport_sas, sas_tlr_supported(): Check kzalloc() return value

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 02:40:07PM -0700, Bart Van Assche wrote:
> Avoid that the following compiler warning is reported when building
> with W=1:
> 
> drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_transport_srp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
> index 698cc4681706..b8f5e4c47579 100644
> --- a/drivers/scsi/scsi_transport_srp.c
> +++ b/drivers/scsi/scsi_transport_srp.c
> @@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo)
>  	if (fast_io_fail_tmo < 0 &&
>  	    dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
>  		return -EINVAL;
> -	if (dev_loss_tmo >= LONG_MAX / HZ)
> +	if (dev_loss_tmo + 0UL >= LONG_MAX / HZ)
>  		return -EINVAL;

That's a weird change..  If we can to promote dev_loss_tmo to
long we should just cast it.  And of course we should always ask
us why we got this warning.  If long is 64-bit and int is 32-bit
the warning makes sense, as for every reasonable comparism
the 32-bit timeout can't be larger than LONG_MAX divided by 100 or 100.

But for 32-bit longs it can, so we should keep it, maybe with a comment.

Re: [PATCH 18/19] scsi_debug: Remove a set-but-not-used variable

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 19/19] iscsi_tcp: Remove a set-but-not-used variable

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 05/19] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

[Bart Van Assche]

On Thu, 2017-08-24 at 11:05 +0200, Christoph Hellwig wrote:
> I don't really see the point of doing more work in exactly the same
> amount of code here, especially given that this is the fast path.
> 
> Do you need this for any future patches?

Hello Christoph,

I will drop this patch.

Bart.

Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning

[Bart Van Assche]

On Thu, 2017-08-24 at 11:11 +0200, Christoph Hellwig wrote:
> On Wed, Aug 23, 2017 at 02:40:07PM -0700, Bart Van Assche wrote:
> > Avoid that the following compiler warning is reported when building
> > with W=1:
> > 
> > drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Johannes Thumshirn <jthumshirn@suse.de>
> > ---
> >  drivers/scsi/scsi_transport_srp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
> > index 698cc4681706..b8f5e4c47579 100644
> > --- a/drivers/scsi/scsi_transport_srp.c
> > +++ b/drivers/scsi/scsi_transport_srp.c
> > @@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo)
> >  	if (fast_io_fail_tmo < 0 &&
> >  	    dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
> >  		return -EINVAL;
> > -	if (dev_loss_tmo >= LONG_MAX / HZ)
> > +	if (dev_loss_tmo + 0UL >= LONG_MAX / HZ)
> >  		return -EINVAL;
> 
> That's a weird change..  If we can to promote dev_loss_tmo to
> long we should just cast it.  And of course we should always ask
> us why we got this warning.  If long is 64-bit and int is 32-bit
> the warning makes sense, as for every reasonable comparism
> the 32-bit timeout can't be larger than LONG_MAX divided by 100 or 100.
> 
> But for 32-bit longs it can, so we should keep it, maybe with a comment.

Hello Christoph,

The purpose of that check is to avoid that dev_loss_tmo * HZ can overflow.
That check is only needed on 32-bit systems since only on these systems
sizeof(long) == sizeof(int). How about changing the type of the dev_loss_tmo
argument from int to long such that no explicit cast is needed?

Thanks,

Bart.

Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote:
> On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote:
> > Only annotate pointers that are shared across threads with __rcu.
> > Use rcu_dereference() when dereferencing an RCU pointer. Protect
> > also the RCU pointer dereferences when freeing RCU pointers. This
> > patch suppresses about twenty sparse complaints about the vpd_pg8[03]
> > pointers.
> 
> Shouldn't the kfrees be kfree_rcu?  or where else is the rcu protection
> for them?

Hello Christoph,

My understanding of the SCSI VPD code is as follows:
* rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread
  updates a VPD buffer while it is being read.
* All code that either updates or reads a VPD buffer holds a reference on
  the SCSI device that buffer is associated with. That is why I think it is
  not needed to use kfree_rcu() in scsi_device_dev_release_usercontext().

Bart.
  

Re: [PATCH 06/19] Document which queue type a function is intended for

[Bart Van Assche]

On Thu, 2017-08-24 at 11:05 +0200, Christoph Hellwig wrote:
> On Wed, Aug 23, 2017 at 02:39:56PM -0700, Bart Van Assche wrote:
> > Document which queue type a function is intended for if this is not
> > easy to derive from the function name.
> 
> How about adding mq and legacy to the function names instead?

Hello Christoph,

Is something like the patch below perhaps what you had in mind?


[PATCH] Document which queue type a function is intended for

Rename several functions to make it easy to see which queue type a
function is intended for.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_lib.c  | 23 ++++++++++++-----------
 drivers/scsi/scsi_priv.h |  2 +-
 drivers/scsi/scsi_scan.c |  2 +-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index efdcd9e79404..f1589a0488ec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2006,8 +2006,8 @@ static enum blk_eh_timer_return scsi_timeout(struct request *req,
 	return scsi_times_out(req);
 }
 
-static int scsi_init_request(struct blk_mq_tag_set *set, struct request *rq,
-		unsigned int hctx_idx, unsigned int numa_node)
+static int scsi_mq_init_request(struct blk_mq_tag_set *set, struct request *rq,
+				unsigned int hctx_idx, unsigned int numa_node)
 {
 	struct Scsi_Host *shost = set->driver_data;
 	const bool unchecked_isa_dma = shost->unchecked_isa_dma;
@@ -2031,8 +2031,8 @@ static int scsi_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	return 0;
 }
 
-static void scsi_exit_request(struct blk_mq_tag_set *set, struct request *rq,
-		unsigned int hctx_idx)
+static void scsi_mq_exit_request(struct blk_mq_tag_set *set, struct request *rq,
+				 unsigned int hctx_idx)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 
@@ -2109,7 +2109,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(__scsi_init_queue);
 
-static int scsi_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
+static int scsi_sq_init_rq(struct request_queue *q, struct request *rq,
+			   gfp_t gfp)
 {
 	struct Scsi_Host *shost = q->rq_alloc_data;
 	const bool unchecked_isa_dma = shost->unchecked_isa_dma;
@@ -2139,7 +2140,7 @@ static int scsi_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
 	return -ENOMEM;
 }
 
-static void scsi_exit_rq(struct request_queue *q, struct request *rq)
+static void scsi_sq_exit_rq(struct request_queue *q, struct request *rq)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 
@@ -2149,7 +2150,7 @@ static void scsi_exit_rq(struct request_queue *q, struct request *rq)
 			       cmd->sense_buffer);
 }
 
-struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
+struct request_queue *scsi_sq_alloc_queue(struct scsi_device *sdev)
 {
 	struct Scsi_Host *shost = sdev->host;
 	struct request_queue *q;
@@ -2160,8 +2161,8 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 	q->cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size;
 	q->rq_alloc_data = shost;
 	q->request_fn = scsi_request_fn;
-	q->init_rq_fn = scsi_init_rq;
-	q->exit_rq_fn = scsi_exit_rq;
+	q->init_rq_fn = scsi_sq_init_rq;
+	q->exit_rq_fn = scsi_sq_exit_rq;
 	q->initialize_rq_fn = scsi_initialize_rq;
 
 	if (blk_init_allocated_queue(q) < 0) {
@@ -2185,8 +2186,8 @@ static const struct blk_mq_ops scsi_mq_ops = {
 #ifdef CONFIG_BLK_DEBUG_FS
 	.show_rq	= scsi_show_rq,
 #endif
-	.init_request	= scsi_init_request,
-	.exit_request	= scsi_exit_request,
+	.init_request	= scsi_mq_init_request,
+	.exit_request	= scsi_mq_exit_request,
 	.initialize_rq_fn = scsi_initialize_rq,
 	.map_queues	= scsi_map_queues,
 };
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index c11c1f9c912c..cdf166634b6f 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -88,7 +88,7 @@ extern void scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
 extern void scsi_io_completion(struct scsi_cmnd *, unsigned int);
 extern void scsi_run_host_queues(struct Scsi_Host *shost);
 extern void scsi_requeue_run_queue(struct work_struct *work);
-extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
+extern struct request_queue *scsi_sq_alloc_queue(struct scsi_device *sdev);
 extern struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev);
 extern void scsi_start_queue(struct scsi_device *sdev);
 extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fd88dabd599d..b331726a6eb4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -268,7 +268,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	if (shost_use_blk_mq(shost))
 		sdev->request_queue = scsi_mq_alloc_queue(sdev);
 	else
-		sdev->request_queue = scsi_alloc_queue(sdev);
+		sdev->request_queue = scsi_sq_alloc_queue(sdev);
 	if (!sdev->request_queue) {
 		/* release fn is set up in scsi_sysfs_device_initialise, so
 		 * have to free and put manually here */
-- 
2.14.0

Re: [PATCH 06/19] Document which queue type a function is intended for

[hch]

On Thu, Aug 24, 2017 at 04:57:03PM +0000, Bart Van Assche wrote:
> Is something like the patch below perhaps what you had in mind?

Yes.  Except that I really don't like the sq naming - blk-mq can
also be used for single queues, so it's a rather confusing name
Something like legacy or old seems to be more suitable.

Re: [PATCH 06/19] Document which queue type a function is intended for

[Bart Van Assche]

On Thu, 2017-08-24 at 18:58 +0200, hch@lst.de wrote:
> On Thu, Aug 24, 2017 at 04:57:03PM +0000, Bart Van Assche wrote:
> > Is something like the patch below perhaps what you had in mind?
> 
> Yes.  Except that I really don't like the sq naming - blk-mq can
> also be used for single queues, so it's a rather confusing name
> Something like legacy or old seems to be more suitable.

Hello Christoph,

Unless anyone prefers otherwise I will use "old" because the same word
is already used in the device mapper to refer to legacy request processing
code.

Bart.

RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

> -----Original Message-----
> From: Bart Van Assche [mailto:Bart.VanAssche@wdc.com]
> Sent: Friday, August 25, 2017 2:54 AM
> To: hch@lst.de
> Cc: jejb@linux.vnet.ibm.com; linux-scsi@vger.kernel.org; hare@suse.de;
> jthumshirn@suse.de; martin.petersen@oracle.com; Seymour, Shane M
> <shane.seymour@hpe.com>
> Subject: Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
> 
> On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote:
> > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote:
> > > Only annotate pointers that are shared across threads with __rcu.
> > > Use rcu_dereference() when dereferencing an RCU pointer. Protect
> > > also the RCU pointer dereferences when freeing RCU pointers. This
> > > patch suppresses about twenty sparse complaints about the
> > > vpd_pg8[03] pointers.
> >
> > Shouldn't the kfrees be kfree_rcu?  or where else is the rcu
> > protection for them?
> 
> Hello Christoph,
> 

Hi Bart,

> My understanding of the SCSI VPD code is as follows:
> * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread
>   updates a VPD buffer while it is being read.

My understanding is that it doesn't do that - you can update an RCU pointer with rcu_assign_pointer() after someone has called rcu_read_lock() and before they call rcu_read_unlock(). 

What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical section when accessing an RCU data item. If you have 2 CPUs in a read-side critical section and a 3rd CPU replacing the pointer using rcu_assign_pointer() one CPU could potentially end up with the old pointer and the other one with the new pointer or both old or both new (the only guarantee you have is that the pointer won't be partially updated with bits of old and the new pointer). To free the old pointer directly you have to call synchronize_rcu() after which you can call kfree() or if you don't call synchronize_rcu() you have to use a delayed freeing mechanism like kfree_rcu() so you can guarantee that the old pointer is still valid while used in a read-side critical section. Using something like kfree_rcu() means that you also don’t have to wait like I believe you can do if you call synchronize_rcu() since you could be forced to wait for a RCU grace period to end before you can call kfree().

So what they really do is ensure that someone updating the pointer can't free the old pointer (in case someone is using it) until everyone has left their read-side critical section (if the read-side critical section started before synchronize_rcu() was called).

You'll find a good example here that uses synchronize_rcu():

https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt

Search for " WHAT ARE SOME EXAMPLE USES OF CORE RCU API?" - it has a lot of other information about RCU.

> * All code that either updates or reads a VPD buffer holds a reference on
>   the SCSI device that buffer is associated with. That is why I think it is
>   not needed to use kfree_rcu() in scsi_device_dev_release_usercontext().

The only counter argument I'd put up into why it shouldn't be done the way you want to is that if someone else sees that code and doesn't understand the context and can't guarantee similar to this situation where all references to the structure should have already been dropped and think that it's ok to directly kfree something returned from rcu_dereference() when it's something that they really shouldn't do. The only real difference is that kfree_rcu will return the memory for reuse when RCU processing gets done in softirq and what you're doing will do it immediately. It's easier to use kfree_rcu() from what I can see.

It is possible I may be being too picky (it's a personal failing sometimes) but is it really that a large overhead to free the RCU pointers in a way that RCU pointers are expected to work even if the pointers shouldn't be accessible to anything?

Shane

> 
> Bart.
> 

Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[hch]

On Fri, Aug 25, 2017 at 05:58:16AM +0000, Seymour, Shane M wrote:
> > My understanding of the SCSI VPD code is as follows:
> > * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread
> >   updates a VPD buffer while it is being read.
> 
> My understanding is that it doesn't do that - you can update an RCU
> pointer with rcu_assign_pointer() after someone has called
> rcu_read_lock() and before they call rcu_read_unlock(). 

Indeed.

> What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical
> section when accessing an RCU data item. If you have 2 CPUs in a
> read-side critical section and a 3rd CPU replacing the pointer using
> rcu_assign_pointer() one CPU could potentially end up with the old pointer
> and the other one with the new pointer or both old or both new (the only
> guarantee you have is that the pointer won't be partially updated with
> bits of old and the new pointer).

Exactly.

> To free the old pointer directly you have to call synchronize_rcu()
> after which you can call kfree() or if you don't call synchronize_rcu()
> you have to use a delayed freeing mechanism like kfree_rcu()

(or call_rcu for the more general case)

> so you can guarantee that the old pointer is still valid while used in a
> read-side critical section. Using something like kfree_rcu() means that
> you also don’t have to wait like I believe you can do if you call
> synchronize_rcu() since you could be forced to wait for a RCU grace
> period to end before you can call kfree().

Yes, call_rcu (including the kfree_rcu helper) schedules a delayed
action after the grace period, synchronize_rcu synchronously waits
for the end of the grace period.

> > * All code that either updates or reads a VPD buffer holds a reference on
> >   the SCSI device that buffer is associated with. That is why I think it is
> >   not needed to use kfree_rcu() in scsi_device_dev_release_usercontext().

But a reference could be dropped during the grace period.  We'll need
to either wait for the grace period after NULLing out the vpd pointers
or before freeing the allocations for them.  Currently the only rcu
synchronization in the scsi code is after assining the vpd pointers
(in which case we'd only need it whe replacing the previous one, which
should not happen in practice anyway), but I can't find anything in the
free path.

Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning

[hch]

On Thu, Aug 24, 2017 at 04:27:07PM +0000, Bart Van Assche wrote:
> 
> The purpose of that check is to avoid that dev_loss_tmo * HZ can overflow.
> That check is only needed on 32-bit systems since only on these systems
> sizeof(long) == sizeof(int). How about changing the type of the dev_loss_tmo
> argument from int to long such that no explicit cast is needed?

Yes, switching the timeout to long sounds useful as that's our normal
type for timeouts.  But it will spread through a lot of the SRP code.

Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning

[Bart Van Assche]

On Fri, 2017-08-25 at 17:29 +0200, hch@lst.de wrote:
> On Thu, Aug 24, 2017 at 04:27:07PM +0000, Bart Van Assche wrote:
> > 
> > The purpose of that check is to avoid that dev_loss_tmo * HZ can overflow.
> > That check is only needed on 32-bit systems since only on these systems
> > sizeof(long) == sizeof(int). How about changing the type of the dev_loss_tmo
> > argument from int to long such that no explicit cast is needed?
> 
> Yes, switching the timeout to long sounds useful as that's our normal
> type for timeouts.  But it will spread through a lot of the SRP code.

Hello Christoph,

Do you agree with the patch below? It seems to be sufficient to suppress
the compiler warning.

Thanks,

Bart.

[PATCH] scsi_transport_srp: Suppress a W=1 compiler warning

Avoid that the following compiler warning is reported when building
with W=1:

drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always false due to limited range of data type [-Wtype-limits]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_transport_srp.c | 2 +-
 include/scsi/scsi_transport_srp.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 698cc4681706..4f6f01cf9968 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -78,7 +78,7 @@ static inline struct srp_rport *shost_to_rport(struct Scsi_Host *shost)
  * parameters must be such that multipath can detect failed paths timely.
  * Hence do not allow all three parameters to be disabled simultaneously.
  */
-int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo)
+int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, long dev_loss_tmo)
 {
 	if (reconnect_delay < 0 && fast_io_fail_tmo < 0 && dev_loss_tmo < 0)
 		return -EINVAL;
diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h
index dd096330734e..56ae198acc73 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -111,7 +111,7 @@ extern struct srp_rport *srp_rport_add(struct Scsi_Host *,
 				       struct srp_rport_identifiers *);
 extern void srp_rport_del(struct srp_rport *);
 extern int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo,
-			 int dev_loss_tmo);
+			 long dev_loss_tmo);
 int srp_parse_tmo(int *tmo, const char *buf);
 extern int srp_reconnect_rport(struct srp_rport *rport);
 extern void srp_start_tl_fail_timers(struct srp_rport *rport);

Re: [PATCH 04/19] Convert a strncmp() call into a strcmp() call

[Hannes Reinecke]

On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> This patch avoids that smatch reports the following warning:
> 
> drivers/scsi/scsi_sysfs.c:117: check_set() error: strncmp() '"-"' too small (2 vs 20)
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 41891db20108..5ed473a87589 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -114,7 +114,7 @@ static int check_set(unsigned long long *val, char *src)
>  {
>  	char *last;
>  
> -	if (strncmp(src, "-", 20) == 0) {
> +	if (strcmp(src, "-") == 0) {
>  		*val = SCAN_WILD_CARD;
>  	} else {
>  		/*
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/19] Document which queue type a function is intended for

[Hannes Reinecke]

On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> Document which queue type a function is intended for if this is not
> easy to derive from the function name.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_lib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Hannes Reinecke]

On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> Only annotate pointers that are shared across threads with __rcu.
> Use rcu_dereference() when dereferencing an RCU pointer. Protect
> also the RCU pointer dereferences when freeing RCU pointers. This
> patch suppresses about twenty sparse complaints about the vpd_pg8[03]
> pointers.
> 
> Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Shane Seymour <shane.seymour@hpe.com>
> ---
>  drivers/scsi/scsi.c       | 6 +++---
>  drivers/scsi/scsi_lib.c   | 8 ++++----
>  drivers/scsi/scsi_sysfs.c | 7 +++++--
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3d38c6d463b8..5bb15e698969 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -426,7 +426,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  	int vpd_len = SCSI_VPD_PG_LEN;
>  	int pg80_supported = 0;
>  	int pg83_supported = 0;
> -	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> +	unsigned char *vpd_buf, *orig_vpd_buf = NULL;
>  
>  	if (!scsi_device_supports_vpd(sdev))
>  		return;
Why drop the __rcu annotation here?
vpd_buf and orig_vpd_buf should always be accessed via rcu pointers.
Did I misunderstood the meaning of the __rcu annotation?

> @@ -474,7 +474,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  			goto retry_pg80;
>  		}
>  		mutex_lock(&sdev->inquiry_mutex);
> -		orig_vpd_buf = sdev->vpd_pg80;
> +		orig_vpd_buf = rcu_dereference(sdev->vpd_pg80);
>  		sdev->vpd_pg80_len = result;
>  		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
>  		mutex_unlock(&sdev->inquiry_mutex);
Yeah, I wasn't quite sure if I need to use rcu_dereference here.

> @@ -503,7 +503,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  			goto retry_pg83;
>  		}
>  		mutex_lock(&sdev->inquiry_mutex);
> -		orig_vpd_buf = sdev->vpd_pg83;
> +		orig_vpd_buf = rcu_dereference(sdev->vpd_pg83);
>  		sdev->vpd_pg83_len = result;
>  		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
>  		mutex_unlock(&sdev->inquiry_mutex);
Same here.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1905962fb992..2ca91d251c5f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3282,7 +3282,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
>  	u8 cur_id_type = 0xff;
>  	u8 cur_id_size = 0;
>  	unsigned char *d, *cur_id_str;
> -	unsigned char __rcu *vpd_pg83;
> +	unsigned char *vpd_pg83;
>  	int id_size = -EINVAL;
>  
>  	rcu_read_lock();
> @@ -3431,7 +3431,7 @@ EXPORT_SYMBOL(scsi_vpd_lun_id);
>  int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
>  {
>  	unsigned char *d;
> -	unsigned char __rcu *vpd_pg83;
> +	unsigned char *vpd_pg83;
>  	int group_id = -EAGAIN, rel_port = -1;
>  
>  	rcu_read_lock();
> @@ -3441,8 +3441,8 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
>  		return -ENXIO;
>  	}
>  
> -	d = sdev->vpd_pg83 + 4;
> -	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
> +	d = vpd_pg83 + 4;
> +	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
>  		switch (d[1] & 0xf) {
>  		case 0x4:
>  			/* Relative target port */
Bah. Yeah, of course.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 5ed473a87589..cf8a2088a9ba 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -456,8 +456,11 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
>  	/* NULL queue means the device can't be used */
>  	sdev->request_queue = NULL;
>  
> -	kfree(sdev->vpd_pg83);
> -	kfree(sdev->vpd_pg80);
> +	mutex_lock(&sdev->inquiry_mutex);
> +	kfree(rcu_dereference(sdev->vpd_pg83));
> +	kfree(rcu_dereference(sdev->vpd_pg80));
> +	mutex_unlock(&sdev->inquiry_mutex);
> +
>  	kfree(sdev->inquiry);
>  	kfree(sdev);
>  
> 
As indicated by Shane, I think using 'kfree_rcu()' here might not be a
bad idea. You never know ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 10/19] sd: Fix indentation

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> This patch avoids that smatch reports the following:
> 
> drivers/scsi/sd.c:3540: sd_suspend_common() warn: inconsistent indenting
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a88639fbedb3..8b3d7994e182 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3537,7 +3537,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  			 * doesn't support sync. There's not much to do and
>  			 * suspend shouldn't fail.
>  			 */
> -			 ret = 0;
> +			ret = 0;
>  		}
>  	}
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 11/19] sd: Remove a useless comparison

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> This patch avoids that gcc reports the following warning when
> building with W=1:
> 
> drivers/scsi/sd.c:315:10: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
>   if (val >= 0 && val <= T10_PI_TYPE3_PROTECTION)
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> ---
>  drivers/scsi/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 8b3d7994e182..7c0f9eb5a5fd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -312,7 +312,7 @@ protection_type_store(struct device *dev, struct device_attribute *attr,
>  	if (err)
>  		return err;
>  
> -	if (val >= 0 && val <= T10_PI_TYPE3_PROTECTION)
> +	if (val <= T10_PI_TYPE3_PROTECTION)
>  		sdkp->protection_type = val;
>  
>  	return count;
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 12/19] sg: Fix type of last blk_trace_setup() argument

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> Avoid that sparse reports the following:
> 
> drivers/scsi/sg.c:1114:41: warning: incorrect type in argument 5 (different address spaces)
> drivers/scsi/sg.c:1114:41:    expected char [noderef] <asn:1>*arg
> drivers/scsi/sg.c:1114:41:    got char *<noident>
> 
> This patch does not change any functionality.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/sg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index f5705a95319f..03194c4b6744 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1110,8 +1110,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
>  		return blk_trace_setup(sdp->device->request_queue,
>  				       sdp->disk->disk_name,
>  				       MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
> -				       NULL,
> -				       (char *)arg);
> +				       NULL, p);
>  	case BLKTRACESTART:
>  		return blk_trace_startstop(sdp->device->request_queue, 1);
>  	case BLKTRACESTOP:
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 13/19] libiscsi: Fix indentation

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> This patch avoids that smatch reports the following:
> 
> drivers/scsi/libiscsi.c:1081: iscsi_handle_reject() warn: inconsistent indenting
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/libiscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 42381adf0769..bd4605a34f54 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1078,7 +1078,7 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  		if (opcode != ISCSI_OP_NOOP_OUT)
>  			return 0;
>  
> -		 if (rejected_pdu.itt == cpu_to_be32(ISCSI_RESERVED_TAG)) {
> +		if (rejected_pdu.itt == cpu_to_be32(ISCSI_RESERVED_TAG)) {
>  			/*
>  			 * nop-out in response to target's nop-out rejected.
>  			 * Just resend.
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 14/19] libsas: Remove a set-but-not-used variable

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> This was detected by building with W=1.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/libsas/sas_scsi_host.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index 87e5079d816b..fc90b8c65860 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -855,7 +855,6 @@ int sas_target_alloc(struct scsi_target *starget)
>  int sas_slave_configure(struct scsi_device *scsi_dev)
>  {
>  	struct domain_device *dev = sdev_to_domain_dev(scsi_dev);
> -	struct sas_ha_struct *sas_ha;
>  
>  	BUG_ON(dev->rphy->identify.device_type != SAS_END_DEVICE);
>  
> @@ -864,8 +863,6 @@ int sas_slave_configure(struct scsi_device *scsi_dev)
>  		return 0;
>  	}
>  
> -	sas_ha = dev->port->ha;
> -
>  	sas_read_port_mode_page(scsi_dev);
>  
>  	if (scsi_dev->tagged_supported) {
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 15/19] libsas: Annotate fall-through in a switch statement

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/libsas/sas_ata.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 87f5e694dbed..70be4425ae0b 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -343,6 +343,7 @@ static int smp_ata_check_ready(struct ata_link *link)
>  	case SAS_END_DEVICE:
>  		if (ex_phy->attached_sata_dev)
>  			return sas_ata_clear_pending(dev, ex_phy);
> +		/* fall through */
>  	default:
>  		return -ENODEV;
>  	}
> 
We should be coming to a consensus on how 'fallthrough' is spelled...
Other than that:

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 16/19] scsi_transport_sas, sas_tlr_supported(): Check kzalloc() return value

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> Check whether memory allocation succeeded before dereferencing
> the pointer to the allocated memory.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_transport_sas.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index a318c46db7cc..1a0fa79fd49a 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -423,6 +423,9 @@ sas_tlr_supported(struct scsi_device *sdev)
>  	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
>  	int ret = 0;
>  
> +	if (!buffer)
> +		goto out;
> +
>  	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
>  		goto out;
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 18/19] scsi_debug: Remove a set-but-not-used variable

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> This patch avoids that gcc reports the following warning when
> building with W=1:
> 
> drivers/scsi/scsi_debug.c:2264:15: warning: variable ?pcontrol? set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_debug.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 3be980d47268..77a0335eb757 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -2261,7 +2261,7 @@ static int resp_ie_l_pg(unsigned char * arr)
>  static int resp_log_sense(struct scsi_cmnd * scp,
>                            struct sdebug_dev_info * devip)
>  {
> -	int ppc, sp, pcontrol, pcode, subpcode, alloc_len, len, n;
> +	int ppc, sp, pcode, subpcode, alloc_len, len, n;
>  	unsigned char arr[SDEBUG_MAX_LSENSE_SZ];
>  	unsigned char *cmd = scp->cmnd;
>  
> @@ -2272,7 +2272,6 @@ static int resp_log_sense(struct scsi_cmnd * scp,
>  		mk_sense_invalid_fld(scp, SDEB_IN_CDB, 1, ppc ? 1 : 0);
>  		return check_condition_result;
>  	}
> -	pcontrol = (cmd[2] & 0xc0) >> 6;
>  	pcode = cmd[2] & 0x3f;
>  	subpcode = cmd[3] & 0xff;
>  	alloc_len = get_unaligned_be16(cmd + 7);
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 19/19] iscsi_tcp: Remove a set-but-not-used variable

[Hannes Reinecke]

On 08/23/2017 11:40 PM, Bart Van Assche wrote:
> This patch avoids that gcc reports the following warning when
> building with W=1:
> 
> drivers/scsi/iscsi_tcp.c:166:24: warning: variable ?session? set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Lee Duncan <lduncan@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/iscsi_tcp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 4842fc0e809d..4d934d6c3e13 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -163,7 +163,6 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
>  	struct iscsi_tcp_conn *tcp_conn;
>  	struct iscsi_sw_tcp_conn *tcp_sw_conn;
>  	struct iscsi_conn *conn;
> -	struct iscsi_session *session;
>  	void (*old_state_change)(struct sock *);
>  
>  	read_lock_bh(&sk->sk_callback_lock);
> @@ -172,7 +171,6 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
>  		read_unlock_bh(&sk->sk_callback_lock);
>  		return;
>  	}
> -	session = conn->session;
>  
>  	iscsi_sw_sk_state_check(sk);
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning

[hch]

This looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

On Fri, 2017-08-25 at 17:49 +0200, Hannes Reinecke wrote:
> On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 3d38c6d463b8..5bb15e698969 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -426,7 +426,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
> >  	int vpd_len = SCSI_VPD_PG_LEN;
> >  	int pg80_supported = 0;
> >  	int pg83_supported = 0;
> > -	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> > +	unsigned char *vpd_buf, *orig_vpd_buf = NULL;
> >  
> >  	if (!scsi_device_supports_vpd(sdev))
> >  		return;
> 
> Why drop the __rcu annotation here?
> vpd_buf and orig_vpd_buf should always be accessed via rcu pointers.
> Did I misunderstood the meaning of the __rcu annotation?

Hello Hannes,

The __rcu annotation must only be used for pointers that are accessed from
multiple threads. The vpd_buf pointer is a local variable that is only used
from a single context. The family of rcu_dereference() functions expects an
__rcu-annotated pointer as argument and returns a regular pointer. See e.g.
the example in Documentation/RCU/whatisRCU.txt.

> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 5ed473a87589..cf8a2088a9ba 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -456,8 +456,11 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
> >  	/* NULL queue means the device can't be used */
> >  	sdev->request_queue = NULL;
> >  
> > -	kfree(sdev->vpd_pg83);
> > -	kfree(sdev->vpd_pg80);
> > +	mutex_lock(&sdev->inquiry_mutex);
> > +	kfree(rcu_dereference(sdev->vpd_pg83));
> > +	kfree(rcu_dereference(sdev->vpd_pg80));
> > +	mutex_unlock(&sdev->inquiry_mutex);
> > +
> >  	kfree(sdev->inquiry);
> >  	kfree(sdev);
> 
> As indicated by Shane, I think using 'kfree_rcu()' here might not be a
> bad idea. You never know ...

I will make that change. Thanks for the reviews!

Bart.

Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

On Fri, 2017-08-25 at 05:58 +0000, Seymour, Shane M wrote:
> > From: Bart Van Assche [mailto:Bart.VanAssche@wdc.com]
> > [ ... ]
> > My understanding of the SCSI VPD code is as follows:
> > * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread
> >   updates a VPD buffer while it is being read.
> 
> My understanding is that it doesn't do that - you can update an RCU pointer
> with rcu_assign_pointer() after someone has called rcu_read_lock() and before
> they call rcu_read_unlock(). 

Hello Shane,

You have either misinterpret my statement or the SCSI VPD handling code. If you
have a look at the SCSI VPD handling code you will see that an rcu_read_lock() /
rcu_read_unlock() pair is sufficient to prevent that the VPD buffer rcu_dereference()
points at is being modified as long as the RCU read lock is held, at least if
rcu_dereference() is only called once. The update side namely does not modify the
VPD buffer after the pointer to that buffer has been published.

> It is possible I may be being too picky (it's a personal failing sometimes)
> but is it really that a large overhead to free the RCU pointers in a way that
> RCU pointers are expected to work even if the pointers shouldn't be accessible
> to anything?

Switching to kfree_rcu() requires more changes because all unsigned char pointers
to VPD data have to be converted into pointers to a structure that contains the
VPD data and the RCU head. Anyway, I will convert the kfree() calls to RCU pointers
into kfree_rcu() pointers.

Bart.

RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

> Hello Shane,
> 
> You have either misinterpret my statement or the SCSI VPD handling code. If
> you have a look at the SCSI VPD handling code you will see that an
> rcu_read_lock() /
> rcu_read_unlock() pair is sufficient to prevent that the VPD buffer
> rcu_dereference() points at is being modified as long as the RCU read lock is
> held, at least if
> rcu_dereference() is only called once. The update side namely does not
> modify the VPD buffer after the pointer to that buffer has been published.

Hi Bart,

I'm pretty sure I understood the code. My main point was that the only thing that RCU guarantees with code in between rcu_read_lock()/rcu_read_unlock() if you dereference an RCU pointer using rcu_dereference() is that the memory that the pointer returned points to won't be freed and will be valid regardless of if you get the new or old pointer. Anything related to the actual contents of what the pointer points to is code specific in regard to what happens to it.

As Christoph pointed out (which I failed to consider fully) by doing a direct kfree in scsi_device_dev_release_usercontext() without a call to synchronize_rcu() if you had some code that that got one of the RCU pointers in a read-side critical section and for some reason scsi_device_dev_release_usercontext() got called you could now have an invalid pointer when RCU makes a guarantee that it must be valid. That's what I believe Christoph was discussing in his reply to my email.

> Switching to kfree_rcu() requires more changes because all unsigned char
> pointers to VPD data have to be converted into pointers to a structure that
> contains the VPD data and the RCU head. Anyway, I will convert the kfree()
> calls to RCU pointers into kfree_rcu() pointers.
> 

Thanks, I appreciate you taking the comments onboard. I've been looking at the new patches today and should post something back by the end of my day today.

Shane