[PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects

[PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects

[Tony Battersby]

This patch fixes the sym53c8xx "setflag" user command to control
disconnect privilege, which has been broken for a long time.

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

NOTE regarding the following change:

 	can_disconnect = (cp->tag != NO_TAG) ||
-		(lp && (lp->curr_flags & SYM_DISC_ENABLED));
+		(lp && (tp->usrflags & SYM_DISC_ENABLED));

In 2.4 kernels, lp == NULL when scanning for devices, and allowing
disconnect when lp == NULL would confuse the code that handles
reselection.  So with 2.4 kernels, the check for lp != NULL had to be
left in even if lp wasn't being dereferenced.  In 2.6 kernels,
lp != NULL when scanning for devices.  In fact, I didn't encounter any
cases where lp == NULL during my testing with 2.6 kernels, so the check
for lp != NULL may be superfluous now.  However, the same check is
performed in other places in the same function, so I left it in to be
safe.

diff -urpN linux-2.6.24-rc2/drivers/scsi/sym53c8xx_2/sym_glue.c linux-2.6.24-rc2-sym2/drivers/scsi/sym53c8xx_2/sym_glue.c
--- linux-2.6.24-rc2/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-11-07 15:05:22.000000000 -0500
+++ linux-2.6.24-rc2-sym2/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-11-07 15:06:41.000000000 -0500
@@ -785,11 +785,6 @@ static int sym53c8xx_slave_configure(str
 	int reqtags, depth_to_use;
 
 	/*
-	 *  Get user flags.
-	 */
-	lp->curr_flags = lp->user_flags;
-
-	/*
 	 *  Select queue depth from driver setup.
 	 *  Donnot use more than configured by user.
 	 *  Use at least 2.
@@ -937,7 +932,9 @@ static void sym_exec_user_command (struc
 				OUTB(np, nc_istat, SIGP|SEM);
 				break;
 			case UC_SETFLAG:
-				tp->usrflags = uc->data;
+				tp->usrflags =
+					(tp->usrflags & ~SYM_DISC_ENABLED) |
+					uc->data;
 				break;
 			}
 		}
@@ -1098,6 +1095,7 @@ printk("sym_user_command: data=%ld\n", u
 		break;
 #endif /* SYM_LINUX_DEBUG_CONTROL_SUPPORT */
 	case UC_SETFLAG:
+		uc->data = SYM_DISC_ENABLED;
 		while (len > 0) {
 			SKIP_SPACES(ptr, len);
 			if	((arg_len = is_keyword(ptr, len, "no_disc")))
diff -urpN linux-2.6.24-rc2/drivers/scsi/sym53c8xx_2/sym_hipd.c linux-2.6.24-rc2-sym2/drivers/scsi/sym53c8xx_2/sym_hipd.c
--- linux-2.6.24-rc2/drivers/scsi/sym53c8xx_2/sym_hipd.c	2007-11-07 15:05:22.000000000 -0500
+++ linux-2.6.24-rc2-sym2/drivers/scsi/sym53c8xx_2/sym_hipd.c	2007-11-07 15:06:41.000000000 -0500
@@ -4984,11 +4984,6 @@ struct sym_lcb *sym_alloc_lcb (struct sy
 	 */
 	lp->head.resel_sa = cpu_to_scr(SCRIPTB_BA(np, resel_bad_lun));
 
-	/*
-	 *  Set user capabilities.
-	 */
-	lp->user_flags = tp->usrflags & (SYM_DISC_ENABLED | SYM_TAGS_ENABLED);
-
 #ifdef SYM_OPT_HANDLE_DEVICE_QUEUEING
 	/*
 	 *  Initialize device queueing.
@@ -5077,7 +5072,7 @@ int sym_queue_scsiio(struct sym_hcb *np,
 	lp = sym_lp(tp, sdev->lun);
 
 	can_disconnect = (cp->tag != NO_TAG) ||
-		(lp && (lp->curr_flags & SYM_DISC_ENABLED));
+		(lp && (tp->usrflags & SYM_DISC_ENABLED));
 
 	msgptr = cp->scsi_smsg;
 	msglen = 0;
diff -urpN linux-2.6.24-rc2/drivers/scsi/sym53c8xx_2/sym_hipd.h linux-2.6.24-rc2-sym2/drivers/scsi/sym53c8xx_2/sym_hipd.h
--- linux-2.6.24-rc2/drivers/scsi/sym53c8xx_2/sym_hipd.h	2007-11-07 15:05:22.000000000 -0500
+++ linux-2.6.24-rc2-sym2/drivers/scsi/sym53c8xx_2/sym_hipd.h	2007-11-07 15:06:41.000000000 -0500
@@ -536,12 +536,6 @@ struct sym_lcb {
 	 *  Set when we want to clear all tasks.
 	 */
 	u_char to_clear;
-
-	/*
-	 *  Capabilities.
-	 */
-	u_char	user_flags;
-	u_char	curr_flags;
 };
 
 /*

Re: [PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects

[James Bottomley]

On Wed, 2007-11-07 at 15:37 -0500, Tony Battersby wrote: 
> This patch fixes the sym53c8xx "setflag" user command to control
> disconnect privilege, which has been broken for a long time.

The first observation is that the sym specific setflags interface is
being replaced by the parallel transport specific sysfs interface.
However, as you've probably noticed, the SPI transport has no flag for
disconnect permission, so how important actually is this?  If it's
important, then we can put it in scsi_transport_spi ... if not, then we
should probably just eliminate it from sym2.

James

Re: [PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects

[Tony Battersby]

James Bottomley wrote:
> On Wed, 2007-11-07 at 15:37 -0500, Tony Battersby wrote: 
>   
>> This patch fixes the sym53c8xx "setflag" user command to control
>> disconnect privilege, which has been broken for a long time.
>>     
>
> The first observation is that the sym specific setflags interface is
> being replaced by the parallel transport specific sysfs interface.
> However, as you've probably noticed, the SPI transport has no flag for
> disconnect permission, so how important actually is this?  If it's
> important, then we can put it in scsi_transport_spi ... if not, then we
> should probably just eliminate it from sym2.
>
> James
Most people would probably just use the HBA's BIOS utility to configure
the NVRAM to disallow disconnects if needed.  However, in my case, the
HBA is inside an embedded device sold by my company, and customers can
attach their own SCSI devices to it.  There is one type of SCSI device
that we support which doesn't work with disconnect privilege enabled, so
my code running in the embedded device needs to be able to
enable/disable disconnect privilege dynamically based on whether or not
the customer has attached this specific broken device.

It would be nice to have a generic way to control it.  That would save
me the bother of sending my patch to control disconnect privilege for
LSI MPT Fusion Ultra320 HBAs via mptctl.  ;-)

Tony

Re: [PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects

[Christoph Hellwig]

On Thu, Nov 08, 2007 at 01:11:01PM -0500, Tony Battersby wrote:
> HBA is inside an embedded device sold by my company, and customers can
> attach their own SCSI devices to it.  There is one type of SCSI device
> that we support which doesn't work with disconnect privilege enabled, so
> my code running in the embedded device needs to be able to
> enable/disable disconnect privilege dynamically based on whether or not
> the customer has attached this specific broken device.
> 
> It would be nice to have a generic way to control it.  That would save
> me the bother of sending my patch to control disconnect privilege for
> LSI MPT Fusion Ultra320 HBAs via mptctl.  ;-)

Yes, the embedded space is a good enough reason to implement it in the
SPI transport class.  And I think we should take that as an opportunity
to kill the sym2 /proc support or at least the write support.