[PATCH] fusion update to current APIs

[PATCH] fusion update to current APIs

[Christoph Hellwig]

 - kill scsi_to_pci_dma_dir usage, both pci and scsi use the same bits
 - kill mptscsih_io_direction and always trust the midlayer
 - kill usage of Scsi_Foo typedefs
 - use <scsi/*.h> headers


--- 1.22/drivers/message/fusion/mptctl.c	2004-05-29 17:10:51 +02:00
+++ edited/drivers/message/fusion/mptctl.c	2004-05-31 13:44:28 +02:00
@@ -84,13 +84,11 @@
 #include <linux/miscdevice.h>
 #include <linux/smp_lock.h>
 
+#include <scsi/scsi_host.h>
+
 #include <asm/io.h>
 #include <asm/uaccess.h>
 
-#include <linux/kdev_t.h>	/* needed for access to Scsi_Host struct */
-#include <linux/blkdev.h>
-#include "../../scsi/scsi.h"
-#include "../../scsi/hosts.h"
 
 #define COPYRIGHT	"Copyright (c) 1999-2004 LSI Logic Corporation"
 #define MODULEAUTHOR	"Steven J. Ralston, Noah Romer, Pamela Delaney"
===== drivers/message/fusion/mptscsih.c 1.41 vs edited =====
--- 1.41/drivers/message/fusion/mptscsih.c	2004-05-29 17:10:51 +02:00
+++ edited/drivers/message/fusion/mptscsih.c	2004-05-31 13:07:29 +02:00
@@ -76,8 +76,12 @@
 #include <linux/reboot.h>	/* notifier code */
 #include <linux/sched.h>
 #include <linux/workqueue.h>
-#include "../../scsi/scsi.h"
+
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_tcq.h>
 
 
 #include "mptbase.h"
@@ -154,16 +158,16 @@
  *  Other private/forward protos...
  */
 static int	mptscsih_io_done(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf, MPT_FRAME_HDR *r);
-static void	mptscsih_report_queue_full(Scsi_Cmnd *sc, SCSIIOReply_t *pScsiReply, SCSIIORequest_t *pScsiReq);
+static void	mptscsih_report_queue_full(struct scsi_cmnd *sc, SCSIIOReply_t *pScsiReply, SCSIIORequest_t *pScsiReq);
 static int	mptscsih_taskmgmt_complete(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf, MPT_FRAME_HDR *r);
 
