[PATCH 0/3] target: Sparse bugfixes and warnings/annotations

[PATCH 0/3] target: Sparse bugfixes and warnings/annotations

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Greetings folks,

This series is a breakout of Fubo's recent sparse patches.  The first
two patches fix real potential deadlocks for sanity check failures in
target_core_device.c, and the third contains the various non-critical
changes and locking annotations.

Thanks again to Fubo for his contribution!

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

Fubo Chen (3):
  target: Drop nacl->device_list_lock on
    core_update_device_list_for_node failure
  target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports
    continue
  target: Minor sparse warning fixes and annotations

 drivers/target/target_core_device.c     |    5 +++++
 drivers/target/target_core_fabric_lib.c |    1 +
 drivers/target/target_core_mib.c        |   20 ++++++++++++++++++++
 drivers/target/target_core_pscsi.c      |    3 +++
 drivers/target/target_core_rd.h         |    2 --
 drivers/target/target_core_transport.c  |    4 +---
 include/target/target_core_base.h       |   22 +++++++++++-----------
 include/target/target_core_transport.h  |    4 ++++
 8 files changed, 45 insertions(+), 16 deletions(-)

[PATCH 1/3] target: Drop nacl->device_list_lock on core_update_device_list_for_node failure

[Nicholas A. Bellinger]

From: Fubo Chen <fubo.chen@gmail.com>

The struct se_node_acl->device_list_lock needs to be released if either
sanity check for struct se_dev_entry->se_lun_acl or deve->se_lun fails.

Signed-off-by: Fubo Chen <fubo.chen@gmail.com>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_device.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 4c2bd9c..95dfe3a 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -391,12 +391,14 @@ int core_update_device_list_for_node(
 				printk(KERN_ERR "struct se_dev_entry->se_lun_acl"
 					" already set for demo mode -> explict"
 					" LUN ACL transition\n");
+				spin_unlock_irq(&nacl->device_list_lock);
 				return -1;
 			}
 			if (deve->se_lun != lun) {
 				printk(KERN_ERR "struct se_dev_entry->se_lun does"
 					" match passed struct se_lun for demo mode"
 					" -> explict LUN ACL transition\n");
+				spin_unlock_irq(&nacl->device_list_lock);
 				return -1;
 			}
 			deve->se_lun_acl = lun_acl;
-- 
1.5.6.5

[PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue

[Nicholas A. Bellinger]

From: Fubo Chen <fubo.chen@gmail.com>

This patch reaquires hba->device_lock and dev->se_port_lock in
se_clear_dev_ports() if lun->lun_se_dev is NULL and we need
to continue in dev->dev_sep_list.

Signed-off-by: Fubo Chen <fubo.chen@gmail.com>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_device.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 95dfe3a..02b835f 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -796,6 +796,8 @@ void se_clear_dev_ports(struct se_device *dev)
 		spin_lock(&lun->lun_sep_lock);
 		if (lun->lun_se_dev == NULL) {
 			spin_unlock(&lun->lun_sep_lock);
+			spin_lock(&hba->device_lock);
+			spin_lock(&dev->se_port_lock);
 			continue;
 		}
 		spin_unlock(&lun->lun_sep_lock);
-- 
1.5.6.5

[PATCH 3/3] target: Minor sparse warning fixes and annotations

[Nicholas A. Bellinger]

From: Fubo Chen <fubo.chen@gmail.com>

This patch addresses the majority of sparse warnings and locking
annotations, and removes a handful of unnecessary struct accessor
macro casts in target_core_base.h.

Signed-off-by: Fubo Chen <fubo.chen@gmail.com>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_device.c     |    1 +
 drivers/target/target_core_fabric_lib.c |    1 +
 drivers/target/target_core_mib.c        |   20 ++++++++++++++++++++
 drivers/target/target_core_pscsi.c      |    3 +++
 drivers/target/target_core_rd.h         |    2 --
 drivers/target/target_core_transport.c  |    4 +---
 include/target/target_core_base.h       |   22 +++++++++++-----------
 include/target/target_core_transport.h  |    4 ++++
 8 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 02b835f..1afddb5 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -586,6 +586,7 @@ static void core_export_port(
  *	Called with struct se_device->se_port_lock spinlock held.
  */
 static void core_release_port(struct se_device *dev, struct se_port *port)
+	__releases(&dev->se_port_lock) __acquires(&dev->se_port_lock)
 {
 	/*
 	 * Wait for any port reference for PR ALL_TG_PT=1 operation
diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 2628564..bddab5d 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -35,6 +35,7 @@
 #include <target/target_core_base.h>
 #include <target/target_core_device.h>
 #include <target/target_core_transport.h>
+#include <target/target_core_fabric_lib.h>
 #include <target/target_core_fabric_ops.h>
 #include <target/target_core_configfs.h>
 
diff --git a/drivers/target/target_core_mib.c b/drivers/target/target_core_mib.c
index d5a48aa..6098d9e 100644
--- a/drivers/target/target_core_mib.c
+++ b/drivers/target/target_core_mib.c
@@ -70,6 +70,7 @@ static inline int list_is_first(const struct list_head *list,
 static void *locate_hba_start(
 	struct seq_file *seq,
 	loff_t *pos)
+	__acquires(&se_global->g_device_lock)
 {
 	spin_lock(&se_global->g_device_lock);
 	return seq_list_start(&se_global->g_se_dev_list, *pos);
@@ -84,6 +85,7 @@ static void *locate_hba_next(
 }
 
 static void locate_hba_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->g_device_lock)
 {
 	spin_unlock(&se_global->g_device_lock);
 }
@@ -98,6 +100,7 @@ static void locate_hba_stop(struct seq_file *seq, void *v)
 static void *scsi_inst_seq_start(
 	struct seq_file *seq,
 	loff_t *pos)
+	__acquires(&se_global->hba_lock)
 {
 	spin_lock(&se_global->hba_lock);
 	return seq_list_start(&se_global->g_hba_list, *pos);
@@ -112,6 +115,7 @@ static void *scsi_inst_seq_next(
 }
 
 static void scsi_inst_seq_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->hba_lock)
 {
 	spin_unlock(&se_global->hba_lock);
 }
@@ -154,6 +158,7 @@ static const struct file_operations scsi_inst_seq_fops = {
  * SCSI Device Table
  */
 static void *scsi_dev_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(&se_global->se_hba_lock)
 {
 	return locate_hba_start(seq, pos);
 }
@@ -164,6 +169,7 @@ static void *scsi_dev_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void scsi_dev_seq_stop(struct seq_file *seq, void *v)
+	 __releases(&se_global->hba_lock)
 {
 	locate_hba_stop(seq, v);
 }
@@ -235,6 +241,7 @@ static const struct file_operations scsi_dev_seq_fops = {
  * SCSI Port Table
  */
 static void *scsi_port_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(&se_global->se_hba_lock)
 {
 	return locate_hba_start(seq, pos);
 }
@@ -245,6 +252,7 @@ static void *scsi_port_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void scsi_port_seq_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->se_hba_lock)
 {
 	locate_hba_stop(seq, v);
 }
@@ -305,6 +313,7 @@ static const struct file_operations scsi_port_seq_fops = {
  * SCSI Transport Table
  */
 static void *scsi_transport_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(&se_global->se_hba_lock)
 {
 	return locate_hba_start(seq, pos);
 }
@@ -315,6 +324,7 @@ static void *scsi_transport_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void scsi_transport_seq_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->se_hba_lock)
 {
 	locate_hba_stop(seq, v);
 }
@@ -390,6 +400,7 @@ static const struct file_operations scsi_transport_seq_fops = {
  * SCSI Target Device Table
  */
 static void *scsi_tgt_dev_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(&se_global->se_hba_lock)
 {
 	return locate_hba_start(seq, pos);
 }
@@ -400,6 +411,7 @@ static void *scsi_tgt_dev_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void scsi_tgt_dev_seq_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->se_hba_lock)
 {
 	locate_hba_stop(seq, v);
 }
@@ -481,6 +493,7 @@ static const struct file_operations scsi_tgt_dev_seq_fops = {
  * SCSI Target Port Table
  */
 static void *scsi_tgt_port_seq_start(struct seq_file *seq, loff_t *pos)
+	 __acquires(&se_global->se_hba_lock)
 {
 	return locate_hba_start(seq, pos);
 }
@@ -491,6 +504,7 @@ static void *scsi_tgt_port_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void scsi_tgt_port_seq_stop(struct seq_file *seq, void *v)
+	 __releases(&se_global->se_hba_lock)
 {
 	locate_hba_stop(seq, v);
 }
@@ -575,6 +589,7 @@ static const struct file_operations scsi_tgt_port_seq_fops = {
  * Iterates through all active TPGs and extracts the info from the ACLs
  */
 static void *scsi_auth_intr_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(&se_global->se_tpg_lock)
 {
 	spin_lock_bh(&se_global->se_tpg_lock);
 	return seq_list_start(&se_global->g_se_tpg_list, *pos);
@@ -587,6 +602,7 @@ static void *scsi_auth_intr_seq_next(struct seq_file *seq, void *v,
 }
 
 static void scsi_auth_intr_seq_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->se_tpg_lock)
 {
 	spin_unlock_bh(&se_global->se_tpg_lock);
 }
@@ -700,6 +716,7 @@ static const struct file_operations scsi_auth_intr_seq_fops = {
  * to list the info fo this table.
  */
 static void *scsi_att_intr_port_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(&se_global->se_tpg_lock)
 {
 	spin_lock_bh(&se_global->se_tpg_lock);
 	return seq_list_start(&se_global->g_se_tpg_list, *pos);
@@ -712,6 +729,7 @@ static void *scsi_att_intr_port_seq_next(struct seq_file *seq, void *v,
 }
 
 static void scsi_att_intr_port_seq_stop(struct seq_file *seq, void *v)
+	__releases(&se_global->se_tpg_lock)
 {
 	spin_unlock_bh(&se_global->se_tpg_lock);
 }
@@ -824,6 +842,7 @@ static const struct file_operations scsi_att_intr_port_seq_fops = {
  * SCSI Logical Unit Table
  */
 static void *scsi_lu_seq_start(struct seq_file *seq, loff_t *pos)
+	 __acquires(&se_global->se_hba_lock)
 {
 	return locate_hba_start(seq, pos);
 }
@@ -834,6 +853,7 @@ static void *scsi_lu_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void scsi_lu_seq_stop(struct seq_file *seq, void *v)
+	 __releases(&se_global->se_hba_lock)
 {
 	locate_hba_stop(seq, v);
 }
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index e795f7d..40c2419 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -442,6 +442,7 @@ static struct se_device *pscsi_create_type_disk(
 	struct pscsi_dev_virt *pdv,
 	struct se_subsystem_dev *se_dev,
 	struct se_hba *hba)
+	__releases(sh->host_lock)
 {
 	struct se_device *dev;
 	struct pscsi_hba_virt *phv = (struct pscsi_hba_virt *)pdv->pdv_se_hba->hba_ptr;
@@ -489,6 +490,7 @@ static struct se_device *pscsi_create_type_rom(
 	struct pscsi_dev_virt *pdv,
 	struct se_subsystem_dev *se_dev,
 	struct se_hba *hba)
+	__releases(sh->host_lock)
 {
 	struct se_device *dev;
 	struct pscsi_hba_virt *phv = (struct pscsi_hba_virt *)pdv->pdv_se_hba->hba_ptr;
@@ -523,6 +525,7 @@ static struct se_device *pscsi_create_type_other(
 	struct pscsi_dev_virt *pdv,
 	struct se_subsystem_dev *se_dev,
 	struct se_hba *hba)
+	__releases(sh->host_lock)
 {
 	struct se_device *dev;
 	struct pscsi_hba_virt *phv = (struct pscsi_hba_virt *)pdv->pdv_se_hba->hba_ptr;
diff --git a/drivers/target/target_core_rd.h b/drivers/target/target_core_rd.h
index 13badfb..3ea19e2 100644
--- a/drivers/target/target_core_rd.h
+++ b/drivers/target/target_core_rd.h
@@ -14,8 +14,6 @@
 #define RD_BLOCKSIZE		512
 #define RD_MAX_SECTORS		1024
 
-extern struct kmem_cache *se_mem_cache;
-
 /* Used in target_core_init_configfs() for virtual LUN 0 access */
 int __init rd_module_init(void);
 void rd_module_exit(void);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8cb628b..5fa5f89 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -229,8 +229,6 @@ static void transport_remove_cmd_from_queue(struct se_cmd *cmd,
 static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
 static void transport_stop_all_task_timers(struct se_cmd *cmd);
 
-int transport_emulate_control_cdb(struct se_task *task);
-
 int init_se_global(void)
 {
 	struct se_global *global;
@@ -4371,7 +4369,7 @@ out:
 	return -1;
 }
 
-extern u32 transport_calc_sg_num(
+u32 transport_calc_sg_num(
 	struct se_task *task,
 	struct se_mem *in_se_mem,
 	u32 task_offset)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index c071907..ba77c11 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -486,8 +486,8 @@ struct se_task {
 	struct list_head t_state_list;
 } ____cacheline_aligned;
 
-#define TASK_CMD(task)	((struct se_cmd *)task->task_se_cmd)
-#define TASK_DEV(task)	((struct se_device *)task->se_dev)
+#define TASK_CMD(task)	((task)->task_se_cmd)
+#define TASK_DEV(task)	((task)->se_dev)
 
 struct se_cmd {
 	/* SAM response code being sent to initiator */
@@ -543,8 +543,8 @@ struct se_cmd {
 	void (*transport_complete_callback)(struct se_cmd *);
 } ____cacheline_aligned;
 
-#define T_TASK(cmd)     ((struct se_transport_task *)(cmd->t_task))
-#define CMD_TFO(cmd) ((struct target_core_fabric_ops *)cmd->se_tfo)
+#define T_TASK(cmd)     ((cmd)->t_task)
+#define CMD_TFO(cmd)	((cmd)->se_tfo)
 
 struct se_tmr_req {
 	/* Task Management function to be preformed */
@@ -611,8 +611,8 @@ struct se_session {
 	struct list_head	sess_acl_list;
 } ____cacheline_aligned;
 
-#define SE_SESS(cmd)		((struct se_session *)(cmd)->se_sess)
-#define SE_NODE_ACL(sess)	((struct se_node_acl *)(sess)->se_node_acl)
+#define SE_SESS(cmd)		((cmd)->se_sess)
+#define SE_NODE_ACL(sess)	((sess)->se_node_acl)
 
 struct se_device;
 struct se_transform_info;
@@ -799,8 +799,8 @@ struct se_device {
 	struct list_head	g_se_dev_list;
 }  ____cacheline_aligned;
 
-#define SE_DEV(cmd)		((struct se_device *)(cmd)->se_lun->lun_se_dev)
-#define SU_DEV(dev)		((struct se_subsystem_dev *)(dev)->se_sub_dev)
+#define SE_DEV(cmd)		((cmd)->se_lun->lun_se_dev)
+#define SU_DEV(dev)		((dev)->se_sub_dev)
 #define DEV_ATTRIB(dev)		(&(dev)->se_sub_dev->se_dev_attrib)
 #define DEV_T10_WWN(dev)	(&(dev)->se_sub_dev->t10_wwn)
 
@@ -829,7 +829,7 @@ struct se_hba {
 	struct se_subsystem_api *transport;
 }  ____cacheline_aligned;
 
-#define SE_HBA(d)		((struct se_hba *)(d)->se_hba)
+#define SE_HBA(dev)		((dev)->se_hba)
 
 struct se_lun {
 	/* See transport_lun_status_table */
@@ -849,7 +849,7 @@ struct se_lun {
 	struct se_port	*lun_sep;
 } ____cacheline_aligned;
 
-#define SE_LUN(c)		((struct se_lun *)(c)->se_lun)
+#define SE_LUN(cmd)		((cmd)->se_lun)
 
 struct se_port {
 	/* RELATIVE TARGET PORT IDENTIFER */
@@ -909,7 +909,7 @@ struct se_portal_group {
 	struct config_group	tpg_param_group;
 } ____cacheline_aligned;
 
-#define TPG_TFO(se_tpg)	((struct target_core_fabric_ops *)(se_tpg)->se_tpg_tfo)
+#define TPG_TFO(se_tpg)	((se_tpg)->se_tpg_tfo)
 
 struct se_wwn {
 	struct target_fabric_configfs *wwn_tf;
diff --git a/include/target/target_core_transport.h b/include/target/target_core_transport.h
index 66f44e5..feaf5d4 100644
--- a/include/target/target_core_transport.h
+++ b/include/target/target_core_transport.h
@@ -109,6 +109,8 @@
 struct se_mem;
 struct se_subsystem_api;
 
+extern struct kmem_cache *se_mem_cache;
+
 extern int init_se_global(void);
 extern void release_se_global(void);
 extern void transport_init_queue_obj(struct se_queue_obj *);
@@ -186,6 +188,8 @@ extern void transport_generic_process_write(struct se_cmd *);
 extern int transport_generic_do_tmr(struct se_cmd *);
 /* From target_core_alua.c */
 extern int core_alua_check_nonop_delay(struct se_cmd *);
+/* From target_core_cdb.c */
+extern int transport_emulate_control_cdb(struct se_task *);
 
 /*
  * Each se_transport_task_t can have N number of possible struct se_task's
-- 
1.5.6.5

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[James Bottomley]

On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> +#define TASK_CMD(task) ((task)->task_se_cmd)
> +#define TASK_DEV(task) ((task)->se_dev)

If sparse is objecting to things like this then sparse needs fixing:
It's decreasing typesafety.  the things being cast are void * ... they'd
be depositable into any pointer whatsoever without the cast.  With the
cast in the #define, we pick up pointer mismatches (as we should).
Without it, we don't.  As long as the define is always a specific type,
it *should* cast to it.

James

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[Nicholas A. Bellinger]

On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > +#define TASK_DEV(task) ((task)->se_dev)
> 
> If sparse is objecting to things like this then sparse needs fixing:
> It's decreasing typesafety.  the things being cast are void * ... they'd
> be depositable into any pointer whatsoever without the cast.  With the
> cast in the #define, we pick up pointer mismatches (as we should).
> Without it, we don't.  As long as the define is always a specific type,
> it *should* cast to it.
> 

Hmmm, good point..  In that case I will go ahead and drop this part of
the patch.

Thanks!

--nab

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[James Bottomley]

On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > +#define TASK_DEV(task) ((task)->se_dev)
> > 
> > If sparse is objecting to things like this then sparse needs fixing:
> > It's decreasing typesafety.  the things being cast are void * ... they'd
> > be depositable into any pointer whatsoever without the cast.  With the
> > cast in the #define, we pick up pointer mismatches (as we should).
> > Without it, we don't.  As long as the define is always a specific type,
> > it *should* cast to it.
> > 
> 
> Hmmm, good point..  In that case I will go ahead and drop this part of
> the patch.

Actually, I misspoke on this.  They're not void *; they're defined as
struct pointers ... so the cast is actually a spurious double cast.  As
long as the rest are, I'm fine with this.

James

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[Nicholas A. Bellinger]

On Mon, 2011-01-24 at 15:51 -0600, James Bottomley wrote:
> On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > > +#define TASK_DEV(task) ((task)->se_dev)
> > > 
> > > If sparse is objecting to things like this then sparse needs fixing:
> > > It's decreasing typesafety.  the things being cast are void * ... they'd
> > > be depositable into any pointer whatsoever without the cast.  With the
> > > cast in the #define, we pick up pointer mismatches (as we should).
> > > Without it, we don't.  As long as the define is always a specific type,
> > > it *should* cast to it.
> > > 
> > 
> > Hmmm, good point..  In that case I will go ahead and drop this part of
> > the patch.
> 
> Actually, I misspoke on this.  They're not void *; they're defined as
> struct pointers ... so the cast is actually a spurious double cast.  As
> long as the rest are, I'm fine with this.
> 

Committed as seperate commit b58b76c -> lio-core-2.6.git/linus-38-rc2,
and picked into the mainline queue @ scsi-post-merge-2.6.git/for-jejb.

Thanks!

--nab

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[Joe Eykholt]

On 1/24/11 1:33 PM, Nicholas A. Bellinger wrote:
> On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
>> On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
>>> -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
>>> -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
>>> +#define TASK_CMD(task) ((task)->task_se_cmd)
>>> +#define TASK_DEV(task) ((task)->se_dev)
Part of the problem with the old code is that task was not parenthesized,
so if TASK_CMD() were used with an expression, you might not get what
you want.  If you did TASK_CMD(p + 5), for example, you would get

     ((struct se_cmd *)p + 5->task_se_cmd)

Which wouldn't compile, but maybe other examples would compile and
would cause strange problems.   So, it's a bad macro as it is.
Just my 2 cents.

      Cheers,
      Joe

>>> If sparse is objecting to things like this then sparse needs fixing:
>>> It's decreasing typesafety.  the things being cast are void * ... they'd
>>> be depositable into any pointer whatsoever without the cast.  With the
>>> cast in the #define, we pick up pointer mismatches (as we should).
>>> Without it, we don't.  As long as the define is always a specific type,
>>> it *should* cast to it.
>>>
>
> Hmmm, good point..  In that case I will go ahead and drop this part of
> the patch.
>
> Thanks!
>
> --nab
>

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[Nicholas A. Bellinger]

On Mon, 2011-01-24 at 15:18 -0800, Joe Eykholt wrote:
> On 1/24/11 1:33 PM, Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> >> On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> >>> -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> >>> -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> >>> +#define TASK_CMD(task) ((task)->task_se_cmd)
> >>> +#define TASK_DEV(task) ((task)->se_dev)
> Part of the problem with the old code is that task was not parenthesized,
> so if TASK_CMD() were used with an expression, you might not get what
> you want.  If you did TASK_CMD(p + 5), for example, you would get
> 
>      ((struct se_cmd *)p + 5->task_se_cmd)
> 
> Which wouldn't compile, but maybe other examples would compile and
> would cause strange problems.   So, it's a bad macro as it is.
> Just my 2 cents.
> 

Good point here Joe..  AFAIK there are no cases of this in actual usage,
but for good measure all of these within target_core_base.h where
converted to:

 	#define TCM_FOO(p)  ((p)->member)

for:

commit b58b76cd21bf461308e7fba175931f8f8c089bd7
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date:   Mon Jan 24 14:29:08 2011 -0800

    target: Remove spurious double cast from structure macro accessors


Thanks for your comments!

--nab

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[Stefan Richter]

On Jan 24 Nicholas A. Bellinger wrote:
> On Mon, 2011-01-24 at 15:51 -0600, James Bottomley wrote:
> > On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> > > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > > > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > > > +#define TASK_DEV(task) ((task)->se_dev)
> > > > 
> > > > If sparse is objecting to things like this then sparse needs fixing:
> > > > It's decreasing typesafety.  the things being cast are void * ... they'd
> > > > be depositable into any pointer whatsoever without the cast.  With the
> > > > cast in the #define, we pick up pointer mismatches (as we should).
> > > > Without it, we don't.  As long as the define is always a specific type,
> > > > it *should* cast to it.
> > > > 
> > > 
> > > Hmmm, good point..  In that case I will go ahead and drop this part of
> > > the patch.
> > 
> > Actually, I misspoke on this.  They're not void *; they're defined as
> > struct pointers ... so the cast is actually a spurious double cast.  As
> > long as the rest are, I'm fine with this.
> > 
> 
> Committed as seperate commit b58b76c -> lio-core-2.6.git/linus-38-rc2,
> and picked into the mainline queue @ scsi-post-merge-2.6.git/for-jejb.

Why do you provide macros for a simple structure member dereference?

And if you *really* need these helpers, why not

static inline struct se_cmd *cmd_of(struct se_task *task)
{
	return task->task_se_cmd;
}

(But then, I see no reason at all not to write it as task->task_se_cmd at
the user sites.)
-- 
Stefan Richter
-=====-==-== ---= ==--=
http://arcgraph.de/sr/

Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue

[Stefan Richter]

On Jan 24 Nicholas A. Bellinger wrote:
> From: Fubo Chen <fubo.chen@gmail.com>
> 
> This patch reaquires hba->device_lock and dev->se_port_lock in
> se_clear_dev_ports() if lun->lun_se_dev is NULL and we need
> to continue in dev->dev_sep_list.
> 
> Signed-off-by: Fubo Chen <fubo.chen@gmail.com>
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/target/target_core_device.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 95dfe3a..02b835f 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -796,6 +796,8 @@ void se_clear_dev_ports(struct se_device *dev)
>  		spin_lock(&lun->lun_sep_lock);
>  		if (lun->lun_se_dev == NULL) {
>  			spin_unlock(&lun->lun_sep_lock);
> +			spin_lock(&hba->device_lock);
> +			spin_lock(&dev->se_port_lock);
>  			continue;
>  		}
>  		spin_unlock(&lun->lun_sep_lock);

The patch might be OK.  But the code that it fixed is... stunning.

void se_clear_dev_ports(struct se_device *dev)
{
	struct se_hba *hba = dev->se_hba;
	struct se_lun *lun;
	struct se_portal_group *tpg;
	struct se_port *sep, *sep_tmp;

	spin_lock(&dev->se_port_lock);
	list_for_each_entry_safe(sep, sep_tmp, &dev->dev_sep_list, sep_list) {
		spin_unlock(&dev->se_port_lock);
		spin_unlock(&hba->device_lock);

		lun = sep->sep_lun;
		tpg = sep->sep_tpg;
		spin_lock(&lun->lun_sep_lock);
		if (lun->lun_se_dev == NULL) {
			spin_unlock(&lun->lun_sep_lock);
			continue;
		}
		spin_unlock(&lun->lun_sep_lock);

		core_dev_del_lun(tpg, lun->unpacked_lun);

		spin_lock(&hba->device_lock);
		spin_lock(&dev->se_port_lock);
	}
	spin_unlock(&dev->se_port_lock);

	return;
}

Can this mess of releasing and reacquiring locks --- which looks all rather
dangerous --- be cleaned up if you move the logical units (?) to be deleted
over to a secondary list?
-- 
Stefan Richter
-=====-==-== ---= ==--=
http://arcgraph.de/sr/

Re: [PATCH 3/3] target: Minor sparse warning fixes and annotations

[Nicholas A. Bellinger]

On Tue, 2011-01-25 at 00:56 +0100, Stefan Richter wrote:
> On Jan 24 Nicholas A. Bellinger wrote:
> > On Mon, 2011-01-24 at 15:51 -0600, James Bottomley wrote:
> > > On Mon, 2011-01-24 at 13:33 -0800, Nicholas A. Bellinger wrote:
> > > > On Mon, 2011-01-24 at 14:56 -0600, James Bottomley wrote:
> > > > > On Mon, 2011-01-24 at 12:37 -0800, Nicholas A. Bellinger wrote:
> > > > > > -#define TASK_CMD(task) ((struct se_cmd *)task->task_se_cmd)
> > > > > > -#define TASK_DEV(task) ((struct se_device *)task->se_dev)
> > > > > > +#define TASK_CMD(task) ((task)->task_se_cmd)
> > > > > > +#define TASK_DEV(task) ((task)->se_dev)
> > > > > 
> > > > > If sparse is objecting to things like this then sparse needs fixing:
> > > > > It's decreasing typesafety.  the things being cast are void * ... they'd
> > > > > be depositable into any pointer whatsoever without the cast.  With the
> > > > > cast in the #define, we pick up pointer mismatches (as we should).
> > > > > Without it, we don't.  As long as the define is always a specific type,
> > > > > it *should* cast to it.
> > > > > 
> > > > 
> > > > Hmmm, good point..  In that case I will go ahead and drop this part of
> > > > the patch.
> > > 
> > > Actually, I misspoke on this.  They're not void *; they're defined as
> > > struct pointers ... so the cast is actually a spurious double cast.  As
> > > long as the rest are, I'm fine with this.
> > > 
> > 
> > Committed as seperate commit b58b76c -> lio-core-2.6.git/linus-38-rc2,
> > and picked into the mainline queue @ scsi-post-merge-2.6.git/for-jejb.
> 
> Why do you provide macros for a simple structure member dereference?
> 
> And if you *really* need these helpers, why not
> 
> static inline struct se_cmd *cmd_of(struct se_task *task)
> {
> 	return task->task_se_cmd;
> }
> 
> (But then, I see no reason at all not to write it as task->task_se_cmd at
> the user sites.)

Most of these accessor macros originally came from the usage of more
than a single pointer deference.  These days only DEV_ATTRIB and
DEV_T10_WWN still deference multiple pointers, so it's really just a
matter of style in modern v4.0 code.

So my perference would be against using inlined cmd_of() callers, and
just access things directly.  Btw at this point this type of cleanup
would be considered for-39 material.

--nab

Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue

[Nicholas A. Bellinger]

On Tue, 2011-01-25 at 01:08 +0100, Stefan Richter wrote:
> On Jan 24 Nicholas A. Bellinger wrote:
> > From: Fubo Chen <fubo.chen@gmail.com>
> > 
> > This patch reaquires hba->device_lock and dev->se_port_lock in
> > se_clear_dev_ports() if lun->lun_se_dev is NULL and we need
> > to continue in dev->dev_sep_list.
> > 
> > Signed-off-by: Fubo Chen <fubo.chen@gmail.com>
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/target/target_core_device.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> > index 95dfe3a..02b835f 100644
> > --- a/drivers/target/target_core_device.c
> > +++ b/drivers/target/target_core_device.c
> > @@ -796,6 +796,8 @@ void se_clear_dev_ports(struct se_device *dev)
> >  		spin_lock(&lun->lun_sep_lock);
> >  		if (lun->lun_se_dev == NULL) {
> >  			spin_unlock(&lun->lun_sep_lock);
> > +			spin_lock(&hba->device_lock);
> > +			spin_lock(&dev->se_port_lock);
> >  			continue;
> >  		}
> >  		spin_unlock(&lun->lun_sep_lock);
> 
> The patch might be OK.  But the code that it fixed is... stunning.
> 
> void se_clear_dev_ports(struct se_device *dev)
> {
> 	struct se_hba *hba = dev->se_hba;
> 	struct se_lun *lun;
> 	struct se_portal_group *tpg;
> 	struct se_port *sep, *sep_tmp;
> 
> 	spin_lock(&dev->se_port_lock);
> 	list_for_each_entry_safe(sep, sep_tmp, &dev->dev_sep_list, sep_list) {
> 		spin_unlock(&dev->se_port_lock);
> 		spin_unlock(&hba->device_lock);
> 
> 		lun = sep->sep_lun;
> 		tpg = sep->sep_tpg;
> 		spin_lock(&lun->lun_sep_lock);
> 		if (lun->lun_se_dev == NULL) {
> 			spin_unlock(&lun->lun_sep_lock);
> 			continue;
> 		}
> 		spin_unlock(&lun->lun_sep_lock);
> 
> 		core_dev_del_lun(tpg, lun->unpacked_lun);
> 
> 		spin_lock(&hba->device_lock);
> 		spin_lock(&dev->se_port_lock);
> 	}
> 	spin_unlock(&dev->se_port_lock);
> 
> 	return;
> }
> 
> Can this mess of releasing and reacquiring locks --- which looks all rather
> dangerous --- be cleaned up if you move the logical units (?) to be deleted
> over to a secondary list?

Fair point on this one.  Having to sleep in core_dev_del_lun() waiting
for all outstanding struct se_cmd -> struct se_lun I/O descriptor
mappings to be shutdown makes this look pretty ugly currently in
se_clear_dev_ports().  However adding another list+lock to this mix
really does not make it any less complex.

Looking at se_clear_dev_ports() again in the two usage contexts
target_core_device.c:se_free_virtual_device() and
target_core_hba.c:core_delete_hba(), I am thinking now that
se_clear_dev_ports() is in fact a genuine piece of left-over legacy
shutdown (eg: IOCTL) cruft from the pre-configfs 'dark ages' where TCM
backend device + port LUN removal was not guaranteed by Linux/VFS (and
hence the extra shutdown logic hacks).

Because TPG port / LUN shutdown from backend HBA+devices is *always*
done via a configfs unlink syscall on parent/child protected struct
config_group structures, I think se_clear_dev_ports() should be able to
be dropped all together now.  I will take a deeper look and see if this
is really in fact safe for v4.0/for-38 code.

Thanks for your comments!

--nab

Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue

[Nicholas A. Bellinger]

On Mon, 2011-01-24 at 17:20 -0800, Nicholas A. Bellinger wrote:
> On Tue, 2011-01-25 at 01:08 +0100, Stefan Richter wrote:
> > On Jan 24 Nicholas A. Bellinger wrote:
> > > From: Fubo Chen <fubo.chen@gmail.com>
> > > 
> > > This patch reaquires hba->device_lock and dev->se_port_lock in
> > > se_clear_dev_ports() if lun->lun_se_dev is NULL and we need
> > > to continue in dev->dev_sep_list.
> > > 
> > > Signed-off-by: Fubo Chen <fubo.chen@gmail.com>
> > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > > ---
> > >  drivers/target/target_core_device.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> > > index 95dfe3a..02b835f 100644
> > > --- a/drivers/target/target_core_device.c
> > > +++ b/drivers/target/target_core_device.c
> > > @@ -796,6 +796,8 @@ void se_clear_dev_ports(struct se_device *dev)
> > >  		spin_lock(&lun->lun_sep_lock);
> > >  		if (lun->lun_se_dev == NULL) {
> > >  			spin_unlock(&lun->lun_sep_lock);
> > > +			spin_lock(&hba->device_lock);
> > > +			spin_lock(&dev->se_port_lock);
> > >  			continue;
> > >  		}
> > >  		spin_unlock(&lun->lun_sep_lock);
> > 
> > The patch might be OK.  But the code that it fixed is... stunning.
> > 
> > void se_clear_dev_ports(struct se_device *dev)
> > {
> > 	struct se_hba *hba = dev->se_hba;
> > 	struct se_lun *lun;
> > 	struct se_portal_group *tpg;
> > 	struct se_port *sep, *sep_tmp;
> > 
> > 	spin_lock(&dev->se_port_lock);
> > 	list_for_each_entry_safe(sep, sep_tmp, &dev->dev_sep_list, sep_list) {
> > 		spin_unlock(&dev->se_port_lock);
> > 		spin_unlock(&hba->device_lock);
> > 
> > 		lun = sep->sep_lun;
> > 		tpg = sep->sep_tpg;
> > 		spin_lock(&lun->lun_sep_lock);
> > 		if (lun->lun_se_dev == NULL) {
> > 			spin_unlock(&lun->lun_sep_lock);
> > 			continue;
> > 		}
> > 		spin_unlock(&lun->lun_sep_lock);
> > 
> > 		core_dev_del_lun(tpg, lun->unpacked_lun);
> > 
> > 		spin_lock(&hba->device_lock);
> > 		spin_lock(&dev->se_port_lock);
> > 	}
> > 	spin_unlock(&dev->se_port_lock);
> > 
> > 	return;
> > }
> > 
> > Can this mess of releasing and reacquiring locks --- which looks all rather
> > dangerous --- be cleaned up if you move the logical units (?) to be deleted
> > over to a secondary list?
> 
> Fair point on this one.  Having to sleep in core_dev_del_lun() waiting
> for all outstanding struct se_cmd -> struct se_lun I/O descriptor
> mappings to be shutdown makes this look pretty ugly currently in
> se_clear_dev_ports().  However adding another list+lock to this mix
> really does not make it any less complex.
> 
> Looking at se_clear_dev_ports() again in the two usage contexts
> target_core_device.c:se_free_virtual_device() and
> target_core_hba.c:core_delete_hba(), I am thinking now that
> se_clear_dev_ports() is in fact a genuine piece of left-over legacy
> shutdown (eg: IOCTL) cruft from the pre-configfs 'dark ages' where TCM
> backend device + port LUN removal was not guaranteed by Linux/VFS (and
> hence the extra shutdown logic hacks).
> 
> Because TPG port / LUN shutdown from backend HBA+devices is *always*
> done via a configfs unlink syscall on parent/child protected struct
> config_group structures, I think se_clear_dev_ports() should be able to
> be dropped all together now.  I will take a deeper look and see if this
> is really in fact safe for v4.0/for-38 code.
> 

Ok, after a quick test w/o se_clear_dev_ports() @ struct se_device and
struct se_hba configfs context shutdown callers using iSCSI target
TargetName+TargetPortalGrouPTag endpoints with normal and demo mode TPG
LUNs, everything appears to be functioning as expected with the inline
patch below.

I am going to do some more testing and code review before pushing into
the upstream LIO tree, but I am pretty confident at this point with the
last assessment of this code being pre-configfs + pre-parent/child VFS
protected port shutdown cruft that can safely be dropped with modern
target_core_fabric_configfs.c code.

Best Regards,

--nab

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 1afddb5..9220dba 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -777,52 +777,12 @@ void se_release_vpd_for_dev(struct se_device *dev)
        return;
 }
 
-/*
- * Called with struct se_hba->device_lock held.
- */
-void se_clear_dev_ports(struct se_device *dev)
-{
-       struct se_hba *hba = dev->se_hba;
-       struct se_lun *lun;
-       struct se_portal_group *tpg;
-       struct se_port *sep, *sep_tmp;
-
-       spin_lock(&dev->se_port_lock);
-       list_for_each_entry_safe(sep, sep_tmp, &dev->dev_sep_list, sep_list) {
-               spin_unlock(&dev->se_port_lock);
-               spin_unlock(&hba->device_lock);
-
-               lun = sep->sep_lun;
-               tpg = sep->sep_tpg;
-               spin_lock(&lun->lun_sep_lock);
-               if (lun->lun_se_dev == NULL) {
-                       spin_unlock(&lun->lun_sep_lock);
-                       spin_lock(&hba->device_lock);
-                       spin_lock(&dev->se_port_lock);
-                       continue;
-               }
-               spin_unlock(&lun->lun_sep_lock);
-
-               core_dev_del_lun(tpg, lun->unpacked_lun);
-
-               spin_lock(&hba->device_lock);
-               spin_lock(&dev->se_port_lock);
-       }
-       spin_unlock(&dev->se_port_lock);
-
-       return;
-}
-
 /*     se_free_virtual_device():
  *
  *     Used for IBLOCK, RAMDISK, and FILEIO Transport Drivers.
  */
 int se_free_virtual_device(struct se_device *dev, struct se_hba *hba)
 {
-       spin_lock(&hba->device_lock);
-       se_clear_dev_ports(dev);
-       spin_unlock(&hba->device_lock);
-
        core_alua_free_lu_gp_mem(dev);
        se_release_device_for_hba(dev);
 
diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
index a99760a..1232da9 100644
--- a/drivers/target/target_core_hba.c
+++ b/drivers/target/target_core_hba.c
@@ -157,8 +157,6 @@ core_delete_hba(struct se_hba *hba)
 
        spin_lock(&hba->device_lock);
        list_for_each_entry_safe(dev, dev_tmp, &hba->hba_dev_list, dev_list) {
-
-               se_clear_dev_ports(dev);
                spin_unlock(&hba->device_lock);
 
                se_release_device_for_hba(dev);

Re: [PATCH 2/3] target: Reaquire hba_lock + se_port_lock during se_clear_dev_ports continue

[Stefan Richter]

On Jan 24 Nicholas A. Bellinger wrote:
> On Tue, 2011-01-25 at 01:08 +0100, Stefan Richter wrote:
> > void se_clear_dev_ports(struct se_device *dev)
[...]
> > Can this mess of releasing and reacquiring locks --- which looks all rather
> > dangerous --- be cleaned up if you move the logical units (?) to be deleted
> > over to a secondary list?
> 
> Fair point on this one.  Having to sleep in core_dev_del_lun() waiting
> for all outstanding struct se_cmd -> struct se_lun I/O descriptor
> mappings to be shutdown makes this look pretty ugly currently in
> se_clear_dev_ports().  However adding another list+lock to this mix
> really does not make it any less complex.

I meant an on-stack throwaway list which you can change inside
se_clear_dev_ports() without any lock.  But I didn't look whether this is
actually possible and beneficial.

> Looking at se_clear_dev_ports() again in the two usage contexts
> target_core_device.c:se_free_virtual_device() and
> target_core_hba.c:core_delete_hba(),
[...]
> config_group structures, I think se_clear_dev_ports() should be able to
> be dropped all together now.

That of course sounds great.

> I will take a deeper look and see if this
> is really in fact safe for v4.0/for-38 code.

As a thought from an outsider:  If drivers/target/ weren't entirely new in
2.6.38, then only a minimal cautious locking fix would be post -rc1
material.  Linus releases so frequently that anything beyond essential
fixes can easily wait for regular merge windows.  "Essential" = very low
risk : reward ratio && with reasonable reward.  Those who directly work
with the code tend to underestimate risk and to overestimate reward. ;-)
-- 
Stefan Richter
-=====-==-== ---= ==--=
http://arcgraph.de/sr/