Re: [PATCH] iSeries virtual disk

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Andrew,

Here is another attempt.  I would appreciate this going into your tree and
also Linus' if possible.  The patch below is against 2.6.3-mm4. I think I
have addressed all the comments that I received.

Unfortunately, things have moved on in the last couple of weeks and to
fix everyone;s abjections, I need to include in this patch a ppc64 specific
version of the dma_mapping routines.  They are pretty straight forward.

So this patch adds a device driver for PPC64 iSeries virtual disks.  Its
only defficiencies over previous published (out of tree) drivers for 2.4
is that it will not recognise dynamically added disks and the IDE emulation
is no longer supported.

Disks are now called /dev/iseries/vd<x><n> (without devfs) or
/dev/viod/disc<n>/part<n> (with devfs).  Up to 7 partitions are supported
on each of up to 32 disks.

I have run this on a 2 way (HT) iSeries machine with 4 virtual disks and
built it for pSeries (with the default pSeries config).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

diff -ruN 2.6.3-mm4/arch/ppc64/Kconfig 2.6.3-mm4-viodasd/arch/ppc64/Kconfig
--- 2.6.3-mm4/arch/ppc64/Kconfig	2004-02-26 15:40:05.000000000 +1100
+++ 2.6.3-mm4-viodasd/arch/ppc64/Kconfig	2004-02-26 16:08:36.000000000 +1100
@@ -271,15 +271,6 @@
 	  If you are running on an iSeries system and you want to use
  	  virtual disks created and managed by OS/400, say Y.
 
-config VIODASD_IDE
-	bool "iSeries Virtual disk IDE emulation"
-	depends on VIODASD
-	help
-	  This causes the iSeries virtual disks to look like IDE disks.
-	  If you have programs or utilities that only support certain
-	  kinds of disks, this option will cause iSeries virtual disks
-	  to pretend to be IDE disks, which may satisfy the program.
-
 config VIOCD
 	tristate "iSeries Virtual I/O CD support"
 	help
diff -ruN 2.6.3-mm4/arch/ppc64/kernel/Makefile 2.6.3-mm4-viodasd/arch/ppc64/kernel/Makefile
--- 2.6.3-mm4/arch/ppc64/kernel/Makefile	2004-02-26 15:40:06.000000000 +1100
+++ 2.6.3-mm4-viodasd/arch/ppc64/kernel/Makefile	2004-02-26 16:28:10.000000000 +1100
@@ -5,7 +5,7 @@
 EXTRA_CFLAGS	+= -mno-minimal-toc
 extra-y		:= head.o vmlinux.lds.s
 
-obj-y               :=	setup.o entry.o traps.o irq.o idle.o \
+obj-y               :=	setup.o entry.o traps.o irq.o idle.o dma.o \
 			time.o process.o signal.o syscalls.o misc.o ptrace.o \
 			align.o semaphore.o bitops.o stab.o pacaData.o \
 			udbg.o binfmt_elf32.o sys_ppc32.o ioctl32.o \
diff -ruN 2.6.3-mm4/arch/ppc64/kernel/dma.c 2.6.3-mm4-viodasd/arch/ppc64/kernel/dma.c
--- 2.6.3-mm4/arch/ppc64/kernel/dma.c	1970-01-01 10:00:00.000000000 +1000
+++ 2.6.3-mm4-viodasd/arch/ppc64/kernel/dma.c	2004-02-24 14:48:31.000000000 +1100
@@ -0,0 +1,173 @@
+/*
+ * Copyright (C) 2004 IBM Corporation
+ *
+ * Implements the generic device dma API for ppc64. Handles
+ * the pci and vio busses
+ */
+
+#include <linux/config.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+/* Include the busses we support */
+#include <linux/pci.h>
+#ifdef CONFIG_PPC_PSERIES
+#include <asm/vio.h>
+#endif
+#include <asm/scatterlist.h>
+#include <asm/bug.h>
+
+int dma_supported(struct device *dev, u64 mask)
+{
+	if (dev->bus == &pci_bus_type)
+		return pci_dma_supported(to_pci_dev(dev), mask);
+#ifdef CONFIG_PPC_PSERIES
+	if (dev->bus == &vio_bus_type)
+		return vio_dma_supported(to_vio_dev(dev), mask);
+#endif
+	BUG();
+	return 0;
+}
+
+int dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	if (dev->bus == &pci_bus_type)
+		return pci_set_dma_mask(to_pci_dev(dev), dma_mask);
+#ifdef CONFIG_PPC_PSERIES
+	if (dev->bus == &vio_bus_type)
+		return vio_set_dma_mask(to_vio_dev(dev), dma_mask);
+#endif
+	BUG();
+	return 0;
+}
+
+void *dma_alloc_coherent(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, int flag)
+{
+	if (dev->bus == &pci_bus_type)
+		return pci_alloc_consistent(to_pci_dev(dev), size, dma_handle);
+#ifdef CONFIG_PPC_PSERIES
+	if (dev->bus == &vio_bus_type)
+		return vio_alloc_consistent(to_vio_dev(dev), size, dma_handle);
+#endif
+	BUG();
+	return 0;
+}
+
+void dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
+		dma_addr_t dma_handle)
+{
+	if (dev->bus == &pci_bus_type)
+		pci_free_consistent(to_pci_dev(dev), size, cpu_addr, dma_handle);
+#ifdef CONFIG_PPC_PSERIES
+	else if (dev->bus == &vio_bus_type)
+		vio_free_consistent(to_vio_dev(dev), size, cpu_addr, dma_handle);
+#endif
+	else
+		BUG();
+}
+
+dma_addr_t dma_map_single(struct device *dev, void *cpu_addr, size_t size,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		return pci_map_single(to_pci_dev(dev), cpu_addr, size, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	if (dev->bus == &vio_bus_type)
+		return vio_map_single(to_vio_dev(dev), cpu_addr, size, (int)direction);
+#endif
+	BUG();
+	return (dma_addr_t)0;
+}
+
+void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		pci_unmap_single(to_pci_dev(dev), dma_addr, size, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	else if (dev->bus == &vio_bus_type)
+		vio_unmap_single(to_vio_dev(dev), dma_addr, size, (int)direction);
+#endif
+	else
+		BUG();
+}
+
+dma_addr_t dma_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		return pci_map_page(to_pci_dev(dev), page, offset, size, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	if (dev->bus == &vio_bus_type)
+		return vio_map_page(to_vio_dev(dev), page, offset, size, (int)direction);
+#endif
+	BUG();
+	return (dma_addr_t)0;
+}
+
+
+void dma_unmap_page(struct device *dev, dma_addr_t dma_address, size_t size,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		pci_unmap_page(to_pci_dev(dev), dma_address, size, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	else if (dev->bus == &vio_bus_type)
+		vio_unmap_page(to_vio_dev(dev), dma_address, size, (int)direction);
+#endif
+	else
+		BUG();
+}
+
+int dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		return pci_map_sg(to_pci_dev(dev), sg, nents, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	if (dev->bus == &vio_bus_type)
+		return vio_map_sg(to_vio_dev(dev), sg, nents, (int)direction);
+#endif
+	BUG();
+	return 0;
+}
+
+void dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nhwentries,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		pci_unmap_sg(to_pci_dev(dev), sg, nhwentries, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	else if (dev->bus == &vio_bus_type)
+		vio_unmap_sg(to_vio_dev(dev), sg, nhwentries, (int)direction);
+#endif
+	else
+		BUG();
+}
+
+void dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		pci_dma_sync_single(to_pci_dev(dev), dma_handle, size, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	else if (dev->bus == &vio_bus_type)
+		vio_dma_sync_single(to_vio_dev(dev), dma_handle, size, (int)direction);
+#endif
+	else
+		BUG();
+}
+
+void dma_sync_sg(struct device *dev, struct scatterlist *sg, int nelems,
+		enum dma_data_direction direction)
+{
+	if (dev->bus == &pci_bus_type)
+		pci_dma_sync_sg(to_pci_dev(dev), sg, nelems, (int)direction);
+#ifdef CONFIG_PPC_PSERIES
+	else if (dev->bus == &vio_bus_type)
+		vio_dma_sync_sg(to_vio_dev(dev), sg, nelems, (int)direction);
+#endif
+	else
+		BUG();
+}
diff -ruN 2.6.3-mm4/arch/ppc64/kernel/pci_dma.c 2.6.3-mm4-viodasd/arch/ppc64/kernel/pci_dma.c
--- 2.6.3-mm4/arch/ppc64/kernel/pci_dma.c	2004-02-18 16:40:26.000000000 +1100
+++ 2.6.3-mm4-viodasd/arch/ppc64/kernel/pci_dma.c	2004-02-26 16:34:45.000000000 +1100
@@ -67,10 +67,10 @@
 struct iSeries_Device_Node iSeries_vio_dev_node  = { .LogicalSlot = 0xFF, .DevTceTable = &virtBusVioTceTable };
 
 struct pci_dev    iSeries_veth_dev_st = { .sysdata = &iSeries_veth_dev_node };
-struct pci_dev    iSeries_vio_dev_st  = { .sysdata = &iSeries_vio_dev_node  };
+struct pci_dev    iSeries_vio_dev_st  = { .sysdata = &iSeries_vio_dev_node, .dev.bus = &pci_bus_type };
 
 struct pci_dev  * iSeries_veth_dev = &iSeries_veth_dev_st;
-struct pci_dev  * iSeries_vio_dev  = &iSeries_vio_dev_st;
+struct device  * iSeries_vio_dev  = &iSeries_vio_dev_st.dev;
 
 /* Device TceTable is stored in Device Node */
 /* struct TceTable * tceTables[256]; */	/* Tce tables for 256 busses
diff -ruN 2.6.3-mm4/arch/ppc64/kernel/viopath.c 2.6.3-mm4-viodasd/arch/ppc64/kernel/viopath.c
--- 2.6.3-mm4/arch/ppc64/kernel/viopath.c	2004-02-04 17:24:36.000000000 +1100
+++ 2.6.3-mm4-viodasd/arch/ppc64/kernel/viopath.c	2004-02-12 16:51:35.000000000 +1100
@@ -35,7 +35,8 @@
 #include <linux/vmalloc.h>
 #include <linux/string.h>
 #include <linux/proc_fs.h>
-#include <linux/pci.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/wait.h>
 
 #include <asm/hardirq.h>	/* for is_atomic */
@@ -48,7 +49,7 @@
 #include <asm/iSeries/iSeries_proc.h>
 #include <asm/iSeries/vio.h>
 
-extern struct pci_dev *iSeries_vio_dev;
+extern struct device *iSeries_vio_dev;
 
 /* Status of the path to each other partition in the system.
  * This is overkill, since we will only ever establish connections
@@ -194,8 +195,7 @@
 	HvLpEvent_Rc hvrc;
 	DECLARE_MUTEX_LOCKED(Semaphore);
 	dma_addr_t dmaa =
-	    pci_map_single(iSeries_vio_dev, buf, PAGE_SIZE,
-			   PCI_DMA_FROMDEVICE);
+	    dma_map_single(iSeries_vio_dev, buf, PAGE_SIZE, DMA_FROM_DEVICE);
 	int len = PAGE_SIZE;
 
 	if (len > blen)
@@ -215,8 +215,7 @@
 
 	down(&Semaphore);
 
-	pci_unmap_single(iSeries_vio_dev, dmaa, PAGE_SIZE,
-			 PCI_DMA_FROMDEVICE);
+	dma_unmap_single(iSeries_vio_dev, dmaa, PAGE_SIZE, DMA_FROM_DEVICE);
 
 	sprintf(buf + strlen(buf), "SRLNBR=");
 	buf[strlen(buf)] = e2a(xItExtVpdPanel.mfgID[2]);
diff -ruN 2.6.3-mm4/drivers/block/Makefile 2.6.3-mm4-viodasd/drivers/block/Makefile
--- 2.6.3-mm4/drivers/block/Makefile	2004-02-26 15:40:12.000000000 +1100
+++ 2.6.3-mm4-viodasd/drivers/block/Makefile	2004-02-26 16:24:57.000000000 +1100
@@ -39,3 +39,5 @@
 obj-$(CONFIG_BLK_DEV_UMEM)	+= umem.o
 obj-$(CONFIG_BLK_DEV_NBD)	+= nbd.o
 obj-$(CONFIG_BLK_DEV_CRYPTOLOOP) += cryptoloop.o
+
+obj-$(CONFIG_VIODASD)		+= viodasd.o
diff -ruN 2.6.3-mm4/drivers/block/viodasd.c 2.6.3-mm4-viodasd/drivers/block/viodasd.c
--- 2.6.3-mm4/drivers/block/viodasd.c	1970-01-01 10:00:00.000000000 +1000
+++ 2.6.3-mm4-viodasd/drivers/block/viodasd.c	2004-02-26 13:54:11.000000000 +1100
@@ -0,0 +1,869 @@
+/* -*- linux-c -*-
+ * viodasd.c
+ *  Authors: Dave Boutcher <boutcher@us.ibm.com>
+ *           Ryan Arnold <ryanarn@us.ibm.com>
+ *           Colin Devilbiss <devilbis@us.ibm.com>
+ *           Stephen Rothwell <sfr@au1.ibm.com>
+ *
+ * (C) Copyright 2000-2004 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * This routine provides access to disk space (termed "DASD" in historical
+ * IBM terms) owned and managed by an OS/400 partition running on the
+ * same box as this Linux partition.
+ *
+ * All disk operations are performed by sending messages back and forth to
+ * the OS/400 partition.
+ */
+#include <linux/major.h>
+#include <linux/fs.h>
+#include <asm/uaccess.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/blkdev.h>
+#include <linux/genhd.h>
+#include <linux/hdreg.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/string.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/seq_file.h>
+#include <linux/completion.h>
+
+#include <asm/iSeries/HvTypes.h>
+#include <asm/iSeries/HvLpEvent.h>
+#include <asm/iSeries/HvLpConfig.h>
+#include <asm/iSeries/vio.h>
+
+MODULE_DESCRIPTION("iSeries Virtual DASD");
+MODULE_AUTHOR("Dave Boutcher");
+MODULE_LICENSE("GPL");
+
+/*
+ * We only support 7 partitions per physical disk....so with minor
+ * numbers 0-255 we get a maximum of 32 disks.
+ */
+#define VIOD_GENHD_DEVFS_NAME	"viod/disc"
+#define VIOD_GENHD_NAME		"iseries/vd"
+
+#define VIOD_VERS		"1.63"
+
+#define VIOD_KERN_WARNING	KERN_WARNING "viod: "
+#define VIOD_KERN_INFO		KERN_INFO "viod: "
+
+enum {
+	PARTITION_SHIFT = 3,
+	MAX_DISKNO = 32,
+	MAX_DISK_NAME = sizeof(((struct gendisk *)0)->disk_name)
+};
+
+static int		viodasd_max_disk;
+static spinlock_t	viodasd_spinlock = SPIN_LOCK_UNLOCKED;
+
+#define VIOMAXREQ		16
+#define VIOMAXBLOCKDMA		12
+
+#define DEVICE_NO(cell)	((struct viodasd_device *)(cell) - &viodasd_devices[0])
+
+extern struct device *iSeries_vio_dev;
+
+struct open_data {
+	u64	disk_size;
+	u16	max_disk;
+	u16	cylinders;
+	u16	tracks;
+	u16	sectors;
+	u16	bytes_per_sector;
+};
+
+struct rw_data {
+	u64	offset;
+	struct {
+		u32	token;
+		u32	reserved;
+		u64	len;
+	} dma_info[VIOMAXBLOCKDMA];
+};
+
+struct vioblocklpevent {
+	struct HvLpEvent	event;
+	u32			reserved;
+	u16			version;
+	u16			sub_result;
+	u16			disk;
+	u16			flags;
+	union {
+		struct open_data	open_data;
+		struct rw_data		rw_data;
+		u64			changed;
+	} u;
+};
+
+#define vioblockflags_ro   0x0001
+
+enum vioblocksubtype {
+	vioblockopen = 0x0001,
+	vioblockclose = 0x0002,
+	vioblockread = 0x0003,
+	vioblockwrite = 0x0004,
+	vioblockflush = 0x0005,
+	vioblockcheck = 0x0007
+};
+
+struct viodasd_waitevent {
+	struct completion	com;
+	int			rc;
+	union {
+		int		changed;	/* Used only for check_change */
+		u16		sub_result;
+	} data;
+};
+
+static const struct vio_error_entry viodasd_err_table[] = {
+	{ 0x0201, EINVAL, "Invalid Range" },
+	{ 0x0202, EINVAL, "Invalid Token" },
+	{ 0x0203, EIO, "DMA Error" },
+	{ 0x0204, EIO, "Use Error" },
+	{ 0x0205, EIO, "Release Error" },
+	{ 0x0206, EINVAL, "Invalid Disk" },
+	{ 0x0207, EBUSY, "Cant Lock" },
+	{ 0x0208, EIO, "Already Locked" },
+	{ 0x0209, EIO, "Already Unlocked" },
+	{ 0x020A, EIO, "Invalid Arg" },
+	{ 0x020B, EIO, "Bad IFS File" },
+	{ 0x020C, EROFS, "Read Only Device" },
+	{ 0x02FF, EIO, "Internal Error" },
+	{ 0x0000, 0, NULL },
+};
+
+/*
+ * Figure out the biggest I/O request (in sectors) we can accept
+ */
+#define VIODASD_MAXSECTORS (4096 / 512 * VIOMAXBLOCKDMA)
+
+/*
+ * Number of disk I/O requests we've sent to OS/400
+ */
+static int num_req_outstanding;
+
+/*
+ * This is our internal structure for keeping track of disk devices
+ */
+struct viodasd_device {
+	u16		cylinders;
+	u16		tracks;
+	u16		sectors;
+	u16		bytes_per_sector;
+	u64		size;
+	int		read_only;
+	spinlock_t	q_lock;
+	struct gendisk	*disk;
+} viodasd_devices[MAX_DISKNO];
+
+static void viodasd_init_disk(struct viodasd_device *d);
+
+/*
+ * End a request
+ */
+static void viodasd_end_request(struct request *req, int uptodate,
+		int num_sectors)
+{
+	if (end_that_request_first(req, uptodate, num_sectors))
+		return;
+        add_disk_randomness(req->rq_disk);
+	end_that_request_last(req);
+}
+
+/*
+ * This rebuilds the partition information for a single disk device
+ */
+static int viodasd_revalidate(struct gendisk *gendisk)
+{
+	struct viodasd_device *device = gendisk->private_data;
+
+	set_capacity(gendisk, device->size >> 9);
+	return 0;
+}
+
+/*
+ * Probe a single disk and fill in the viodasd_device structure
+ * for it.
+ */
+static void probe_disk(struct viodasd_device *d)
+{
+	HvLpEvent_Rc hvrc;
+	struct viodasd_waitevent we;
+	int dev_no = DEVICE_NO(d);
+
+	init_completion(&we.com);
+
+	/* Send the open event to OS/400 */
+	hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
+			HvLpEvent_Type_VirtualIo,
+			viomajorsubtype_blockio | vioblockopen,
+			HvLpEvent_AckInd_DoAck, HvLpEvent_AckType_ImmediateAck,
+			viopath_sourceinst(viopath_hostLp),
+			viopath_targetinst(viopath_hostLp),
+			(u64)(unsigned long)&we, VIOVERSION << 16,
+			((u64)dev_no << 48) | ((u64)vioblockflags_ro << 32),
+			0, 0, 0);
+	if (hvrc != 0) {
+		printk(VIOD_KERN_WARNING "bad rc on HV open %d\n", (int)hvrc);
+		return;
+	}
+
+	wait_for_completion(&we.com);
+
+	if (we.rc != 0)
+		return;
+
+	/* Send the close event to OS/400.  We DON'T expect a response */
+	hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
+			HvLpEvent_Type_VirtualIo,
+			viomajorsubtype_blockio | vioblockclose,
+			HvLpEvent_AckInd_NoAck, HvLpEvent_AckType_ImmediateAck,
+			viopath_sourceinst(viopath_hostLp),
+			viopath_targetinst(viopath_hostLp),
+			0, VIOVERSION << 16,
+			((u64)dev_no << 48) | ((u64)vioblockflags_ro << 32),
+			0, 0, 0);
+	if (hvrc != 0)
+		printk(VIOD_KERN_WARNING
+		       "bad rc sending event to OS/400 %d\n", (int)hvrc);
+	else {
+		printk(VIOD_KERN_INFO "disk %d: %lu sectors (%lu MB) "
+				"CHS=%d/%d/%d sector size %d\n",
+				dev_no, (unsigned long)(d->size >> 9),
+				(unsigned long)(d->size >> 20),
+				(int)d->cylinders, (int)d->tracks,
+				(int)d->sectors, (int)d->bytes_per_sector);
+		viodasd_init_disk(d);
+	}
+}
+
+/*
+ * External open entry point.
+ */
+static int viodasd_open(struct inode *ino, struct file *fil)
+{
+	struct viodasd_device *d = ino->i_bdev->bd_disk->private_data;
+	HvLpEvent_Rc hvrc;
+	struct viodasd_waitevent we;
+
+	init_completion(&we.com);
+
+	/* Send the open event to OS/400 */
+	hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
+			HvLpEvent_Type_VirtualIo,
+			viomajorsubtype_blockio | vioblockopen,
+			HvLpEvent_AckInd_DoAck, HvLpEvent_AckType_ImmediateAck,
+			viopath_sourceinst(viopath_hostLp),
+			viopath_targetinst(viopath_hostLp),
+			(u64)(unsigned long)&we, VIOVERSION << 16,
+			((u64)DEVICE_NO(d) << 48) /* | ((u64)flags << 32) */,
+			0, 0, 0);
+	if (hvrc != 0) {
+		printk(VIOD_KERN_WARNING "HV open failed %d\n", (int)hvrc);
+		return -EIO;
+	}
+
+	wait_for_completion(&we.com);
+
+	/* Check the return code */
+	if (we.rc != 0) {
+		const struct vio_error_entry *err =
+			vio_lookup_rc(viodasd_err_table, we.data.sub_result);
+
+		printk(VIOD_KERN_WARNING
+				"bad rc opening disk: %d:0x%04x (%s)\n",
+				(int)we.rc, we.data.sub_result, err->msg);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/*
+ * External release entry point.
+ */
+static int viodasd_release(struct inode *ino, struct file *fil)
+{
+	struct viodasd_device *d = ino->i_bdev->bd_disk->private_data;
+	HvLpEvent_Rc hvrc;
+
+	/* Send the event to OS/400.  We DON'T expect a response */
+	hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
+			HvLpEvent_Type_VirtualIo,
+			viomajorsubtype_blockio | vioblockclose,
+			HvLpEvent_AckInd_NoAck, HvLpEvent_AckType_ImmediateAck,
+			viopath_sourceinst(viopath_hostLp),
+			viopath_targetinst(viopath_hostLp),
+			0, VIOVERSION << 16,
+			((u64)DEVICE_NO(d) << 48) /* | ((u64)flags << 32) */,
+			0, 0, 0);
+	if (hvrc != 0)
+		printk(VIOD_KERN_WARNING "HV close call failed %d\n",
+				(int)hvrc);
+	return 0;
+}
+
+
+/* External ioctl entry point.
+ */
+static int viodasd_ioctl(struct inode *ino, struct file *fil,
+			 unsigned int cmd, unsigned long arg)
+{
+	int err;
+	unsigned char sectors;
+	unsigned char heads;
+	unsigned short cylinders;
+	struct hd_geometry *geo;
+	struct gendisk *gendisk;
+	struct viodasd_device *d;
+
+	switch (cmd) {
+	case HDIO_GETGEO:
+		geo = (struct hd_geometry *)arg;
+		if (geo == NULL)
+			return -EINVAL;
+		err = verify_area(VERIFY_WRITE, geo, sizeof(*geo));
+		if (err)
+			return err;
+		gendisk = ino->i_bdev->bd_disk;
+		d = gendisk->private_data;
+		sectors = d->sectors;
+		if (sectors == 0)
+			sectors = 32;
+		heads = d->tracks;
+		if (heads == 0)
+			heads = 64;
+		cylinders = d->cylinders;
+		if (cylinders == 0)
+			cylinders = get_capacity(gendisk) / (sectors * heads);
+		if (__put_user(sectors, &geo->sectors) ||
+		    __put_user(heads, &geo->heads) ||
+		    __put_user(cylinders, &geo->cylinders) ||
+		    __put_user(get_start_sect(ino->i_bdev), &geo->start))
+			return -EFAULT;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+/*
+ * Send an actual I/O request to OS/400
+ */
+static int send_request(struct request *req)
+{
+	u64 start;
+	int direction;
+	int nsg;
+	u16 viocmd;
+	HvLpEvent_Rc hvrc;
+	struct vioblocklpevent *bevent;
+	struct scatterlist sg[VIOMAXBLOCKDMA];
+	int sgindex;
+	int statindex;
+	struct viodasd_device *d;
+	unsigned long flags;
+
+	start = (u64)req->sector << 9;
+
+	if (rq_data_dir(req) == READ) {
+		direction = DMA_FROM_DEVICE;
+		viocmd = viomajorsubtype_blockio | vioblockread;
+		statindex = 0;
+	} else {
+		direction = DMA_TO_DEVICE;
+		viocmd = viomajorsubtype_blockio | vioblockwrite;
+		statindex = 1;
+	}
+
+        d = req->rq_disk->private_data;
+
+	/* Now build the scatter-gather list */
+	nsg = blk_rq_map_sg(req->q, req, sg);
+	nsg = dma_map_sg(iSeries_vio_dev, sg, nsg, direction);
+
+	spin_lock_irqsave(&viodasd_spinlock, flags);
+	num_req_outstanding++;
+
+	/* This optimization handles a single DMA block */
+	if (nsg == 1)
+		hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
+				HvLpEvent_Type_VirtualIo, viocmd,
+				HvLpEvent_AckInd_DoAck,
+				HvLpEvent_AckType_ImmediateAck,
+				viopath_sourceinst(viopath_hostLp),
+				viopath_targetinst(viopath_hostLp),
+				(u64)(unsigned long)req, VIOVERSION << 16,
+				((u64)DEVICE_NO(d) << 48), start,
+				((u64)sg[0].dma_address) << 32,
+				sg[0].dma_length);
+	else {
+		bevent = (struct vioblocklpevent *)
+			vio_get_event_buffer(viomajorsubtype_blockio);
+		if (bevent == NULL) {
+			num_req_outstanding--;
+			spin_unlock_irqrestore(&viodasd_spinlock, flags);
+			dma_unmap_sg(iSeries_vio_dev, sg, nsg, direction);
+			printk(VIOD_KERN_WARNING
+			       "error allocating disk event buffer\n");
+			return -1;
+		}
+
+		/*
+		 * Now build up the actual request.  Note that we store
+		 * the pointer to the request in the correlation
+		 * token so we can match the response up later
+		 */
+		memset(bevent, 0, sizeof(struct vioblocklpevent));
+		bevent->event.xFlags.xValid = 1;
+		bevent->event.xFlags.xFunction = HvLpEvent_Function_Int;
+		bevent->event.xFlags.xAckInd = HvLpEvent_AckInd_DoAck;
+		bevent->event.xFlags.xAckType = HvLpEvent_AckType_ImmediateAck;
+		bevent->event.xType = HvLpEvent_Type_VirtualIo;
+		bevent->event.xSubtype = viocmd;
+		bevent->event.xSourceLp = HvLpConfig_getLpIndex();
+		bevent->event.xTargetLp = viopath_hostLp;
+		bevent->event.xSizeMinus1 =
+			offsetof(struct vioblocklpevent, u.rw_data.dma_info) +
+			(sizeof(bevent->u.rw_data.dma_info[0]) * nsg) - 1;
+		bevent->event.xSourceInstanceId =
+			viopath_sourceinst(viopath_hostLp);
+		bevent->event.xTargetInstanceId =
+			viopath_targetinst(viopath_hostLp);
+		bevent->event.xCorrelationToken = (u64)req;
+		bevent->version = VIOVERSION;
+		bevent->disk = DEVICE_NO(d);
+		bevent->u.rw_data.offset = start;
+
+		/*
+		 * Copy just the dma information from the sg list
+		 * into the request
+		 */
+		for (sgindex = 0; sgindex < nsg; sgindex++) {
+			bevent->u.rw_data.dma_info[sgindex].token =
+				sg[sgindex].dma_address;
+			bevent->u.rw_data.dma_info[sgindex].len =
+				sg[sgindex].dma_length;
+		}
+
+		/* Send the request */
+		hvrc = HvCallEvent_signalLpEvent(&bevent->event);
+		vio_free_event_buffer(viomajorsubtype_blockio, bevent);
+	}
+
+	if (hvrc != HvLpEvent_Rc_Good) {
+		num_req_outstanding--;
+		spin_unlock_irqrestore(&viodasd_spinlock, flags);
+		dma_unmap_sg(iSeries_vio_dev, sg, nsg, direction);
+		printk(VIOD_KERN_WARNING
+		       "error sending disk event to OS/400 (rc %d)\n",
+		       (int)hvrc);
+		return -1;
+	}
+	spin_unlock_irqrestore(&viodasd_spinlock, flags);
+	return 0;
+}
+
+/*
+ * This is the external request processing routine
+ */
+static void do_viodasd_request(request_queue_t *q)
+{
+	struct request *req;
+
+	while ((req = elv_next_request(q)) != NULL) {
+		/*
+		 * If we already have the maximum number of requests
+		 * outstanding to OS/400 just bail out. We'll come
+		 * back later.
+		 */
+		if (num_req_outstanding >= VIOMAXREQ)
+			break;
+
+		/* dequeue the current request from the queue */
+		blkdev_dequeue_request(req);
+		/* check that request contains a valid command */
+		if (!blk_fs_request(req)) {
+			viodasd_end_request(req, 0, req->hard_nr_sectors);
+			continue;
+		}
+
+		/* Try sending the request */
+		if (send_request(req) != 0)
+			viodasd_end_request(req, 0, req->hard_nr_sectors);
+	}
+}
+
+/*
+ * Check for changed disks
+ */
+static int viodasd_check_change(struct gendisk *gendisk)
+{
+	struct viodasd_waitevent we;
+	HvLpEvent_Rc hvrc;
+	struct viodasd_device *d = gendisk->private_data;
+
+	/* Check that we are dealing with a valid hosting partition */
+	if (viopath_hostLp == HvLpIndexInvalid) {
+		printk(VIOD_KERN_WARNING "Invalid hosting partition\n");
+		return -EIO;
+	}
+
+	init_completion(&we.com);
+
+	/* Send the open event to OS/400 */
+	hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
+			HvLpEvent_Type_VirtualIo,
+			viomajorsubtype_blockio | vioblockcheck,
+			HvLpEvent_AckInd_DoAck, HvLpEvent_AckType_ImmediateAck,
+			viopath_sourceinst(viopath_hostLp),
+			viopath_targetinst(viopath_hostLp),
+			(u64)(unsigned long)&we, VIOVERSION << 16,
+			((u64)DEVICE_NO(d) << 48), 0, 0, 0);
+	if (hvrc != 0) {
+		printk(VIOD_KERN_WARNING "bad rc on signalLpEvent %d\n",
+				(int)hvrc);
+		return -EIO;
+	}
+
+	wait_for_completion(&we.com);
+
+	/* Check the return code.  If bad, assume no change */
+	if (we.rc != 0) {
+		printk(VIOD_KERN_WARNING
+			"bad rc %d on check_change. Assuming no change\n",
+			(int)we.rc);
+		return 0;
+	}
+
+	return we.data.changed;
+}
+
+/*
+ * Our file operations table
+ */
+static struct block_device_operations viodasd_fops = {
+	.owner = THIS_MODULE,
+	.open = viodasd_open,
+	.release = viodasd_release,
+	.ioctl = viodasd_ioctl,
+	.media_changed = viodasd_check_change,
+	.revalidate_disk = viodasd_revalidate
+};
+
+/* returns the total number of scatterlist elements converted */
+static int block_event_to_scatterlist(const struct vioblocklpevent *bevent,
+		struct scatterlist *sg, int *total_len)
+{
+	int i, numsg;
+	const struct rw_data *rw_data = &bevent->u.rw_data;
+	static const int offset =
+		offsetof(struct vioblocklpevent, u.rw_data.dma_info);
+	static const int element_size = sizeof(rw_data->dma_info[0]);
+
+	numsg = ((bevent->event.xSizeMinus1 + 1) - offset) / element_size;
+	if (numsg > VIOMAXBLOCKDMA)
+		numsg = VIOMAXBLOCKDMA;
+
+	*total_len = 0;
+	memset(sg, 0, sizeof(sg[0]) * VIOMAXBLOCKDMA);
+
+	for (i = 0; (i < numsg) && (rw_data->dma_info[i].len > 0); ++i) {
+		sg[i].dma_address = rw_data->dma_info[i].token;
+		sg[i].dma_length = rw_data->dma_info[i].len;
+		*total_len += rw_data->dma_info[i].len;
+	}
+	return i;
+}
+
+/*
+ * Restart all queues, starting with the one _after_ the disk given,
+ * thus reducing the chance of starvation of higher numbered disks.
+ */
+static void viodasd_restart_all_queues_starting_from(int first_index)
+{
+	int i;
+
+	for (i = first_index + 1; i < MAX_DISKNO; ++i)
+		if (viodasd_devices[i].disk)
+			blk_run_queue(viodasd_devices[i].disk->queue);
+	for (i = 0; i <= first_index; ++i)
+		if (viodasd_devices[i].disk)
+			blk_run_queue(viodasd_devices[i].disk->queue);
+}
+
+/*
+ * For read and write requests, decrement the number of outstanding requests,
+ * Free the DMA buffers we allocated.
+ */
+static int viodasd_handleReadWrite(struct vioblocklpevent *bevent)
+{
+	int num_sg, num_sect, pci_direction, total_len;
+	struct request *req;
+	struct scatterlist sg[VIOMAXBLOCKDMA];
+	struct HvLpEvent *event = &bevent->event;
+	unsigned long irq_flags;
+	int device_no;
+	int error;
+	spinlock_t *qlock;
+
+	num_sg = block_event_to_scatterlist(bevent, sg, &total_len);
+	num_sect = total_len >> 9;
+	if (event->xSubtype == (viomajorsubtype_blockio | vioblockread))
+		pci_direction = DMA_FROM_DEVICE;
+	else
+		pci_direction = DMA_TO_DEVICE;
+	dma_unmap_sg(iSeries_vio_dev, sg, num_sg, pci_direction);
+
+	/*
+	 * Since this is running in interrupt mode, we need to make sure
+	 * we're not stepping on any global I/O operations
+	 */
+	spin_lock_irqsave(&viodasd_spinlock, irq_flags);
+	num_req_outstanding--;
+	spin_unlock_irqrestore(&viodasd_spinlock, irq_flags);
+
+	req = (struct request *)bevent->event.xCorrelationToken;
+	device_no = DEVICE_NO(req->rq_disk->private_data);
+
+	error = event->xRc != HvLpEvent_Rc_Good;
+	if (error) {
+		const struct vio_error_entry *err;
+		err = vio_lookup_rc(viodasd_err_table, bevent->sub_result);
+		printk(VIOD_KERN_WARNING "read/write error %d:0x%04x (%s)\n",
+				event->xRc, bevent->sub_result, err->msg);
+		num_sect = req->hard_nr_sectors;
+	}
+	qlock = req->q->queue_lock;
+	spin_lock(qlock);
+	viodasd_end_request(req, !error, num_sect);
+	spin_unlock(qlock);
+
+	/* Finally, try to get more requests off of this device's queue */
+	viodasd_restart_all_queues_starting_from(device_no);
+
+	return 0;
+}
+
+/* This routine handles incoming block LP events */
+static void vioHandleBlockEvent(struct HvLpEvent *event)
+{
+	struct vioblocklpevent *bevent = (struct vioblocklpevent *)event;
+	struct viodasd_waitevent *pwe;
+
+	if (event == NULL)
+		/* Notification that a partition went away! */
+		return;
+	/* First, we should NEVER get an int here...only acks */
+	if (event->xFlags.xFunction == HvLpEvent_Function_Int) {
+		printk(VIOD_KERN_WARNING
+		       "Yikes! got an int in viodasd event handler!\n");
+		if (event->xFlags.xAckInd == HvLpEvent_AckInd_DoAck) {
+			event->xRc = HvLpEvent_Rc_InvalidSubtype;
+			HvCallEvent_ackLpEvent(event);
+		}
+	}
+
+	switch (event->xSubtype & VIOMINOR_SUBTYPE_MASK) {
+	case vioblockopen:
+		/*
+		 * Handle a response to an open request.  We get all the
+		 * disk information in the response, so update it.  The
+		 * correlation token contains a pointer to a waitevent
+		 * structure that has a completion in it.  update the
+		 * return code in the waitevent structure and post the
+		 * completion to wake up the guy who sent the request
+		 */
+		pwe = (struct viodasd_waitevent *)event->xCorrelationToken;
+		pwe->rc = event->xRc;
+		pwe->data.sub_result = bevent->sub_result;
+		if (event->xRc == HvLpEvent_Rc_Good) {
+			const struct open_data *data = &bevent->u.open_data;
+			struct viodasd_device *device =
+				&viodasd_devices[bevent->disk];
+			device->read_only =
+				bevent->flags & vioblockflags_ro;
+			device->size = data->disk_size;
+			device->cylinders = data->cylinders;
+			device->tracks = data->tracks;
+			device->sectors = data->sectors;
+			device->bytes_per_sector = data->bytes_per_sector;
+			viodasd_max_disk = data->max_disk;
+		}
+		complete(&pwe->com);
+		break;
+	case vioblockclose:
+		break;
+	case vioblockcheck:
+		pwe = (struct viodasd_waitevent *)event->xCorrelationToken;
+		pwe->rc = event->xRc;
+		pwe->data.changed = bevent->u.changed;
+		complete(&pwe->com);
+		break;
+#if 0
+	case vioblockflush:
+		complete((struct completion *)event->xCorrelationToken);
+		break;
+#endif
+	case vioblockread:
+	case vioblockwrite:
+		viodasd_handleReadWrite(bevent);
+		break;
+
+	default:
+		printk(VIOD_KERN_WARNING "invalid subtype!");
+		if (event->xFlags.xAckInd == HvLpEvent_AckInd_DoAck) {
+			event->xRc = HvLpEvent_Rc_InvalidSubtype;
+			HvCallEvent_ackLpEvent(event);
+		}
+	}
+}
+
+static void viodasd_init_disk(struct viodasd_device *d)
+{
+	struct gendisk *g;
+	struct request_queue *q;
+	int disk_no = DEVICE_NO(d);
+
+	if (d->disk)
+		return;
+
+	/* create the request queue for the disk */
+	spin_lock_init(&d->q_lock);
+	q = blk_init_queue(do_viodasd_request, &d->q_lock);
+	if (q == NULL) {
+		printk(VIOD_KERN_WARNING "cannot allocate queue for disk %d\n",
+				disk_no);
+		return;
+	}
+	g = alloc_disk(1 << PARTITION_SHIFT);
+	if (g == NULL) {
+		printk(VIOD_KERN_WARNING
+				"cannot allocate disk structure for disk %d\n",
+				disk_no);
+		blk_cleanup_queue(q);
+		return;
+	}
+
+	d->disk = g;
+	blk_queue_max_hw_segments(q, VIOMAXBLOCKDMA);
+	blk_queue_max_sectors(q, VIODASD_MAXSECTORS);
+	g->major = VIODASD_MAJOR;
+	g->first_minor = disk_no << PARTITION_SHIFT;
+	if (disk_no >= 26)
+		snprintf(g->disk_name, sizeof(g->disk_name),
+				VIOD_GENHD_NAME "%c%c",
+				'a' + (disk_no / 26) - 1, 'a' + (disk_no % 26));
+	else
+		snprintf(g->disk_name, sizeof(g->disk_name),
+				VIOD_GENHD_NAME "%c", 'a' + (disk_no % 26));
+	snprintf(g->devfs_name, sizeof(g->devfs_name),
+			"%s%d", VIOD_GENHD_DEVFS_NAME, disk_no);
+	g->fops = &viodasd_fops;
+	g->queue = q;
+	g->private_data = (void *)d;
+	set_capacity(g, d->size >> 9);
+
+	/* register us in the global list */
+	add_disk(g);
+}
+
+/*
+ * Initialize the whole device driver.  Handle module and non-module
+ * versions
+ */
+static int __init viodasd_init(void)
+{
+	int i;
+
+	/* Try to open to our host lp */
+	if (viopath_hostLp == HvLpIndexInvalid)
+		vio_set_hostlp();
+
+	if (viopath_hostLp == HvLpIndexInvalid) {
+		printk(VIOD_KERN_WARNING "invalid hosting partition\n");
+		return -EIO;
+	}
+
+	printk(VIOD_KERN_INFO "vers " VIOD_VERS ", hosting partition %d\n",
+			viopath_hostLp);
+
+        /* register the block device */
+	if (register_blkdev(VIODASD_MAJOR, VIOD_GENHD_NAME)) {
+		printk(VIOD_KERN_WARNING
+				"Unable to get major number %d for %s\n",
+				VIODASD_MAJOR, VIOD_GENHD_NAME);
+		return -EIO;
+	}
+	/* Actually open the path to the hosting partition */
+	if (viopath_open(viopath_hostLp, viomajorsubtype_blockio,
+				VIOMAXREQ + 2)) {
+		printk(VIOD_KERN_WARNING
+		       "error opening path to host partition %d\n",
+		       viopath_hostLp);
+		unregister_blkdev(VIODASD_MAJOR, VIOD_GENHD_NAME);
+		return -EIO;
+	}
+
+	/* Initialize our request handler */
+	vio_setHandler(viomajorsubtype_blockio, vioHandleBlockEvent);
+
+	viodasd_max_disk = MAX_DISKNO - 1;
+	for (i = 0; (i <= viodasd_max_disk) && (i < MAX_DISKNO); i++) {
+		/*
+		 * Note that probe_disk has side effects:
+		 *  a) it updates the size of the disk
+		 *  b) it updates viodasd_max_disk
+		 *  c) it registers the disk if it has not done so already
+		 */
+		probe_disk(&viodasd_devices[i]);
+	}
+
+	if (viodasd_max_disk > (MAX_DISKNO - 1))
+		printk(VIOD_KERN_INFO
+			"Only examining the first %d of %d disks connected\n",
+			MAX_DISKNO, viodasd_max_disk + 1);
+
+	return 0;
+}
+module_init(viodasd_init);
+
+void viodasd_exit(void)
+{
+	int i;
+	struct viodasd_device *d;
+
+        for (i = 0; i < MAX_DISKNO; i++) {
+		d = &viodasd_devices[i];
+		if (d->disk) {
+			if (d->disk->queue)
+				blk_cleanup_queue(d->disk->queue);
+			del_gendisk(d->disk);
+			put_disk(d->disk);
+			d->disk = NULL;
+		}
+	}
+	vio_clearHandler(viomajorsubtype_blockio);
+	unregister_blkdev(VIODASD_MAJOR, VIOD_GENHD_NAME);
+	viopath_close(viopath_hostLp, viomajorsubtype_blockio, VIOMAXREQ + 2);
+}
+
+module_exit(viodasd_exit);
diff -ruN 2.6.3-mm4/include/asm-ppc64/dma-mapping.h 2.6.3-mm4-viodasd/include/asm-ppc64/dma-mapping.h
--- 2.6.3-mm4/include/asm-ppc64/dma-mapping.h	2002-12-24 17:12:29.000000000 +1100
+++ 2.6.3-mm4-viodasd/include/asm-ppc64/dma-mapping.h	2004-02-24 16:04:33.000000000 +1100
@@ -1 +1,75 @@
-#include <asm-generic/dma-mapping.h>
+/* Copyright (C) 2004 IBM
+ *
+ * Implements the generic device dma API for ppc64. Handles
+ * the pci and vio busses
+ */
+
+#ifndef _ASM_DMA_MAPPING_H
+#define _ASM_DMA_MAPPING_H
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/cache.h>
+/* need struct page definitions */
+#include <linux/mm.h>
+#include <asm/scatterlist.h>
+#include <asm/bug.h>
+
+extern int dma_supported(struct device *dev, u64 mask);
+extern int dma_set_mask(struct device *dev, u64 dma_mask);
+extern void *dma_alloc_coherent(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, int flag);
+extern void dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
+		dma_addr_t dma_handle);
+extern dma_addr_t dma_map_single(struct device *dev, void *cpu_addr,
+		size_t size, enum dma_data_direction direction);
+extern void dma_unmap_single(struct device *dev, dma_addr_t dma_addr,
+		size_t size, enum dma_data_direction direction);
+extern dma_addr_t dma_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size,
+		enum dma_data_direction direction);
+extern void dma_unmap_page(struct device *dev, dma_addr_t dma_address,
+		size_t size, enum dma_data_direction direction);
+extern int dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+		enum dma_data_direction direction);
+extern void dma_unmap_sg(struct device *dev, struct scatterlist *sg,
+		int nhwentries, enum dma_data_direction direction);
+extern void dma_sync_single(struct device *dev, dma_addr_t dma_handle,
+		size_t size, enum dma_data_direction direction);
+extern void dma_sync_sg(struct device *dev, struct scatterlist *sg, int nelems,
+		enum dma_data_direction direction);
+
+/* Now for the API extensions over the pci_ one */
+
+#define dma_alloc_noncoherent(d, s, h, f) dma_alloc_coherent(d, s, h, f)
+#define dma_free_noncoherent(d, s, v, h) dma_free_coherent(d, s, v, h)
+#define dma_is_consistent(d)	(1)
+
+static inline int
+dma_get_cache_alignment(void)
+{
+	/* no easy way to get cache size on all processors, so return
+	 * the maximum possible, to be safe */
+	return (1 << L1_CACHE_SHIFT_MAX);
+}
+
+static inline void
+dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
+		      unsigned long offset, size_t size,
+		      enum dma_data_direction direction)
+{
+	/* just sync everything, that's all the pci API can do */
+	dma_sync_single(dev, dma_handle, offset+size, direction);
+}
+
+static inline void
+dma_cache_sync(void *vaddr, size_t size,
+	       enum dma_data_direction direction)
+{
+	/* could define this in terms of the dma_cache ... operations,
+	 * but if you get this on a platform, you should convert the platform
+	 * to using the generic device DMA API */
+	BUG();
+}
+
+#endif	/* _ASM_DMA_MAPPING_H */
diff -ruN 2.6.3-mm4/include/asm-ppc64/vio.h 2.6.3-mm4-viodasd/include/asm-ppc64/vio.h
--- 2.6.3-mm4/include/asm-ppc64/vio.h	2004-02-18 16:41:01.000000000 +1100
+++ 2.6.3-mm4-viodasd/include/asm-ppc64/vio.h	2004-02-26 17:06:08.000000000 +1100
@@ -17,6 +17,7 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/device.h>
+#include <linux/pci.h>
 #include <asm/hvcall.h>
 #include <asm/prom.h>
 #include <asm/scatterlist.h>
@@ -65,6 +66,33 @@
 void vio_free_consistent(struct vio_dev *dev, size_t size, void *vaddr, 
 			 dma_addr_t dma_handle);
 
+static inline int vio_dma_supported(struct vio_dev *hwdev, u64 mask)
+{
+	return 1;
+}
+
+#define vio_map_page(dev, page, off, size, dir) \
+		vio_map_single(dev, (page_address(page) + (off)), size, dir)
+#define vio_unmap_page(dev,addr,sz,dir) vio_unmap_single(dev,addr,sz,dir)
+
+
+static inline void vio_dma_sync_single(struct vio_dev *hwdev,
+				       dma_addr_t dma_handle,
+				       size_t size, int direction)
+{
+	BUG_ON(direction == PCI_DMA_NONE);
+	/* nothing to do */
+}
+
+static inline void vio_dma_sync_sg(struct vio_dev *hwdev,
+				   struct scatterlist *sg,
+				   int nelems, int direction)
+{
+	BUG_ON(direction == PCI_DMA_NONE);
+	/* nothing to do */
+}
+static inline int vio_set_dma_mask(struct vio_dev *dev, u64 mask) { return -EIO; }
+
 extern struct bus_type vio_bus_type;
 
 struct vio_device_id {
diff -ruN 2.6.3-mm4/include/linux/major.h 2.6.3-mm4-viodasd/include/linux/major.h
--- 2.6.3-mm4/include/linux/major.h	2003-09-29 11:40:33.000000000 +1000
+++ 2.6.3-mm4-viodasd/include/linux/major.h	2004-02-26 16:36:24.000000000 +1100
@@ -126,6 +126,9 @@
 #define COMPAQ_CISS_MAJOR6      110
 #define COMPAQ_CISS_MAJOR7      111
 
+#define VIODASD_MAJOR		112
+#define VIOCD_MAJOR		113
+
 #define ATARAID_MAJOR		114
 
 #define SCSI_DISK8_MAJOR	128

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Jeff Garzik]

Mozilla is being annoying and not quoting your patch, so bear with me. 
comments:

1) return an error instead of BUG() (and no error return) in the generic 
DMA routines that can return a meaningful value

2) num_req_outstanding accessed without lock in do_viodasd_request 
(driver's request_fn).  all other accesses are inside spinlock.

3) is viodasd_revalidate really needed?

4) why do you call blkdev_dequeue_request() in do_viodasd_request() 
rather than viodasd_end_request() ?  Or just use end_request() ?

5) is it really OK to call viodasd_open() and viodasd_release() multiple 
times?  These functions do not look guarded against multiple openers.

6) access to a struct viodasd_device in viodasd_ioctl() is completely 
unprotected.  OK, or asking for trouble?

7) use sg_dma_address() and sg_dma_len() accessors instead of directly 
referencing the struct scatterlist elements.  (several places)

8) send_request() probably wants a common error-exit+cleanup path, 
instead of duplicating the same cleanup code multiple times

9) viodasd_restart_all_queues_starting_from -- are you sure you don't 
want to make the function name even longer?  Maybe try for a new record?

10) in viodasd_handleReadWrite() you obtain the queue lock via 
spin_lock(), but the rest of the kernel uses spin_lock_irq() or 
spin_lock_irqsave()

11) viodasd_handleReadWrite, vioHandleBlockEvent -- follow the style in 
the rest of the driver, and eliminate the StudlyCaps.

12) don't you need to set blk_queue_max_phys_segments() too?

13) in viodasd_init(), don't you need to undo the effects of 
vio_set_hostlp() if an error occurs?

14) why does vio_set_dma_mask() always return an error?  That seems 
rather useless and unwanted.

Hey, I just merged iSeries veth, so I had to give you some more work... ;-)

	Jeff


P.S.  I so wish that people had named the API function 
dma_alloc_incoherent() rather than dma_alloc_noncoherent :)

Re: [PATCH] iSeries virtual disk

[Jens Axboe]

On Thu, Feb 26 2004, Jeff Garzik wrote:
> 4) why do you call blkdev_dequeue_request() in do_viodasd_request() 
> rather than viodasd_end_request() ?  Or just use end_request() ?

It makes the queueing simpler (you could potentially leave the _last_
request on the queue, but it's probably not worth the hassle).

They should not call elv_next_request() only to bail if
num_req_outstanding is too big, since it has side effects (moving
request from io scheduler core to dispatch, which makes it ineligible
for merging, sorting, etc).

	do {
		if (too_many_queued)
			break;

		rq = elv_next_request(q);
		if (!rq)
			break;

		...
	}

> 12) don't you need to set blk_queue_max_phys_segments() too?

Yep

> P.S.  I so wish that people had named the API function 
> dma_alloc_incoherent() rather than dma_alloc_noncoherent :)

;-)

-- 
Jens Axboe

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Jeff,

Thanks for your comments, sorry I missed you first time around.

Firstly, even considering your comments below, would you object to
the driver being included now and being fixed up later?

On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
>
> 1) return an error instead of BUG() (and no error return) in the generic 
> DMA routines that can return a meaningful value

These routines are a work in progress and arch specific. The current
version is enough to get all the current supported drivers working.
There will be updates in due course.

I will address the rest later - I am late for dinner/LUG meeting.

> Hey, I just merged iSeries veth, so I had to give you some more work... ;-)

Thanks, its just what I needed :-(

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Jeff Garzik]

Stephen Rothwell wrote:
> Hi Jeff,
> 
> Thanks for your comments, sorry I missed you first time around.
> 
> Firstly, even considering your comments below, would you object to
> the driver being included now and being fixed up later?

The locking and request-queue stuff needs to be fixed up first.

I don't mind if you put off the cosmetic/very-minor stuff.

	Jeff

Re: [PATCH] iSeries virtual disk

[Christoph Hellwig]

On Thu, Feb 26, 2004 at 05:23:25PM +1100, Stephen Rothwell wrote:
> Unfortunately, things have moved on in the last couple of weeks and to
> fix everyone;s abjections, I need to include in this patch a ppc64 specific
> version of the dma_mapping routines.  They are pretty straight forward.

While thes dma mapping changes look good they really don't belong into a
patch for a new driver. Can you split them out into a separate patch? 

> Disks are now called /dev/iseries/vd<x><n> (without devfs) or
> /dev/viod/disc<n>/part<n> (with devfs).  Up to 7 partitions are supported
> on each of up to 32 disks.

Hmm, different names for devfs vs non-devfs seems silly.  But I think we
can't easily get rid of the disc<foo>/part sillyness.  Can you at least
make both use the same prefix?


>  config VIOCD
>  	tristate "iSeries Virtual I/O CD support"
>  	help

What happened to this driver, btw?

> +void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
> +		enum dma_data_direction direction)
> +{
> +	if (dev->bus == &pci_bus_type)
> +		pci_unmap_single(to_pci_dev(dev), dma_addr, size, (int)direction);
>
> 
> +#ifdef CONFIG_PPC_PSERIES
> +	else if (dev->bus == &vio_bus_type)
> +		vio_unmap_single(to_vio_dev(dev), dma_addr, size, (int)direction);
> +#endif

So vio on iseries claims to be pci?  That's a little silly if you ask
me..

> +#include <linux/major.h>
> +#include <linux/fs.h>
> +#include <asm/uaccess.h>

please put all asm/* headers after linux/*

> +#include <linux/hdreg.h>

probably not needed anymore without ide emulation.

> +#include <linux/seq_file.h>

you don't use this anymore, do you?

> +static int		viodasd_max_disk;

the code surrounding this doesn't look right yet.  You first initialize
it to the maximum value and then reset it in a magic even handler?
I think that logic needs some clarification.

> +#define DEVICE_NO(cell)	((struct viodasd_device *)(cell) - &viodasd_devices[0])

Aniother idea:  It might be better to just put a disk_no member into
struct viodasd_device - then you can allocate the struct viodasd_device
dynamically one by one and easily support lots of disks without overhead
for lowend configurations (remember we have a 32bit dev_t now)

> +extern struct device *iSeries_vio_dev;

shouldn't this be in some header?

> +static void viodasd_init_disk(struct viodasd_device *d);

What's preventing this one to be implemented above it's usage? or even
better merging it into it's only caller.

> +static void viodasd_end_request(struct request *req, int uptodate,
> +		int num_sectors)
> +{
> +	if (end_that_request_first(req, uptodate, num_sectors))
> +		return;
> +        add_disk_randomness(req->rq_disk);
> +	end_that_request_last(req);
> +}

indentation looks messed up here.

> +static void viodasd_init_disk(struct viodasd_device *d)
> +{
> +	struct gendisk *g;
> +	struct request_queue *q;
> +	int disk_no = DEVICE_NO(d);
> +
> +	if (d->disk)
> +		return;

as there's only one caller it shouldn't happen, should it?

> +	g->private_data = (void *)d;

no need to cast to void * here - this is implicit in C

> +        for (i = 0; i < MAX_DISKNO; i++) {
> +		d = &viodasd_devices[i];
> +		if (d->disk) {

How can d->disk ever be NULLL here?

> 
> +			if (d->disk->queue)
> +				blk_cleanup_queue(d->disk->queue);
> +			del_gendisk(d->disk);

the queue cleanups needs to be after put_gendisk.  Also where did you
see d->disk->queue as NULL?

Re: [PATCH] iSeries virtual disk

[Linus Torvalds]

On Thu, 26 Feb 2004, Stephen Rothwell wrote:
> 
> Unfortunately, things have moved on in the last couple of weeks and to
> fix everyone;s abjections, I need to include in this patch a ppc64 specific
> version of the dma_mapping routines.  They are pretty straight forward.

I'd _really_ like to see those as a separate patch and separately ack'ed 
as having been tested on the different DMA architectures.

		Linus

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Jeff,

On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
>
> 1) return an error instead of BUG() (and no error return) in the generic 
> DMA routines that can return a meaningful value

I have removed the DMA changes from this patch.

> 2) num_req_outstanding accessed without lock in do_viodasd_request 
> (driver's request_fn).  all other accesses are inside spinlock.

This is actually OK because:
	1) if we see a value too large, when it get decremented by
	handle_read_write, all the queue requst functions will get rerun.
	2) in send_request, if we get an error and decrement the count
	to zero, then the count could have been at most 1 (sonce sends
	are serialised) so in the request funtion, we would not have
	stopped processing requests.

> 3) is viodasd_revalidate really needed?

Not any more - its gone.

> 4) why do you call blkdev_dequeue_request() in do_viodasd_request() 
> rather than viodasd_end_request() ?  Or just use end_request() ?

See Jens' response.

> 5) is it really OK to call viodasd_open() and viodasd_release() multiple 
> times?  These functions do not look guarded against multiple openers.

It is OK.

> 6) access to a struct viodasd_device in viodasd_ioctl() is completely 
> unprotected.  OK, or asking for trouble?

Its OK, beacuse the viodasd_request is completely static data after the
disk is probed - and before that you cannot get the ioctl ...

> 7) use sg_dma_address() and sg_dma_len() accessors instead of directly 
> referencing the struct scatterlist elements.  (several places)

done.

> 8) send_request() probably wants a common error-exit+cleanup path, 
> instead of duplicating the same cleanup code multiple times

done.

> 9) viodasd_restart_all_queues_starting_from -- are you sure you don't 
> want to make the function name even longer?  Maybe try for a new record?

:-)

> 10) in viodasd_handleReadWrite() you obtain the queue lock via 
> spin_lock(), but the rest of the kernel uses spin_lock_irq() or 
> spin_lock_irqsave()

fixed.

> 11) viodasd_handleReadWrite, vioHandleBlockEvent -- follow the style in 
> the rest of the driver, and eliminate the StudlyCaps.

done.

> 12) don't you need to set blk_queue_max_phys_segments() too?

done.

> 13) in viodasd_init(), don't you need to undo the effects of 
> vio_set_hostlp() if an error occurs?

No and, in fact, we can't.

> 14) why does vio_set_dma_mask() always return an error?  That seems 
> rather useless and unwanted.

dma stuff removed ...

New patch following soon.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Jens,

On Thu, 26 Feb 2004 08:40:43 +0100 Jens Axboe <axboe@suse.de> wrote:
>
> On Thu, Feb 26 2004, Jeff Garzik wrote:
> > 4) why do you call blkdev_dequeue_request() in do_viodasd_request() 
> > rather than viodasd_end_request() ?  Or just use end_request() ?
> 
> It makes the queueing simpler (you could potentially leave the _last_
> request on the queue, but it's probably not worth the hassle).

Thanks.

> They should not call elv_next_request() only to bail if
> num_req_outstanding is too big, since it has side effects (moving
> request from io scheduler core to dispatch, which makes it ineligible
> for merging, sorting, etc).
> 
> 	do {
> 		if (too_many_queued)
> 			break;
> 
> 		rq = elv_next_request(q);
> 		if (!rq)
> 			break;
> 
> 		...
> 	}

OK, fixed.

> > 12) don't you need to set blk_queue_max_phys_segments() too?
> 
> Yep

Done.

New patch following soon ...
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Linus,

On Thu, 26 Feb 2004 09:35:18 -0800 (PST) Linus Torvalds <torvalds@osdl.org> wrote:
>
> On Thu, 26 Feb 2004, Stephen Rothwell wrote:
> > 
> > Unfortunately, things have moved on in the last couple of weeks and to
> > fix everyone;s abjections, I need to include in this patch a ppc64 specific
> > version of the dma_mapping routines.  They are pretty straight forward.
> 
> I'd _really_ like to see those as a separate patch and separately ack'ed 
> as having been tested on the different DMA architectures.

Fine.  I don't know what I was thinking at the time, I really didn't
need that for the viodasd patch anyway :-(

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Christoph,

On Thu, 26 Feb 2004 10:51:56 +0100 Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Feb 26, 2004 at 05:23:25PM +1100, Stephen Rothwell wrote:
> > Unfortunately, things have moved on in the last couple of weeks and to
> > fix everyone;s abjections, I need to include in this patch a ppc64 specific
> > version of the dma_mapping routines.  They are pretty straight forward.
> 
> While thes dma mapping changes look good they really don't belong into a
> patch for a new driver. Can you split them out into a separate patch? 

Yeah, I don't know what I was thinking :-(

> > Disks are now called /dev/iseries/vd<x><n> (without devfs) or
> > /dev/viod/disc<n>/part<n> (with devfs).  Up to 7 partitions are supported
> > on each of up to 32 disks.
> 
> Hmm, different names for devfs vs non-devfs seems silly.  But I think we
> can't easily get rid of the disc<foo>/part sillyness.  Can you at least
> make both use the same prefix?

I was trying to maintain compatability with a previously published
(out of tree) version of the driver, but I think I have broken that
anyway, so I will change ths as well.

> >  config VIOCD
> >  	tristate "iSeries Virtual I/O CD support"
> >  	help
> 
> What happened to this driver, btw?

Coming soon to a mailing list near you :-) (I decided to separate them)

> So vio on iseries claims to be pci?  That's a little silly if you ask
> me..

It is an expedience and changes are already in train to change this.

> > +#include <linux/major.h>
> > +#include <linux/fs.h>
> > +#include <asm/uaccess.h>
> 
> please put all asm/* headers after linux/*

done.

> > +#include <linux/hdreg.h>
> 
> probably not needed anymore without ide emulation.

it is, actually. for HDIO_GETGEO ioctl stuff.

> > +#include <linux/seq_file.h>
> 
> you don't use this anymore, do you?

Thanks, missed removing it with the rest of the proc stuff.

> > +static int		viodasd_max_disk;
> 
> the code surrounding this doesn't look right yet.  You first initialize
> it to the maximum value and then reset it in a magic even handler?
> I think that logic needs some clarification.

The "magic event handler" is synchronous with the probe_disk routine.  I
agree it is a bit confusing, but, at least I have the comment there about
the side effects of the probe_disk routine.  Changed slightly.

> > +#define DEVICE_NO(cell)	((struct viodasd_device *)(cell) - &viodasd_devices[0])
> 
> Aniother idea:  It might be better to just put a disk_no member into
> struct viodasd_device - then you can allocate the struct viodasd_device
> dynamically one by one and easily support lots of disks without overhead
> for lowend configurations (remember we have a 32bit dev_t now)

Can I leave this for now?

> > +extern struct device *iSeries_vio_dev;
> 
> shouldn't this be in some header?

Yes, it should, but can I leave this to the future as well, please?

> > +static void viodasd_init_disk(struct viodasd_device *d);
> 
> What's preventing this one to be implemented above it's usage? or even
> better merging it into it's only caller.

Nothing, now.

> > +static void viodasd_end_request(struct request *req, int uptodate,
> > +		int num_sectors)
> > +{
> > +	if (end_that_request_first(req, uptodate, num_sectors))
> > +		return;
> > +        add_disk_randomness(req->rq_disk);
> > +	end_that_request_last(req);
> > +}
> 
> indentation looks messed up here.

Yeah, spaces v. tabs.

> > +static void viodasd_init_disk(struct viodasd_device *d)
> > +{
> > +	struct gendisk *g;
> > +	struct request_queue *q;
> > +	int disk_no = DEVICE_NO(d);
> > +
> > +	if (d->disk)
> > +		return;
> 
> as there's only one caller it shouldn't happen, should it?

Correct.

> > +	g->private_data = (void *)d;
> 
> no need to cast to void * here - this is implicit in C

OK.

> > +        for (i = 0; i < MAX_DISKNO; i++) {
> > +		d = &viodasd_devices[i];
> > +		if (d->disk) {
> 
> How can d->disk ever be NULLL here?

Because it was never initialised ... we only create the gendisks for
disks that exist.

> > +			if (d->disk->queue)
> > +				blk_cleanup_queue(d->disk->queue);
> > +			del_gendisk(d->disk);
> 
> the queue cleanups needs to be after put_gendisk.  Also where did you
> see d->disk->queue as NULL?

OK, fixed.

New patch following soon ...
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Jeff Garzik]

Stephen Rothwell wrote:
> On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
>>2) num_req_outstanding accessed without lock in do_viodasd_request 
>>(driver's request_fn).  all other accesses are inside spinlock.
> 
> 
> This is actually OK because:
> 	1) if we see a value too large, when it get decremented by
> 	handle_read_write, all the queue requst functions will get rerun.
> 	2) in send_request, if we get an error and decrement the count
> 	to zero, then the count could have been at most 1 (sonce sends
> 	are serialised) so in the request funtion, we would not have
> 	stopped processing requests.

That doesn't solve the race though...  IMO protect it with the spinlock 
and be done with it...


>>5) is it really OK to call viodasd_open() and viodasd_release() multiple 
>>times?  These functions do not look guarded against multiple openers.
> 
> 
> It is OK.

I need more explanation than that.

Multiple openers _are_ supported at the block layer, and this driver 
does not guard against multiple openers.

Thanks,

	Jeff

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Jeff,

Thanks for the feedback, comments follow:

On Thu, 26 Feb 2004 20:50:17 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
>
> Stephen Rothwell wrote:
> > On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
> >>2) num_req_outstanding accessed without lock in do_viodasd_request 
> >>(driver's request_fn).  all other accesses are inside spinlock.
> > 
> > 
> > This is actually OK because:
> > 	1) if we see a value too large, when it get decremented by
> > 	handle_read_write, all the queue requst functions will get rerun.
> > 	2) in send_request, if we get an error and decrement the count
> > 	to zero, then the count could have been at most 1 (sonce sends
> > 	are serialised) so in the request funtion, we would not have
> > 	stopped processing requests.
> 
> That doesn't solve the race though...  IMO protect it with the spinlock 
> and be done with it...

As you said elsewhere, we can quibble after it is merged.

> >>5) is it really OK to call viodasd_open() and viodasd_release() multiple 
> >>times?  These functions do not look guarded against multiple openers.
> > 
> > It is OK.
> 
> I need more explanation than that.
> 
> Multiple openers _are_ supported at the block layer, and this driver 
> does not guard against multiple openers.

Sorry - the underlying Hypervisor calls supprto multiple open.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Jeff Garzik]

Cool, OK with me at this point.

	Jeff

Re: [PATCH] iSeries virtual disk

[Christoph Hellwig]

> > it to the maximum value and then reset it in a magic even handler?
> > I think that logic needs some clarification.
> 
> The "magic event handler" is synchronous with the probe_disk routine.  I
> agree it is a bit confusing, but, at least I have the comment there about
> the side effects of the probe_disk routine.  Changed slightly.

The code that is in Linus' tree is still b0rked:

 - you set viodasd_max_disk in viodasd_open which looks completely bogus:
    o the value is never used after module_init, and as long as module_init
      and blkdev ->open under BKL they are serialized.
    o even if they weren't you wouldn't ever get an open call for a device
      > viodasd_max_disk
    o that means if you actually got there it would either be the same or
      decreased
    o if it was decreased in parallel to module_init your loop in
      module_init would be totally screwed.
  - now to that loop in module_init:
    o they only thing that it actually archives is that it breaks out of
      the loop if a probe_disk fails - but you could archive that much
      more easier by just returning an error from the probe_disk and
      use a break out of the loop.  The >= MAX_DISKNO check could then
      easily happen on the i used as loop counter.

> > for lowend configurations (remember we have a 32bit dev_t now)
> 
> Can I leave this for now?

It's really awkwards.  And IBM will most likely want lots of disks soon
anyway :)

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

Hi Christoph,

On Fri, 27 Feb 2004 11:32:02 +0000 Christoph Hellwig <hch@infradead.org> wrote:
>
> > > it to the maximum value and then reset it in a magic even handler?
> > > I think that logic needs some clarification.
> > 
> > The "magic event handler" is synchronous with the probe_disk routine.  I
> > agree it is a bit confusing, but, at least I have the comment there about
> > the side effects of the probe_disk routine.  Changed slightly.
> 
> The code that is in Linus' tree is still b0rked:
> 
>  - you set viodasd_max_disk in viodasd_open which looks completely bogus:
>     o the value is never used after module_init, and as long as module_init
>       and blkdev ->open under BKL they are serialized.
>     o even if they weren't you wouldn't ever get an open call for a device
>       > viodasd_max_disk
>     o that means if you actually got there it would either be the same or
>       decreased
>     o if it was decreased in parallel to module_init your loop in
>       module_init would be totally screwed.
>   - now to that loop in module_init:
>     o they only thing that it actually archives is that it breaks out of
>       the loop if a probe_disk fails - but you could archive that much
>       more easier by just returning an error from the probe_disk and
>       use a break out of the loop.  The >= MAX_DISKNO check could then
>       easily happen on the i used as loop counter.

In theory you can hot add virtual disks to an iSeries partition (up to 64
disks per partition).  We used to support that by registering all 32 disks
with Linux and then probing the hypervisor when any disk was opened at
which point the hypervisor lets us know that the value of viod_max_disk
should be increased.  I took that code out in the name of simplicity and
in the hope of getting the driver accepted.  (In fact it was your comments
that made me do it.)  I will investigate your suggestion of using
blk_register_region for the disks that don't exist yet over the next while
at which point (assuming I can figure out how to do it) the value of
viodasd_max_disk may again rise over time.

viodasd_max_disk will never decrease, because you cannot remove a virtual
disk from a partition while it is running.

It doesn't break out of the loop when probe_disk fails because it is
possible to have gaps in the list if disks (e.g. I have have disks 1 2 4 7
and viodasd_max_disk will be 7 but probe_disk will "fail" for the other
disks).

In fact, the admin could add a disk while that loop is running so that
viodasd_max_disk could change even then. They would have to be quick
though :-)

Any clearer?

> 
> > > for lowend configurations (remember we have a 32bit dev_t now)
> > 
> > Can I leave this for now?
> 
> It's really awkwards.  And IBM will most likely want lots of disks soon
> anyway :)

The viodasd driver (as used in distros for a couple of years) has never
supported more than 32 disks ...  If you want lots of disks on an iSeries
partition you would used real direct attached SCSI controllers with real
disks.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Christoph Hellwig]

> In theory you can hot add virtual disks to an iSeries partition (up to 64
> disks per partition).  We used to support that by registering all 32 disks
> with Linux and then probing the hypervisor when any disk was opened at
> which point the hypervisor lets us know that the value of viod_max_disk
> should be increased.  I took that code out in the name of simplicity and
> in the hope of getting the driver accepted.  (In fact it was your comments
> that made me do it.)

Sure - above comments still stand in a hot-add enviroment.  ->open can't
happen before gendisk is registered, hot-add or not.  And the module_init
loop is bogus hot-add or not. 

> I will investigate your suggestion of using
> blk_register_region for the disks that don't exist yet over the next while
> at which point (assuming I can figure out how to do it) the value of
> viodasd_max_disk may again rise over time.

But this doesn't make the current code any better.  With hot-adding disks
you would actually need a variable like viodasd_max_disk that replaces the
current i loop iteractor in module_init with a file-wide variable, but that's
it basically.

> It doesn't break out of the loop when probe_disk fails because it is
> possible to have gaps in the list if disks (e.g. I have have disks 1 2 4 7
> and viodasd_max_disk will be 7 but probe_disk will "fail" for the other
> disks).

That's not how I read your code.  But to actually understand what it's
doing we need to know what open_data.max_disk actually is.

is this the maximum number of disks currently configured (if so the
interface would be absolutely braindead, but the current code would
match your above description although beein rather confusing).

Or does it return the currently opened disks index?

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

On Fri, 27 Feb 2004 12:13:00 +0000 Christoph Hellwig <hch@infradead.org> wrote:
>
> That's not how I read your code.  But to actually understand what it's
> doing we need to know what open_data.max_disk actually is.
> 
> is this the maximum number of disks currently configured (if so the
> interface would be absolutely braindead, but the current code would
> match your above description although beein rather confusing).

It is the highest index of all the configured disks. Did you read my
previous email?  If disks 1 2 4 and 7 are configured, the
open_data.max_disk will be 7. If the admin thena adds disk 23, the next
time we probe (by calling the HV open interface) the returned max_disk
will be 23.

I do not apologise for braindead interfaces in code I didn't write ...
In this case I just have to use it.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] iSeries virtual disk

[Christoph Hellwig]

On Sat, Feb 28, 2004 at 12:26:14AM +1100, Stephen Rothwell wrote:
> It is the highest index of all the configured disks. Did you read my
> previous email?  If disks 1 2 4 and 7 are configured, the
> open_data.max_disk will be 7. If the admin thena adds disk 23, the next
> time we probe (by calling the HV open interface) the returned max_disk
> will be 23.
> 
> I do not apologise for braindead interfaces in code I didn't write ...
> In this case I just have to use it.

You don't need to apologize.  It's braindead and I don't really care
who messed it up.  But again even with this the logic isn't good.

Here's a totally untested patch to fix it up:

--- 1.1/drivers/block/viodasd.c	Fri Feb 27 06:25:15 2004
+++ edited/drivers/block/viodasd.c	Fri Feb 27 15:37:06 2004
@@ -70,7 +70,7 @@
 	MAX_DISK_NAME = sizeof(((struct gendisk *)0)->disk_name)
 };
 
-static int		viodasd_max_disk;
+static int		viodasd_max_disk = 1; /* probe at least one disk */
 static spinlock_t	viodasd_spinlock = SPIN_LOCK_UNLOCKED;
 
 #define VIOMAXREQ		16
@@ -209,7 +209,6 @@
 				(int)we.rc, we.sub_result, err->msg);
 		return -EIO;
 	}
-	viodasd_max_disk = we.max_disk;
 
 	return 0;
 }
@@ -451,10 +450,9 @@
 }
 
 /*
- * Probe a single disk and fill in the viodasd_device structure
- * for it.
+ * Probe a single disk and fill in the viodasd_device structure for it.
  */
-static void probe_disk(struct viodasd_device *d)
+static void __init probe_disk(struct viodasd_device *d)
 {
 	HvLpEvent_Rc hvrc;
 	struct viodasd_waitevent we;
@@ -483,6 +481,14 @@
 
 	if (we.rc != 0)
 		return;
+
+	if (we.max_disk > (MAX_DISKNO - 1)) {
+		printk(VIOD_KERN_INFO
+			"Only examining the first %d of %d disks connected\n",
+			MAX_DISKNO, we.max_disk + 1);
+		we.max_disk = MAX_DISKNO - 1;
+	}
+
 	viodasd_max_disk = we.max_disk;
 
 	/* Send the close event to OS/400.  We DON'T expect a response */
@@ -744,27 +750,13 @@
 	/* Initialize our request handler */
 	vio_setHandler(viomajorsubtype_blockio, handle_block_event);
 
-	viodasd_max_disk = MAX_DISKNO - 1;
-	for (i = 0; (i <= viodasd_max_disk) && (i < MAX_DISKNO); i++) {
-		/*
-		 * Note that probe_disk has side effects:
-		 *  a) it updates the size of the disk
-		 *  b) it updates viodasd_max_disk
-		 *  c) it registers the disk if it has not done so already
-		 */
+	/* probe_disk updates viodasd_max_disk */
+	for (i = 0; i <= viodasd_max_disk; i++)
 		probe_disk(&viodasd_devices[i]);
-	}
-
-	if (viodasd_max_disk > (MAX_DISKNO - 1))
-		printk(VIOD_KERN_INFO
-			"Only examining the first %d of %d disks connected\n",
-			MAX_DISKNO, viodasd_max_disk + 1);
-
 	return 0;
 }
-module_init(viodasd_init);
 
-void viodasd_exit(void)
+static void __exit viodasd_exit(void)
 {
 	int i;
 	struct viodasd_device *d;
@@ -783,4 +775,5 @@
 	viopath_close(viopath_hostLp, viomajorsubtype_blockio, VIOMAXREQ + 2);
 }
 
+module_init(viodasd_init);
 module_exit(viodasd_exit);

Re: [PATCH] iSeries virtual disk

[Christoph Hellwig]

On Fri, Feb 27, 2004 at 02:37:17PM +0100, Christoph Hellwig wrote:
> Here's a totally untested patch to fix it up:

And a better one that makes viodasd_max_disks properly start at 0.


--- 1.1/drivers/block/viodasd.c	Fri Feb 27 06:25:15 2004
+++ edited/drivers/block/viodasd.c	Fri Feb 27 15:44:03 2004
@@ -209,7 +209,6 @@
 				(int)we.rc, we.sub_result, err->msg);
 		return -EIO;
 	}
-	viodasd_max_disk = we.max_disk;
 
 	return 0;
 }
@@ -451,10 +450,9 @@
 }
 
 /*
- * Probe a single disk and fill in the viodasd_device structure
- * for it.
+ * Probe a single disk and fill in the viodasd_device structure for it.
  */
-static void probe_disk(struct viodasd_device *d)
+static void __init probe_disk(struct viodasd_device *d)
 {
 	HvLpEvent_Rc hvrc;
 	struct viodasd_waitevent we;
@@ -483,7 +481,8 @@
 
 	if (we.rc != 0)
 		return;
-	viodasd_max_disk = we.max_disk;
+
+	viodasd_max_disk = min(we.max_disk - 1, MAX_DISKNO);
 
 	/* Send the close event to OS/400.  We DON'T expect a response */
 	hvrc = HvCallEvent_signalLpEventFast(viopath_hostLp,
@@ -744,27 +743,13 @@
 	/* Initialize our request handler */
 	vio_setHandler(viomajorsubtype_blockio, handle_block_event);
 
-	viodasd_max_disk = MAX_DISKNO - 1;
-	for (i = 0; (i <= viodasd_max_disk) && (i < MAX_DISKNO); i++) {
-		/*
-		 * Note that probe_disk has side effects:
-		 *  a) it updates the size of the disk
-		 *  b) it updates viodasd_max_disk
-		 *  c) it registers the disk if it has not done so already
-		 */
+	/* probe_disk updates viodasd_max_disk */
+	for (i = 0; i < viodasd_max_disk; i++)
 		probe_disk(&viodasd_devices[i]);
-	}
-
-	if (viodasd_max_disk > (MAX_DISKNO - 1))
-		printk(VIOD_KERN_INFO
-			"Only examining the first %d of %d disks connected\n",
-			MAX_DISKNO, viodasd_max_disk + 1);
-
 	return 0;
 }
-module_init(viodasd_init);
 
-void viodasd_exit(void)
+static void __exit viodasd_exit(void)
 {
 	int i;
 	struct viodasd_device *d;
@@ -783,4 +768,5 @@
 	viopath_close(viopath_hostLp, viomajorsubtype_blockio, VIOMAXREQ + 2);
 }
 
+module_init(viodasd_init);
 module_exit(viodasd_exit);

Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

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

On Fri, 27 Feb 2004 13:44:12 +0000 Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Feb 27, 2004 at 02:37:17PM +0100, Christoph Hellwig wrote:
> > Here's a totally untested patch to fix it up:
> 
> And a better one that makes viodasd_max_disks properly start at 0.

The problem with this approach is that if disk 0 is not configured, the
underlying Hypervisor call will fail and we will never raise the value of
viodasd_max_disk and so we will never probe any of the other disks.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]