[PATCH] Perverting cciss

[PATCH] Perverting cciss

[Hannes Reinecke]

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

Hi All,

this patch adds the SG_IO ioctl to the cciss driver.
As the driver is capable of sending SCSI CDBs to the controller there is
no reason why we shouldn't exploit it.
This way we get to use all the nice sg_utils for the cciss driver.
And a persistent device name for free.

Makes one wonder why this one is not implemented as a SCSI driver in the
first place.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: cciss-add-SG_IO --]
[-- Type: text/plain, Size: 5048 bytes --]

Add SG_IO ioctl to cciss

The cciss driver is actually capable of sending SCSI CDB to the
controller. So it would only be logical to have it accepting 
SG_IO ioctl, too.
Now we can use all the nice sg_utils and get a persistent device name
for free, too.

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 0c716ee..33e7b83 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -45,6 +45,9 @@ #include <linux/dma-mapping.h>
 #include <linux/blkdev.h>
 #include <linux/genhd.h>
 #include <linux/completion.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_ioctl.h>
+#include <scsi/sg.h>
 
 #define CCISS_DRIVER_VERSION(maj,min,submin) ((maj<<16)|(min<<8)|(submin))
 #define DRIVER_NAME "HP CISS Driver (v 3.6.14)"
@@ -1152,6 +1155,163 @@ #endif
 			kfree(ioc);
 			return status;
 		}
