[PATCH] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[PATCH] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[Koskinen Aaro (NSN - FI/Helsinki)]

Fix for the sym53c8xx_2 driver to initialize lun's to_clear flag after
a bus reset (a failed clear can trigger a bus reset and it should not
be attemped again after that).

Signed-off-by: aaro.koskinen@nsn.com

---

diff -uprN -X linux-2.6.27.5-orig/Documentation/dontdiff linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.c linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.c
--- linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2008-11-07 19:55:34.000000000 +0200
+++ linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.c	2008-11-19 12:20:27.000000000 +0200
@@ -1897,6 +1897,15 @@ void sym_start_up(struct Scsi_Host *shos
 		tp->head.sval = 0;
 		tp->head.wval = np->rv_scntl3;
 		tp->head.uval = 0;
+		if (tp->lun0p)
+			tp->lun0p->to_clear = 0;
+		if (tp->lunmp) {
+			int ln;
+
+			for (ln = 1; ln < SYM_CONF_MAX_LUN; ln++)
+				if (tp->lunmp[ln])
+					tp->lunmp[ln]->to_clear = 0;
+		}
 	}
 
 	/*

Re: [PATCH] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[Mike Christie]

Koskinen Aaro (NSN - FI/Helsinki) wrote:
> Fix for the sym53c8xx_2 driver to initialize lun's to_clear flag after
> a bus reset (a failed clear can trigger a bus reset and it should not
> be attemped again after that).
> 
> Signed-off-by: aaro.koskinen@nsn.com
> 

I think you just want to fix up your signed off line to be: 
Signed-off-by: Aaro Koskinen <aaro.koskinen@nsn.com>

James might to that for you when he merges it so you do not have to resend.


> ---
> 
> diff -uprN -X linux-2.6.27.5-orig/Documentation/dontdiff linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.c linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.c
> --- linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2008-11-07 19:55:34.000000000 +0200
> +++ linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.c	2008-11-19 12:20:27.000000000 +0200
> @@ -1897,6 +1897,15 @@ void sym_start_up(struct Scsi_Host *shos
>  		tp->head.sval = 0;
>  		tp->head.wval = np->rv_scntl3;
>  		tp->head.uval = 0;
> +		if (tp->lun0p)
> +			tp->lun0p->to_clear = 0;
> +		if (tp->lunmp) {
> +			int ln;
> +
> +			for (ln = 1; ln < SYM_CONF_MAX_LUN; ln++)
> +				if (tp->lunmp[ln])
> +					tp->lunmp[ln]->to_clear = 0;
> +		}
>  	}
>  
>  	/*
> --
> 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

Re: [PATCH] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[Aaro Koskinen]

(Resent with proper formatting)

Fix for the sym53c8xx_2 driver to initialize lun's to_clear flag after
a bus reset (a failed clear can trigger a bus reset and it should not
be attemped again after that).

Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
---
 drivers/scsi/sym53c8xx_2/sym_hipd.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..b126a7a 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -1897,6 +1897,15 @@ void sym_start_up(struct Scsi_Host *shost, int reason)
 		tp->head.sval = 0;
 		tp->head.wval = np->rv_scntl3;
 		tp->head.uval = 0;
+		if (tp->lun0p)
+			tp->lun0p->to_clear = 0;
+		if (tp->lunmp) {
+			int ln;
+
+			for (ln = 1; ln < SYM_CONF_MAX_LUN; ln++)
+				if (tp->lunmp[ln])
+					tp->lunmp[ln]->to_clear = 0;
+		}
 	}
 
 	/*
-- 
1.5.4.3

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Tony Battersby]

Just found another problem with this patch: sym_alloc_lcb() allocates
memory with GFP_KERNEL, and you are calling it while holding a spinlock
(np->s.host->host_lock).  Either do the allocation without holding the
spinlock, or else use GFP_ATOMIC.

BUG: sleeping function called from invalid context at mm/slab.c:3043
in_atomic():0, irqs_disabled():1
2 locks held by insmod/1216:
 #0:  (&shost->scan_mutex){--..}, at: [<c026386b>]
scsi_scan_host_selected+0x4b/0x140
 #1:  (shost->host_lock){++..}, at: [<d092eb5c>]
sym53c8xx_slave_alloc+0x4c/0x190 [sym53c8xx]
irq event stamp: 7322
hardirqs last  enabled at (7321): [<c0145d5b>] trace_hardirqs_on+0xb/0x10
hardirqs last disabled at (7322): [<c014120b>] trace_hardirqs_off+0xb/0x10
softirqs last  enabled at (7204): [<c0127c32>] __do_softirq+0x102/0x120
softirqs last disabled at (7185): [<c0127ca7>] do_softirq+0x57/0x60
Pid: 1216, comm: insmod Not tainted 2.6.27.10 #2
 [<c0127ca7>] ? do_softirq+0x57/0x60
 [<c011b2e6>] __might_sleep+0xc6/0xf0
 [<c016bcfd>] __kmalloc+0x11d/0x150
 [<d09304b1>] ? sym_alloc_lcb+0xe1/0x180 [sym53c8xx]
 [<d09304b1>] sym_alloc_lcb+0xe1/0x180 [sym53c8xx]
 [<d092ec02>] sym53c8xx_slave_alloc+0xf2/0x190 [sym53c8xx]
 [<c026236f>] scsi_alloc_sdev+0x18f/0x200
 [<c025aac0>] ? scsi_device_lookup_by_target+0x60/0x80
 [<c026254d>] scsi_probe_and_add_lun+0xcd/0xb40
 [<c02632da>] __scsi_scan_target+0x20a/0x6c0
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c0327302>] ? __mutex_lock_common+0x1f2/0x2f0
 [<c026386b>] ? scsi_scan_host_selected+0x4b/0x140
 [<c0263802>] scsi_scan_channel+0x72/0x90
 [<c02638ed>] scsi_scan_host_selected+0xcd/0x140
 [<c0265eaa>] ? scsi_proc_host_add+0x4a/0xa0
 [<c02639d6>] do_scsi_scan_host+0x76/0x80
 [<c0263c8a>] scsi_scan_host+0x15a/0x190
 [<c0328ab9>] ? _spin_unlock_irqrestore+0x49/0x60
 [<d0937c8a>] sym2_probe+0x89a/0x92e [sym53c8xx]
 [<c01f4e2e>] pci_device_probe+0x5e/0x80
 [<c024717e>] driver_probe_device+0x7e/0x170
 [<c02472e5>] __driver_attach+0x75/0x80
 [<c0246a59>] bus_for_each_dev+0x49/0x70
 [<c0246ff9>] driver_attach+0x19/0x20
 [<c0247270>] ? __driver_attach+0x0/0x80
 [<c024635c>] bus_add_driver+0xac/0x220
 [<c01f4a40>] ? pci_device_remove+0x0/0x40
 [<c024747f>] driver_register+0x4f/0x120
 [<c01eb9b2>] ? __spin_lock_init+0x32/0x60
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<c01f4cae>] __pci_register_driver+0x5e/0xa0
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<d0864087>] sym2_init+0x87/0xf6 [sym53c8xx]
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<c010102a>] _stext+0x2a/0x140
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c014d725>] sys_init_module+0x85/0x1b0
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c01ddb94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0103031>] sysenter_do_call+0x12/0x35
 [<c01211f8>] ? __mmdrop+0x28/0x30
 =======================

Tony Battersby
Cybernetics
 

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Aaro Koskinen]

Thanks. The updated patch is below.

...

Make the sym53c8xx_2 driver slave_alloc/destroy less unsafe. References
to the destroyed LCB are cleared from the target structure (instead of
leaving a dangling pointer), and when the last LCB for the target is
destroyed the reference to the upper layer target data is cleared. The
host lock is used to prevent a race with the interrupt handler. Also
user commands are prevented for targets with all LCBs destroyed.

Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
---
 drivers/scsi/sym53c8xx_2/sym_glue.c |   66 +++++++++++++++++++++++++++-------
 drivers/scsi/sym53c8xx_2/sym_hipd.c |   40 ++++++++++++++++++++-
 drivers/scsi/sym53c8xx_2/sym_hipd.h |    2 +
 3 files changed, 93 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
index d39107b..073e5b6 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.c
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
@@ -737,11 +737,14 @@ static int sym53c8xx_slave_alloc(struct scsi_device *sdev)
 	struct sym_hcb *np = sym_get_hcb(sdev->host);
 	struct sym_tcb *tp = &np->target[sdev->id];
 	struct sym_lcb *lp;
+	unsigned long flags;
+	int error;
 
 	if (sdev->id >= SYM_CONF_MAX_TARGET || sdev->lun >= SYM_CONF_MAX_LUN)
 		return -ENXIO;
 
-	tp->starget = sdev->sdev_target;
+	spin_lock_irqsave(np->s.host->host_lock, flags);
+
 	/*
 	 * Fail the device init if the device is flagged NOSCAN at BOOT in
 	 * the NVRAM.  This may speed up boot and maintain coherency with
@@ -753,26 +756,37 @@ static int sym53c8xx_slave_alloc(struct scsi_device *sdev)
 
 	if (tp->usrflags & SYM_SCAN_BOOT_DISABLED) {
 		tp->usrflags &= ~SYM_SCAN_BOOT_DISABLED;
-		starget_printk(KERN_INFO, tp->starget,
+		starget_printk(KERN_INFO, sdev->sdev_target,
 				"Scan at boot disabled in NVRAM\n");
-		return -ENXIO;
+		error = -ENXIO;
+		goto out;
 	}
 
 	if (tp->usrflags & SYM_SCAN_LUNS_DISABLED) {
-		if (sdev->lun != 0)
-			return -ENXIO;
-		starget_printk(KERN_INFO, tp->starget,
+		if (sdev->lun != 0) {
+			error = -ENXIO;
+			goto out;
+		}
+		starget_printk(KERN_INFO, sdev->sdev_target,
 				"Multiple LUNs disabled in NVRAM\n");
 	}
 
 	lp = sym_alloc_lcb(np, sdev->id, sdev->lun);
-	if (!lp)
-		return -ENOMEM;
+	if (!lp) {
+		error = -ENOMEM;
+		goto out;
+	}
+	if (tp->nlcb == 1)
+		tp->starget = sdev->sdev_target;
 
 	spi_min_period(tp->starget) = tp->usr_period;
 	spi_max_width(tp->starget) = tp->usr_width;
 
-	return 0;
+	error = 0;
+out:
+	spin_unlock_irqrestore(np->s.host->host_lock, flags);
+
+	return error;
 }
 
 /*
@@ -819,12 +833,34 @@ static int sym53c8xx_slave_configure(struct scsi_device *sdev)
 static void sym53c8xx_slave_destroy(struct scsi_device *sdev)
 {
 	struct sym_hcb *np = sym_get_hcb(sdev->host);
-	struct sym_lcb *lp = sym_lp(&np->target[sdev->id], sdev->lun);
+	struct sym_tcb *tp = &np->target[sdev->id];
+	struct sym_lcb *lp = sym_lp(tp, sdev->lun);
+	unsigned long flags;
+
+	spin_lock_irqsave(np->s.host->host_lock, flags);
+
+	if (lp->busy_itlq || lp->busy_itl) {
+		/*
+		 * This really shouldn't happen, but we can't return an error
+		 * so let's try to stop all on-going I/O.
+		 */
+		starget_printk(KERN_WARNING, tp->starget,
+			       "Removing busy LCB (%d)\n", sdev->lun);
+		sym_reset_scsi_bus(np, 1);
+	}
 
