[PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage

[PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage

[kxie-ut6Up61K2wZBDgjK7y7TUQ]

[PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage

From: Karen Xie <kxie-ut6Up61K2wZBDgjK7y7TUQ@public.gmane.org>

The iscsi ddp functionality could be used by multiple iscsi entities,
add a refcnt to keep track of it, so we would not release it pre-maturely.

Signed-off-by: Karen Xie <kxie-ut6Up61K2wZBDgjK7y7TUQ@public.gmane.org>
---

 drivers/scsi/cxgb3i/cxgb3i_ddp.c |   11 +++++++----
 drivers/scsi/cxgb3i/cxgb3i_ddp.h |    2 ++
 2 files changed, 9 insertions(+), 4 deletions(-)


diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.c b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
index 99c9125..b6fdb5e 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.c
@@ -603,9 +603,10 @@ void cxgb3i_ddp_cleanup(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);
+	ddp_log_info("t3dev 0x%p, release ddp 0x%p, ref %d.\n",
+			tdev, ddp, atomic_read(&ddp->refcnt));
 
-	if (ddp) {
+	if (ddp && atomic_dec_and_test(&ddp->refcnt)) {
 		tdev->ulp_iscsi = NULL;
 		while (i < ddp->nppods) {
 			struct cxgb3i_gather_list *gl = ddp->gl_map[i];
@@ -631,12 +632,13 @@ void cxgb3i_ddp_cleanup(struct t3cdev *tdev)
  */
 static void ddp_init(struct t3cdev *tdev)
 {
-	struct cxgb3i_ddp_info *ddp;
+	struct cxgb3i_ddp_info *ddp = tdev->ulp_iscsi;
 	struct ulp_iscsi_info uinfo;
 	unsigned int ppmax, bits;
 	int i, err;
 
-	if (tdev->ulp_iscsi) {
+	if (ddp) {
+		atomic_inc(&ddp->refcnt);
 		ddp_log_warn("t3dev 0x%p, ddp 0x%p already set up.\n",
 				tdev, tdev->ulp_iscsi);
 		return;
@@ -670,6 +672,7 @@ static void ddp_init(struct t3cdev *tdev)
 					  ppmax *
 					  sizeof(struct cxgb3i_gather_list *));
 	spin_lock_init(&ddp->map_lock);
+	atomic_set(&ddp->refcnt, 1);
 
 	ddp->tdev = tdev;
 	ddp->pdev = uinfo.pdev;
diff --git a/drivers/scsi/cxgb3i/cxgb3i_ddp.h b/drivers/scsi/cxgb3i/cxgb3i_ddp.h
index 0d296de..0e699e8 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_ddp.h
+++ b/drivers/scsi/cxgb3i/cxgb3i_ddp.h
@@ -54,6 +54,7 @@ struct cxgb3i_gather_list {
  * struct cxgb3i_ddp_info - cxgb3i direct data placement for pdu payload
  *
  * @list:	list head to link elements
+ * @refcnt:	count of iscsi entities using it
  * @tdev:	pointer to t3cdev used by cxgb3 driver
  * @max_txsz:	max tx packet size for ddp
  * @max_rxsz:	max rx packet size for ddp
@@ -70,6 +71,7 @@ struct cxgb3i_gather_list {
  */
 struct cxgb3i_ddp_info {
 	struct list_head list;
+	atomic_t refcnt;
 	struct t3cdev *tdev;
 	struct pci_dev *pdev;
 	unsigned int max_txsz;

Re: [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage

[James Bottomley]

On Sat, 2009-06-13 at 14:29 -0700, kxie@chelsio.com wrote:
> [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage
> 
> From: Karen Xie <kxie@chelsio.com>
> 
> The iscsi ddp functionality could be used by multiple iscsi entities,
> add a refcnt to keep track of it, so we would not release it pre-maturely.

The code is fine as it stands but, I wonder, would you consider using a
kref for this?  The reasons are twofold:

     1. It's a well understood kernel paradigm that works ... this would
        be helpful for the far distant time when chelsio no longer wants
        to maintain this driver.
     2. It includes useful and appropriate warnings for common bugs in
        refcounted code, such as use of a freed reference.

James

RE: [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage

[Karen Xie]

Hi, James,

Thanks, I've re-submitted the patches to use kref instead.

Best regards,
Karen

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] 
Sent: Saturday, June 13, 2009 4:53 PM
To: Karen Xie
Cc: linux-scsi@vger.kernel.org; open-iscsi@googlegroups.com;
michaelc@cs.wisc.edu
Subject: Re: [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp
usage

On Sat, 2009-06-13 at 14:29 -0700, kxie@chelsio.com wrote:
> [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage
> 
> From: Karen Xie <kxie@chelsio.com>
> 
> The iscsi ddp functionality could be used by multiple iscsi entities,
> add a refcnt to keep track of it, so we would not release it
pre-maturely.

The code is fine as it stands but, I wonder, would you consider using a
kref for this?  The reasons are twofold:

     1. It's a well understood kernel paradigm that works ... this would
        be helpful for the far distant time when chelsio no longer wants
        to maintain this driver.
     2. It includes useful and appropriate warnings for common bugs in
        refcounted code, such as use of a freed reference.

James