+	case SG_IO: {
+		struct sg_io_hdr hdr;
+		CommandList_struct *c;
+		char *buff = NULL;
+		u64bit temp64;
+		unsigned long flags;
+		DECLARE_COMPLETION_ONSTACK(wait);
+
+		if (!capable(CAP_SYS_RAWIO))
+			return -EPERM;
+
+		if (copy_from_user(&hdr, argp, sizeof(hdr)))
+			return -EFAULT;
+
+		if (hdr.interface_id != 'S')
+			return -EINVAL;
+
+		/* cciss only supports 16-byte commands */
+		if (hdr.cmd_len > 16)
+			return -EINVAL;
+
+		/* We don't support proper scatter-gather (yet) */
+		if (hdr.iovec_count)
+			return -EINVAL;
+
+		if ((hdr.dxfer_len < 1) &&
+		    (hdr.dxfer_direction != SG_DXFER_NONE))
+			return -EINVAL;
+
+		if (hdr.dxfer_len > 0) {
+			buff = kmalloc(hdr.dxfer_len, GFP_KERNEL);
+			if (buff == NULL)
+				return -EFAULT;
+		}
+		if ((hdr.dxfer_direction == SG_DXFER_TO_DEV) ||
+		    (hdr.dxfer_direction == SG_DXFER_TO_FROM_DEV)) {
+			/* Copy the data into the buffer we created */
+			if (copy_from_user (buff, hdr.dxferp,
+					    hdr.dxfer_len)) {
+				kfree(buff);
+				return -EFAULT;
+			}
+		} else
+			memset(buff, 0, hdr.dxfer_len);
+
+		if ((c = cmd_alloc(host, 0)) == NULL) {
+			kfree(buff);
+			return -ENOMEM;
+		}
+
+		/* Copy CDB */
+		if (copy_from_user(c->Request.CDB, hdr.cmdp, hdr.cmd_len))
+			return -EFAULT;
+
+		/* Fill in the command type */
+		c->cmd_type = CMD_IOCTL_PEND;
+		/* Fill in Command Header */
+		c->Header.ReplyQueue = 0;
+		if (hdr.dxfer_len > 0) {
+			c->Header.SGList = 1;
+			c->Header.SGTotal = 1;
+		} else {
+			c->Header.SGList = 0;
+			c->Header.SGTotal = 0;
+		}
+		/* Default to LUN the ioctl was directed to */
+		c->Header.LUN.LogDev.VolId = drv->LunID & 0x3FFFFFFF;
+		c->Header.LUN.LogDev.Mode = 0x01; /* Logical volume */
+		c->Header.Tag.lower = c->busaddr;
+
+		/* Fill in Request block */
+		c->Request.CDBLen = hdr.cmd_len;
+		c->Request.Type.Type = TYPE_CMD;
+		c->Request.Type.Attribute = ATTR_SIMPLE;
+		switch(hdr.dxfer_direction) {
+		    case SG_DXFER_NONE:
+			c->Request.Type.Direction = XFER_NONE;
+			break;
+		    case SG_DXFER_TO_DEV:
+			c->Request.Type.Direction = XFER_WRITE;
+			break;
+		    case SG_DXFER_FROM_DEV:
+			c->Request.Type.Direction = XFER_READ;
+			break;
+		    case SG_DXFER_TO_FROM_DEV:
+			c->Request.Type.Direction = XFER_RSVD;
+			break;
+		}
+		c->Request.Timeout = hdr.timeout;
+
+		/* Fill in the scatter gather information */
+		if (hdr.dxfer_len > 0) {
+			temp64.val = pci_map_single(host->pdev, buff,
+						    hdr.dxfer_len,
+						    PCI_DMA_BIDIRECTIONAL);
+			c->SG[0].Addr.lower = temp64.val32.lower;
+			c->SG[0].Addr.upper = temp64.val32.upper;
+			c->SG[0].Len = hdr.dxfer_len;
+			c->SG[0].Ext = 0;
+		}
+		c->waiting = &wait;
+
+		/* Put the request on the tail of the request queue */
+		spin_lock_irqsave(CCISS_LOCK(ctlr), flags);
+		addQ(&host->reqQ, c);
+		host->Qdepth++;
+		start_io(host);
+		spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);
+
+		wait_for_completion(&wait);
+
+		/* unlock the buffers from DMA */
+		temp64.val32.lower = c->SG[0].Addr.lower;
+		temp64.val32.upper = c->SG[0].Addr.upper;
+		pci_unmap_single(host->pdev, (dma_addr_t) temp64.val,
+				 hdr.dxfer_len,
+				 PCI_DMA_BIDIRECTIONAL);
+
+		/* Copy the error information out */
+		hdr.status = c->err_info->ScsiStatus;
+		if (c->err_info->SenseLen && hdr.mx_sb_len > 0) {
+			int sense_len = c->err_info->SenseLen;
+
+			if (sense_len > hdr.mx_sb_len)
+				sense_len = hdr.mx_sb_len;
+
+			if (copy_to_user(hdr.sbp, c->err_info->SenseInfo,
+					 sense_len)) {
+				kfree(buff);
+				cmd_free(host, c, 0);
+				return -EFAULT;
+			}
+			hdr.sb_len_wr = sense_len;
+		}
+		hdr.resid = c->err_info->ResidualCnt;
+		/* Copy out the header */
+		if (copy_to_user(argp, &hdr, sizeof(hdr))) {
+			kfree(buff);
+			cmd_free(host, c, 0);
+			return -EFAULT;
+		}
+
+		if ((hdr.dxfer_direction == SG_DXFER_FROM_DEV) ||
+		    (hdr.dxfer_direction == SG_DXFER_TO_FROM_DEV)) {
+			/* Copy the data out to the buffer we created */
+			if (copy_to_user
+			    (hdr.dxferp, buff, hdr.dxfer_len)) {
+				kfree(buff);
+				cmd_free(host, c, 0);
+				return -EFAULT;
+			}
+		}
+
+		kfree(buff);
+		cmd_free(host, c, 0);
+		return 0;
+		}
 	default:
 		return -ENOTTY;
 	}

Re: [PATCH] Perverting cciss

[Christoph Hellwig]

On Thu, Apr 05, 2007 at 11:58:06AM +0200, Hannes Reinecke wrote:
> Hi All,
> 
> this patch adds the SG_IO ioctl to the cciss driver.
> As the driver is capable of sending SCSI CDBs to the controller there is
> no reason why we shouldn't exploit it.
> This way we get to use all the nice sg_utils for the cciss driver.
> And a persistent device name for free.

Instead of adding yet another implementation of SG_IO please implement
support for REQ_TYPE_BLOCK_PC requests and add all the nice block layer
passthrough ioctls to it.

> Makes one wonder why this one is not implemented as a SCSI driver in the
> first place.

It probably should have been a scsi driver, but it's far too late now.

Re: [PATCH] Perverting cciss

[James Bottomley]

On Thu, 2007-04-05 at 11:03 +0100, Christoph Hellwig wrote:
> On Thu, Apr 05, 2007 at 11:58:06AM +0200, Hannes Reinecke wrote:
> > Hi All,
> > 
> > this patch adds the SG_IO ioctl to the cciss driver.
> > As the driver is capable of sending SCSI CDBs to the controller there is
> > no reason why we shouldn't exploit it.
> > This way we get to use all the nice sg_utils for the cciss driver.
> > And a persistent device name for free.
> 
> Instead of adding yet another implementation of SG_IO please implement
> support for REQ_TYPE_BLOCK_PC requests and add all the nice block layer
> passthrough ioctls to it.

Actually, I happen to know that HP is in the process of implementing
this correctly (via REQ_TYPE_BLOCK_PC).  I can't reveal the details but
it has something to do with a well known Linux High Availability company
needing SG_IO for sg_persist to work ...

James

RE: [PATCH] Perverting cciss

[Miller, Mike (OS Dev)]

James wrote: 
> 
> On Thu, 2007-04-05 at 11:03 +0100, Christoph Hellwig wrote:
> > On Thu, Apr 05, 2007 at 11:58:06AM +0200, Hannes Reinecke wrote:
> > > Hi All,
> > > 
> > > this patch adds the SG_IO ioctl to the cciss driver.
> > > As the driver is capable of sending SCSI CDBs to the controller 
> > > there is no reason why we shouldn't exploit it.
> > > This way we get to use all the nice sg_utils for the cciss driver.
> > > And a persistent device name for free.
> > 
> > Instead of adding yet another implementation of SG_IO 
> please implement 
> > support for REQ_TYPE_BLOCK_PC requests and add all the nice block 
> > layer passthrough ioctls to it.
> 
> Actually, I happen to know that HP is in the process of 
> implementing this correctly (via REQ_TYPE_BLOCK_PC).  I can't 
> reveal the details but it has something to do with a well 
> known Linux High Availability company needing SG_IO for 
> sg_persist to work ...
> 
> James

NAK. Please do not add this patch to cciss. We are working SG_IO
internally and will post the patch soon.

mikem


> 
> 
> 

Re: [PATCH] Perverting cciss

[Stephen Cameron]

Hannes Reinecke wrote:

> this patch adds the SG_IO ioctl to the cciss driver.
> As the driver is capable of sending SCSI CDBs to the controller there is
> no reason why we shouldn't exploit it.
> This way we get to use all the nice sg_utils for the cciss driver.
> And a persistent device name for free.

Christoph Hellwigg wrote:

> Instead of adding yet another implementation of SG_IO please implement
> support for REQ_TYPE_BLOCK_PC requests and add all the nice block layer
> passthrough ioctls to it. 

James Bottomley wrote:

> Actually, I happen to know that HP is in the process of implementing
> this correctly (via REQ_TYPE_BLOCK_PC).  I can't reveal the details but
> it has something to do with a well known Linux High Availability company
> needing SG_IO for sg_persist to work ... 