-static int	mptscsih_AddSGE(MPT_SCSI_HOST *hd, Scsi_Cmnd *SCpnt,
+static int	mptscsih_AddSGE(MPT_SCSI_HOST *hd, struct scsi_cmnd *SCpnt,
 				 SCSIIORequest_t *pReq, int req_idx);
 static void	mptscsih_freeChainBuffers(MPT_SCSI_HOST *hd, int req_idx);
 static int	mptscsih_initChainBuffers (MPT_SCSI_HOST *hd, int init);
-static void	copy_sense_data(Scsi_Cmnd *sc, MPT_SCSI_HOST *hd, MPT_FRAME_HDR *mf, SCSIIOReply_t *pScsiReply);
+static void	copy_sense_data(struct scsi_cmnd *sc, MPT_SCSI_HOST *hd, MPT_FRAME_HDR *mf, SCSIIOReply_t *pScsiReply);
 static int	mptscsih_tm_pending_wait(MPT_SCSI_HOST * hd);
-static u32	SCPNT_TO_LOOKUP_IDX(Scsi_Cmnd *sc);
+static u32	SCPNT_TO_LOOKUP_IDX(struct scsi_cmnd *sc);
 static MPT_FRAME_HDR *mptscsih_search_pendingQ(MPT_SCSI_HOST *hd, int scpnt_idx);
 static void	post_pendingQ_commands(MPT_SCSI_HOST *hd);
 
@@ -248,105 +252,12 @@
 	driver_setup = MPTSCSIH_DRIVER_SETUP;
 
 #ifdef MPTSCSIH_DBG_TIMEOUT
-static Scsi_Cmnd *foo_to[8];
+static struct scsi_cmnd *foo_to[8];
 #endif
 
 static struct scsi_host_template driver_template;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
-/*
- *  Private inline routines...
- */
-/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
-/* 19991030 -sralston
- *  Return absolute SCSI data direction:
- *     1 = _DATA_OUT
- *     0 = _DIR_NONE
- *    -1 = _DATA_IN
- *
- * Changed: 3-20-2002 pdelaney to use the default data
- * direction and the defines set up in the
- * 2.4 kernel series
- *     1 = _DATA_OUT	changed to SCSI_DATA_WRITE (1)
- *     0 = _DIR_NONE	changed to SCSI_DATA_NONE (3)
- *    -1 = _DATA_IN	changed to SCSI_DATA_READ (2)
- * If the direction is unknown, fall through to original code.
- *
- * Mid-layer bug fix(): sg interface generates the wrong data
- * direction in some cases. Set the direction the hard way for
- * the most common commands.
- */
-static inline int
-mptscsih_io_direction(Scsi_Cmnd *cmd)
-{
-	switch (cmd->cmnd[0]) {
-	case WRITE_6:
-	case WRITE_10:
-	case WRITE_16:
-		return SCSI_DATA_WRITE;
-		break;
-	case READ_6:
-	case READ_10:
-	case READ_16:
-		return SCSI_DATA_READ;
-		break;
-	}
-
-	if (cmd->sc_data_direction != SCSI_DATA_UNKNOWN)
-		return cmd->sc_data_direction;
-
-	switch (cmd->cmnd[0]) {
-	/*  _DATA_OUT commands	*/
-	case WRITE_6:		case WRITE_10:		case WRITE_12:
-	case WRITE_16:
-	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
-	case WRITE_VERIFY:	case WRITE_VERIFY_12:
-	case COMPARE:		case COPY:		case COPY_VERIFY:
-	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
-	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
-	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
-	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
-	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
-	case REASSIGN_BLOCKS:
-	case PERSISTENT_RESERVE_OUT:
-	case 0xea:
-	case 0xa3:
-		return SCSI_DATA_WRITE;
-
-	/*  No data transfer commands  */
-	case SEEK_6:		case SEEK_10:
-	case RESERVE:		case RELEASE:
-	case TEST_UNIT_READY:
-	case START_STOP:
-	case ALLOW_MEDIUM_REMOVAL:
-		return SCSI_DATA_NONE;
-
-	/*  Conditional data transfer commands	*/
-	case FORMAT_UNIT:
-		if (cmd->cmnd[1] & 0x10)	/* FmtData (data out phase)? */
-			return SCSI_DATA_WRITE;
-		else
-			return SCSI_DATA_NONE;
-
-	case VERIFY:
-		if (cmd->cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
-			return SCSI_DATA_WRITE;
-		else
-			return SCSI_DATA_NONE;
-
-	case RESERVE_10:
-		if (cmd->cmnd[1] & 0x03)	/* RESERVE:{LongID|Extent} (data out phase)? */
-			return SCSI_DATA_WRITE;
-		else
-			return SCSI_DATA_NONE;
-
-	/*  Must be data _IN!  */
-	default:
-		return SCSI_DATA_READ;
-	}
-} /* mptscsih_io_direction() */
-
-/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
  *	mptscsih_add_sge - Place a simple SGE at address pAddr.
  *	@pAddr: virtual address for SGE
@@ -458,13 +369,13 @@
  *	mptscsih_AddSGE - Add a SGE (plus chain buffers) to the
  *	SCSIIORequest_t Message Frame.
  *	@hd: Pointer to MPT_SCSI_HOST structure
- *	@SCpnt: Pointer to Scsi_Cmnd structure
+ *	@SCpnt: Pointer to scsi_cmnd structure
  *	@pReq: Pointer to SCSIIORequest_t structure
  *
  *	Returns ...
  */
 static int
-mptscsih_AddSGE(MPT_SCSI_HOST *hd, Scsi_Cmnd *SCpnt,
+mptscsih_AddSGE(MPT_SCSI_HOST *hd, struct scsi_cmnd *SCpnt,
 		SCSIIORequest_t *pReq, int req_idx)
 {
 	char 	*psge;
@@ -498,7 +409,7 @@
 		sges_left = pci_map_sg(hd->ioc->pcidev,
 			       (struct scatterlist *) SCpnt->request_buffer,
 			       SCpnt->use_sg,
-			       scsi_to_pci_dma_dir(SCpnt->sc_data_direction));
+			       SCpnt->sc_data_direction);
 		if (sges_left == 0)
 			return FAILED;
 	} else if (SCpnt->request_bufflen) {
@@ -508,7 +419,7 @@
 		buf_dma_addr = pci_map_single(hd->ioc->pcidev,
 				      SCpnt->request_buffer,
 				      SCpnt->request_bufflen,
-				      scsi_to_pci_dma_dir(SCpnt->sc_data_direction));
+				      SCpnt->sc_data_direction);
 
 		/* We hide it here for later unmap. */
 		my_priv = (scPrivate *) &SCpnt->SCp;
@@ -708,7 +619,7 @@
 static int
 mptscsih_io_done(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf, MPT_FRAME_HDR *mr)
 {
-	Scsi_Cmnd	*sc;
+	struct scsi_cmnd	*sc;
 	MPT_SCSI_HOST	*hd;
 	SCSIIORequest_t	*pScsiReq;
 	SCSIIOReply_t	*pScsiReply;
@@ -946,14 +857,14 @@
 	/* Unmap the DMA buffers, if any. */
 	if (sc->use_sg) {
 		pci_unmap_sg(ioc->pcidev, (struct scatterlist *) sc->request_buffer,
-			    sc->use_sg, scsi_to_pci_dma_dir(sc->sc_data_direction));
+			    sc->use_sg, sc->sc_data_direction);
 	} else if (sc->request_bufflen) {
 		scPrivate	*my_priv;
 
 		my_priv = (scPrivate *) &sc->SCp;
 		pci_unmap_single(ioc->pcidev, (dma_addr_t)(ulong)my_priv->p1,
 			   sc->request_bufflen,
-			   scsi_to_pci_dma_dir(sc->sc_data_direction));
+			   sc->sc_data_direction);
 	}
 
 	hd->ScsiLookup[req_idx] = NULL;
@@ -974,7 +885,7 @@
 flush_doneQ(MPT_SCSI_HOST *hd)
 {
 	MPT_DONE_Q	*buffer;
-	Scsi_Cmnd	*SCpnt;
+	struct scsi_cmnd	*SCpnt;
 	unsigned long	 flags;
 
 	/* Flush the doneQ.
@@ -992,9 +903,9 @@
 		 */
 		Q_DEL_ITEM(buffer);
 
-		/* Set the Scsi_Cmnd pointer
+		/* Set the struct scsi_cmnd pointer
 		 */
-		SCpnt = (Scsi_Cmnd *) buffer->argp;
+		SCpnt = (struct scsi_cmnd *) buffer->argp;
 		buffer->argp = NULL;
 
 		/* Add to the freeQ
@@ -1015,7 +926,7 @@
  * Calling function will finish processing.
  */
 static void
-search_doneQ_for_cmd(MPT_SCSI_HOST *hd, Scsi_Cmnd *SCpnt)
+search_doneQ_for_cmd(MPT_SCSI_HOST *hd, struct scsi_cmnd *SCpnt)
 {
 	unsigned long	 flags;
 	MPT_DONE_Q	*buffer;
@@ -1024,12 +935,12 @@
 	if (!Q_IS_EMPTY(&hd->doneQ)) {
 		buffer = hd->doneQ.head;
 		do {
-			Scsi_Cmnd *sc = (Scsi_Cmnd *) buffer->argp;
+			struct scsi_cmnd *sc = (struct scsi_cmnd *) buffer->argp;
 			if (SCpnt == sc) {
 				Q_DEL_ITEM(buffer);
 				SCpnt->result = sc->result;
 
-				/* Set the Scsi_Cmnd pointer
+				/* Set the struct scsi_cmnd pointer
 				 */
 				buffer->argp = NULL;
 
@@ -1057,7 +968,7 @@
 static void
 mptscsih_flush_running_cmds(MPT_SCSI_HOST *hd)
 {
-	Scsi_Cmnd	*SCpnt;
+	struct scsi_cmnd	*SCpnt;
 	MPT_FRAME_HDR	*mf;
 	MPT_DONE_Q	*buffer;
 	int		 ii;
@@ -1093,7 +1004,7 @@
 					pci_unmap_sg(hd->ioc->pcidev,
 						(struct scatterlist *) SCpnt->request_buffer,
 						SCpnt->use_sg,
-						scsi_to_pci_dma_dir(SCpnt->sc_data_direction));
+						SCpnt->sc_data_direction);
 				} else if (SCpnt->request_bufflen) {
 					scPrivate	*my_priv;
 
@@ -1101,7 +1012,7 @@
 					pci_unmap_single(hd->ioc->pcidev,
 						(dma_addr_t)(ulong)my_priv->p1,
 						SCpnt->request_bufflen,
-						scsi_to_pci_dma_dir(SCpnt->sc_data_direction));
+						SCpnt->sc_data_direction);
 				}
 			}
 			SCpnt->result = DID_RESET << 16;
@@ -1123,7 +1034,7 @@
 				buffer = hd->freeQ.head;
 				Q_DEL_ITEM(buffer);
 
-				/* Set the Scsi_Cmnd pointer
+				/* Set the struct scsi_cmnd pointer
 				 */
 				buffer->argp = (void *)SCpnt;
 
@@ -1149,7 +1060,7 @@
  *	mptscsih_search_running_cmds - Delete any commands associated
  *		with the specified target and lun. Function called only
  *		when a lun is disable by mid-layer.
- *		Do NOT access the referenced Scsi_Cmnd structure or
+ *		Do NOT access the referenced scsi_cmnd structure or
  *		members. Will cause either a paging or NULL ptr error.
  *	@hd: Pointer to a SCSI HOST structure
  *	@target: target id
@@ -1307,7 +1218,7 @@
 /*
  *	mptscsih_report_queue_full - Report QUEUE_FULL status returned
  *	from a SCSI target device.
- *	@sc: Pointer to Scsi_Cmnd structure
+ *	@sc: Pointer to scsi_cmnd structure
  *	@pScsiReply: Pointer to SCSIIOReply_t
  *	@pScsiReq: Pointer to original SCSI request
  *
@@ -1316,7 +1227,7 @@
  *	printk() API call, not more than once every 10 seconds.
  */
 static void
-mptscsih_report_queue_full(Scsi_Cmnd *sc, SCSIIOReply_t *pScsiReply, SCSIIORequest_t *pScsiReq)
+mptscsih_report_queue_full(struct scsi_cmnd *sc, SCSIIOReply_t *pScsiReply, SCSIIORequest_t *pScsiReq)
 {
 	long time = jiffies;
 
@@ -1492,7 +1403,7 @@
 		hd->is_multipath = 1;
 	}
 
-	/* SCSI needs Scsi_Cmnd lookup table!
+	/* SCSI needs scsi_cmnd lookup table!
 	 * (with size equal to req_depth*PtrSz!)
 	 */
 	sz = hd->ioc->req_depth * sizeof(void *);
@@ -1966,7 +1877,7 @@
  *	mptscsih_info - Return information about MPT adapter
  *	@SChost: Pointer to Scsi_Host structure
  *
- *	(linux Scsi_Host_Template.info routine)
+ *	(linux scsi_host_template.info routine)
  *
  *	Returns pointer to buffer where information was written.
  */
@@ -2176,7 +2087,7 @@
 /**
  *	mptscsih_proc_info - Return information about MPT adapter
  *
- *	(linux Scsi_Host_Template.info routine)
+ *	(linux scsi_host_template.info routine)
  *
  * 	buffer: if write, user data; if read, buffer for user
  * 	length: if write, return length;
@@ -2224,17 +2135,17 @@
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
  *	mptscsih_qcmd - Primary Fusion MPT SCSI initiator IO start routine.
- *	@SCpnt: Pointer to Scsi_Cmnd structure
+ *	@SCpnt: Pointer to scsi_cmnd structure
  *	@done: Pointer SCSI mid-layer IO completion function
  *
- *	(linux Scsi_Host_Template.queuecommand routine)
+ *	(linux scsi_host_template.queuecommand routine)
  *	This is the primary SCSI IO start routine.  Create a MPI SCSIIORequest
- *	from a linux Scsi_Cmnd request and send it to the IOC.
+ *	from a linux scsi_cmnd request and send it to the IOC.
  *
  *	Returns 0. (rtn value discarded by linux scsi mid-layer)
  */
 int
-mptscsih_qcmd(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
+mptscsih_qcmd(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
 {
 	MPT_SCSI_HOST		*hd;
 	MPT_FRAME_HDR		*mf;
@@ -2244,7 +2155,6 @@
 	unsigned long		 flags;
 	int	 target;
 	int	 lun;
-	int	 datadir;
 	u32	 datalen;
 	u32	 scsictl;
 	u32	 scsidir;
@@ -2295,21 +2205,15 @@
 
 	ADD_INDEX_LOG(my_idx);
 
-	/*
-	 *  The scsi layer should be handling this stuff
-	 *  (In 2.3.x it does -DaveM)
-	 */
-
 	/*  BUG FIX!  19991030 -sralston
 	 *    TUR's being issued with scsictl=0x02000000 (DATA_IN)!
 	 *    Seems we may receive a buffer (datalen>0) even when there
 	 *    will be no data transfer!  GRRRRR...
 	 */
-	datadir = mptscsih_io_direction(SCpnt);
-	if (datadir == SCSI_DATA_READ) {
+	if (SCpnt->sc_data_direction == DMA_FROM_DEVICE) {
 		datalen = SCpnt->request_bufflen;
 		scsidir = MPI_SCSIIO_CONTROL_READ;	/* DATA IN  (host<--ioc<--dev) */
-	} else if (datadir == SCSI_DATA_WRITE) {
+	} else if (SCpnt->sc_data_direction == DMA_TO_DEVICE) {
 		datalen = SCpnt->request_bufflen;
 		scsidir = MPI_SCSIIO_CONTROL_WRITE;	/* DATA OUT (host-->ioc-->dev) */
 	} else {
@@ -2495,7 +2399,7 @@
 		buffer = hd->freeQ.head;
 		Q_DEL_ITEM(buffer);
 
-		/* Set the Scsi_Cmnd pointer
+		/* Set the scsi_cmnd pointer
 		 */
 		buffer->argp = (void *)SCpnt;
 
@@ -2789,15 +2693,15 @@
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
- *	mptscsih_abort - Abort linux Scsi_Cmnd routine, new_eh variant
- *	@SCpnt: Pointer to Scsi_Cmnd structure, IO to be aborted
+ *	mptscsih_abort - Abort linux scsi_cmnd routine, new_eh variant
+ *	@SCpnt: Pointer to scsi_cmnd structure, IO to be aborted
  *
- *	(linux Scsi_Host_Template.eh_abort_handler routine)
+ *	(linux scsi_host_template.eh_abort_handler routine)
  *
  *	Returns SUCCESS or FAILED.
  */
 int
-mptscsih_abort(Scsi_Cmnd * SCpnt)
+mptscsih_abort(struct scsi_cmnd * SCpnt)
 {
 	MPT_SCSI_HOST	*hd;
 	MPT_FRAME_HDR	*mf;
@@ -2892,14 +2796,14 @@
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
  *	mptscsih_dev_reset - Perform a SCSI TARGET_RESET!  new_eh variant
- *	@SCpnt: Pointer to Scsi_Cmnd structure, IO which reset is due to
+ *	@SCpnt: Pointer to scsi_cmnd structure, IO which reset is due to
  *
- *	(linux Scsi_Host_Template.eh_dev_reset_handler routine)
+ *	(linux scsi_host_template.eh_dev_reset_handler routine)
  *
  *	Returns SUCCESS or FAILED.
  */
 int
-mptscsih_dev_reset(Scsi_Cmnd * SCpnt)
+mptscsih_dev_reset(struct scsi_cmnd * SCpnt)
 {
 	MPT_SCSI_HOST	*hd;
 	spinlock_t	*host_lock = SCpnt->device->host->host_lock;
@@ -2947,14 +2851,14 @@
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
  *	mptscsih_bus_reset - Perform a SCSI BUS_RESET!	new_eh variant
- *	@SCpnt: Pointer to Scsi_Cmnd structure, IO which reset is due to
+ *	@SCpnt: Pointer to scsi_cmnd structure, IO which reset is due to
  *
- *	(linux Scsi_Host_Template.eh_bus_reset_handler routine)
+ *	(linux scsi_host_template.eh_bus_reset_handler routine)
  *
  *	Returns SUCCESS or FAILED.
  */
 int
-mptscsih_bus_reset(Scsi_Cmnd * SCpnt)
+mptscsih_bus_reset(struct scsi_cmnd * SCpnt)
 {
 	MPT_SCSI_HOST	*hd;
 	spinlock_t	*host_lock = SCpnt->device->host->host_lock;
@@ -3002,14 +2906,14 @@
 /**
  *	mptscsih_host_reset - Perform a SCSI host adapter RESET!
  *	new_eh variant
- *	@SCpnt: Pointer to Scsi_Cmnd structure, IO which reset is due to
+ *	@SCpnt: Pointer to scsi_cmnd structure, IO which reset is due to
  *
- *	(linux Scsi_Host_Template.eh_host_reset_handler routine)
+ *	(linux scsi_host_template.eh_host_reset_handler routine)
  *
  *	Returns SUCCESS or FAILED.
  */
 int
-mptscsih_host_reset(Scsi_Cmnd *SCpnt)
+mptscsih_host_reset(struct scsi_cmnd *SCpnt)
 {
 	MPT_SCSI_HOST *  hd;
 	int              status = SUCCESS;
@@ -3245,7 +3140,7 @@
  *	Init memory once per id (not LUN).
  */
 int
-mptscsih_slave_alloc(Scsi_Device *device)
+mptscsih_slave_alloc(struct scsi_device *device)
 {
 	struct Scsi_Host	*host = device->host;
 	MPT_SCSI_HOST		*hd;
@@ -3285,7 +3180,7 @@
  *	Called if no device present or device being unloaded
  */
 void
-mptscsih_slave_destroy(Scsi_Device *device)
+mptscsih_slave_destroy(struct scsi_device *device)
 {
 	struct Scsi_Host	*host = device->host;
 	MPT_SCSI_HOST		*hd;
@@ -3350,7 +3245,7 @@
  *	Return non-zero if fails.
  */
 int
-mptscsih_slave_configure(Scsi_Device *device)
+mptscsih_slave_configure(struct scsi_device *device)
 {
 	struct Scsi_Host	*sh = device->host;
 	VirtDevice		*pTarget;
@@ -3433,7 +3328,7 @@
  *
  */
 static void
-copy_sense_data(Scsi_Cmnd *sc, MPT_SCSI_HOST *hd, MPT_FRAME_HDR *mf, SCSIIOReply_t *pScsiReply)
+copy_sense_data(struct scsi_cmnd *sc, MPT_SCSI_HOST *hd, MPT_FRAME_HDR *mf, SCSIIOReply_t *pScsiReply)
 {
 	VirtDevice	*target;
 	SCSIIORequest_t	*pReq;
@@ -3511,7 +3406,7 @@
 }
 
 static u32
-SCPNT_TO_LOOKUP_IDX(Scsi_Cmnd *sc)
+SCPNT_TO_LOOKUP_IDX(struct scsi_cmnd *sc)
 {
 	MPT_SCSI_HOST *hd;
 	int i;
@@ -3613,7 +3508,7 @@
 #if defined(MPT_DEBUG_DV) || defined(MPT_DEBUG_DV_TINY)
 		{
 			u16		 req_idx = le16_to_cpu(mf->u.frame.hwhdr.msgctxu.fld.req_idx);
-			Scsi_Cmnd	*sc = hd->ScsiLookup[req_idx];
+			struct scsi_cmnd	*sc = hd->ScsiLookup[req_idx];
 			printk(MYIOC_s_INFO_FMT "Issued SCSI cmd (sc=%p) idx=%d (mf=%p)\n",
 					hd->ioc->name, sc, req_idx, mf);
 		}

Re: [PATCH] fusion update to current APIs

[Jeremy Higdon]

On Mon, May 31, 2004 at 01:52:29PM +0200, Christoph Hellwig wrote:
>  - kill scsi_to_pci_dma_dir usage, both pci and scsi use the same bits
>  - kill mptscsih_io_direction and always trust the midlayer

Christoph, the midlayer gets it wrong sometimes.

Specifically, when the sg driver can't tell the direction, because the
input and output buffer sizes are identical (and an older sg interface
is being used), the sg driver assumes that the direction is inbound
(SCSI_READ).

There are several apps out there that get that do this, but which work
because the host drivers override the direction passed in from sg.

Now if we have to go out and get all these apps fixed, there will be
a lot of pain out there :-)

jeremy

Re: [PATCH] fusion update to current APIs

[Matthew Wilcox]

On Fri, Jun 11, 2004 at 05:36:08PM -0700, Jeremy Higdon wrote:
> Christoph, the midlayer gets it wrong sometimes.

... then the midlayer needs to get fixed rather than have all this code
duplicated in a million different drivers with subtly different bugs in
each one.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] fusion update to current APIs

[Jeremy Higdon]

On Sat, Jun 12, 2004 at 04:45:18AM +0100, Matthew Wilcox wrote:
> On Fri, Jun 11, 2004 at 05:36:08PM -0700, Jeremy Higdon wrote:
> > Christoph, the midlayer gets it wrong sometimes.
> 
> ... then the midlayer needs to get fixed rather than have all this code
> duplicated in a million different drivers with subtly different bugs in
> each one.

That makes sense to me.  In fact, I think we could just fix it in sg.
It's only a problem with the older sg interfaces that don't specify
a direction.

To review, the problem is that many sg1 and sg2 apps don't tightly
trim the reply length.  Thus, even though they're issuing a Write
command, they will set the reply length to a value greater than
SZ_SG_HEADER.

so in sg_write(), hp->dxfer_direction gets set to SG_DXFER_TO_FROM_DEV:
	if (input_size > 0)
                hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
                    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
        else
                hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;

then in sg_common_write, the data direction of the SCSI request is
set to SCSI_DATA_READ:
        switch (hp->dxfer_direction) {
        case SG_DXFER_TO_FROM_DEV:
        case SG_DXFER_FROM_DEV:
                SRpnt->sr_data_direction = SCSI_DATA_READ;
                break;
        case SG_DXFER_TO_DEV:
                SRpnt->sr_data_direction = SCSI_DATA_WRITE;
                break;
        case SG_DXFER_UNKNOWN:
                SRpnt->sr_data_direction = SCSI_DATA_UNKNOWN;
                break;
        default:
                SRpnt->sr_data_direction = SCSI_DATA_NONE;
                break;
        }


Then when the host driver sends the Write command to the disk with the
data direction set the other way, you usually get a SCSI bus reset.

So we should probably put the direction interpretation in sg_write() near
the bottom just before calling sg_common_write().

Something like this, where sg_direction() does what the host drivers
are currently doing.  I haven't actually tried this yet, but I can
code something up and give it a whirl if folks would like (I'm a little
short on time tonight :-).

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Fri Jun 11 22:11:20 2004
@@ -552,11 +552,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;
@@ -566,6 +561,7 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	hp->dxfer_direction = sg_direction(cmnd);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }


jeremy

Re: [PATCH] fusion update to current APIs

[Matthew Wilcox]

On Fri, Jun 11, 2004 at 10:13:53PM -0700, Jeremy Higdon wrote:
> On Sat, Jun 12, 2004 at 04:45:18AM +0100, Matthew Wilcox wrote:
> > ... then the midlayer needs to get fixed rather than have all this code
> > duplicated in a million different drivers with subtly different bugs in
> > each one.
> 
> That makes sense to me.  In fact, I think we could just fix it in sg.
> It's only a problem with the older sg interfaces that don't specify
> a direction.
[...]
> Something like this, where sg_direction() does what the host drivers
> are currently doing.  I haven't actually tried this yet, but I can
> code something up and give it a whirl if folks would like (I'm a little
> short on time tonight :-).

This looks like exactly the right approach to take.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

Okay, here's my proposed patch to sg.

If it's able to determine the direction based on the input and output
buffer sizes, it uses those.  If both input and output buffer sizes
are non-zero, then it resorts to the command bytes.  Folks familiar
with the mptscsi driver will recognize the sg_direction function :-)

I've tested this using sg_utils, and it seems to work, but those are
probably well-behaved.  I will endeavor to test tomorrow with some
RAID utilities that have not been well-behaved, in conjunction with
removing the direction-override code from the host driver.

If this works, then we can remove the nasty direction code from all
the host drivers.

jeremy


===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Mon Jun 14 22:36:55 2004
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {
@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;
@@ -566,6 +616,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else
+		hp->dxfer_direction = sg_direction(cmnd);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Douglas Gilbert]

Jeremy Higdon wrote:
> Okay, here's my proposed patch to sg.
> 
> If it's able to determine the direction based on the input and output
> buffer sizes, it uses those.  If both input and output buffer sizes
> are non-zero, then it resorts to the command bytes.  Folks familiar
> with the mptscsi driver will recognize the sg_direction function :-)
> 
> I've tested this using sg_utils, and it seems to work, but those are
> probably well-behaved.  I will endeavor to test tomorrow with some
> RAID utilities that have not been well-behaved, in conjunction with
> removing the direction-override code from the host driver.
> 
> If this works, then we can remove the nasty direction code from all
> the host drivers.
> 
> jeremy

So I guess this patch only applies to sg_header usage since the
users of sg_io_hdr (including SG_IO ioctl users) must explicitly
give the data direction.

> ===== drivers/scsi/sg.c 1.90 vs edited =====
> --- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
> +++ edited/drivers/scsi/sg.c	Mon Jun 14 22:36:55 2004
> @@ -480,6 +480,61 @@
>  	return (0 == err) ? count : err;
>  }
>  
> +
> +static int
> +sg_direction(char *cmnd)
> +{
> +	switch (cmnd[0]) {
> +	/*  _DATA_OUT commands	*/
> +	case WRITE_6:		case WRITE_10:		case WRITE_12:
> +	case WRITE_16:
> +	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
> +	case WRITE_VERIFY:	case WRITE_VERIFY_12:
> +	case COMPARE:		case COPY:		case COPY_VERIFY:
> +	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
> +	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
> +	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
> +	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
> +	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
> +	case REASSIGN_BLOCKS:
> +	case PERSISTENT_RESERVE_OUT:
> +	case 0xea:

Perhaps you might like to tell us what the 0xea vendor
specific command is (for the record)?

> +	case 0xa3:

and 0xa3 is MAINTENANCE IN which would be ..._FROM_DEV .
Did you mean 0xa4 (MAINTENANCE OUT)?

> +		return SG_DXFER_TO_DEV;
> +
> +	/*  No data transfer commands  */
> +	case SEEK_6:		case SEEK_10:
> +	case RESERVE:		case RELEASE:
> +	case TEST_UNIT_READY:
> +	case START_STOP:
> +	case ALLOW_MEDIUM_REMOVAL:
> +		return SG_DXFER_NONE;
> +
> +	/*  Conditional data transfer commands	*/
> +	case FORMAT_UNIT:
> +		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	case VERIFY:
> +		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	case RESERVE_10:
> +		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	/*  Must be data _IN!  */
> +	default:
> +		return SG_DXFER_FROM_DEV;
> +	}
> +}
> +
>  static ssize_t
>  sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
>  {
> @@ -552,11 +607,6 @@
>  	hp->cmd_len = (unsigned char) cmd_size;
>  	hp->iovec_count = 0;
>  	hp->mx_sb_len = 0;
> -	if (input_size > 0)
> -		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
> -		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
> -	else
> -		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
>  	hp->dxfer_len = mxsize;
>  	hp->dxferp = (char __user *)buf + cmd_size;
>  	hp->sbp = NULL;
> @@ -566,6 +616,18 @@
>  	hp->usr_ptr = NULL;
>  	if (__copy_from_user(cmnd, buf, cmd_size))
>  		return -EFAULT;
> +	/*
> +	 * If data direction is indeterminate because both input and output size
> +	 * are greater than 0, use command bytes to determine direction.
> +	 */
> +	if (input_size == 0 && mxsize > 0)
> +		hp->dxfer_direction = SG_DXFER_FROM_DEV;
> +	else if (input_size > 0 && mxsize == 0)
> +		hp->dxfer_direction = SG_DXFER_TO_DEV;
> +	else if (input_size == 0 && mxsize == 0)
> +		hp->dxfer_direction = SG_DXFER_NONE;
> +	else
> +		hp->dxfer_direction = sg_direction(cmnd);
>  	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
>  	return (k < 0) ? k : count;
>  }
> -

Doug Gilbert

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeff Garzik]

Jeremy Higdon wrote:
> +static int
> +sg_direction(char *cmnd)
> +{
> +	switch (cmnd[0]) {
> +	/*  _DATA_OUT commands	*/
> +	case WRITE_6:		case WRITE_10:		case WRITE_12:
> +	case WRITE_16:
> +	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
> +	case WRITE_VERIFY:	case WRITE_VERIFY_12:
> +	case COMPARE:		case COPY:		case COPY_VERIFY:
> +	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
> +	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
> +	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
> +	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
> +	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
> +	case REASSIGN_BLOCKS:
> +	case PERSISTENT_RESERVE_OUT:
> +	case 0xea:
> +	case 0xa3:
> +		return SG_DXFER_TO_DEV;
> +
> +	/*  No data transfer commands  */
> +	case SEEK_6:		case SEEK_10:
> +	case RESERVE:		case RELEASE:
> +	case TEST_UNIT_READY:
> +	case START_STOP:
> +	case ALLOW_MEDIUM_REMOVAL:
> +		return SG_DXFER_NONE;
> +
> +	/*  Conditional data transfer commands	*/
> +	case FORMAT_UNIT:
> +		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	case VERIFY:
> +		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	case RESERVE_10:
> +		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	/*  Must be data _IN!  */
> +	default:
> +		return SG_DXFER_FROM_DEV;
> +	}
> +}


This is definitely moving backwards from the direction we want to go.

The entity creating the SCSI command needs to specify data direction, 
_not_ tables hardcoded into the kernel.  Bart recently removed such a 
table from IDE.

These tables are fundamentally broken anyway, due to vendor-reserved 
commands where the data directions are not specified, but simply "known" 
by the submittor.

If /dev/sg's userland interface does not permit userland to provide the 
data direction, then let's just consider it broken and move to Axboe's 
bsg post-haste.

	Jeff

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Tue, Jun 15, 2004 at 04:47:11PM +1000, Douglas Gilbert wrote:
> 
> So I guess this patch only applies to sg_header usage since the
> users of sg_io_hdr (including SG_IO ioctl users) must explicitly
> give the data direction.

Correct.  Several apps declare a response length greater than the
absolute minimum, not realizing that this confuses sg, causing
direction to get set incorrectly, and requiring the host drivers
to have their own tables.

> >+static int
> >+sg_direction(char *cmnd)
> >+{
> >+	switch (cmnd[0]) {
> >+	/*  _DATA_OUT commands	*/
> >+	case WRITE_6:		case WRITE_10:		case WRITE_12:
> >+	case WRITE_16:
> >+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
> >+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
> >+	case COMPARE:		case COPY:		case COPY_VERIFY:
> >+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
> >+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
> >+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
> >+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
> >+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
> >+	case REASSIGN_BLOCKS:
> >+	case PERSISTENT_RESERVE_OUT:
> >+	case 0xea:
> 
> Perhaps you might like to tell us what the 0xea vendor
> specific command is (for the record)?

I would like to, but I don't know any more than you do.

> >+	case 0xa3:
> 
> and 0xa3 is MAINTENANCE IN which would be ..._FROM_DEV .
> Did you mean 0xa4 (MAINTENANCE OUT)?

The table is mostly copied from drivers/message/fusion/mptscsih.c.
It was presumably developed through experience, though I have no
firsthand (or secondhand) knowledge of that.

jeremy

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Tue, Jun 15, 2004 at 02:54:41AM -0400, Jeff Garzik wrote:
> 
> This is definitely moving backwards from the direction we want to go.

You really need to look at this in the context of the original thread.


> The entity creating the SCSI command needs to specify data direction, 
> _not_ tables hardcoded into the kernel.  Bart recently removed such a 
> table from IDE.

Yes.  In this case, sg is the entity.  This table causes sg to get
it right much more often than it did.

> These tables are fundamentally broken anyway, due to vendor-reserved 
> commands where the data directions are not specified, but simply "known" 
> by the submittor.

If the reply length is trimmed to the minimum, it will still work as
well as it does today.

> If /dev/sg's userland interface does not permit userland to provide the 
> data direction, then let's just consider it broken and move to Axboe's 
> bsg post-haste.

If you look at Documentation/scsi/scsi-generic.txt and then refer to
the v1 and v2 sg requests, you'll notice that you can't find a way to
specify direction.  Sg tries to figure it out, but often gets it wrong.
Note that in the v3 sg interface, data direction is explicitly
specified (as it should have been originally, imho).


For those new to the thread, the idea is to remove the direction
overrides from various host drivers and centralize them in sg.  If
we remove the direction overrides from the host drivers and do not
add this code to sg, then several apps that use the sg v1 and v2
interfaces will suddenly break.


jeremy

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Arjan van de Ven]

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


> For those new to the thread, the idea is to remove the direction
> overrides from various host drivers and centralize them in sg.  If
> we remove the direction overrides from the host drivers and do not
> add this code to sg, then several apps that use the sg v1 and v2
> interfaces will suddenly break.

can we add a printk to mark the v1 and v2 interfaces deprecated please ?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Christoph Hellwig]

Not too happy about working around broken apps in the kernel.  At least
remove vendor-specific commands here and add a big printk about the app
writer beeing stupid when it detects a mismatch, or even better return
EINVAL for that case.

On Mon, Jun 14, 2004 at 11:08:11PM -0700, Jeremy Higdon wrote:
> ===== drivers/scsi/sg.c 1.90 vs edited =====
> --- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
> +++ edited/drivers/scsi/sg.c	Mon Jun 14 22:36:55 2004
> @@ -480,6 +480,61 @@
>  	return (0 == err) ? count : err;
>  }
>  
> +
> +static int
> +sg_direction(char *cmnd)
> +{
> +	switch (cmnd[0]) {
> +	/*  _DATA_OUT commands	*/
> +	case WRITE_6:		case WRITE_10:		case WRITE_12:
> +	case WRITE_16:
> +	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
> +	case WRITE_VERIFY:	case WRITE_VERIFY_12:
> +	case COMPARE:		case COPY:		case COPY_VERIFY:
> +	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
> +	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
> +	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
> +	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
> +	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
> +	case REASSIGN_BLOCKS:
> +	case PERSISTENT_RESERVE_OUT:
> +	case 0xea:
> +	case 0xa3:
> +		return SG_DXFER_TO_DEV;
> +
> +	/*  No data transfer commands  */
> +	case SEEK_6:		case SEEK_10:
> +	case RESERVE:		case RELEASE:
> +	case TEST_UNIT_READY:
> +	case START_STOP:
> +	case ALLOW_MEDIUM_REMOVAL:
> +		return SG_DXFER_NONE;
> +
> +	/*  Conditional data transfer commands	*/
> +	case FORMAT_UNIT:
> +		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	case VERIFY:
> +		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	case RESERVE_10:
> +		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
> +			return SG_DXFER_TO_DEV;
> +		else
> +			return SG_DXFER_NONE;
> +
> +	/*  Must be data _IN!  */
> +	default:
> +		return SG_DXFER_FROM_DEV;
> +	}
> +}
> +
>  static ssize_t
>  sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
>  {
> @@ -552,11 +607,6 @@
>  	hp->cmd_len = (unsigned char) cmd_size;
>  	hp->iovec_count = 0;
>  	hp->mx_sb_len = 0;
> -	if (input_size > 0)
> -		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
> -		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
> -	else
> -		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
>  	hp->dxfer_len = mxsize;
>  	hp->dxferp = (char __user *)buf + cmd_size;
>  	hp->sbp = NULL;
> @@ -566,6 +616,18 @@
>  	hp->usr_ptr = NULL;
>  	if (__copy_from_user(cmnd, buf, cmd_size))
>  		return -EFAULT;
> +	/*
> +	 * If data direction is indeterminate because both input and output size
> +	 * are greater than 0, use command bytes to determine direction.
> +	 */
> +	if (input_size == 0 && mxsize > 0)
> +		hp->dxfer_direction = SG_DXFER_FROM_DEV;
> +	else if (input_size > 0 && mxsize == 0)
> +		hp->dxfer_direction = SG_DXFER_TO_DEV;
> +	else if (input_size == 0 && mxsize == 0)
> +		hp->dxfer_direction = SG_DXFER_NONE;
> +	else
> +		hp->dxfer_direction = sg_direction(cmnd);
>  	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
>  	return (k < 0) ? k : count;
>  }
> -
> 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
---end quoted text---

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Tue, Jun 15, 2004 at 09:57:49AM +0200, Arjan van de Ven wrote:
> 
> can we add a printk to mark the v1 and v2 interfaces deprecated please ?

New patch with a printk:

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Tue Jun 15 01:36:28 2004
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {
@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;
@@ -566,6 +616,22 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count"
+		       " %ld and reply_len %d; inferring from command 0x%x\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

Christoph wants a more explicit message.

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Tue Jun 15 01:46:10 2004
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {
@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;
@@ -566,6 +616,23 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to trim count and reply_len\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

One more comment from hch via IRC.  When we have to guess direction,
print a suggestion to use SG v3 interface.

I also removed the vendor unique commands from the switch.

jeremy

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Tue Jun 15 02:08:51 2004
@@ -480,6 +480,59 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {
@@ -552,11 +605,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;
@@ -566,6 +614,23 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to use the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Tue, Jun 15, 2004 at 02:10:43AM -0700, Jeremy Higdon wrote:
> One more comment from hch via IRC.  When we have to guess direction,
> print a suggestion to use SG v3 interface.
> 
> I also removed the vendor unique commands from the switch.

I had a bug in that last patch.

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Tue Jun 15 02:23:53 2004
@@ -480,6 +480,59 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {
@@ -552,11 +605,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;
@@ -566,6 +614,23 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && old_hdr.reply_len <= SZ_SG_HEADER)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to use the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeff Garzik]

Jeremy Higdon wrote:
> On Tue, Jun 15, 2004 at 04:47:11PM +1000, Douglas Gilbert wrote:
> 
>>So I guess this patch only applies to sg_header usage since the
>>users of sg_io_hdr (including SG_IO ioctl users) must explicitly
>>give the data direction.
> 
> 
> Correct.  Several apps declare a response length greater than the
> absolute minimum, not realizing that this confuses sg, causing
> direction to get set incorrectly, and requiring the host drivers
> to have their own tables.


We're still at "hacking the kernel to fix broken apps"  :(:(

	Jeff

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Patrick Mansfield]

On Tue, Jun 15, 2004 at 02:31:48AM -0700, Jeremy Higdon wrote:
> On Tue, Jun 15, 2004 at 02:10:43AM -0700, Jeremy Higdon wrote:
> > One more comment from hch via IRC.  When we have to guess direction,
> > print a suggestion to use SG v3 interface.

But print only once, so borked applications that continually poll devices
won't generate lots of these messages.

> +	/*
> +	 * If data direction is indeterminate because both input and output size
> +	 * are greater than 0, use command bytes to determine direction.
> +	 */
> +	if (input_size == 0 && mxsize > 0)
> +		hp->dxfer_direction = SG_DXFER_FROM_DEV;
> +	else if (input_size > 0 && old_hdr.reply_len <= SZ_SG_HEADER)
> +		hp->dxfer_direction = SG_DXFER_TO_DEV;
> +	else if (input_size == 0 && mxsize == 0)
> +		hp->dxfer_direction = SG_DXFER_NONE;
> +	else {
> +		hp->dxfer_direction = sg_direction(cmnd);
> +		printk("sg_write: cannot infer direction from count %ld "
> +		       "and reply_len %d; inferring from command 0x%x.  Please "
> +		       "fix the application to use the sg version 3 interface\n",
> +		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
> +	}
>  	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
>  	return (k < 0) ? k : count;
>  }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Tue, Jun 15, 2004 at 11:07:22AM -0400, Jeff Garzik wrote:
> Jeremy Higdon wrote:
> >On Tue, Jun 15, 2004 at 04:47:11PM +1000, Douglas Gilbert wrote:
> >
> >>So I guess this patch only applies to sg_header usage since the
> >>users of sg_io_hdr (including SG_IO ioctl users) must explicitly
> >>give the data direction.
> >
> >
> >Correct.  Several apps declare a response length greater than the
> >absolute minimum, not realizing that this confuses sg, causing
> >direction to get set incorrectly, and requiring the host drivers
> >to have their own tables.
> 
> 
> We're still at "hacking the kernel to fix broken apps"  :(:(
> 
> 	Jeff

Okay.  How about this then.  At least it will fail in a noisy way,
so that host adapter driver maintainers will not be pressured to "fix"
their drivers (which is how the direction override code ended up in
qlogic and mpt in the first place).

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Tue Jun 15 14:04:39 2004
@@ -552,9 +552,19 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
+	if (input_size > 0) {	/* data is being sent to device */
+		if (old_hdr.reply_len <= SZ_SG_HEADER)
+			hp->dxfer_direction = SG_DXFER_TO_DEV;
+		else {
+			printk("sg_write: cannot infer direction from count %ld "
+			       "and reply_len %d.  Command 0x%x will probably fail. "
+			       "Please fix the application to use proper counts or "
+			       "the sg version 3 interface\n",
+			       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+			/* SG_DXFER_TO_FROM_DEV is equivalent to SG_DXFER_FROM_DEV */
+			hp->dxfer_direction = SG_DXFER_TO_FROM_DEV;
+		}
+	}
 	else
 		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeff Garzik]

Jeremy Higdon wrote:
> Okay.  How about this then.  At least it will fail in a noisy way,
> so that host adapter driver maintainers will not be pressured to "fix"
> their drivers (which is how the direction override code ended up in
> qlogic and mpt in the first place).

ok with me :)

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Tue, Jun 15, 2004 at 02:34:58PM -0700, Jeremy Higdon wrote:
> 
> Okay.  How about this then.  At least it will fail in a noisy way,
> so that host adapter driver maintainers will not be pressured to "fix"
> their drivers (which is how the direction override code ended up in
> qlogic and mpt in the first place).

Sorry -- I sent that one out too quickly -- it uses cmnd[0] before it is
set.  This patch is smaller and more correct.

Does anyone have objections to this?  I don't think we need to ratelimit
these printks, because whatever is causing them is likely experiencing
I/O errors and will probably give up.

===== drivers/scsi/sg.c 1.90 vs edited =====
--- 1.90/drivers/scsi/sg.c	Sat May 29 10:57:23 2004
+++ edited/drivers/scsi/sg.c	Tue Jun 15 15:05:47 2004
@@ -566,6 +566,17 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

I still think that this change should go into the sg1/sg2 command
issue path, so that a warning is issued when sg has to guess on
command direction.  This will encourage developers to fix up apps
that don't set buffer sizes properly.

Doug, can you comment on this?  If you don't like it, I'll drop
it  :-)

This patch is against 2.6.9-rc1.

thanks,

jeremy

===== drivers/scsi/sg.c 1.95 vs edited =====
--- 1.95/drivers/scsi/sg.c	2004-08-07 19:11:33 -07:00
+++ edited/drivers/scsi/sg.c	2004-08-26 00:04:25 -07:00
@@ -564,6 +564,17 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Douglas Gilbert]

Jeremy Higdon wrote:
> I still think that this change should go into the sg1/sg2 command
> issue path, so that a warning is issued when sg has to guess on
> command direction.  This will encourage developers to fix up apps
> that don't set buffer sizes properly.
> 
> Doug, can you comment on this?  If you don't like it, I'll drop
> it  :-)
> 
> This patch is against 2.6.9-rc1.
> 
> thanks,
> 
> jeremy
> 
> ===== drivers/scsi/sg.c 1.95 vs edited =====
> --- 1.95/drivers/scsi/sg.c	2004-08-07 19:11:33 -07:00
> +++ edited/drivers/scsi/sg.c	2004-08-26 00:04:25 -07:00
> @@ -564,6 +564,17 @@
>  	hp->usr_ptr = NULL;
>  	if (__copy_from_user(cmnd, buf, cmd_size))
>  		return -EFAULT;
> +	/*
> +	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
> +	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
> +	 * is a non-zero input_size, so emit a warning.
> +	 */
> +	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
> +		printk("sg_write: cannot infer direction from count %ld "
> +		       "and reply_len %d.  Command 0x%x will probably fail. "
> +		       "Please fix the application to use proper counts or "
> +		       "the sg version 3 interface\n",
> +		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
>  	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
>  	return (k < 0) ? k : count;
>  }
> 
Jeremy,
Perhaps the first argument of printk() could be prefixed with
KERN_WARNING to make it a bit more syslog friendly.
Otherwise ok.

Doug Gilbert

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Thu, Aug 26, 2004 at 06:44:40PM +1000, Douglas Gilbert wrote:
> Jeremy,
> Perhaps the first argument of printk() could be prefixed with
> KERN_WARNING to make it a bit more syslog friendly.
> Otherwise ok.
> 
> Doug Gilbert

You're right.  It's fixed in this patch.
James, if you're satisfied, please apply.

thanks,
jeremy

signed-off-by: jeremy@sgi.com

===== drivers/scsi/sg.c 1.95 vs edited =====
--- 1.95/drivers/scsi/sg.c	2004-08-07 19:11:33 -07:00
+++ edited/drivers/scsi/sg.c	2004-08-26 17:19:58 -07:00
@@ -564,6 +564,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk(KERN_WARNING
+		       "sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Thu, Aug 26, 2004 at 06:12:53PM -0700, Jeremy Higdon wrote:
> On Thu, Aug 26, 2004 at 06:44:40PM +1000, Douglas Gilbert wrote:
> > Jeremy,
> > Perhaps the first argument of printk() could be prefixed with
> > KERN_WARNING to make it a bit more syslog friendly.
> > Otherwise ok.
> > 
> > Doug Gilbert
> 
> You're right.  It's fixed in this patch.
> James, if you're satisfied, please apply.

This is one of those days when I seem to be working against myself.
It turns out that our RAID vendor modified their agent to trim
the "reply_len" field for write commands, but did not trim the
"count" field for read commands.  So in this case, sg actually
guesses correctly when it needs to, and the app works.  Only with
this patch, /var/log/messages gets filled with this new error
message.

Doug, what do you think is the right thing to do?

jeremy

> thanks,
> jeremy
> 
> signed-off-by: jeremy@sgi.com
> 
> ===== drivers/scsi/sg.c 1.95 vs edited =====
> --- 1.95/drivers/scsi/sg.c	2004-08-07 19:11:33 -07:00
> +++ edited/drivers/scsi/sg.c	2004-08-26 17:19:58 -07:00
> @@ -564,6 +564,18 @@
>  	hp->usr_ptr = NULL;
>  	if (__copy_from_user(cmnd, buf, cmd_size))
>  		return -EFAULT;
> +	/*
> +	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
> +	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
> +	 * is a non-zero input_size, so emit a warning.
> +	 */
> +	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
> +		printk(KERN_WARNING
> +		       "sg_write: cannot infer direction from count %ld "
> +		       "and reply_len %d.  Command 0x%x will probably fail. "
> +		       "Please fix the application to use proper counts or "
> +		       "the sg version 3 interface\n",
> +		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
>  	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
>  	return (k < 0) ? k : count;
>  }

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Douglas Gilbert]

Jeremy Higdon wrote:
> On Thu, Aug 26, 2004 at 06:12:53PM -0700, Jeremy Higdon wrote:
> 
>>On Thu, Aug 26, 2004 at 06:44:40PM +1000, Douglas Gilbert wrote:
>>
>>>Jeremy,
>>>Perhaps the first argument of printk() could be prefixed with
>>>KERN_WARNING to make it a bit more syslog friendly.
>>>Otherwise ok.
>>>
>>>Doug Gilbert
>>
>>You're right.  It's fixed in this patch.
>>James, if you're satisfied, please apply.
> 
> 
> This is one of those days when I seem to be working against myself.
> It turns out that our RAID vendor modified their agent to trim
> the "reply_len" field for write commands, but did not trim the
> "count" field for read commands.  So in this case, sg actually
> guesses correctly when it needs to, and the app works.  Only with
> this patch, /var/log/messages gets filled with this new error
> message.
> 
> Doug, what do you think is the right thing to do?

Jeremy,
You could use a (block scope) static and only print out
the warning the first time it is detected.

Doug Gilbert

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

[Jeremy Higdon]

On Sat, Aug 28, 2004 at 03:08:09PM +1000, Douglas Gilbert wrote:
> Jeremy Higdon wrote:
> >On Thu, Aug 26, 2004 at 06:12:53PM -0700, Jeremy Higdon wrote:
> >
> >
> >This is one of those days when I seem to be working against myself.
> >It turns out that our RAID vendor modified their agent to trim
> >the "reply_len" field for write commands, but did not trim the
> >"count" field for read commands.  So in this case, sg actually
> >guesses correctly when it needs to, and the app works.  Only with
> >this patch, /var/log/messages gets filled with this new error
> >message.
> >
> >Doug, what do you think is the right thing to do?
> 
> Jeremy,
> You could use a (block scope) static and only print out
> the warning the first time it is detected.
> 
> Doug Gilbert

Yes, that's a good idea -- I'll add a rate limiter and also print
out the process name like James does in his recent deprecated
ioctl warning patch.  I'll send a new patch out later this weekend.
Sorry for the churn.

thanks

jeremy

Re: [PATCH] sg.c to warn about ambiguous data direction

[Jeremy Higdon]

On Sat, Aug 28, 2004 at 02:39:45AM -0700, Jeremy Higdon wrote:
> > Jeremy,
> > You could use a (block scope) static and only print out
> > the warning the first time it is detected.
> > 
> > Doug Gilbert
> 
> Yes, that's a good idea -- I'll add a rate limiter and also print
> out the process name like James does in his recent deprecated
> ioctl warning patch.  I'll send a new patch out later this weekend.
> Sorry for the churn.
> 
> thanks
> 
> jeremy

Okay, I'm pretty happy with this.  I get this kind of output:

sg_write: data in/out 512/512 bytes for SCSI command 0x8--guessing data in;
   program java not setting count and/or reply_len properly
printk: 11 messages suppressed.
sg_write: data in/out 512/512 bytes for SCSI command 0x8--guessing data in;
   program java not setting count and/or reply_len properly
printk: 4 messages suppressed.


signed-off-by: jeremy@sgi.com

===== drivers/scsi/sg.c 1.95 vs edited =====
--- 1.95/drivers/scsi/sg.c	2004-08-07 19:11:33 -07:00
+++ edited/drivers/scsi/sg.c	2004-08-29 23:22:21 -07:00
@@ -564,6 +564,19 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		if (printk_ratelimit())
+			printk(KERN_WARNING
+			       "sg_write: data in/out %d/%d bytes for SCSI command 0x%x--"
+			       "guessing data in;\n" KERN_WARNING "   "
+			       "program %s not setting count and/or reply_len properly\n",
+			       old_hdr.reply_len - SZ_SG_HEADER, input_size,
+			       (unsigned int) cmnd[0], current->comm);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }