[PATCH] hptiop: backout ioctl mess

[PATCH] hptiop: backout ioctl mess

[Christoph Hellwig]

The hptiop just got merged with a horrible amount of really bad ioctl
code that is against the standards for new scsi drivers.  This patch
backs it out (and fixes a small bug where scsi_add_host is called to
early).  We can re-add proper APIs once we agree on them.

Please send this to Linus for 2.6.19 so that the ioctls don't make it
into a release.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/hptiop.c
===================================================================
--- linux-2.6.orig/drivers/scsi/hptiop.c	2006-07-06 14:21:19.000000000 +0200
+++ linux-2.6/drivers/scsi/hptiop.c	2006-07-07 16:10:47.000000000 +0200
@@ -45,10 +45,6 @@
 static const char driver_name_long[] = "RocketRAID 3xxx SATA Controller driver";
 static const char driver_ver[] = "v1.0 (060426)";
 
-static DEFINE_SPINLOCK(hptiop_hba_list_lock);
-static LIST_HEAD(hptiop_hba_list);
-static int hptiop_cdev_major = -1;
-
 static void hptiop_host_request_callback(struct hptiop_hba *hba, u32 tag);
 static void hptiop_iop_request_callback(struct hptiop_hba *hba, u32 tag);
 static void hptiop_message_callback(struct hptiop_hba *hba, u32 msg);
@@ -620,532 +616,11 @@
 	return queue_depth;
 }
 
-struct hptiop_getinfo {
-	char __user *buffer;
-	loff_t buflength;
-	loff_t bufoffset;
-	loff_t buffillen;
-	loff_t filpos;
-};
-
-static void hptiop_copy_mem_info(struct hptiop_getinfo *pinfo,
-					char *data, int datalen)
-{
-	if (pinfo->filpos < pinfo->bufoffset) {
-		if (pinfo->filpos + datalen <= pinfo->bufoffset) {
-			pinfo->filpos += datalen;
-			return;
-		} else {
-			data += (pinfo->bufoffset - pinfo->filpos);
-			datalen  -= (pinfo->bufoffset - pinfo->filpos);
-			pinfo->filpos = pinfo->bufoffset;
-		}
-	}
-
-	pinfo->filpos += datalen;
-	if (pinfo->buffillen == pinfo->buflength)
-		return;
-
-	if (pinfo->buflength - pinfo->buffillen < datalen)
-		datalen = pinfo->buflength - pinfo->buffillen;
-
-	if (copy_to_user(pinfo->buffer + pinfo->buffillen, data, datalen))
-		return;
-
-	pinfo->buffillen += datalen;
-}
-
-static int hptiop_copy_info(struct hptiop_getinfo *pinfo, char *fmt, ...)
-{
-	va_list args;
-	char buf[128];
-	int len;
-
-	va_start(args, fmt);
-	len = vsnprintf(buf, sizeof(buf), fmt, args);
-	va_end(args);
-	hptiop_copy_mem_info(pinfo, buf, len);
-	return len;
-}
-
-static void hptiop_ioctl_done(struct hpt_ioctl_k *arg)
-{
-	arg->done = NULL;
-	wake_up(&arg->hba->ioctl_wq);
-}
-
-static void hptiop_do_ioctl(struct hpt_ioctl_k *arg)
-{
-	struct hptiop_hba *hba = arg->hba;
-	u32 val;
-	struct hpt_iop_request_ioctl_command __iomem *req;
-	int ioctl_retry = 0;
-
-	dprintk("scsi%d: hptiop_do_ioctl\n", hba->host->host_no);
-
-	/*
-	 * check (in + out) buff size from application.
-	 * outbuf must be dword aligned.
-	 */
-	if (((arg->inbuf_size + 3) & ~3) + arg->outbuf_size >
-			hba->max_request_size
-				- sizeof(struct hpt_iop_request_header)
-				- 4 * sizeof(u32)) {
-		dprintk("scsi%d: ioctl buf size (%d/%d) is too large\n",
-				hba->host->host_no,
-				arg->inbuf_size, arg->outbuf_size);
-		arg->result = HPT_IOCTL_RESULT_FAILED;
-		return;
-	}
-
-retry:
-	spin_lock_irq(hba->host->host_lock);
-
-	val = readl(&hba->iop->inbound_queue);
-	if (val == IOPMU_QUEUE_EMPTY) {
-		spin_unlock_irq(hba->host->host_lock);
-		dprintk("scsi%d: no free req for ioctl\n", hba->host->host_no);
-		arg->result = -1;
-		return;
-	}
-
-	req = (struct hpt_iop_request_ioctl_command __iomem *)
-			((unsigned long)hba->iop + val);
-
-	writel(HPT_CTL_CODE_LINUX_TO_IOP(arg->ioctl_code),
-			&req->ioctl_code);
-	writel(arg->inbuf_size, &req->inbuf_size);
-	writel(arg->outbuf_size, &req->outbuf_size);
-
-	/*
-	 * use the buffer on the IOP local memory first, then copy it
-	 * back to host.
-	 * the caller's request buffer shoudl be little-endian.
-	 */
-	if (arg->inbuf_size)
-		memcpy_toio(req->buf, arg->inbuf, arg->inbuf_size);
-
-	/* correct the controller ID for IOP */
-	if ((arg->ioctl_code == HPT_IOCTL_GET_CHANNEL_INFO ||
-		arg->ioctl_code == HPT_IOCTL_GET_CONTROLLER_INFO_V2 ||
-		arg->ioctl_code == HPT_IOCTL_GET_CONTROLLER_INFO)
-		&& arg->inbuf_size >= sizeof(u32))
-		writel(0, req->buf);
-
-	writel(IOP_REQUEST_TYPE_IOCTL_COMMAND, &req->header.type);
-	writel(0, &req->header.flags);
-	writel(offsetof(struct hpt_iop_request_ioctl_command, buf)
-			+ arg->inbuf_size, &req->header.size);
-	writel((u32)(unsigned long)arg, &req->header.context);
-	writel(BITS_PER_LONG > 32 ? (u32)((unsigned long)arg>>32) : 0,
-			&req->header.context_hi32);
-	writel(IOP_RESULT_PENDING, &req->header.result);
-
-	arg->result = HPT_IOCTL_RESULT_FAILED;
-	arg->done = hptiop_ioctl_done;
-
-	writel(val, &hba->iop->inbound_queue);
-	hptiop_pci_posting_flush(hba->iop);
-
-	spin_unlock_irq(hba->host->host_lock);
-
-	wait_event_timeout(hba->ioctl_wq, arg->done == NULL, 60 * HZ);
-
-	if (arg->done != NULL) {
-		hptiop_reset_hba(hba);
-		if (ioctl_retry++ < 3)
-			goto retry;
-	}
-
-	dprintk("hpt_iop_ioctl %x result %d\n",
-			arg->ioctl_code, arg->result);
-}
-
-static int __hpt_do_ioctl(struct hptiop_hba *hba, u32 code, void *inbuf,
-			u32 insize, void *outbuf, u32 outsize)
-{
-	struct hpt_ioctl_k arg;
-	arg.hba = hba;
-	arg.ioctl_code = code;
-	arg.inbuf = inbuf;
-	arg.outbuf = outbuf;
-	arg.inbuf_size = insize;
-	arg.outbuf_size = outsize;
-	arg.bytes_returned = NULL;
-	hptiop_do_ioctl(&arg);
-	return arg.result;
-}
-
-static inline int hpt_id_valid(__le32 id)
-{
-	return id != 0 && id != cpu_to_le32(0xffffffff);
-}
-
-static int hptiop_get_controller_info(struct hptiop_hba *hba,
-					struct hpt_controller_info *pinfo)
-{
-	int id = 0;
-
-	return __hpt_do_ioctl(hba, HPT_IOCTL_GET_CONTROLLER_INFO,
-		&id, sizeof(int), pinfo, sizeof(*pinfo));
-}
-
-
-static int hptiop_get_channel_info(struct hptiop_hba *hba, int bus,
-					struct hpt_channel_info *pinfo)
-{
-	u32 ids[2];
-
-	ids[0] = 0;
-	ids[1] = bus;
-	return __hpt_do_ioctl(hba, HPT_IOCTL_GET_CHANNEL_INFO,
-				ids, sizeof(ids), pinfo, sizeof(*pinfo));
-
-}
-
-static int hptiop_get_logical_devices(struct hptiop_hba *hba,
-					__le32 *pids, int maxcount)
-{
-	int i;
-	u32 count = maxcount - 1;
-
-	if (__hpt_do_ioctl(hba, HPT_IOCTL_GET_LOGICAL_DEVICES,
-			&count, sizeof(u32),
-			pids, sizeof(u32) * maxcount))
-		return -1;
-
-	maxcount = le32_to_cpu(pids[0]);
-	for (i = 0; i < maxcount; i++)
-		pids[i] = pids[i+1];
-
-	return maxcount;
-}
-
-static int hptiop_get_device_info_v3(struct hptiop_hba *hba, __le32 id,
-				struct hpt_logical_device_info_v3 *pinfo)
-{
-	return __hpt_do_ioctl(hba, HPT_IOCTL_GET_DEVICE_INFO_V3,
-				&id, sizeof(u32),
-				pinfo, sizeof(*pinfo));
-}
-
-static const char *get_array_status(struct hpt_logical_device_info_v3 *devinfo)
-{
-	static char s[64];
-	u32 flags = le32_to_cpu(devinfo->u.array.flags);
-	u32 trans_prog = le32_to_cpu(devinfo->u.array.transforming_progress);
-	u32 reb_prog = le32_to_cpu(devinfo->u.array.rebuilding_progress);
-
-	if (flags & ARRAY_FLAG_DISABLED)
-		return "Disabled";
-	else if (flags & ARRAY_FLAG_TRANSFORMING)
-		sprintf(s, "Expanding/Migrating %d.%d%%%s%s",
-			trans_prog / 100,
-			trans_prog % 100,
-			(flags & (ARRAY_FLAG_NEEDBUILDING|ARRAY_FLAG_BROKEN))?
-					", Critical" : "",
-			((flags & ARRAY_FLAG_NEEDINITIALIZING) &&
-			 !(flags & ARRAY_FLAG_REBUILDING) &&
-			 !(flags & ARRAY_FLAG_INITIALIZING))?
-					", Unintialized" : "");
-	else if ((flags & ARRAY_FLAG_BROKEN) &&
-				devinfo->u.array.array_type != AT_RAID6)
-		return "Critical";
-	else if (flags & ARRAY_FLAG_REBUILDING)
-		sprintf(s,
-			(flags & ARRAY_FLAG_NEEDINITIALIZING)?
-				"%sBackground initializing %d.%d%%" :
-					"%sRebuilding %d.%d%%",
-			(flags & ARRAY_FLAG_BROKEN)? "Critical, " : "",
-			reb_prog / 100,
-			reb_prog % 100);
-	else if (flags & ARRAY_FLAG_VERIFYING)
-		sprintf(s, "%sVerifying %d.%d%%",
-			(flags & ARRAY_FLAG_BROKEN)? "Critical, " : "",
-			reb_prog / 100,
-			reb_prog % 100);
-	else if (flags & ARRAY_FLAG_INITIALIZING)
-		sprintf(s, "%sForground initializing %d.%d%%",
-			(flags & ARRAY_FLAG_BROKEN)? "Critical, " : "",
-			reb_prog / 100,
-			reb_prog % 100);
-	else if (flags & ARRAY_FLAG_NEEDTRANSFORM)
-		sprintf(s,"%s%s%s", "Need Expanding/Migrating",
-			(flags & ARRAY_FLAG_BROKEN)? "Critical, " : "",
-			((flags & ARRAY_FLAG_NEEDINITIALIZING) &&
-			 !(flags & ARRAY_FLAG_REBUILDING) &&
-			 !(flags & ARRAY_FLAG_INITIALIZING))?
-				", Unintialized" : "");
-	else if (flags & ARRAY_FLAG_NEEDINITIALIZING &&
-		!(flags & ARRAY_FLAG_REBUILDING) &&
-		!(flags & ARRAY_FLAG_INITIALIZING))
-		sprintf(s,"%sUninitialized",
-			(flags & ARRAY_FLAG_BROKEN)? "Critical, " : "");
-	else if ((flags & ARRAY_FLAG_NEEDBUILDING) ||
-			(flags & ARRAY_FLAG_BROKEN))
-		return "Critical";
-	else
-		return "Normal";
-	return s;
-}
-
-static void hptiop_dump_devinfo(struct hptiop_hba *hba,
-			struct hptiop_getinfo *pinfo, __le32 id, int indent)
-{
-	struct hpt_logical_device_info_v3 devinfo;
-	int i;
-	u64 capacity;
-
-	for (i = 0; i < indent; i++)
-		hptiop_copy_info(pinfo, "\t");
-
-	if (hptiop_get_device_info_v3(hba, id, &devinfo)) {
-		hptiop_copy_info(pinfo, "unknown\n");
-		return;
-	}
-
-	switch (devinfo.type) {
-
-	case LDT_DEVICE: {
-		struct hd_driveid *driveid;
-		u32 flags = le32_to_cpu(devinfo.u.device.flags);
-
-		driveid = (struct hd_driveid *)devinfo.u.device.ident;
-		/* model[] is 40 chars long, but we just want 20 chars here */
-		driveid->model[20] = 0;
-
-		if (indent)
-			if (flags & DEVICE_FLAG_DISABLED)
-				hptiop_copy_info(pinfo,"Missing\n");
-			else
-				hptiop_copy_info(pinfo, "CH%d %s\n",
-					devinfo.u.device.path_id + 1,
-					driveid->model);
-		else {
-			capacity = le64_to_cpu(devinfo.capacity) * 512;
-			do_div(capacity, 1000000);
-			hptiop_copy_info(pinfo,
-				"CH%d %s, %lluMB, %s %s%s%s%s\n",
-				devinfo.u.device.path_id + 1,
-				driveid->model,
-				capacity,
-				(flags & DEVICE_FLAG_DISABLED)?
-					"Disabled" : "Normal",
-				devinfo.u.device.read_ahead_enabled?
-						"[RA]" : "",
-				devinfo.u.device.write_cache_enabled?
-						"[WC]" : "",
-				devinfo.u.device.TCQ_enabled?
-						"[TCQ]" : "",
-				devinfo.u.device.NCQ_enabled?
-						"[NCQ]" : ""
-			);
-		}
-		break;
-	}
-
-	case LDT_ARRAY:
-		if (devinfo.target_id != INVALID_TARGET_ID)
-			hptiop_copy_info(pinfo, "[DISK %d_%d] ",
-					devinfo.vbus_id, devinfo.target_id);
-
-		capacity = le64_to_cpu(devinfo.capacity) * 512;
-		do_div(capacity, 1000000);
-		hptiop_copy_info(pinfo, "%s (%s), %lluMB, %s\n",
-			devinfo.u.array.name,
-			devinfo.u.array.array_type==AT_RAID0? "RAID0" :
-				devinfo.u.array.array_type==AT_RAID1? "RAID1" :
-				devinfo.u.array.array_type==AT_RAID5? "RAID5" :
-				devinfo.u.array.array_type==AT_RAID6? "RAID6" :
-				devinfo.u.array.array_type==AT_JBOD? "JBOD" :
-					"unknown",
-			capacity,
-			get_array_status(&devinfo));
-		for (i = 0; i < devinfo.u.array.ndisk; i++) {
-			if (hpt_id_valid(devinfo.u.array.members[i])) {
-				if (cpu_to_le16(1<<i) &
-					devinfo.u.array.critical_members)
-					hptiop_copy_info(pinfo, "\t*");
-				hptiop_dump_devinfo(hba, pinfo,
-					devinfo.u.array.members[i], indent+1);
-			}
-			else
-				hptiop_copy_info(pinfo, "\tMissing\n");
-		}
-		if (id == devinfo.u.array.transform_source) {
-			hptiop_copy_info(pinfo, "\tExpanding/Migrating to:\n");
-			hptiop_dump_devinfo(hba, pinfo,
-				devinfo.u.array.transform_target, indent+1);
-		}
-		break;
-	}
-}
-
 static ssize_t hptiop_show_version(struct class_device *class_dev, char *buf)
 {
 	return snprintf(buf, PAGE_SIZE, "%s\n", driver_ver);
 }
 
-static ssize_t hptiop_cdev_read(struct file *filp, char __user *buf,
-				size_t count, loff_t *ppos)
-{
-	struct hptiop_hba *hba = filp->private_data;
-	struct hptiop_getinfo info;
-	int i, j, ndev;
-	struct hpt_controller_info con_info;
-	struct hpt_channel_info chan_info;
-	__le32 ids[32];
-
-	info.buffer     = buf;
-	info.buflength  = count;
-	info.bufoffset  = ppos ? *ppos : 0;
-	info.filpos     = 0;
-	info.buffillen  = 0;
-
-	if (hptiop_get_controller_info(hba, &con_info))
-		return -EIO;
-
-	for (i = 0; i < con_info.num_buses; i++) {
-		if (hptiop_get_channel_info(hba, i, &chan_info) == 0) {
-			if (hpt_id_valid(chan_info.devices[0]))
-				hptiop_dump_devinfo(hba, &info,
-						chan_info.devices[0], 0);
-			if (hpt_id_valid(chan_info.devices[1]))
-				hptiop_dump_devinfo(hba, &info,
-						chan_info.devices[1], 0);
-		}
-	}
-
-	ndev = hptiop_get_logical_devices(hba, ids,
-					sizeof(ids) / sizeof(ids[0]));
-
-	/*
-	 * if hptiop_get_logical_devices fails, ndev==-1 and it just
-	 * output nothing here
-	 */
-	for (j = 0; j < ndev; j++)
-		hptiop_dump_devinfo(hba, &info, ids[j], 0);
-
-	if (ppos)
-		*ppos += info.buffillen;
-
-	return info.buffillen;
-}
-
-static int hptiop_cdev_ioctl(struct inode *inode,  struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	struct hptiop_hba *hba = file->private_data;
-	struct hpt_ioctl_u ioctl_u;
-	struct hpt_ioctl_k ioctl_k;
-	u32 bytes_returned;
-	int err = -EINVAL;
-
-	if (copy_from_user(&ioctl_u,
-		(void __user *)arg, sizeof(struct hpt_ioctl_u)))
-		return -EINVAL;
-
-	if (ioctl_u.magic != HPT_IOCTL_MAGIC)
-		return -EINVAL;
-
-	ioctl_k.ioctl_code = ioctl_u.ioctl_code;
-	ioctl_k.inbuf = NULL;
-	ioctl_k.inbuf_size = ioctl_u.inbuf_size;
-	ioctl_k.outbuf = NULL;
-	ioctl_k.outbuf_size = ioctl_u.outbuf_size;
-	ioctl_k.hba = hba;
-	ioctl_k.bytes_returned = &bytes_returned;
-
-	/* verify user buffer */
-	if ((ioctl_k.inbuf_size && !access_ok(VERIFY_READ,
-			ioctl_u.inbuf, ioctl_k.inbuf_size)) ||
-		(ioctl_k.outbuf_size && !access_ok(VERIFY_WRITE,
-			ioctl_u.outbuf, ioctl_k.outbuf_size)) ||
-		(ioctl_u.bytes_returned && !access_ok(VERIFY_WRITE,
-			ioctl_u.bytes_returned, sizeof(u32))) ||
-		ioctl_k.inbuf_size + ioctl_k.outbuf_size > 0x10000) {
-
-		dprintk("scsi%d: got bad user address\n", hba->host->host_no);
-		return -EINVAL;
-	}
-
-	/* map buffer to kernel. */
-	if (ioctl_k.inbuf_size) {
-		ioctl_k.inbuf = kmalloc(ioctl_k.inbuf_size, GFP_KERNEL);
-		if (!ioctl_k.inbuf) {
-			dprintk("scsi%d: fail to alloc inbuf\n",
-					hba->host->host_no);
-			err = -ENOMEM;
-			goto err_exit;
-		}
-
-		if (copy_from_user(ioctl_k.inbuf,
-				ioctl_u.inbuf, ioctl_k.inbuf_size)) {
-			goto err_exit;
-		}
-	}
-
-	if (ioctl_k.outbuf_size) {
-		ioctl_k.outbuf = kmalloc(ioctl_k.outbuf_size, GFP_KERNEL);
-		if (!ioctl_k.outbuf) {
-			dprintk("scsi%d: fail to alloc outbuf\n",
-					hba->host->host_no);
-			err = -ENOMEM;
-			goto err_exit;
-		}
-	}
-
-	hptiop_do_ioctl(&ioctl_k);
-
-	if (ioctl_k.result == HPT_IOCTL_RESULT_OK) {
-		if (ioctl_k.outbuf_size &&
-			copy_to_user(ioctl_u.outbuf,
-				ioctl_k.outbuf, ioctl_k.outbuf_size))
-			goto err_exit;
-
-		if (ioctl_u.bytes_returned &&
-			copy_to_user(ioctl_u.bytes_returned,
-				&bytes_returned, sizeof(u32)))
-			goto err_exit;
-
-		err = 0;
-	}
-
-err_exit:
-	kfree(ioctl_k.inbuf);
-	kfree(ioctl_k.outbuf);
-
-	return err;
-}
-
-static int hptiop_cdev_open(struct inode *inode, struct file *file)
-{
-	struct hptiop_hba *hba;
-	unsigned i = 0, minor = iminor(inode);
-	int ret = -ENODEV;
-
-	spin_lock(&hptiop_hba_list_lock);
-	list_for_each_entry(hba, &hptiop_hba_list, link) {
-		if (i == minor) {
-			file->private_data = hba;
-			ret = 0;
-			goto out;
-		}
-		i++;
-	}
-
-out:
-	spin_unlock(&hptiop_hba_list_lock);
-	return ret;
-}
-
-static struct file_operations hptiop_cdev_fops = {
-	.owner = THIS_MODULE,
-	.read  = hptiop_cdev_read,
-	.ioctl = hptiop_cdev_ioctl,
-	.open  = hptiop_cdev_open,
-};
-
 static ssize_t hptiop_show_fw_version(struct class_device *class_dev, char *buf)
 {
 	struct Scsi_Host *host = class_to_shost(class_dev);
@@ -1296,19 +771,13 @@
 		goto unmap_pci_bar;
 	}
 
-	if (scsi_add_host(host, &pcidev->dev)) {
-		printk(KERN_ERR "scsi%d: scsi_add_host failed\n",
-					hba->host->host_no);
-		goto unmap_pci_bar;
-	}
-
 	pci_set_drvdata(pcidev, host);
 
 	if (request_irq(pcidev->irq, hptiop_intr, IRQF_SHARED,
 					driver_name, hba)) {
 		printk(KERN_ERR "scsi%d: request irq %d failed\n",
 					hba->host->host_no, pcidev->irq);
-		goto remove_scsi_host;
+		goto unmap_pci_bar;
 	}
 
 	/* Allocate request mem */
@@ -1355,9 +824,12 @@
 	if (hptiop_initialize_iop(hba))
 		goto free_request_mem;
 
-	spin_lock(&hptiop_hba_list_lock);
-	list_add_tail(&hba->link, &hptiop_hba_list);
-	spin_unlock(&hptiop_hba_list_lock);
+	if (scsi_add_host(host, &pcidev->dev)) {
+		printk(KERN_ERR "scsi%d: scsi_add_host failed\n",
+					hba->host->host_no);
+		goto free_request_mem;
+	}
+
 
 	scsi_scan_host(host);
 
@@ -1372,9 +844,6 @@
 free_request_irq:
 	free_irq(hba->pcidev->irq, hba);
 
-remove_scsi_host:
-	scsi_remove_host(host);
-
 unmap_pci_bar:
 	iounmap(hba->iop);
 
@@ -1422,10 +891,6 @@
 
 	scsi_remove_host(host);
 
-	spin_lock(&hptiop_hba_list_lock);
-	list_del_init(&hba->link);
-	spin_unlock(&hptiop_hba_list_lock);
-
 	hptiop_shutdown(pcidev);
 
 	free_irq(hba->pcidev->irq, hba);
@@ -1462,27 +927,12 @@
 
 static int __init hptiop_module_init(void)
 {
-	int error;
-
 	printk(KERN_INFO "%s %s\n", driver_name_long, driver_ver);
-
-	error = pci_register_driver(&hptiop_pci_driver);
-	if (error < 0)
-		return error;
-
-	hptiop_cdev_major = register_chrdev(0, "hptiop", &hptiop_cdev_fops);
-	if (hptiop_cdev_major < 0) {
-		printk(KERN_WARNING "unable to register hptiop device.\n");
-		return hptiop_cdev_major;
-	}
-
-	return 0;
+	return pci_register_driver(&hptiop_pci_driver);
 }
 
 static void __exit hptiop_module_exit(void)
 {
-	dprintk("hptiop_module_exit\n");
-	unregister_chrdev(hptiop_cdev_major, "hptiop");
 	pci_unregister_driver(&hptiop_pci_driver);
 }
 

Re: [PATCH] hptiop: backout ioctl mess

[HighPoint Linux Team]

Christoph Hellwig wrote:
> The hptiop just got merged with a horrible amount of really bad ioctl
> code that is against the standards for new scsi drivers.  This patch
> backs it out (and fixes a small bug where scsi_add_host is called to
> early).  We can re-add proper APIs once we agree on them.

What is the standard implemetation of private ioctls for scsi drivers?
Thanks.

HighPoint Linux Team

Re: [PATCH] hptiop: backout ioctl mess

[Douglas Gilbert]

HighPoint Linux Team wrote:
> Christoph Hellwig wrote:
>> The hptiop just got merged with a horrible amount of really bad ioctl
>> code that is against the standards for new scsi drivers.  This patch
>> backs it out (and fixes a small bug where scsi_add_host is called to
>> early).  We can re-add proper APIs once we agree on them.
> 
> What is the standard implemetation of private ioctls for scsi drivers?

Don't get caught.

You might try something creative like:
#define foo ioctl

More seriously see:
http://lwn.net/Articles/191653/
for a recent discussion.

Doug Gilbert

Re: [PATCH] hptiop: backout ioctl mess

[Arjan van de Ven]

On Mon, 2006-07-31 at 10:53 +0800, HighPoint Linux Team wrote:
> Christoph Hellwig wrote:
> > The hptiop just got merged with a horrible amount of really bad ioctl
> > code that is against the standards for new scsi drivers.  This patch
> > backs it out (and fixes a small bug where scsi_add_host is called to
> > early).  We can re-add proper APIs once we agree on them.
> 
> What is the standard implemetation of private ioctls for scsi drivers?
> Thanks.

Hi,

the real answer is: try really hard to use generic ioctls for your
functionality. What exactly do you need private ioctls for?
It's quite possible that there is a generic way to do the same thing
already, or something that can easily be extended to do what you need...

Greetings,
   Arjan van de Ven
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [PATCH] hptiop: backout ioctl mess

[HighPoint Linux Team]

Arjan van de Ven wrote:
> > Christoph Hellwig wrote:
> > > The hptiop just got merged with a horrible amount of really bad ioctl
> > > code that is against the standards for new scsi drivers.  This patch
> > > backs it out (and fixes a small bug where scsi_add_host is called to
> > > early).  We can re-add proper APIs once we agree on them.
> > 
> > What is the standard implemetation of private ioctls for scsi drivers?
> > Thanks.
>
> Hi,
>
> the real answer is: try really hard to use generic ioctls for your
> functionality. What exactly do you need private ioctls for?
> It's quite possible that there is a generic way to do the same thing
> already, or something that can easily be extended to do what you need...

We use private ioctls for RAID management. The driver registers a char
device for application to access the IOP. Several scsi drivers
(aacraid, 3w-9xxx, megaraid) use register_chrdev() for ioctls - is
this deprecated?

Another way is using scsi_host_template.ioctl interface but application
has no device node to open when there is no device on the controller.
Same problem on using sg_ioctl or scsi passthrough interface.


HighPoint Linux Team

Re: [PATCH] hptiop: backout ioctl mess

[Douglas Gilbert]

Arjan van de Ven wrote:
> On Mon, 2006-07-31 at 10:53 +0800, HighPoint Linux Team wrote:
>> Christoph Hellwig wrote:
>>> The hptiop just got merged with a horrible amount of really bad ioctl
>>> code that is against the standards for new scsi drivers.  This patch
>>> backs it out (and fixes a small bug where scsi_add_host is called to
>>> early).  We can re-add proper APIs once we agree on them.
>> What is the standard implemetation of private ioctls for scsi drivers?
>> Thanks.
> 
> Hi,
> 
> the real answer is: try really hard to use generic ioctls for your
> functionality. 

Now there is an idea :-) Pick a vendor specific opcode
like 0xff (and maybe have a module parameter to override
it to something else) and then use the SG_IO ioctl to
send "highpoint" specific SCSI commands that provide
the same functionality within the LLD that Christoph
just stripped out.

One problem with this approach is interacting with the
LLD in the absence of any (real) SCSI devices. It shouldn't be
too hard to trick the mid level into believing there are
SCSI devices connected (see the scsi_debug driver). May
I suggest a LUN slightly above the WLUN range (e.g.
49409+10). The fake device could then be created with:
# cd /sys/class/scsi_host/host0
# echo "- - 49419" > scan

The fake device would need to respond to an INQUIRY and a few
other simple SCSI commands to keep the mid-level happy.
May I also suggest the "processor" peripheral device type
which stops other ULDs getting involved. Then you can talk
to the LLD via a sg device node.

What exactly do you need private ioctls for?
> It's quite possible that there is a generic way to do the same thing
> already, or something that can easily be extended to do what you need...

Is the above generic enough?

Doug Gilbert

Re: [PATCH] hptiop: backout ioctl mess

[Arjan van de Ven]

On Wed, 2006-08-02 at 23:29 -0400, Douglas Gilbert wrote:
> > It's quite possible that there is a generic way to do the same thing
> > already, or something that can easily be extended to do what you need...
> 
> Is the above generic enough?

obviously not since the semantics still don't make sense on a higher up
level.

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [PATCH] hptiop: backout ioctl mess

[James Bottomley]

On Sun, 2006-07-30 at 19:13 +0200, Christoph Hellwig wrote:
> The hptiop just got merged with a horrible amount of really bad ioctl
> code that is against the standards for new scsi drivers.  This patch
> backs it out (and fixes a small bug where scsi_add_host is called to
> early).  We can re-add proper APIs once we agree on them.

I'm assuming the model you want to use is megaraid_sas, which only has a
couple of defined ioctls?

> Please send this to Linus for 2.6.19 so that the ioctls don't make it
> into a release.

OK, provided you take the lead in sorting out the ioctl infrastructure
for this driver ...

James

Re: [PATCH] hptiop: backout ioctl mess

[HighPoint Linux Team]

James Bottomley wrote:
> On Sun, 2006-07-30 at 19:13 +0200, Christoph Hellwig wrote:
> > The hptiop just got merged with a horrible amount of really bad ioctl
> > code that is against the standards for new scsi drivers.  This patch
> > backs it out (and fixes a small bug where scsi_add_host is called to
> > early).  We can re-add proper APIs once we agree on them.
> 
> I'm assuming the model you want to use is megaraid_sas, which only has a
> couple of defined ioctls?
> 
> > Please send this to Linus for 2.6.19 so that the ioctls don't make it
> > into a release.
> 
> OK, provided you take the lead in sorting out the ioctl infrastructure
> for this driver ...
> 
> James
> 

Is there a suggested solution now? We just want RAID management
functions to work on this driver. Is it ok to use register_chrdev()
for the management interface like megaraid_sas?

HighPoint Linux Team