How about something like this:
(Since my mailer is sure to wreck the patch, it can be found intact here:
http://cciss.cvs.sourceforge.net/*checkout*/cciss/patches/kernel.org-2.6/cciss_sg_io_block_pc.patch?revision=1.1

-- steve

Add SG_IO to cciss driver.

Not completely sure all the error handling is correct.

---

 linux-2.6.21-rc6/drivers/block/cciss.c |   90 ++++++++++++++++++++-------------
 1 files changed, 56 insertions(+), 34 deletions(-)

diff -puN linux-2.6.21-rc6/drivers/block/cciss.c~cciss_sg_io_ioctl
linux-2.6.21-rc6/drivers/block/cciss.c
--- cciss_sg_io/linux-2.6.21-rc6/drivers/block/cciss.c~cciss_sg_io_ioctl	2007-04-06
08:11:59.000000000 -0500
+++ cciss_sg_io-scameron/linux-2.6.21-rc6/drivers/block/cciss.c	2007-04-06 11:38:40.000000000
-0500
@@ -45,7 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/genhd.h>
 #include <linux/completion.h>
-
+#include <scsi/sg.h>
 #define CCISS_DRIVER_VERSION(maj,min,submin) ((maj<<16)|(min<<8)|(submin))
 #define DRIVER_NAME "HP CISS Driver (v 3.6.14)"
 #define DRIVER_VERSION CCISS_DRIVER_VERSION(3,6,14)
@@ -1152,6 +1152,7 @@ static int cciss_ioctl(struct inode *ino
 			kfree(ioc);
 			return status;
 		}
+	case SG_IO: return scsi_cmd_ioctl(filep, disk, cmd, argp);
 	default:
 		return -ENOTTY;
 	}
@@ -2361,6 +2362,14 @@ static inline void complete_command(ctlr
 				       " byte 2 = 0x%x\n", cmd,
 				       cmd->err_info->SenseInfo[2]
 				    );
+				if (blk_pc_request(cmd->rq)) {
+					if (cmd->rq->sense) {
+						if (cmd->rq->sense_len > cmd->err_info->SenseLen)
+							cmd->rq->sense_len = cmd->err_info->SenseLen;
+						memcpy(cmd->rq->sense, cmd->err_info->SenseInfo, cmd->rq->sense_len);
+					} else
+						cmd->rq->sense_len = 0;
+				}
 				/* check the sense key */
 				sense_key = 0xf & cmd->err_info->SenseInfo[2];
 				/* no status or recovered error */
@@ -2374,14 +2383,16 @@ static inline void complete_command(ctlr
 			}
 			break;
 		case CMD_DATA_UNDERRUN:
-			printk(KERN_WARNING "cciss: cmd %p has"
-			       " completed with data underrun "
-			       "reported\n", cmd);
+			if (blk_fs_request(cmd->rq))
+				printk(KERN_WARNING "cciss: cmd %p has"
+				       " completed with data underrun "
+				       "reported\n", cmd);
 			break;
 		case CMD_DATA_OVERRUN:
-			printk(KERN_WARNING "cciss: cmd %p has"
-			       " completed with data overrun "
-			       "reported\n", cmd);
+			if (blk_fs_request(cmd->rq))
+				printk(KERN_WARNING "cciss: cmd %p has"
+				       " completed with data overrun "
+				       "reported\n", cmd);
 			break;
 		case CMD_INVALID:
 			printk(KERN_WARNING "cciss: cmd %p is "
@@ -2443,9 +2454,12 @@ static inline void complete_command(ctlr
 		resend_cciss_cmd(h, cmd);
 		return;
 	}
-
-	cmd->rq->completion_data = cmd;
 	cmd->rq->errors = status;
+	if (blk_pc_request(cmd->rq)) {
+		cmd->rq->errors |= 0xff & cmd->err_info->ScsiStatus;
+		cmd->rq->data_len = cmd->err_info->ResidualCnt;
+	}
+	cmd->rq->completion_data = cmd;
 	blk_add_trace_rq(cmd->rq->q, cmd->rq, BLK_TA_COMPLETE);
 	blk_complete_request(cmd->rq);
 }
@@ -2539,32 +2553,40 @@ static void do_cciss_request(request_que
 #endif				/* CCISS_DEBUG */
 
 	c->Header.SGList = c->Header.SGTotal = seg;
-	if(h->cciss_read == CCISS_READ_10) {
-		c->Request.CDB[1] = 0;
-		c->Request.CDB[2] = (start_blk >> 24) & 0xff;	//MSB
-		c->Request.CDB[3] = (start_blk >> 16) & 0xff;
-		c->Request.CDB[4] = (start_blk >> 8) & 0xff;
-		c->Request.CDB[5] = start_blk & 0xff;
-		c->Request.CDB[6] = 0;	// (sect >> 24) & 0xff; MSB
-		c->Request.CDB[7] = (creq->nr_sectors >> 8) & 0xff;
-		c->Request.CDB[8] = creq->nr_sectors & 0xff;
-		c->Request.CDB[9] = c->Request.CDB[11] = c->Request.CDB[12] = 0;
+	if (likely(blk_fs_request(creq))) {
+		if(h->cciss_read == CCISS_READ_10) {
+			c->Request.CDB[1] = 0;
+			c->Request.CDB[2] = (start_blk >> 24) & 0xff;	//MSB
+			c->Request.CDB[3] = (start_blk >> 16) & 0xff;
+			c->Request.CDB[4] = (start_blk >> 8) & 0xff;
+			c->Request.CDB[5] = start_blk & 0xff;
+			c->Request.CDB[6] = 0;	// (sect >> 24) & 0xff; MSB
+			c->Request.CDB[7] = (creq->nr_sectors >> 8) & 0xff;
+			c->Request.CDB[8] = creq->nr_sectors & 0xff;
+			c->Request.CDB[9] = c->Request.CDB[11] = c->Request.CDB[12] = 0;
+		} else {
+			c->Request.CDBLen = 16;
+			c->Request.CDB[1]= 0;
+			c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
+			c->Request.CDB[3]= (start_blk >> 48) & 0xff;
+			c->Request.CDB[4]= (start_blk >> 40) & 0xff;
+			c->Request.CDB[5]= (start_blk >> 32) & 0xff;
+			c->Request.CDB[6]= (start_blk >> 24) & 0xff;
+			c->Request.CDB[7]= (start_blk >> 16) & 0xff;
+			c->Request.CDB[8]= (start_blk >>  8) & 0xff;
+			c->Request.CDB[9]= start_blk & 0xff;
+			c->Request.CDB[10]= (creq->nr_sectors >>  24) & 0xff;
+			c->Request.CDB[11]= (creq->nr_sectors >>  16) & 0xff;
+			c->Request.CDB[12]= (creq->nr_sectors >>  8) & 0xff;
+			c->Request.CDB[13]= creq->nr_sectors & 0xff;
+			c->Request.CDB[14] = c->Request.CDB[15] = 0;
+		}
+	} else if (blk_pc_request(creq)) {
+		c->Request.CDBLen = creq->cmd_len;
+		memcpy(c->Request.CDB, creq->cmd, BLK_MAX_CDB);
 	} else {
-		c->Request.CDBLen = 16;
-		c->Request.CDB[1]= 0;
-		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
-		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
-		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
-		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
-		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
-		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
-		c->Request.CDB[8]= (start_blk >>  8) & 0xff;
-		c->Request.CDB[9]= start_blk & 0xff;
-		c->Request.CDB[10]= (creq->nr_sectors >>  24) & 0xff;
-		c->Request.CDB[11]= (creq->nr_sectors >>  16) & 0xff;
-		c->Request.CDB[12]= (creq->nr_sectors >>  8) & 0xff;
-		c->Request.CDB[13]= creq->nr_sectors & 0xff;
-		c->Request.CDB[14] = c->Request.CDB[15] = 0;
+		printk(KERN_WARNING "cciss%d: bad request type %d\n", h->ctlr, creq->cmd_type);
+		BUG();
 	}
 
 	spin_lock_irq(q->queue_lock);
_



 
____________________________________________________________________________________
8:00? 8:25? 8:40? Find a flick in no time 
with the Yahoo! Search movie showtime shortcut.
http://tools.search.yahoo.com/shortcuts/#news

Re: [PATCH] Perverting cciss

[Christoph Hellwig]

On Fri, Apr 06, 2007 at 10:07:52AM -0700, Stephen Cameron wrote:
> How about something like this:
> (Since my mailer is sure to wreck the patch, it can be found intact here:
> http://cciss.cvs.sourceforge.net/*checkout*/cciss/patches/kernel.org-2.6/cciss_sg_io_block_pc.patch?revision=1.1

Looks good except for some codingstyle nits:

> @@ -45,7 +45,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/genhd.h>
>  #include <linux/completion.h>
> -
> +#include <scsi/sg.h>

Please don't remove the empty line after the include statements.

> +	case SG_IO: return scsi_cmd_ioctl(filep, disk, cmd, argp);

should be split into two lines for readability.

Also is there a reason you don't want to pass down the various
other ioctls scsi_cmd_ioctl can handle to it?

> +				if (blk_pc_request(cmd->rq)) {
> +					if (cmd->rq->sense) {
> +						if (cmd->rq->sense_len > cmd->err_info->SenseLen)
> +							cmd->rq->sense_len = cmd->err_info->SenseLen;
> +						memcpy(cmd->rq->sense, cmd->err_info->SenseInfo, cmd->rq->sense_len);
> +					} else
> +						cmd->rq->sense_len = 0;
> +				}

Please add some linebreaks so that lines don't grow longer than 80
characters.

Re: [PATCH] Perverting cciss

[Stephen Cameron]

--- Christoph Hellwig <hch@infradead.org> wrote:
 
> Also is there a reason you don't want to pass down the various
> other ioctls scsi_cmd_ioctl can handle to it?

Didn't think about it, SG_IO is the one people were
clamoring for.

SG_GET_VERSION_NUM:
easily doable.

SCSI_IOCTL_GET_IDLUN:
Hmm, cciss is a block driver, and doesn't
really have an ID/LUN which are meaningful
in the normal sense of this ioctl.  If we
report something for this one, it is going to
conflict with some other device on the system
if that info is used in the usual way.  That is,
disk devices on cciss do not have any bus/target/lun
as far as linux is concerned, and aren't addressable
using those.

SCSI_IOCTL_GET_BUS_NUMBER:
Same problem as SCSI_IOCTL_GET_IDLUN.

SG_SET_TIMEOUT:
SG_GET_TIMEOUT:
Doable.  I don't think timeout will be honored though,
and I think this has to do with the sg driver, which
isn't involved with cciss anyhow, as cciss is not
a scsi driver (except for tape drives).

SG_GET_RESERVED_SIZE:
SG_SET_RESERVED_SIZE:
I think this is an sg driver thing and won't make
any difference for cciss, but we could pass it
through, I guess.

SG_EMULATED_HOST:

Er....  I guess we can do it:

/*
 * will always return that we are ATAPI even for a real SCSI drive, I'm not
 * so sure this is worth doing anything about (why would you care??)
 */
static int sg_emulated_host(request_queue_t *q, int __user *p)
{
        return put_user(1, p);
}

What's it for?



CDROM_SEND_PACKET:
CDROMCLOSETRAY:
CDROMEJECT:

I think all the CDROM stuff doesn't really apply.  CD-ROMs
on smartarray are not supported, nor are CD-ROMs presented
even if you attach one (excepting some weird tape device
that HP sells which sometimes pretends to be a CDROM for
booting purposes.) (Why would you put a cdrom on a RAID 
controller?)  I guess we could pass it through, it might not 
hurt, probably doesn't help though, and might give people
the mistaken impression that they can connect a SCSI cd-rom 
to a smartarray.

SCSI_IOCTL_SEND_COMMAND:

Doable, I think.

It'll be tomorrow at best before I can try anything out
or fix the other stuff you pointed out though.

-- steve




 
____________________________________________________________________________________
Bored stiff? Loosen up... 
Download and play hundreds of games for free on Yahoo! Games.
http://games.yahoo.com/games/front