-	if (lp->itlq_tbl)
-		sym_mfree_dma(lp->itlq_tbl, SYM_CONF_MAX_TASK * 4, "ITLQ_TBL");
-	kfree(lp->cb_tags);
-	sym_mfree_dma(lp, sizeof(*lp), "LCB");
+	if (sym_free_lcb(np, sdev->id, sdev->lun) == 0) {
+		/*
+		 * It was the last unit for this target.
+		 */
+		tp->head.sval        = 0;
+		tp->head.wval        = np->rv_scntl3;
+		tp->head.uval        = 0;
+		tp->tgoal.check_nego = 1;
+		tp->starget	     = NULL;
+	}
+
+	spin_unlock_irqrestore(np->s.host->host_lock, flags);
 }
 
 /*
@@ -890,6 +926,8 @@ static void sym_exec_user_command (struct sym_hcb *np, struct sym_usrcmd *uc)
 			if (!((uc->target >> t) & 1))
 				continue;
 			tp = &np->target[t];
+			if (!tp->nlcb)
+				continue;
 
 			switch (uc->cmd) {
 
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..a38ed04 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -4953,7 +4953,7 @@ struct sym_lcb *sym_alloc_lcb (struct sym_hcb *np, u_char tn, u_char ln)
 	 */
 	if (ln && !tp->lunmp) {
 		tp->lunmp = kcalloc(SYM_CONF_MAX_LUN, sizeof(struct sym_lcb *),
-				GFP_KERNEL);
+				GFP_ATOMIC);
 		if (!tp->lunmp)
 			goto fail;
 	}
@@ -4973,6 +4973,7 @@ struct sym_lcb *sym_alloc_lcb (struct sym_hcb *np, u_char tn, u_char ln)
 		tp->lun0p = lp;
 		tp->head.lun0_sa = cpu_to_scr(vtobus(lp));
 	}
+	tp->nlcb++;
 
 	/*
 	 *  Let the itl task point to error handling.
@@ -5050,6 +5051,43 @@ fail:
 }
 
 /*
+ *  Lun control block deallocation. Returns the number of valid remaing LCBs
+ *  for the target.
+ */
+int sym_free_lcb(struct sym_hcb *np, u_char tn, u_char ln)
+{
+	struct sym_tcb *tp = &np->target[tn];
+	struct sym_lcb *lp = sym_lp(tp, ln);
+
+	tp->nlcb--;
+
+	if (ln) {
+		if (!tp->nlcb) {
+			kfree(tp->lunmp);
+			sym_mfree_dma(tp->luntbl, 256, "LUNTBL");
+			tp->lunmp = NULL;
+			tp->luntbl = NULL;
+			tp->head.luntbl_sa = cpu_to_scr(vtobus(np->badluntbl));
+		} else {
+			tp->luntbl[ln] = cpu_to_scr(vtobus(&np->badlun_sa));
+			tp->lunmp[ln] = NULL;
+		}
+	} else {
+		tp->lun0p = NULL;
+		tp->head.lun0_sa = cpu_to_scr(vtobus(&np->badlun_sa));
+	}
+
+	if (lp->itlq_tbl) {
+		sym_mfree_dma(lp->itlq_tbl, SYM_CONF_MAX_TASK*4, "ITLQ_TBL");
+		kfree(lp->cb_tags);
+	}
+
+	sym_mfree_dma(lp, sizeof(*lp), "LCB");
+
+	return tp->nlcb;
+}
+
+/*
  *  Queue a SCSI IO to the controller.
  */
 int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *cmd, struct sym_ccb *cp)
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.h b/drivers/scsi/sym53c8xx_2/sym_hipd.h
index ad07880..2ec25e1 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.h
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.h
@@ -400,6 +400,7 @@ struct sym_tcb {
 	 *  An array of bus addresses is used on reselection.
 	 */
 	u32	*luntbl;	/* LCBs bus address table	*/
+	int	nlcb;		/* Number of valid LCBs (including LUN #0) */
 
 	/*
 	 *  LUN table used by the C code.
@@ -1061,6 +1062,7 @@ int sym_clear_tasks(struct sym_hcb *np, int cam_status, int target, int lun, int
 struct sym_ccb *sym_get_ccb(struct sym_hcb *np, struct scsi_cmnd *cmd, u_char tag_order);
 void sym_free_ccb(struct sym_hcb *np, struct sym_ccb *cp);
 struct sym_lcb *sym_alloc_lcb(struct sym_hcb *np, u_char tn, u_char ln);
+int sym_free_lcb(struct sym_hcb *np, u_char tn, u_char ln);
 int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *csio, struct sym_ccb *cp);
 int sym_abort_scsiio(struct sym_hcb *np, struct scsi_cmnd *ccb, int timed_out);
 int sym_reset_scsi_target(struct sym_hcb *np, int target);
-- 
1.5.4.3

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[James Bottomley]

On Tue, 2008-12-30 at 12:10 +0200, Aaro Koskinen wrote:
> Thanks. The updated patch is below.

Tony,

Since we're lacking an active maintainer on this one and you seem
interested, could you run these patches through your tests to make sure
they're reasonably OK and then respond with an ack?

Thanks,

James

Re: [PATCH] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[Tony Battersby]

This patch works as advertised and fixes a real bug.

Tested-by: Tony Battersby <tonyb@cybernetics.com>