All of lore.kernel.org
 help / color / mirror / Atom feed
* iscsi bugfixes and cleanups
@ 2009-04-01 18:11 michaelc
  2009-04-01 18:11 ` [PATCH 1/7] cxgb3i - subscribe to error notification from cxgb3 driver michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi

The following patches fix a bug in cxgb3i handling of card reset,
fix up the the fix up for handling iscsi pool allocation failures,
and then there is some cleanups. cxgb3i was exporting some functions
that are not needed.

Please apply for the feature window if possible because it has
the cleanups in there. If this misses the feature window I will
rebuild the patchset to only include fixes for the rc kernels.

Patchset was made over linus's tree, but builds and applies over
scsi-misc that was updated today.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/7] cxgb3i - subscribe to error notification from cxgb3 driver
  2009-04-01 18:11 iscsi bugfixes and cleanups michaelc
@ 2009-04-01 18:11 ` michaelc
  2009-04-01 18:11   ` [PATCH 2/7] cxgb3i - re-initialize ddp settings after chip reset michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Karen Xie, Mike Christie

From: Karen Xie <kxie@chelsio.com>

[PATCH 2/5 2.6.30] cxgb3i - subscribe to error notification from cxgb3 driver

From: Karen Xie <kxie@chelsio.com>

Add error notification handling function which is called during chip reset.

Signed-off-by: Karen Xie <kxie@chelsio.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/cxgb3i/cxgb3i.h       |   10 ++++++----
 drivers/scsi/cxgb3i/cxgb3i_init.c  |   25 +++++++++++++++++++++++--
 drivers/scsi/cxgb3i/cxgb3i_iscsi.c |   27 +++++++++++++++++++++++----
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/cxgb3i/cxgb3i.h b/drivers/scsi/cxgb3i/cxgb3i.h
index a7cf550..0942227 100644
--- a/drivers/scsi/cxgb3i/cxgb3i.h
+++ b/drivers/scsi/cxgb3i/cxgb3i.h
@@ -66,10 +66,12 @@ struct cxgb3i_hba {
  * @pdev:	pointer to pci dev
  * @hba_cnt:	# of hbas (the same as # of ports)
  * @hba:	all the hbas on this adapter
+ * @flags:	bit flag for adapter event/status
  * @tx_max_size: max. tx packet size supported
  * @rx_max_size: max. rx packet size supported
  * @tag_format: ddp tag format settings
  */
+#define CXGB3I_ADAPTER_FLAG_RESET	0x1
 struct cxgb3i_adapter {
 	struct list_head list_head;
 	spinlock_t lock;
@@ -78,6 +80,7 @@ struct cxgb3i_adapter {
 	unsigned char hba_cnt;
 	struct cxgb3i_hba *hba[MAX_NPORTS];
 
+	unsigned int flags;
 	unsigned int tx_max_size;
 	unsigned int rx_max_size;
 
@@ -137,10 +140,9 @@ struct cxgb3i_task_data {
 int cxgb3i_iscsi_init(void);
 void cxgb3i_iscsi_cleanup(void);
 
-struct cxgb3i_adapter *cxgb3i_adapter_add(struct t3cdev *);
-void cxgb3i_adapter_remove(struct t3cdev *);
-int cxgb3i_adapter_ulp_init(struct cxgb3i_adapter *);
-void cxgb3i_adapter_ulp_cleanup(struct cxgb3i_adapter *);
+struct cxgb3i_adapter *cxgb3i_adapter_find_by_tdev(struct t3cdev *);
+struct cxgb3i_adapter *cxgb3i_adapter_open(struct t3cdev *);
+void cxgb3i_adapter_close(struct t3cdev *);
 
 struct cxgb3i_hba *cxgb3i_hba_find_by_netdev(struct net_device *);
 struct cxgb3i_hba *cxgb3i_hba_host_add(struct cxgb3i_adapter *,
diff --git a/drivers/scsi/cxgb3i/cxgb3i_init.c b/drivers/scsi/cxgb3i/cxgb3i_init.c
index 1ce9f24..833dbfa 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_init.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_init.c
@@ -26,6 +26,7 @@ MODULE_VERSION(DRV_MODULE_VERSION);
 
 static void open_s3_dev(struct t3cdev *);
 static void close_s3_dev(struct t3cdev *);
+static void s3_err_handler(struct t3cdev *tdev, u32 status, u32 error);
 
 static cxgb3_cpl_handler_func cxgb3i_cpl_handlers[NUM_CPL_CMDS];
 static struct cxgb3_client t3c_client = {
@@ -33,6 +34,7 @@ static struct cxgb3_client t3c_client = {
 	.handlers = cxgb3i_cpl_handlers,
 	.add = open_s3_dev,
 	.remove = close_s3_dev,
+	.err_handler = s3_err_handler,
 };
 
 /**
@@ -49,7 +51,7 @@ static void open_s3_dev(struct t3cdev *t3dev)
 	}
 
 	cxgb3i_sdev_add(t3dev, &t3c_client);
-	cxgb3i_adapter_add(t3dev);
+	cxgb3i_adapter_open(t3dev);
 }
 
 /**
@@ -58,10 +60,29 @@ static void open_s3_dev(struct t3cdev *t3dev)
  */
 static void close_s3_dev(struct t3cdev *t3dev)
 {
-	cxgb3i_adapter_remove(t3dev);
+	cxgb3i_adapter_close(t3dev);
 	cxgb3i_sdev_remove(t3dev);
 }
 
+static void s3_err_handler(struct t3cdev *tdev, u32 status, u32 error)
+{
+	struct cxgb3i_adapter *snic = cxgb3i_adapter_find_by_tdev(tdev);
+
+	cxgb3i_log_info("snic 0x%p, tdev 0x%p, status 0x%x, err 0x%x.\n",
+			snic, tdev, status, error);
+	if (!snic)
+		return;
+
+	switch (status) {
+	case OFFLOAD_STATUS_DOWN:
+		snic->flags |= CXGB3I_ADAPTER_FLAG_RESET;
+		break;
+	case OFFLOAD_STATUS_UP:
+		snic->flags &= ~CXGB3I_ADAPTER_FLAG_RESET;
+		break;
+	}
+}
+
 /**
  * cxgb3i_init_module - module init entry point
  *
diff --git a/drivers/scsi/cxgb3i/cxgb3i_iscsi.c b/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
index e185ded..ff6bfd6 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
@@ -53,11 +53,30 @@ static LIST_HEAD(cxgb3i_snic_list);
 static DEFINE_RWLOCK(cxgb3i_snic_rwlock);
 
 /**
- * cxgb3i_adapter_add - init a s3 adapter structure and any h/w settings
+ * cxgb3i_adpater_find_by_tdev - find the cxgb3i_adapter structure via t3cdev
+ * @tdev: t3cdev pointer
+ */
+struct cxgb3i_adapter *cxgb3i_adapter_find_by_tdev(struct t3cdev *tdev)
+{
+	struct cxgb3i_adapter *snic;
+
+	read_lock(&cxgb3i_snic_rwlock);
+	list_for_each_entry(snic, &cxgb3i_snic_list, list_head) {
+		if (snic->tdev == tdev) {
+			read_unlock(&cxgb3i_snic_rwlock);
+			return snic;
+		}
+	}
+	read_unlock(&cxgb3i_snic_rwlock);
+	return NULL;
+}
+
+/**
+ * cxgb3i_adapter_open - init a s3 adapter structure and any h/w settings
  * @t3dev: t3cdev adapter
  * return the resulting cxgb3i_adapter struct
  */
-struct cxgb3i_adapter *cxgb3i_adapter_add(struct t3cdev *t3dev)
+struct cxgb3i_adapter *cxgb3i_adapter_open(struct t3cdev *t3dev)
 {
 	struct cxgb3i_adapter *snic;
 	struct adapter *adapter = tdev2adap(t3dev);
@@ -101,10 +120,10 @@ free_snic:
 }
 
 /**
- * cxgb3i_adapter_remove - release the resources held and cleanup h/w settings
+ * cxgb3i_adapter_close - release the resources held and cleanup h/w settings
  * @t3dev: t3cdev adapter
  */
-void cxgb3i_adapter_remove(struct t3cdev *t3dev)
+void cxgb3i_adapter_close(struct t3cdev *t3dev)
 {
 	int i;
 	struct cxgb3i_adapter *snic;
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/7] cxgb3i - re-initialize ddp settings after chip reset
  2009-04-01 18:11 ` [PATCH 1/7] cxgb3i - subscribe to error notification from cxgb3 driver michaelc
@ 2009-04-01 18:11   ` michaelc
  2009-04-01 18:11     ` [PATCH 3/7] cxgb3i - re-read ddp settings information " michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Karen Xie, Mike Christie

From: Karen Xie <kxie@chelsio.com>

[PATCH 3/5 2.6.30] cxgb3i - re-initialize ddp settings after chip reset

From: Karen Xie <kxie@chelsio.com>

Re-initialize the ddp settings after chip reset. It includes re-initialize
the related registers and the ddp map.

Signed-off-by: Karen Xie <kxie@chelsio.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/cxgb3i/cxgb3i_ddp.c |  242 ++++++++++++++++++--------------------
 drivers/scsi/cxgb3i/cxgb3i_ddp.h |    3 +-
 2 files changed, 118 insertions(+), 127 deletions(-)

diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.c b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
index 4eb6f55..bb1eebf 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
@@ -66,9 +66,6 @@ static unsigned char ddp_page_order[DDP_PGIDX_MAX] = {0, 1, 2, 4};
 static unsigned char ddp_page_shift[DDP_PGIDX_MAX] = {12, 13, 14, 16};
 static unsigned char page_idx = DDP_PGIDX_MAX;
 
-static LIST_HEAD(cxgb3i_ddp_list);
-static DEFINE_RWLOCK(cxgb3i_ddp_rwlock);
-
 /*
  * functions to program the pagepod in h/w
  */
@@ -113,8 +110,8 @@ static int set_ddp_map(struct cxgb3i_ddp_info *ddp, struct pagepod_hdr *hdr,
 	return 0;
 }
 
-static int clear_ddp_map(struct cxgb3i_ddp_info *ddp, unsigned int idx,
-			 unsigned int npods)
+static void clear_ddp_map(struct cxgb3i_ddp_info *ddp, unsigned int tag,
+			 unsigned int idx, unsigned int npods)
 {
 	unsigned int pm_addr = (idx << PPOD_SIZE_SHIFT) + ddp->llimit;
 	int i;
@@ -122,13 +119,17 @@ static int clear_ddp_map(struct cxgb3i_ddp_info *ddp, unsigned int idx,
 	for (i = 0; i < npods; i++, idx++, pm_addr += PPOD_SIZE) {
 		struct sk_buff *skb = ddp->gl_skb[idx];
 
+		if (!skb) {
+			ddp_log_error("ddp tag 0x%x, 0x%x, %d/%u, skb NULL.\n",
+					tag, idx, i, npods);
+			continue;
+		}
 		ddp->gl_skb[idx] = NULL;
 		memset((skb->head + sizeof(struct ulp_mem_io)), 0, PPOD_SIZE);
 		ulp_mem_io_set_hdr(skb, pm_addr);
 		skb->priority = CPL_PRIORITY_CONTROL;
 		cxgb3_ofld_send(ddp->tdev, skb);
 	}
-	return 0;
 }
 
 static inline int ddp_find_unused_entries(struct cxgb3i_ddp_info *ddp,
@@ -453,15 +454,15 @@ void cxgb3i_ddp_tag_release(struct t3cdev *tdev, u32 tag)
 		struct cxgb3i_gather_list *gl = ddp->gl_map[idx];
 		unsigned int npods;
 
-		if (!gl) {
-			ddp_log_error("release ddp 0x%x, idx 0x%x, gl NULL.\n",
-				      tag, idx);
+		if (!gl || !gl->nelem) {
+			ddp_log_error("release 0x%x, idx 0x%x, gl 0x%p, %u.\n",
+				      tag, idx, gl, gl ? gl->nelem : 0);
 			return;
 		}
 		npods = (gl->nelem + PPOD_PAGES_MAX - 1) >> PPOD_PAGES_SHIFT;
 		ddp_log_debug("ddp tag 0x%x, release idx 0x%x, npods %u.\n",
 			      tag, idx, npods);
-		clear_ddp_map(ddp, idx, npods);
+		clear_ddp_map(ddp, tag, idx, npods);
 		ddp_unmark_entries(ddp, idx, npods);
 		cxgb3i_ddp_release_gl(gl, ddp->pdev);
 	} else
@@ -564,7 +565,87 @@ int cxgb3i_setup_conn_digest(struct t3cdev *tdev, unsigned int tid,
 }
 EXPORT_SYMBOL_GPL(cxgb3i_setup_conn_digest);
 
-static int ddp_init(struct t3cdev *tdev)
+
+/**
+ * cxgb3i_adapter_ddp_info - read the adapter's ddp information
+ * @tdev: t3cdev adapter
+ * @tformat: tag format
+ * @txsz: max tx pdu payload size, filled in by this func.
+ * @rxsz: max rx pdu payload size, filled in by this func.
+ * setup the tag format for a given iscsi entity
+ */
+int cxgb3i_adapter_ddp_info(struct t3cdev *tdev,
+			    struct cxgb3i_tag_format *tformat,
+			    unsigned int *txsz, unsigned int *rxsz)
+{
+	struct cxgb3i_ddp_info *ddp;
+	unsigned char idx_bits;
+
+	if (!tformat)
+		return -EINVAL;
+
+	if (!tdev->ulp_iscsi)
+		return -EINVAL;
+
+	ddp = (struct cxgb3i_ddp_info *)tdev->ulp_iscsi;
+
+	idx_bits = 32 - tformat->sw_bits;
+	tformat->rsvd_bits = ddp->idx_bits;
+	tformat->rsvd_shift = PPOD_IDX_SHIFT;
+	tformat->rsvd_mask = (1 << tformat->rsvd_bits) - 1;
+
+	ddp_log_info("tag format: sw %u, rsvd %u,%u, mask 0x%x.\n",
+		      tformat->sw_bits, tformat->rsvd_bits,
+		      tformat->rsvd_shift, tformat->rsvd_mask);
+
+	*txsz = min_t(unsigned int, ULP2_MAX_PDU_PAYLOAD,
+			ddp->max_txsz - ISCSI_PDU_NONPAYLOAD_LEN);
+	*rxsz = min_t(unsigned int, ULP2_MAX_PDU_PAYLOAD,
+			ddp->max_rxsz - ISCSI_PDU_NONPAYLOAD_LEN);
+	ddp_log_info("max payload size: %u/%u, %u/%u.\n",
+		     *txsz, ddp->max_txsz, *rxsz, ddp->max_rxsz);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxgb3i_adapter_ddp_info);
+
+/**
+ * ddp_release - release the cxgb3 adapter's ddp resource
+ * @tdev: t3cdev adapter
+ * release all the resource held by the ddp pagepod manager for a given
+ * adapter if needed
+ */
+static void ddp_release(struct t3cdev *tdev)
+{
+	int i = 0;
+	struct cxgb3i_ddp_info *ddp = (struct cxgb3i_ddp_info *)tdev->ulp_iscsi;
+
+	ddp_log_info("t3dev 0x%p, release ddp 0x%p.\n", tdev, ddp);
+
+	if (ddp) {
+		tdev->ulp_iscsi = NULL;
+		while (i < ddp->nppods) {
+			struct cxgb3i_gather_list *gl = ddp->gl_map[i];
+			if (gl) {
+				int npods = (gl->nelem + PPOD_PAGES_MAX - 1)
+						>> PPOD_PAGES_SHIFT;
+				ddp_log_info("t3dev 0x%p, ddp %d + %d.\n",
+						tdev, i, npods);
+				kfree(gl);
+				ddp_free_gl_skb(ddp, i, npods);
+				i += npods;
+			} else
+				i++;
+		}
+		cxgb3i_free_big_mem(ddp);
+	}
+}
+
+/**
+ * ddp_init - initialize the cxgb3 adapter's ddp resource
+ * @tdev: t3cdev adapter
+ * initialize the ddp pagepod manager for a given adapter
+ */
+static void ddp_init(struct t3cdev *tdev)
 {
 	struct cxgb3i_ddp_info *ddp;
 	struct ulp_iscsi_info uinfo;
@@ -572,6 +653,12 @@ static int ddp_init(struct t3cdev *tdev)
 	int i, err;
 	static int vers_printed;
 
+	if (tdev->ulp_iscsi) {
+		ddp_log_warn("t3dev 0x%p, ddp 0x%p already set up.\n",
+				tdev, tdev->ulp_iscsi);
+		return;
+	}
+
 	if (!vers_printed) {
 		printk(KERN_INFO "%s", version);
 		vers_printed = 1;
@@ -581,7 +668,7 @@ static int ddp_init(struct t3cdev *tdev)
 	if (err < 0) {
 		ddp_log_error("%s, failed to get iscsi param err=%d.\n",
 				 tdev->name, err);
-		return err;
+		return;
 	}
 
 	ppmax = (uinfo.ulimit - uinfo.llimit + 1) >> PPOD_SIZE_SHIFT;
@@ -598,7 +685,7 @@ static int ddp_init(struct t3cdev *tdev)
 	if (!ddp) {
 		ddp_log_warn("%s unable to alloc ddp 0x%d, ddp disabled.\n",
 			     tdev->name, ppmax);
-		return 0;
+		return;
 	}
 	ddp->gl_map = (struct cxgb3i_gather_list **)(ddp + 1);
 	ddp->gl_skb = (struct sk_buff **)(((char *)ddp->gl_map) +
@@ -632,141 +719,46 @@ static int ddp_init(struct t3cdev *tdev)
 
 	tdev->ulp_iscsi = ddp;
 
-	/* add to the list */
-	write_lock(&cxgb3i_ddp_rwlock);
-	list_add_tail(&ddp->list, &cxgb3i_ddp_list);
-	write_unlock(&cxgb3i_ddp_rwlock);
-
-	ddp_log_info("nppods %u (0x%x ~ 0x%x), bits %u, mask 0x%x,0x%x "
-			"pkt %u/%u, %u/%u.\n",
-			ppmax, ddp->llimit, ddp->ulimit, ddp->idx_bits,
-			ddp->idx_mask, ddp->rsvd_tag_mask,
-			ddp->max_txsz, uinfo.max_txsz,
+	ddp_log_info("tdev 0x%p, nppods %u, bits %u, mask 0x%x,0x%x pkt %u/%u,"
+			" %u/%u.\n",
+			tdev, ppmax, ddp->idx_bits, ddp->idx_mask,
+			ddp->rsvd_tag_mask, ddp->max_txsz, uinfo.max_txsz,
 			ddp->max_rxsz, uinfo.max_rxsz);
-	return 0;
+	return;
 
 free_ddp_map:
 	cxgb3i_free_big_mem(ddp);
-	return err;
 }
 
-/**
- * cxgb3i_adapter_ddp_init - initialize the adapter's ddp resource
- * @tdev: t3cdev adapter
- * @tformat: tag format
- * @txsz: max tx pdu payload size, filled in by this func.
- * @rxsz: max rx pdu payload size, filled in by this func.
- * initialize the ddp pagepod manager for a given adapter if needed and
- * setup the tag format for a given iscsi entity
- */
-int cxgb3i_adapter_ddp_init(struct t3cdev *tdev,
-			    struct cxgb3i_tag_format *tformat,
-			    unsigned int *txsz, unsigned int *rxsz)
-{
-	struct cxgb3i_ddp_info *ddp;
-	unsigned char idx_bits;
-
-	if (!tformat)
-		return -EINVAL;
-
-	if (!tdev->ulp_iscsi) {
-		int err = ddp_init(tdev);
-		if (err < 0)
-			return err;
-	}
-	ddp = (struct cxgb3i_ddp_info *)tdev->ulp_iscsi;
-
-	idx_bits = 32 - tformat->sw_bits;
-	tformat->rsvd_bits = ddp->idx_bits;
-	tformat->rsvd_shift = PPOD_IDX_SHIFT;
-	tformat->rsvd_mask = (1 << tformat->rsvd_bits) - 1;
-
-	ddp_log_info("tag format: sw %u, rsvd %u,%u, mask 0x%x.\n",
-		      tformat->sw_bits, tformat->rsvd_bits,
-		      tformat->rsvd_shift, tformat->rsvd_mask);
-
-	*txsz = min_t(unsigned int, ULP2_MAX_PDU_PAYLOAD,
-			ddp->max_txsz - ISCSI_PDU_NONPAYLOAD_LEN);
-	*rxsz = min_t(unsigned int, ULP2_MAX_PDU_PAYLOAD,
-			ddp->max_rxsz - ISCSI_PDU_NONPAYLOAD_LEN);
-	ddp_log_info("max payload size: %u/%u, %u/%u.\n",
-		     *txsz, ddp->max_txsz, *rxsz, ddp->max_rxsz);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(cxgb3i_adapter_ddp_init);
-
-static void ddp_release(struct cxgb3i_ddp_info *ddp)
-{
-	int i = 0;
-	struct t3cdev *tdev = ddp->tdev;
-
-	tdev->ulp_iscsi = NULL;
-	while (i < ddp->nppods) {
-		struct cxgb3i_gather_list *gl = ddp->gl_map[i];
-		if (gl) {
-			int npods = (gl->nelem + PPOD_PAGES_MAX - 1)
-				     >> PPOD_PAGES_SHIFT;
-
-			kfree(gl);
-			ddp_free_gl_skb(ddp, i, npods);
-		} else
-			i++;
-	}
-	cxgb3i_free_big_mem(ddp);
-}
-
-/**
- * cxgb3i_adapter_ddp_cleanup - release the adapter's ddp resource
- * @tdev: t3cdev adapter
- * release all the resource held by the ddp pagepod manager for a given
- * adapter if needed
- */
-void cxgb3i_adapter_ddp_cleanup(struct t3cdev *tdev)
-{
-	struct cxgb3i_ddp_info *ddp;
-
-	/* remove from the list */
-	write_lock(&cxgb3i_ddp_rwlock);
-	list_for_each_entry(ddp, &cxgb3i_ddp_list, list) {
-		if (ddp->tdev == tdev) {
-			list_del(&ddp->list);
-			break;
-		}
-	}
-	write_unlock(&cxgb3i_ddp_rwlock);
-
-	if (ddp)
-		ddp_release(ddp);
-}
-EXPORT_SYMBOL_GPL(cxgb3i_adapter_ddp_cleanup);
+static struct cxgb3_client t3c_ddp_client = {
+	.name = "iscsiddp_cxgb3",
+	.add = ddp_init,
+	.remove = ddp_release,
+};
 
 /**
  * cxgb3i_ddp_init_module - module init entry point
- * initialize any driver wide global data structures
+ * initialize any driver wide global data structures and register with the
+ * cxgb3 module
  */
 static int __init cxgb3i_ddp_init_module(void)
 {
 	page_idx = cxgb3i_ddp_find_page_index(PAGE_SIZE);
 	ddp_log_info("system PAGE_SIZE %lu, ddp idx %u.\n",
 		     PAGE_SIZE, page_idx);
+
+	cxgb3_register_client(&t3c_ddp_client);
 	return 0;
 }
 
 /**
  * cxgb3i_ddp_exit_module - module cleanup/exit entry point
- * go through the ddp list and release any resource held.
+ * go through the ddp list, unregister with the cxgb3 module and release
+ * any resource held.
  */
 static void __exit cxgb3i_ddp_exit_module(void)
 {
-	struct cxgb3i_ddp_info *ddp;
-
-	/* release all ddp manager if there is any */
-	write_lock(&cxgb3i_ddp_rwlock);
-	list_for_each_entry(ddp, &cxgb3i_ddp_list, list) {
-		list_del(&ddp->list);
-		ddp_release(ddp);
-	}
-	write_unlock(&cxgb3i_ddp_rwlock);
+	cxgb3_unregister_client(&t3c_ddp_client);
 }
 
 module_init(cxgb3i_ddp_init_module);
diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.h b/drivers/scsi/cxgb3i/cxgb3i_ddp.h
index 75a63a8..87f032b 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.h
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.h
@@ -301,7 +301,6 @@ int cxgb3i_setup_conn_pagesize(struct t3cdev *, unsigned int tid, int reply,
 int cxgb3i_setup_conn_digest(struct t3cdev *, unsigned int tid,
 				int hcrc, int dcrc, int reply);
 int cxgb3i_ddp_find_page_index(unsigned long pgsz);
-int cxgb3i_adapter_ddp_init(struct t3cdev *, struct cxgb3i_tag_format *,
+int cxgb3i_adapter_ddp_info(struct t3cdev *, struct cxgb3i_tag_format *,
 			    unsigned int *txsz, unsigned int *rxsz);
-void cxgb3i_adapter_ddp_cleanup(struct t3cdev *);
 #endif
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/7] cxgb3i - re-read ddp settings information after chip reset
  2009-04-01 18:11   ` [PATCH 2/7] cxgb3i - re-initialize ddp settings after chip reset michaelc
@ 2009-04-01 18:11     ` michaelc
  2009-04-01 18:11       ` [PATCH 4/7] cxgb3i - close all tcp connections upon " michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Mike Christie, Karen Xie

From: Mike Christie <michaelc@cs.wisc.edu>

Orignally from Karen Xie, but merge conflicts/errors fixed up by
Mike Christie.

Re-read the ddp tag information after reset.

Signed-off-by: Karen Xie <kxie@chelsio.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/cxgb3i/cxgb3i.h       |    2 +-
 drivers/scsi/cxgb3i/cxgb3i_iscsi.c |  114 ++++++++++++++++++++++--------------
 2 files changed, 70 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/cxgb3i/cxgb3i.h b/drivers/scsi/cxgb3i/cxgb3i.h
index 0942227..d362860 100644
--- a/drivers/scsi/cxgb3i/cxgb3i.h
+++ b/drivers/scsi/cxgb3i/cxgb3i.h
@@ -141,7 +141,7 @@ int cxgb3i_iscsi_init(void);
 void cxgb3i_iscsi_cleanup(void);
 
 struct cxgb3i_adapter *cxgb3i_adapter_find_by_tdev(struct t3cdev *);
-struct cxgb3i_adapter *cxgb3i_adapter_open(struct t3cdev *);
+void cxgb3i_adapter_open(struct t3cdev *);
 void cxgb3i_adapter_close(struct t3cdev *);
 
 struct cxgb3i_hba *cxgb3i_hba_find_by_netdev(struct net_device *);
diff --git a/drivers/scsi/cxgb3i/cxgb3i_iscsi.c b/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
index ff6bfd6..fff8e43 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
@@ -71,37 +71,34 @@ struct cxgb3i_adapter *cxgb3i_adapter_find_by_tdev(struct t3cdev *tdev)
 	return NULL;
 }
 
-/**
- * cxgb3i_adapter_open - init a s3 adapter structure and any h/w settings
- * @t3dev: t3cdev adapter
- * return the resulting cxgb3i_adapter struct
- */
-struct cxgb3i_adapter *cxgb3i_adapter_open(struct t3cdev *t3dev)
+static inline int adapter_update(struct cxgb3i_adapter *snic)
 {
-	struct cxgb3i_adapter *snic;
-	struct adapter *adapter = tdev2adap(t3dev);
-	int i;
+	cxgb3i_log_info("snic 0x%p, t3dev 0x%p, updating.\n",
+			snic, snic->tdev);
+	return cxgb3i_adapter_ddp_info(snic->tdev, &snic->tag_format,
+					&snic->tx_max_size,
+					&snic->rx_max_size);
+}
 
-	snic = kzalloc(sizeof(*snic), GFP_KERNEL);
-	if (!snic) {
-		cxgb3i_api_debug("cxgb3 %s, OOM.\n", t3dev->name);
-		return NULL;
-	}
-	spin_lock_init(&snic->lock);
+static int adapter_add(struct cxgb3i_adapter *snic)
+{
+	struct t3cdev *t3dev = snic->tdev;
+	struct adapter *adapter = tdev2adap(t3dev);
+	int i, err;
 
-	snic->tdev = t3dev;
 	snic->pdev = adapter->pdev;
 	snic->tag_format.sw_bits = sw_tag_idx_bits + sw_tag_age_bits;
 
-	if (cxgb3i_adapter_ddp_init(t3dev, &snic->tag_format,
+	err = cxgb3i_adapter_ddp_info(t3dev, &snic->tag_format,
 				    &snic->tx_max_size,
-				    &snic->rx_max_size) < 0)
-		goto free_snic;
+				    &snic->rx_max_size);
+	if (err < 0)
+		return err;
 
 	for_each_port(adapter, i) {
 		snic->hba[i] = cxgb3i_hba_host_add(snic, adapter->port[i]);
 		if (!snic->hba[i])
-			goto ulp_cleanup;
+			return -EINVAL;
 	}
 	snic->hba_cnt = adapter->params.nports;
 
@@ -110,13 +107,40 @@ struct cxgb3i_adapter *cxgb3i_adapter_open(struct t3cdev *t3dev)
 	list_add_tail(&snic->list_head, &cxgb3i_snic_list);
 	write_unlock(&cxgb3i_snic_rwlock);
 
-	return snic;
+	cxgb3i_log_info("t3dev 0x%p open, snic 0x%p, %u scsi hosts added.\n",
+			t3dev, snic, snic->hba_cnt);
+	return 0;
+}
 
-ulp_cleanup:
-	cxgb3i_adapter_ddp_cleanup(t3dev);
-free_snic:
-	kfree(snic);
-	return NULL;
+/**
+ * cxgb3i_adapter_open - init a s3 adapter structure and any h/w settings
+ * @t3dev: t3cdev adapter
+ */
+void cxgb3i_adapter_open(struct t3cdev *t3dev)
+{
+	struct cxgb3i_adapter *snic = cxgb3i_adapter_find_by_tdev(t3dev);
+	int err;
+
+	if (snic)
+		err = adapter_update(snic);
+	else {
+		snic = kzalloc(sizeof(*snic), GFP_KERNEL);
+		if (snic) {
+			spin_lock_init(&snic->lock);
+			snic->tdev = t3dev;
+			err = adapter_add(snic);
+		} else
+			err = -ENOMEM;
+	}
+
+	if (err < 0) {
+		cxgb3i_log_info("snic 0x%p, f 0x%x, t3dev 0x%p open, err %d.\n",
+				snic, snic ? snic->flags : 0, t3dev, err);
+		if (snic) {
+			snic->flags &= ~CXGB3I_ADAPTER_FLAG_RESET;
+			cxgb3i_adapter_close(t3dev);
+		}
+	}
 }
 
 /**
@@ -125,31 +149,29 @@ free_snic:
  */
 void cxgb3i_adapter_close(struct t3cdev *t3dev)
 {
+	struct cxgb3i_adapter *snic = cxgb3i_adapter_find_by_tdev(t3dev);
 	int i;
-	struct cxgb3i_adapter *snic;
+
+	if (!snic || snic->flags & CXGB3I_ADAPTER_FLAG_RESET) {
+		cxgb3i_log_info("t3dev 0x%p close, snic 0x%p, f 0x%x.\n",
+				t3dev, snic, snic ? snic->flags : 0);
+		return;
+	}
 
 	/* remove from the list */
 	write_lock(&cxgb3i_snic_rwlock);
-	list_for_each_entry(snic, &cxgb3i_snic_list, list_head) {
-		if (snic->tdev == t3dev) {
-			list_del(&snic->list_head);
-			break;
-		}
-	}
+	list_del(&snic->list_head);
 	write_unlock(&cxgb3i_snic_rwlock);
 
-	if (snic) {
-		for (i = 0; i < snic->hba_cnt; i++) {
-			if (snic->hba[i]) {
-				cxgb3i_hba_host_remove(snic->hba[i]);
-				snic->hba[i] = NULL;
-			}
+	for (i = 0; i < snic->hba_cnt; i++) {
+		if (snic->hba[i]) {
+			cxgb3i_hba_host_remove(snic->hba[i]);
+			snic->hba[i] = NULL;
 		}
-
-		/* release ddp resources */
-		cxgb3i_adapter_ddp_cleanup(snic->tdev);
-		kfree(snic);
 	}
+	cxgb3i_log_info("t3dev 0x%p close, snic 0x%p, %u scsi hosts removed.\n",
+			t3dev, snic, snic->hba_cnt);
+	kfree(snic);
 }
 
 /**
@@ -189,7 +211,8 @@ struct cxgb3i_hba *cxgb3i_hba_host_add(struct cxgb3i_adapter *snic,
 	shost = iscsi_host_alloc(&cxgb3i_host_template,
 				 sizeof(struct cxgb3i_hba), 1);
 	if (!shost) {
-		cxgb3i_log_info("iscsi_host_alloc failed.\n");
+		cxgb3i_log_info("snic 0x%p, ndev 0x%p, host_alloc failed.\n",
+				snic, ndev);
 		return NULL;
 	}
 
@@ -207,7 +230,8 @@ struct cxgb3i_hba *cxgb3i_hba_host_add(struct cxgb3i_adapter *snic,
 	pci_dev_get(snic->pdev);
 	err = iscsi_host_add(shost, &snic->pdev->dev);
 	if (err) {
-		cxgb3i_log_info("iscsi_host_add failed.\n");
+		cxgb3i_log_info("snic 0x%p, ndev 0x%p, host_add failed.\n",
+				snic, ndev);
 		goto pci_dev_put;
 	}
 
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/7] cxgb3i - close all tcp connections upon chip reset
  2009-04-01 18:11     ` [PATCH 3/7] cxgb3i - re-read ddp settings information " michaelc
@ 2009-04-01 18:11       ` michaelc
  2009-04-01 18:11         ` [PATCH 5/7] cxgb3i -- merge cxgb3i_ddp into cxgb3i module michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Karen Xie, Mike Christie

From: Karen Xie <kxie@chelsio.com>

[PATCH 5/5 2.6.30] cxgb3i - close all tcp connections upon chip reset

From: Karen Xie <kxie@chelsio.com>

Keep track of offloaded tcp connections per adapter. Close all of the
connections upon reset.

Signed-off-by: Karen Xie <kxie@chelsio.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/cxgb3i/cxgb3i_offload.c |  101 ++++++++++++++++++++++------------
 drivers/scsi/cxgb3i/cxgb3i_offload.h |   11 ++--
 2 files changed, 71 insertions(+), 41 deletions(-)

diff --git a/drivers/scsi/cxgb3i/cxgb3i_offload.c b/drivers/scsi/cxgb3i/cxgb3i_offload.c
index c2e434e..4d8654c 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_offload.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_offload.c
@@ -94,29 +94,30 @@ static int c3cn_get_port(struct s3_conn *c3cn, struct cxgb3i_sdev_data *cdata)
 	if (!cdata)
 		goto error_out;
 
-	if (c3cn->saddr.sin_port != 0) {
-		idx = ntohs(c3cn->saddr.sin_port) - cxgb3_sport_base;
-		if (idx < 0 || idx >= cxgb3_max_connect)
-			return 0;
-		if (!test_and_set_bit(idx, cdata->sport_map))
-			return -EADDRINUSE;
+	if (c3cn->saddr.sin_port) {
+		cxgb3i_log_error("connect, sin_port NON-ZERO %u.\n",
+				 c3cn->saddr.sin_port);
+		return -EADDRINUSE;
 	}
 
-	/* the sport_map_next may not be accurate but that is okay, sport_map
-	   should be */
-	start = idx = cdata->sport_map_next;
+	spin_lock_bh(&cdata->lock);
+	start = idx = cdata->sport_next;
 	do {
 		if (++idx >= cxgb3_max_connect)
 			idx = 0;
-		if (!(test_and_set_bit(idx, cdata->sport_map))) {
+		if (!cdata->sport_conn[idx]) {
 			c3cn->saddr.sin_port = htons(cxgb3_sport_base + idx);
-			cdata->sport_map_next = idx;
+			cdata->sport_next = idx;
+			cdata->sport_conn[idx] = c3cn;
+			spin_unlock_bh(&cdata->lock);
+
 			c3cn_conn_debug("%s reserve port %u.\n",
 					cdata->cdev->name,
 					cxgb3_sport_base + idx);
 			return 0;
 		}
 	} while (idx != start);
+	spin_unlock_bh(&cdata->lock);
 
 error_out:
 	return -EADDRNOTAVAIL;
@@ -124,15 +125,19 @@ error_out:
 
 static void c3cn_put_port(struct s3_conn *c3cn)
 {
-	struct cxgb3i_sdev_data *cdata = CXGB3_SDEV_DATA(c3cn->cdev);
+	if (!c3cn->cdev)
+		return;
 
 	if (c3cn->saddr.sin_port) {
+		struct cxgb3i_sdev_data *cdata = CXGB3_SDEV_DATA(c3cn->cdev);
 		int idx = ntohs(c3cn->saddr.sin_port) - cxgb3_sport_base;
 
 		c3cn->saddr.sin_port = 0;
 		if (idx < 0 || idx >= cxgb3_max_connect)
 			return;
-		clear_bit(idx, cdata->sport_map);
+		spin_lock_bh(&cdata->lock);
+		cdata->sport_conn[idx] = NULL;
+		spin_unlock_bh(&cdata->lock);
 		c3cn_conn_debug("%s, release port %u.\n",
 				cdata->cdev->name, cxgb3_sport_base + idx);
 	}
@@ -1305,11 +1310,7 @@ static void c3cn_release_offload_resources(struct s3_conn *c3cn)
 	struct t3cdev *cdev = c3cn->cdev;
 	unsigned int tid = c3cn->tid;
 
-	if (!cdev)
-		return;
-
 	c3cn->qset = 0;
-
 	c3cn_free_cpl_skbs(c3cn);
 
 	if (c3cn->wr_avail != c3cn->wr_max) {
@@ -1317,18 +1318,22 @@ static void c3cn_release_offload_resources(struct s3_conn *c3cn)
 		reset_wr_list(c3cn);
 	}
 
-	if (c3cn->l2t) {
-		l2t_release(L2DATA(cdev), c3cn->l2t);
-		c3cn->l2t = NULL;
-	}
-
-	if (c3cn->state == C3CN_STATE_CONNECTING) /* we have ATID */
-		s3_free_atid(cdev, tid);
-	else {		/* we have TID */
-		cxgb3_remove_tid(cdev, (void *)c3cn, tid);
-		c3cn_put(c3cn);
+	if (cdev) {
+		if (c3cn->l2t) {
+			l2t_release(L2DATA(cdev), c3cn->l2t);
+			c3cn->l2t = NULL;
+		}
+		if (c3cn->state == C3CN_STATE_CONNECTING)
+			/* we have ATID */
+			s3_free_atid(cdev, tid);
+		else {
+			/* we have TID */
+			cxgb3_remove_tid(cdev, (void *)c3cn, tid);
+			c3cn_put(c3cn);
+		}
 	}
 
+	c3cn->dst_cache = NULL;
 	c3cn->cdev = NULL;
 }
 
@@ -1417,17 +1422,18 @@ static void c3cn_active_close(struct s3_conn *c3cn)
 }
 
 /**
- * cxgb3i_c3cn_release - close and release an iscsi tcp connection
+ * cxgb3i_c3cn_release - close and release an iscsi tcp connection and any
+ * 	resource held
  * @c3cn: the iscsi tcp connection
  */
 void cxgb3i_c3cn_release(struct s3_conn *c3cn)
 {
 	c3cn_conn_debug("c3cn 0x%p, s %u, f 0x%lx.\n",
 			c3cn, c3cn->state, c3cn->flags);
-	if (likely(c3cn->state != C3CN_STATE_CONNECTING))
-		c3cn_active_close(c3cn);
-	else
+	if (unlikely(c3cn->state == C3CN_STATE_CONNECTING))
 		c3cn_set_flag(c3cn, C3CN_ACTIVE_CLOSE_NEEDED);
+	else if (likely(c3cn->state != C3CN_STATE_CLOSED))
+		c3cn_active_close(c3cn);
 	c3cn_put(c3cn);
 }
 
@@ -1656,7 +1662,6 @@ int cxgb3i_c3cn_connect(struct s3_conn *c3cn, struct sockaddr_in *usin)
 	c3cn_set_state(c3cn, C3CN_STATE_CLOSED);
 	ip_rt_put(rt);
 	c3cn_put_port(c3cn);
-	c3cn->daddr.sin_port = 0;
 	return err;
 }
 
@@ -1776,10 +1781,25 @@ out_err:
 static void sdev_data_cleanup(struct cxgb3i_sdev_data *cdata)
 {
 	struct adap_ports *ports = &cdata->ports;
+	struct s3_conn *c3cn;
 	int i;
 
+	for (i = 0; i < cxgb3_max_connect; i++) {
+		if (cdata->sport_conn[i]) {
+			c3cn = cdata->sport_conn[i];
+			cdata->sport_conn[i] = NULL;
+
+			spin_lock_bh(&c3cn->lock);
+			c3cn->cdev = NULL;
+			c3cn_set_flag(c3cn, C3CN_OFFLOAD_DOWN);
+			c3cn_closed(c3cn);
+			spin_unlock_bh(&c3cn->lock);
+		}
+	}
+
 	for (i = 0; i < ports->nports; i++)
 		NDEV2CDATA(ports->lldevs[i]) = NULL;
+
 	cxgb3i_free_big_mem(cdata);
 }
 
@@ -1821,21 +1841,27 @@ void cxgb3i_sdev_add(struct t3cdev *cdev, struct cxgb3_client *client)
 	struct cxgb3i_sdev_data *cdata;
 	struct ofld_page_info rx_page_info;
 	unsigned int wr_len;
-	int mapsize = DIV_ROUND_UP(cxgb3_max_connect,
-				   8 * sizeof(unsigned long));
+	int mapsize = cxgb3_max_connect * sizeof(struct s3_conn *);
 	int i;
 
 	cdata =  cxgb3i_alloc_big_mem(sizeof(*cdata) + mapsize, GFP_KERNEL);
-	if (!cdata)
+	if (!cdata) {
+		cxgb3i_log_warn("t3dev 0x%p, offload up, OOM %d.\n",
+				cdev, mapsize);
 		return;
+	}
 
 	if (cdev->ctl(cdev, GET_WR_LEN, &wr_len) < 0 ||
 	    cdev->ctl(cdev, GET_PORTS, &cdata->ports) < 0 ||
-	    cdev->ctl(cdev, GET_RX_PAGE_INFO, &rx_page_info) < 0)
+	    cdev->ctl(cdev, GET_RX_PAGE_INFO, &rx_page_info) < 0) {
+		cxgb3i_log_warn("t3dev 0x%p, offload up, ioctl failed.\n",
+				cdev);
 		goto free_cdata;
+	}
 
 	s3_init_wr_tab(wr_len);
 
+	spin_lock_init(&cdata->lock);
 	INIT_LIST_HEAD(&cdata->list);
 	cdata->cdev = cdev;
 	cdata->client = client;
@@ -1847,6 +1873,7 @@ void cxgb3i_sdev_add(struct t3cdev *cdev, struct cxgb3_client *client)
 	list_add_tail(&cdata->list, &cdata_list);
 	write_unlock(&cdata_rwlock);
 
+	cxgb3i_log_info("t3dev 0x%p, offload up, added.\n", cdev);
 	return;
 
 free_cdata:
@@ -1861,6 +1888,8 @@ void cxgb3i_sdev_remove(struct t3cdev *cdev)
 {
 	struct cxgb3i_sdev_data *cdata = CXGB3_SDEV_DATA(cdev);
 
+	cxgb3i_log_info("t3dev 0x%p, offload down, remove.\n", cdev);
+
 	write_lock(&cdata_rwlock);
 	list_del(&cdata->list);
 	write_unlock(&cdata_rwlock);
diff --git a/drivers/scsi/cxgb3i/cxgb3i_offload.h b/drivers/scsi/cxgb3i/cxgb3i_offload.h
index 275f23f..dab759d 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_offload.h
+++ b/drivers/scsi/cxgb3i/cxgb3i_offload.h
@@ -135,11 +135,11 @@ enum c3cn_flags {
 	C3CN_ABORT_RPL_PENDING,	/* expecting an abort reply */
 	C3CN_TX_DATA_SENT,	/* already sent a TX_DATA WR */
 	C3CN_ACTIVE_CLOSE_NEEDED,	/* need to be closed */
+	C3CN_OFFLOAD_DOWN	/* offload function off */
 };
 
 /**
  * cxgb3i_sdev_data - Per adapter data.
- *
  * Linked off of each Ethernet device port on the adapter.
  * Also available via the t3cdev structure since we have pointers to our port
  * net_device's there ...
@@ -148,16 +148,17 @@ enum c3cn_flags {
  * @cdev:	t3cdev adapter
  * @client:	CPL client pointer
  * @ports:	array of adapter ports
- * @sport_map_next: next index into the port map
- * @sport_map:	source port map
+ * @sport_next: next port
+ * @sport_conn:	source port connection
  */
 struct cxgb3i_sdev_data {
 	struct list_head list;
 	struct t3cdev *cdev;
 	struct cxgb3_client *client;
 	struct adap_ports ports;
-	unsigned int sport_map_next;
-	unsigned long sport_map[0];
+	spinlock_t lock;
+	unsigned int sport_next;
+	struct s3_conn *sport_conn[0];
 };
 #define NDEV2CDATA(ndev) (*(struct cxgb3i_sdev_data **)&(ndev)->ec_ptr)
 #define CXGB3_SDEV_DATA(cdev) NDEV2CDATA((cdev)->lldev)
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/7] cxgb3i -- merge cxgb3i_ddp into cxgb3i module
  2009-04-01 18:11       ` [PATCH 4/7] cxgb3i - close all tcp connections upon " michaelc
@ 2009-04-01 18:11         ` michaelc
  2009-04-01 18:11           ` [PATCH 6/7] cxgb3i: call ddp release function directly michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Karen Xie, Mike Christie

From: Karen Xie <kxie@chelsio.com>

[PATCH 2.6.30] cxgb3i -- merge cxgb3i_ddp into cxgb3i module

From: Karen Xie <kxie@chelsio.com>

- Merge cxgb3i_ddp.ko to cxgb3i.ko as there is no other users.
- Bump the driver version up to 1.0.2.

Signed-off-by: Karen Xie <kxie@chelsio.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/cxgb3i/Kbuild           |    4 +-
 drivers/scsi/cxgb3i/cxgb3i_ddp.c     |   63 ++++++----------------------------
 drivers/scsi/cxgb3i/cxgb3i_ddp.h     |    3 ++
 drivers/scsi/cxgb3i/cxgb3i_init.c    |    6 ++-
 drivers/scsi/cxgb3i/cxgb3i_offload.h |    2 +-
 5 files changed, 21 insertions(+), 57 deletions(-)

diff --git a/drivers/scsi/cxgb3i/Kbuild b/drivers/scsi/cxgb3i/Kbuild
index ee7d6d2..25a2032 100644
--- a/drivers/scsi/cxgb3i/Kbuild
+++ b/drivers/scsi/cxgb3i/Kbuild
@@ -1,4 +1,4 @@
 EXTRA_CFLAGS += -I$(TOPDIR)/drivers/net/cxgb3
 
-cxgb3i-y := cxgb3i_init.o cxgb3i_iscsi.o cxgb3i_pdu.o cxgb3i_offload.o
-obj-$(CONFIG_SCSI_CXGB3_ISCSI) += cxgb3i_ddp.o cxgb3i.o
+cxgb3i-y := cxgb3i_init.o cxgb3i_iscsi.o cxgb3i_pdu.o cxgb3i_offload.o cxgb3i_ddp.o
+obj-$(CONFIG_SCSI_CXGB3_ISCSI) += cxgb3i.o
diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.c b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
index bb1eebf..275d2da 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
@@ -23,19 +23,6 @@
 
 #include "cxgb3i_ddp.h"
 
-#define DRV_MODULE_NAME         "cxgb3i_ddp"
-#define DRV_MODULE_VERSION      "1.0.0"
-#define DRV_MODULE_RELDATE      "Dec. 1, 2008"
-
-static char version[] =
-	"Chelsio S3xx iSCSI DDP " DRV_MODULE_NAME
-	" v" DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
-
-MODULE_AUTHOR("Karen Xie <kxie@chelsio.com>");
-MODULE_DESCRIPTION("cxgb3i ddp pagepod manager");
-MODULE_LICENSE("GPL");
-MODULE_VERSION(DRV_MODULE_VERSION);
-
 #define ddp_log_error(fmt...) printk(KERN_ERR "cxgb3i_ddp: ERR! " fmt)
 #define ddp_log_warn(fmt...)  printk(KERN_WARNING "cxgb3i_ddp: WARN! " fmt)
 #define ddp_log_info(fmt...)  printk(KERN_INFO "cxgb3i_ddp: " fmt)
@@ -212,7 +199,6 @@ int cxgb3i_ddp_find_page_index(unsigned long pgsz)
 	ddp_log_debug("ddp page size 0x%lx not supported.\n", pgsz);
 	return DDP_PGIDX_MAX;
 }
-EXPORT_SYMBOL_GPL(cxgb3i_ddp_find_page_index);
 
 static inline void ddp_gl_unmap(struct pci_dev *pdev,
 				struct cxgb3i_gather_list *gl)
@@ -335,7 +321,6 @@ error_out:
 	kfree(gl);
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(cxgb3i_ddp_make_gl);
 
 /**
  * cxgb3i_ddp_release_gl - release a page buffer list
@@ -349,7 +334,6 @@ void cxgb3i_ddp_release_gl(struct cxgb3i_gather_list *gl,
 	ddp_gl_unmap(pdev, gl);
 	kfree(gl);
 }
-EXPORT_SYMBOL_GPL(cxgb3i_ddp_release_gl);
 
 /**
  * cxgb3i_ddp_tag_reserve - set up ddp for a data transfer
@@ -431,7 +415,6 @@ unmark_entries:
 	ddp_unmark_entries(ddp, idx, npods);
 	return err;
 }
-EXPORT_SYMBOL_GPL(cxgb3i_ddp_tag_reserve);
 
 /**
  * cxgb3i_ddp_tag_release - release a ddp tag
@@ -469,7 +452,6 @@ void cxgb3i_ddp_tag_release(struct t3cdev *tdev, u32 tag)
 		ddp_log_error("ddp tag 0x%x, idx 0x%x > max 0x%x.\n",
 			      tag, idx, ddp->nppods);
 }
-EXPORT_SYMBOL_GPL(cxgb3i_ddp_tag_release);
 
 static int setup_conn_pgidx(struct t3cdev *tdev, unsigned int tid, int pg_idx,
 			    int reply)
@@ -510,7 +492,6 @@ int cxgb3i_setup_conn_host_pagesize(struct t3cdev *tdev, unsigned int tid,
 {
 	return setup_conn_pgidx(tdev, tid, page_idx, reply);
 }
-EXPORT_SYMBOL_GPL(cxgb3i_setup_conn_host_pagesize);
 
 /**
  * cxgb3i_setup_conn_pagesize - setup the conn.'s ddp page size
@@ -527,7 +508,6 @@ int cxgb3i_setup_conn_pagesize(struct t3cdev *tdev, unsigned int tid,
 
 	return setup_conn_pgidx(tdev, tid, pgidx, reply);
 }
-EXPORT_SYMBOL_GPL(cxgb3i_setup_conn_pagesize);
 
 /**
  * cxgb3i_setup_conn_digest - setup conn. digest setting
@@ -563,7 +543,6 @@ int cxgb3i_setup_conn_digest(struct t3cdev *tdev, unsigned int tid,
 	cxgb3_ofld_send(tdev, skb);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(cxgb3i_setup_conn_digest);
 
 
 /**
@@ -606,7 +585,6 @@ int cxgb3i_adapter_ddp_info(struct t3cdev *tdev,
 		     *txsz, ddp->max_txsz, *rxsz, ddp->max_rxsz);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(cxgb3i_adapter_ddp_info);
 
 /**
  * ddp_release - release the cxgb3 adapter's ddp resource
@@ -651,7 +629,6 @@ static void ddp_init(struct t3cdev *tdev)
 	struct ulp_iscsi_info uinfo;
 	unsigned int ppmax, bits;
 	int i, err;
-	static int vers_printed;
 
 	if (tdev->ulp_iscsi) {
 		ddp_log_warn("t3dev 0x%p, ddp 0x%p already set up.\n",
@@ -659,11 +636,6 @@ static void ddp_init(struct t3cdev *tdev)
 		return;
 	}
 
-	if (!vers_printed) {
-		printk(KERN_INFO "%s", version);
-		vers_printed = 1;
-	}
-
 	err = tdev->ctl(tdev, ULP_ISCSI_GET_PARAMS, &uinfo);
 	if (err < 0) {
 		ddp_log_error("%s, failed to get iscsi param err=%d.\n",
@@ -730,36 +702,23 @@ free_ddp_map:
 	cxgb3i_free_big_mem(ddp);
 }
 
-static struct cxgb3_client t3c_ddp_client = {
-	.name = "iscsiddp_cxgb3",
-	.add = ddp_init,
-	.remove = ddp_release,
-};
-
 /**
- * cxgb3i_ddp_init_module - module init entry point
- * initialize any driver wide global data structures and register with the
- * cxgb3 module
+ * cxgb3i_ddp_init - initialize ddp functions
  */
-static int __init cxgb3i_ddp_init_module(void)
+void cxgb3i_ddp_init(struct t3cdev *tdev)
 {
-	page_idx = cxgb3i_ddp_find_page_index(PAGE_SIZE);
-	ddp_log_info("system PAGE_SIZE %lu, ddp idx %u.\n",
-		     PAGE_SIZE, page_idx);
-
-	cxgb3_register_client(&t3c_ddp_client);
-	return 0;
+	if (page_idx == DDP_PGIDX_MAX) {
+		page_idx = cxgb3i_ddp_find_page_index(PAGE_SIZE);
+		ddp_log_info("system PAGE_SIZE %lu, ddp idx %u.\n",
+				PAGE_SIZE, page_idx);
+	}
+	ddp_init(tdev);
 }
 
 /**
- * cxgb3i_ddp_exit_module - module cleanup/exit entry point
- * go through the ddp list, unregister with the cxgb3 module and release
- * any resource held.
+ * cxgb3i_ddp_cleaup - clean up ddp function
  */
-static void __exit cxgb3i_ddp_exit_module(void)
+void cxgb3i_ddp_cleanup(struct t3cdev *tdev)
 {
-	cxgb3_unregister_client(&t3c_ddp_client);
+	ddp_release(tdev);
 }
-
-module_init(cxgb3i_ddp_init_module);
-module_exit(cxgb3i_ddp_exit_module);
diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.h b/drivers/scsi/cxgb3i/cxgb3i_ddp.h
index 87f032b..0d296de 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.h
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.h
@@ -303,4 +303,7 @@ int cxgb3i_setup_conn_digest(struct t3cdev *, unsigned int tid,
 int cxgb3i_ddp_find_page_index(unsigned long pgsz);
 int cxgb3i_adapter_ddp_info(struct t3cdev *, struct cxgb3i_tag_format *,
 			    unsigned int *txsz, unsigned int *rxsz);
+
+void cxgb3i_ddp_init(struct t3cdev *);
+void cxgb3i_ddp_cleanup(struct t3cdev *);
 #endif
diff --git a/drivers/scsi/cxgb3i/cxgb3i_init.c b/drivers/scsi/cxgb3i/cxgb3i_init.c
index 833dbfa..042d9bc 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_init.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_init.c
@@ -12,8 +12,8 @@
 #include "cxgb3i.h"
 
 #define DRV_MODULE_NAME         "cxgb3i"
-#define DRV_MODULE_VERSION	"1.0.1"
-#define DRV_MODULE_RELDATE	"Jan. 2009"
+#define DRV_MODULE_VERSION	"1.0.2"
+#define DRV_MODULE_RELDATE	"Mar. 2009"
 
 static char version[] =
 	"Chelsio S3xx iSCSI Driver " DRV_MODULE_NAME
@@ -50,6 +50,7 @@ static void open_s3_dev(struct t3cdev *t3dev)
 		vers_printed = 1;
 	}
 
+	cxgb3i_ddp_init(t3dev);
 	cxgb3i_sdev_add(t3dev, &t3c_client);
 	cxgb3i_adapter_open(t3dev);
 }
@@ -62,6 +63,7 @@ static void close_s3_dev(struct t3cdev *t3dev)
 {
 	cxgb3i_adapter_close(t3dev);
 	cxgb3i_sdev_remove(t3dev);
+	cxgb3i_ddp_cleanup(t3dev);
 }
 
 static void s3_err_handler(struct t3cdev *tdev, u32 status, u32 error)
diff --git a/drivers/scsi/cxgb3i/cxgb3i_offload.h b/drivers/scsi/cxgb3i/cxgb3i_offload.h
index dab759d..ebfca96 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_offload.h
+++ b/drivers/scsi/cxgb3i/cxgb3i_offload.h
@@ -16,7 +16,7 @@
 #define _CXGB3I_OFFLOAD_H
 
 #include <linux/skbuff.h>
-#include <net/tcp.h>
+#include <linux/in.h>
 
 #include "common.h"
 #include "adapter.h"
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/7] cxgb3i: call ddp release function directly
  2009-04-01 18:11         ` [PATCH 5/7] cxgb3i -- merge cxgb3i_ddp into cxgb3i module michaelc
@ 2009-04-01 18:11           ` michaelc
  2009-04-01 18:11             ` [PATCH 7/7] libiscsi: fix iscsi pool error path michaelc
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Mike Christie

From: Mike Christie <michaelc@cs.wisc.edu>

cxgb3i_ddp_cleanup just calls ddp_release directly so there is
no reason for the wrapper. This patch just renames ddp_release
to cxgb3i_ddp_cleanup and removes the old wrapper function.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/cxgb3i/cxgb3i_ddp.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.c b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
index 275d2da..d06a661 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
@@ -587,12 +587,12 @@ int cxgb3i_adapter_ddp_info(struct t3cdev *tdev,
 }
 
 /**
- * ddp_release - release the cxgb3 adapter's ddp resource
+ * cxgb3i_ddp_cleanup - release the cxgb3 adapter's ddp resource
  * @tdev: t3cdev adapter
  * release all the resource held by the ddp pagepod manager for a given
  * adapter if needed
  */
-static void ddp_release(struct t3cdev *tdev)
+void cxgb3i_ddp_cleanup(struct t3cdev *tdev)
 {
 	int i = 0;
 	struct cxgb3i_ddp_info *ddp = (struct cxgb3i_ddp_info *)tdev->ulp_iscsi;
@@ -714,11 +714,3 @@ void cxgb3i_ddp_init(struct t3cdev *tdev)
 	}
 	ddp_init(tdev);
 }
-
-/**
- * cxgb3i_ddp_cleaup - clean up ddp function
- */
-void cxgb3i_ddp_cleanup(struct t3cdev *tdev)
-{
-	ddp_release(tdev);
-}
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 7/7] libiscsi: fix iscsi pool error path
  2009-04-01 18:11           ` [PATCH 6/7] cxgb3i: call ddp release function directly michaelc
@ 2009-04-01 18:11             ` michaelc
  2009-04-02 14:02               ` Jean Delvare
  0 siblings, 1 reply; 13+ messages in thread
From: michaelc @ 2009-04-01 18:11 UTC (permalink / raw)
  To: linux-scsi; +Cc: Jean Delvare, Mike Christie

From: Jean Delvare <jdelvare@suse.de>

Le lundi 30 mars 2009, Chris Wright a écrit :
> q->queue could be ERR_PTR(-ENOMEM) which will break unwinding
> on error.  Make iscsi_pool_free more defensive.
>

Making the freeing of q->queue dependent on q->pool being set looks
really weird (although it is correct at the moment. But this seems
to be fixable in a much simpler way.

With the benefit that only the error case is slowed down. In both
cases we have a problem if q->queue contains an error value but it's
not -ENOMEM. Apparently this can't happen today, but it doesn't feel
right to assume this will always be true. Maybe it's the right time
to fix this as well.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/libiscsi.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index dfaa8ad..6896283 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1999,8 +1999,10 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);
-	if (q->queue == ERR_PTR(-ENOMEM))
+	if (IS_ERR(q->queue)) {
+		q->queue = NULL;
 		goto enomem;
+	}
 
 	for (i = 0; i < max; i++) {
 		q->pool[i] = kzalloc(item_size, GFP_KERNEL);
-- 
1.6.0.6

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 7/7] libiscsi: fix iscsi pool error path
  2009-04-01 18:11             ` [PATCH 7/7] libiscsi: fix iscsi pool error path michaelc
@ 2009-04-02 14:02               ` Jean Delvare
  2009-04-02 15:15                 ` Mike Christie
  0 siblings, 1 reply; 13+ messages in thread
From: Jean Delvare @ 2009-04-02 14:02 UTC (permalink / raw)
  To: Mike Christie; +Cc: linux-scsi, Chris Wright, James Bottomley

Hi Mike,

Sorry for the late answer, I have been traveling for the last 2 days.

Le mercredi 01 avril 2009, michaelc@cs.wisc.edu a écrit :
> From: Jean Delvare <jdelvare@suse.de>
> 
> Le lundi 30 mars 2009, Chris Wright a écrit :
> > q->queue could be ERR_PTR(-ENOMEM) which will break unwinding
> > on error.  Make iscsi_pool_free more defensive.
> >
> 
> Making the freeing of q->queue dependent on q->pool being set looks
> really weird (although it is correct at the moment. But this seems
> to be fixable in a much simpler way.
> 
> With the benefit that only the error case is slowed down. In both
> cases we have a problem if q->queue contains an error value but it's
> not -ENOMEM. Apparently this can't happen today, but it doesn't feel
> right to assume this will always be true. Maybe it's the right time
> to fix this as well.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> ---
>  drivers/scsi/libiscsi.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index dfaa8ad..6896283 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1999,8 +1999,10 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
>  
>  	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
>  			      GFP_KERNEL, NULL);
> -	if (q->queue == ERR_PTR(-ENOMEM))
> +	if (IS_ERR(q->queue)) {

This indeed solves the problem I had underlined before, but
introduces a new one. Right now the only error returned by kfifo_init
is -ENOMEM. This may however change in the future, and then the
above code would silently convert the other error code to -ENOMEM.
This might make it difficult to trace errors.

I know this is all just theoretical, at the moment your code is
correct, but I don't much like relying on assumptions which are not
guaranteed to last. So I would rather cleanly transmit the error code
up to the caller, or change the calling convention of kfifo_init() to
return NULL on error. The former will add a few lines of code, the
latter will result in faster code but is obviously more intrusive.
The latter might also be undesirable for some reason I am
overlooking; I'm really not familiar with kfifo.

Or you can consider I am too perfectionist and ignore me and go with
the current fix, that's also fine with me ;)

> +		q->queue = NULL;
>  		goto enomem;
> +	}
>  
>  	for (i = 0; i < max; i++) {
>  		q->pool[i] = kzalloc(item_size, GFP_KERNEL);



-- 
Jean Delvare
Suse L3
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 7/7] libiscsi: fix iscsi pool error path
  2009-04-02 14:02               ` Jean Delvare
@ 2009-04-02 15:15                 ` Mike Christie
  2009-04-02 15:27                   ` Chris Wright
  0 siblings, 1 reply; 13+ messages in thread
From: Mike Christie @ 2009-04-02 15:15 UTC (permalink / raw)
  To: Jean Delvare; +Cc: linux-scsi, Chris Wright, James Bottomley

Jean Delvare wrote:
> Hi Mike,
> 
> Sorry for the late answer, I have been traveling for the last 2 days.
> 
> Le mercredi 01 avril 2009, michaelc@cs.wisc.edu a écrit :
>> From: Jean Delvare <jdelvare@suse.de>
>>
>> Le lundi 30 mars 2009, Chris Wright a écrit :
>>> q->queue could be ERR_PTR(-ENOMEM) which will break unwinding
>>> on error.  Make iscsi_pool_free more defensive.
>>>
>> Making the freeing of q->queue dependent on q->pool being set looks
>> really weird (although it is correct at the moment. But this seems
>> to be fixable in a much simpler way.
>>
>> With the benefit that only the error case is slowed down. In both
>> cases we have a problem if q->queue contains an error value but it's
>> not -ENOMEM. Apparently this can't happen today, but it doesn't feel
>> right to assume this will always be true. Maybe it's the right time
>> to fix this as well.
>>
>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
>> ---
>>  drivers/scsi/libiscsi.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index dfaa8ad..6896283 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -1999,8 +1999,10 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
>>  
>>  	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
>>  			      GFP_KERNEL, NULL);
>> -	if (q->queue == ERR_PTR(-ENOMEM))
>> +	if (IS_ERR(q->queue)) {
> 
> This indeed solves the problem I had underlined before, but
> introduces a new one. Right now the only error returned by kfifo_init
> is -ENOMEM. This may however change in the future, and then the
> above code would silently convert the other error code to -ENOMEM.
> This might make it difficult to trace errors.

What do you mean by converting other errors codes to -ENOMEM?

> 
> I know this is all just theoretical, at the moment your code is
> correct, but I don't much like relying on assumptions which are not
> guaranteed to last. So I would rather cleanly transmit the error code
> up to the caller, or change the calling convention of kfifo_init() to

What do you mean by cleanly transmitting the error code up the caller?



> return NULL on error. The former will add a few lines of code, the
> latter will result in faster code but is obviously more intrusive.
> The latter might also be undesirable for some reason I am
> overlooking; I'm really not familiar with kfifo.
> 
> Or you can consider I am too perfectionist and ignore me and go with
> the current fix, that's also fine with me ;)
> 

I think it is fine for now. We really do  not care what the error is. If 
it fails, it fails. If kfifo_init() were to return some new error code 
for different errors we would have to evaluate the kfifo_init() change 
as well as the iscsi code to made sure we could handle it or not, until 
then it is safest to fail on all errors instead of possible limping 
along and possibly causing corruption.

I am also going to remove iscsi's use of kfifos in the next feature window.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 7/7] libiscsi: fix iscsi pool error path
  2009-04-02 15:15                 ` Mike Christie
@ 2009-04-02 15:27                   ` Chris Wright
  2009-04-02 16:05                     ` Mike Christie
  0 siblings, 1 reply; 13+ messages in thread
From: Chris Wright @ 2009-04-02 15:27 UTC (permalink / raw)
  To: Mike Christie; +Cc: Jean Delvare, linux-scsi, Chris Wright, James Bottomley

* Mike Christie (michaelc@cs.wisc.edu) wrote:
>>> -	if (q->queue == ERR_PTR(-ENOMEM))
>>> +	if (IS_ERR(q->queue)) {
>>
>> This indeed solves the problem I had underlined before, but
>> introduces a new one. Right now the only error returned by kfifo_init
>> is -ENOMEM. This may however change in the future, and then the
>> above code would silently convert the other error code to -ENOMEM.
>> This might make it difficult to trace errors.
>
> What do you mean by converting other errors codes to -ENOMEM?

Jean means the goto label...it will jump to essentially return -ENOMEM.

>> I know this is all just theoretical, at the moment your code is
>> correct, but I don't much like relying on assumptions which are not
>> guaranteed to last. So I would rather cleanly transmit the error code
>> up to the caller, or change the calling convention of kfifo_init() to
>
> What do you mean by cleanly transmitting the error code up the caller?

It's handwavy here...meaning if kfifo_init returned some other
error for some other condition, propagating that up the stack to the
iscsi_pool_init() caller.  Probably not all that critical to return
ENOMEM vs someother errno in this case.

thanks,
-chris

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 7/7] libiscsi: fix iscsi pool error path
  2009-04-02 15:27                   ` Chris Wright
@ 2009-04-02 16:05                     ` Mike Christie
  2009-04-02 17:26                       ` Chris Wright
  0 siblings, 1 reply; 13+ messages in thread
From: Mike Christie @ 2009-04-02 16:05 UTC (permalink / raw)
  To: Chris Wright; +Cc: Jean Delvare, linux-scsi, James Bottomley

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

Chris Wright wrote:
> * Mike Christie (michaelc@cs.wisc.edu) wrote:
>>>> -	if (q->queue == ERR_PTR(-ENOMEM))
>>>> +	if (IS_ERR(q->queue)) {
>>> This indeed solves the problem I had underlined before, but
>>> introduces a new one. Right now the only error returned by kfifo_init
>>> is -ENOMEM. This may however change in the future, and then the
>>> above code would silently convert the other error code to -ENOMEM.
>>> This might make it difficult to trace errors.
>> What do you mean by converting other errors codes to -ENOMEM?
> 
> Jean means the goto label...it will jump to essentially return -ENOMEM.
> 

Ah, ok, I can agree with passing up errors in general. I did the 
attached patch to propogate the error code upwards. It does not really 
make much difference now. The iscsi pool init caller just checks for 
error or no error and does not even print out the error. It will not 
make tracing errors easier or more difficult. To be useful, we would 
have to propgate the error higher to userspace so the user can see it. I 
do not think I have time to get that in the current feature window, so 
that would have to wait unless you have patches I can test now.

[-- Attachment #2: libiscsi-propogate-pool-error.patch --]
[-- Type: text/plain, Size: 1460 bytes --]

libiscsi: propogate errors from kfifo_init to iscsi_pool_init callers

Don't convert kfifo_init's error code to -ENOMEM when passing the
error to the iscsi_pool_init caller.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6896283..cf86d59 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1984,6 +1984,7 @@ int
 iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 {
 	int i, num_arrays = 1;
+	int rc = -ENOMEM;
 
 	memset(q, 0, sizeof(*q));
 
@@ -1995,20 +1996,21 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		return -ENOMEM;
+		return rc;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);
 	if (IS_ERR(q->queue)) {
 		q->queue = NULL;
-		goto enomem;
+		rc = PTR_ERR(q->queue);		
+		goto fail;
 	}
 
 	for (i = 0; i < max; i++) {
 		q->pool[i] = kzalloc(item_size, GFP_KERNEL);
 		if (q->pool[i] == NULL) {
 			q->max = i;
-			goto enomem;
+			goto fail;
 		}
 		__kfifo_put(q->queue, (void*)&q->pool[i], sizeof(void*));
 	}
@@ -2020,9 +2022,9 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 
 	return 0;
 
-enomem:
+fail:
 	iscsi_pool_free(q);
-	return -ENOMEM;
+	return rc;
 }
 EXPORT_SYMBOL_GPL(iscsi_pool_init);
 

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 7/7] libiscsi: fix iscsi pool error path
  2009-04-02 16:05                     ` Mike Christie
@ 2009-04-02 17:26                       ` Chris Wright
  0 siblings, 0 replies; 13+ messages in thread
From: Chris Wright @ 2009-04-02 17:26 UTC (permalink / raw)
  To: Mike Christie; +Cc: Chris Wright, Jean Delvare, linux-scsi, James Bottomley

* Mike Christie (michaelc@cs.wisc.edu) wrote:
> Ah, ok, I can agree with passing up errors in general. I did the  
> attached patch to propogate the error code upwards. It does not really  
> make much difference now. The iscsi pool init caller just checks for  
> error or no error and does not even print out the error. It will not  
> make tracing errors easier or more difficult. To be useful, we would  
> have to propgate the error higher to userspace so the user can see it. I  
> do not think I have time to get that in the current feature window, so  
> that would have to wait unless you have patches I can test now.

Yup, I think it's just fine as is.

> libiscsi: propogate errors from kfifo_init to iscsi_pool_init callers
> 
> Don't convert kfifo_init's error code to -ENOMEM when passing the
> error to the iscsi_pool_init caller.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

Acked-by: Chris Wright <chrisw@sous-sol.org>


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2009-04-02 17:26 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-01 18:11 iscsi bugfixes and cleanups michaelc
2009-04-01 18:11 ` [PATCH 1/7] cxgb3i - subscribe to error notification from cxgb3 driver michaelc
2009-04-01 18:11   ` [PATCH 2/7] cxgb3i - re-initialize ddp settings after chip reset michaelc
2009-04-01 18:11     ` [PATCH 3/7] cxgb3i - re-read ddp settings information " michaelc
2009-04-01 18:11       ` [PATCH 4/7] cxgb3i - close all tcp connections upon " michaelc
2009-04-01 18:11         ` [PATCH 5/7] cxgb3i -- merge cxgb3i_ddp into cxgb3i module michaelc
2009-04-01 18:11           ` [PATCH 6/7] cxgb3i: call ddp release function directly michaelc
2009-04-01 18:11             ` [PATCH 7/7] libiscsi: fix iscsi pool error path michaelc
2009-04-02 14:02               ` Jean Delvare
2009-04-02 15:15                 ` Mike Christie
2009-04-02 15:27                   ` Chris Wright
2009-04-02 16:05                     ` Mike Christie
2009-04-02 17:26                       ` Chris Wright

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.