2008-10-03 00:09:38

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: [RFC/PATCH] Block device for the ISS simulator

The ISS simulator is a simple powerpc simulator used among other things
for hardware bringup. It implements a simple memory mapped block device
interface.

This is a simple block driver that attaches to it. Note that the choice
of a major device number is fishy, though because it's a simulator and
not real hardware, it's not necessarily a big deal.

Comments welcome,

Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---

(And yes, I will try to get ISS to implement an IDE emulation instead
but that's not what's there at this stage)

arch/powerpc/boot/dts/iss4xx.dts | 5
drivers/block/Kconfig | 4
drivers/block/Makefile | 1
drivers/block/iss_blk.c | 365 +++++++++++++++++++++++++++++++++++++++
4 files changed, 375 insertions(+)

--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-work/drivers/block/iss_blk.c 2008-09-23 11:12:03.000000000 +1000
@@ -0,0 +1,365 @@
+/*
+ * Simple block device for the ISS simulator
+ */
+
+#undef DEBUG
+
+#include <linux/major.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/fs.h>
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/ioctl.h>
+#include <linux/blkdev.h>
+#include <linux/of.h>
+
+#include <asm/io.h>
+
+#define MAJOR_NR 63 /* FIXME */
+#define NUM_ISS_BLK_MINOR 4
+
+/* Command codes */
+enum {
+ ISS_BD_CMD_NOP = 0,
+ ISS_BD_CMD_OPEN = 1,
+ ISS_BD_CMD_CLOSE = 2,
+ ISS_BD_CMD_READ = 3,
+ ISS_BD_CMD_WRITE = 4,
+ ISS_BD_CMD_STATUS = 5,
+ ISS_BD_CMD_CHKCHANGE = 6,
+ ISS_BD_CMD_SYNC = 7,
+ ISS_BD_CMD_GET_BLKSIZE = 8,
+ ISS_BD_CMD_GET_DEVSIZE = 9,
+};
+
+/* Status codes */
+enum {
+ ISS_BD_STATUS_OK = 0,
+ ISS_BD_STATUS_OP_ER = 1, /* Open error */
+ ISS_BD_ALREADY_OPEN = 2, /* Block file already open */
+ ISS_BD_NOT_OPEN = 3, /* Block file not open */
+ ISS_BD_BAD_DEV_NUM = 4, /* Bad device number */
+ ISS_BD_BAD_SEC_CNT = 5, /* Bad sector number */
+ ISS_BD_SEEK_ERROR = 6, /* Bad sector count */
+ ISS_BD_RW_ERROR = 7, /* Read/Write error */
+ ISS_BD_SIZE_ERROR = 8, /* Unable to determine file size */
+};
+
+struct iss_blk_regs {
+ u8 cmd;
+ u8 pad0[3];
+ u32 stat;
+ u32 sector;
+ u32 count;
+ u32 devno;
+ u32 size;
+ u8 pad1[0x1e8];
+ u8 data[0x800];
+};
+
+struct iss_blk {
+ struct gendisk *disk;
+ unsigned int devno;
+ unsigned int sectsize;
+ unsigned int capacity;
+ unsigned int present;
+ unsigned int changed;
+} iss_blks[NUM_ISS_BLK_MINOR];
+
+static spinlock_t iss_blk_qlock;
+static spinlock_t iss_blk_reglock;
+static struct iss_blk_regs __iomem *iss_blk_regs;
+
+static void iss_blk_setup(struct iss_blk *ib)
+{
+ unsigned long flags;
+ u32 stat;
+
+ pr_debug("iss_blk_setup %d\n", ib->devno);
+
+ spin_lock_irqsave(&iss_blk_reglock, flags);
+ out_8(iss_blk_regs->data, 0);
+ out_be32(&iss_blk_regs->devno, ib->devno);
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_OPEN);
+ stat = in_be32(&iss_blk_regs->stat);
+ if (stat != ISS_BD_STATUS_OK) {
+ pr_debug(" -> no file\n");
+ goto failed;
+ }
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_GET_BLKSIZE);
+ ib->sectsize = in_be32(&iss_blk_regs->size);
+ if (ib->sectsize != 512) {
+ pr_err("issblk: unsupported sector size %d\n", ib->sectsize);
+ goto failed;
+ }
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_GET_DEVSIZE);
+ ib->capacity = in_be32(&iss_blk_regs->size);
+ ib->present = 1;
+ ib->changed = 0;
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+
+ pr_debug(" -> 0x%x sectors 0f %d bytes\n",
+ ib->capacity, ib->sectsize);
+
+ blk_queue_bounce_limit(ib->disk->queue, BLK_BOUNCE_HIGH);
+ blk_queue_hardsect_size(ib->disk->queue, ib->sectsize);
+ set_capacity(ib->disk, ib->capacity);
+ return;
+
+ failed:
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+}
+
+static int __iss_blk_read(struct iss_blk *ib, void *buffer,
+ unsigned long sector, unsigned long count)
+{
+ unsigned long lcount, flags;
+ u32 stat;
+
+ pr_debug("__iss_blk_read 0x%ld sectors @ 0x%lx\n", count, sector);
+
+ while(count) {
+ lcount = min(count, 4ul);
+ spin_lock_irqsave(&iss_blk_reglock, flags);
+ out_be32(&iss_blk_regs->devno, ib->devno);
+ out_be32(&iss_blk_regs->sector, sector);
+ out_be32(&iss_blk_regs->count, lcount);
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_READ);
+ stat = in_be32(&iss_blk_regs->stat);
+ if (stat != ISS_BD_STATUS_OK) {
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+ return -EIO;
+ }
+ memcpy_fromio(buffer, &iss_blk_regs->data, lcount * ib->sectsize);
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+ count -= lcount;
+ sector += lcount;
+ buffer += lcount * ib->sectsize;
+ }
+ return 0;
+}
+
+static int __iss_blk_write(struct iss_blk *ib, void *buffer,
+ unsigned long sector, unsigned long count)
+{
+ unsigned long lcount, flags;
+ u32 stat;
+
+ pr_debug("__iss_blk_write 0x%ld sectors @ 0x%lx\n", count, sector);
+
+ while(count) {
+ lcount = min(count, 4ul);
+ spin_lock_irqsave(&iss_blk_reglock, flags);
+ out_be32(&iss_blk_regs->devno, ib->devno);
+ out_be32(&iss_blk_regs->sector, sector);
+ out_be32(&iss_blk_regs->count, lcount);
+ memcpy_toio(&iss_blk_regs->data, buffer, lcount * ib->sectsize);
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_WRITE);
+ stat = in_be32(&iss_blk_regs->stat);
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+ if (stat != ISS_BD_STATUS_OK)
+ return -EIO;
+ count -= lcount;
+ sector += lcount;
+ buffer += lcount * ib->sectsize;
+ }
+ return 0;
+}
+
+static void iss_blk_do_request(struct request_queue * q)
+{
+ struct iss_blk *ib = q->queuedata;
+ struct request *req;
+ int rc = 0;
+
+ pr_debug("iss_do_request dev %d\n", ib->devno);
+
+ while ((req = elv_next_request(q)) != NULL) {
+ pr_debug(" -> req @ %p, changed: %d\n", req, ib->changed);
+ if (ib->changed) {
+ end_request(req, 0); /* failure */
+ continue;
+ }
+ switch (rq_data_dir(req)) {
+ case READ:
+ rc = __iss_blk_read(ib, req->buffer, req->sector,
+ req->current_nr_sectors);
+ break;
+ case WRITE:
+ rc = __iss_blk_write(ib, req->buffer, req->sector,
+ req->current_nr_sectors);
+ };
+
+ pr_debug(" -> ending request, rc = %d\n", rc);
+ if (rc)
+ end_request(req, 0); /* failure */
+ else
+ end_request(req, 1); /* success */
+ }
+}
+
+static int iss_blk_release(struct inode *inode, struct file *file)
+{
+ struct gendisk *disk = inode->i_bdev->bd_disk;
+ struct iss_blk *ib = disk->private_data;
+ unsigned long flags;
+
+ pr_debug("issblk%d: release !\n", disk->first_minor);
+
+ spin_lock_irqsave(&iss_blk_reglock, flags);
+ out_be32(&iss_blk_regs->devno, ib->devno);
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_SYNC);
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+
+ return 0;
+}
+
+static int iss_blk_revalidate(struct gendisk *disk)
+{
+ struct iss_blk *ib = disk->private_data;
+ unsigned long flags;
+
+ pr_debug("issblk%d: revalidate !\n", disk->first_minor);
+
+ if (ib->present && ib->changed) {
+ spin_lock_irqsave(&iss_blk_reglock, flags);
+ out_be32(&iss_blk_regs->devno, ib->devno);
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_CLOSE);
+ ib->present = ib->changed = 0;
+ spin_unlock_irqrestore(&iss_blk_reglock, flags);
+ }
+ iss_blk_setup(ib);
+ return 0;
+}
+
+static int iss_blk_media_changed(struct gendisk *disk)
+{
+ struct iss_blk *ib = disk->private_data;
+
+ pr_debug("issblk%d: media_changed !\n", disk->first_minor);
+
+ /* Not implemented -> query change status from ISS */
+
+ return ib->changed;
+}
+
+static int iss_blk_open(struct inode *inode, struct file *file)
+{
+ struct gendisk *disk = inode->i_bdev->bd_disk;
+ struct iss_blk *ib = disk->private_data;
+
+ pr_debug("issblk%d: open !\n", disk->first_minor);
+
+ check_disk_change(inode->i_bdev);
+ if (ib->changed)
+ iss_blk_setup(ib);
+ if (!ib->present)
+ return -ENOMEDIUM;
+ return 0;
+}
+
+static struct block_device_operations iss_blk_fops = {
+ .owner = THIS_MODULE,
+ .open = iss_blk_open,
+ .release = iss_blk_release,
+ .media_changed = iss_blk_media_changed,
+ .revalidate_disk = iss_blk_revalidate,
+};
+
+static int __init iss_blk_init(void)
+{
+ struct device_node *np;
+ int i;
+
+ pr_debug("iss_regs offsets:\n");
+ pr_debug(" cmd : 0x%x\n", offsetof(struct iss_blk_regs, cmd));
+ pr_debug(" stat : 0x%x\n", offsetof(struct iss_blk_regs, stat));
+ pr_debug(" sector : 0x%x\n", offsetof(struct iss_blk_regs, sector));
+ pr_debug(" count : 0x%x\n", offsetof(struct iss_blk_regs, count));
+ pr_debug(" devno : 0x%x\n", offsetof(struct iss_blk_regs, devno));
+ pr_debug(" size : 0x%x\n", offsetof(struct iss_blk_regs, size));
+ pr_debug(" data : 0x%x\n", offsetof(struct iss_blk_regs, data));
+
+ np = of_find_node_by_path("/iss-block");
+ if (np == NULL)
+ return -ENODEV;
+ iss_blk_regs = of_iomap(np, 0);
+ if (iss_blk_regs == NULL) {
+ pr_err("issblk: Failed to map registers\n");
+ return -ENOMEM;
+ }
+
+ if (register_blkdev(MAJOR_NR, "iss_blk"))
+ return -EIO;
+
+ spin_lock_init(&iss_blk_qlock);
+ spin_lock_init(&iss_blk_reglock);
+
+ printk(KERN_INFO "ISS Block driver initializing for %d minors\n",
+ NUM_ISS_BLK_MINOR);
+
+ for (i = 0; i < NUM_ISS_BLK_MINOR; i++) {
+ struct gendisk *disk = alloc_disk(1);
+ struct request_queue *q;
+ struct iss_blk *ib = &iss_blks[i];
+
+ if (!disk) {
+ pr_err("issblk%d: Failed to allocate disk\n", i);
+ break;
+ }
+
+ q = blk_init_queue(iss_blk_do_request, &iss_blk_qlock);
+ if (q == NULL) {
+ pr_err("issblk%d: Failed to init queue\n", i);
+ put_disk(disk);
+ break;
+ }
+ q->queuedata = ib;
+
+ ib->disk = disk;
+ ib->devno = i;
+ ib->present = 0;
+ ib->changed = 0;
+ ib->capacity = 0;
+ ib->sectsize = 512;
+
+ disk->major = MAJOR_NR;
+ disk->first_minor = i;
+ disk->fops = &iss_blk_fops;
+ disk->private_data = &iss_blks[i];
+ disk->flags = GENHD_FL_REMOVABLE;
+ disk->queue = q;
+ sprintf(disk->disk_name, "issblk%d", i);
+
+ iss_blk_setup(ib);
+
+ add_disk(disk);
+ }
+
+ return 0;
+}
+
+static void __exit iss_blk_exit(void)
+{
+ int i;
+
+ unregister_blkdev(MAJOR_NR, "iss_blk");
+
+ for (i = 0; i < NUM_ISS_BLK_MINOR; i++) {
+ struct iss_blk *ib = &iss_blks[i];
+
+ if (ib->present) {
+ out_be32(&iss_blk_regs->devno, ib->devno);
+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_CLOSE);
+ }
+ }
+}
+
+module_init(iss_blk_init);
+module_exit(iss_blk_exit);
+
+MODULE_DESCRIPTION("ISS Simulator Block Device");
+MODULE_LICENSE("GPL");
Index: linux-work/drivers/block/Kconfig
===================================================================
--- linux-work.orig/drivers/block/Kconfig 2008-07-17 14:43:58.000000000 +1000
+++ linux-work/drivers/block/Kconfig 2008-09-23 11:12:03.000000000 +1000
@@ -357,6 +357,10 @@ config BLK_DEV_XIP
will prevent RAM block device backing store memory from being
allocated from highmem (only a problem for highmem systems).

+config BLK_DEV_ISS
+ bool "Support ISS Simulator Block Device"
+ default n
+
config CDROM_PKTCDVD
tristate "Packet writing on CD/DVD media"
depends on !UML
Index: linux-work/drivers/block/Makefile
===================================================================
--- linux-work.orig/drivers/block/Makefile 2008-07-17 14:43:58.000000000 +1000
+++ linux-work/drivers/block/Makefile 2008-09-23 11:12:03.000000000 +1000
@@ -30,5 +30,6 @@ obj-$(CONFIG_VIODASD) += viodasd.o
obj-$(CONFIG_BLK_DEV_SX8) += sx8.o
obj-$(CONFIG_BLK_DEV_UB) += ub.o
obj-$(CONFIG_BLK_DEV_HD) += hd.o
+obj-$(CONFIG_BLK_DEV_ISS) += iss_blk.o

obj-$(CONFIG_XEN_BLKDEV_FRONTEND) += xen-blkfront.o
Index: linux-work/arch/powerpc/boot/dts/iss4xx.dts
===================================================================
--- linux-work.orig/arch/powerpc/boot/dts/iss4xx.dts 2008-09-23 11:12:02.000000000 +1000
+++ linux-work/arch/powerpc/boot/dts/iss4xx.dts 2008-09-23 11:12:03.000000000 +1000
@@ -101,6 +101,11 @@
};
};

+ iss-block {
+ compatible = "ibm,iss-sim-block-device";
+ reg = <0 0xEF701000 0x1000>;
+ };
+
chosen {
linux,stdout-path = "/plb/opb/serial@40000200";
};


2008-10-03 04:44:18

by Andi Kleen

[permalink] [raw]
Subject: Re: [RFC/PATCH] Block device for the ISS simulator

Benjamin Herrenschmidt <[email protected]> writes:

> The ISS simulator is a simple powerpc simulator used among other things
> for hardware bringup. It implements a simple memory mapped block device
> interface.

...
>
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-work/drivers/block/iss_blk.c 2008-09-23 11:12:03.000000000 +1000
> @@ -0,0 +1,365 @@
> +/*
> + * Simple block device for the ISS simulator
> + */

The first paragraph in your description above should be in this comment.

-Andi

2008-10-03 08:35:31

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC/PATCH] Block device for the ISS simulator

On Fri, Oct 03, 2008 at 10:08:42AM +1000, Benjamin Herrenschmidt wrote:
> The ISS simulator is a simple powerpc simulator used among other things
> for hardware bringup. It implements a simple memory mapped block device
> interface.
>
> This is a simple block driver that attaches to it. Note that the choice
> of a major device number is fishy, though because it's a simulator and
> not real hardware, it's not necessarily a big deal.

Please don't put in more block devices for every bloody simulator in the
world. Just emulated some simpler enough existing hardware.

2008-10-03 09:31:52

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [RFC/PATCH] Block device for the ISS simulator

On Fri, 2008-10-03 at 04:35 -0400, Christoph Hellwig wrote:
> On Fri, Oct 03, 2008 at 10:08:42AM +1000, Benjamin Herrenschmidt wrote:
> > The ISS simulator is a simple powerpc simulator used among other things
> > for hardware bringup. It implements a simple memory mapped block device
> > interface.
> >
> > This is a simple block driver that attaches to it. Note that the choice
> > of a major device number is fishy, though because it's a simulator and
> > not real hardware, it's not necessarily a big deal.
>
> Please don't put in more block devices for every bloody simulator in the
> world. Just emulated some simpler enough existing hardware.

I'm not sure I want it merged, but having it on the list "for the
record" is useful and it might be useful to get comments in case it's
terminally busted :-)

I'm trying to get ISS to implement an IDE interface in fact.

Cheers,
Ben.

2008-10-03 12:04:37

by Josh Boyer

[permalink] [raw]
Subject: Re: [RFC/PATCH] Block device for the ISS simulator

On Fri, Oct 03, 2008 at 10:08:42AM +1000, Benjamin Herrenschmidt wrote:
>+static void iss_blk_setup(struct iss_blk *ib)
>+{
>+ unsigned long flags;
>+ u32 stat;
>+
>+ pr_debug("iss_blk_setup %d\n", ib->devno);
>+
>+ spin_lock_irqsave(&iss_blk_reglock, flags);
>+ out_8(iss_blk_regs->data, 0);
>+ out_be32(&iss_blk_regs->devno, ib->devno);
>+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_OPEN);
>+ stat = in_be32(&iss_blk_regs->stat);

Should probably use the ioread/iowrite functions instead of raw out/in.
Same comment throughout.

<snip>

>+static void iss_blk_do_request(struct request_queue * q)
>+{
>+ struct iss_blk *ib = q->queuedata;
>+ struct request *req;
>+ int rc = 0;
>+
>+ pr_debug("iss_do_request dev %d\n", ib->devno);
>+
>+ while ((req = elv_next_request(q)) != NULL) {
>+ pr_debug(" -> req @ %p, changed: %d\n", req, ib->changed);
>+ if (ib->changed) {
>+ end_request(req, 0); /* failure */
>+ continue;
>+ }
>+ switch (rq_data_dir(req)) {
>+ case READ:
>+ rc = __iss_blk_read(ib, req->buffer, req->sector,
>+ req->current_nr_sectors);
>+ break;
>+ case WRITE:
>+ rc = __iss_blk_write(ib, req->buffer, req->sector,
>+ req->current_nr_sectors);
>+ };
>+
>+ pr_debug(" -> ending request, rc = %d\n", rc);
>+ if (rc)
>+ end_request(req, 0); /* failure */
>+ else
>+ end_request(req, 1); /* success */

Could possibly just do:

end_request(req, (!rc));

<snip>

>+static int __init iss_blk_init(void)
>+{
>+ struct device_node *np;
>+ int i;
>+
>+ pr_debug("iss_regs offsets:\n");
>+ pr_debug(" cmd : 0x%x\n", offsetof(struct iss_blk_regs, cmd));
>+ pr_debug(" stat : 0x%x\n", offsetof(struct iss_blk_regs, stat));
>+ pr_debug(" sector : 0x%x\n", offsetof(struct iss_blk_regs, sector));
>+ pr_debug(" count : 0x%x\n", offsetof(struct iss_blk_regs, count));
>+ pr_debug(" devno : 0x%x\n", offsetof(struct iss_blk_regs, devno));
>+ pr_debug(" size : 0x%x\n", offsetof(struct iss_blk_regs, size));
>+ pr_debug(" data : 0x%x\n", offsetof(struct iss_blk_regs, data));
>+
>+ np = of_find_node_by_path("/iss-block");
>+ if (np == NULL)
>+ return -ENODEV;
>+ iss_blk_regs = of_iomap(np, 0);

of_find_node_by_path increments the refcount for that node. Need to
do an of_node_put when you are done.

>+ if (iss_blk_regs == NULL) {
>+ pr_err("issblk: Failed to map registers\n");
>+ return -ENOMEM;
>+ }
>+
>+ if (register_blkdev(MAJOR_NR, "iss_blk"))
>+ return -EIO;
>+
>+ spin_lock_init(&iss_blk_qlock);
>+ spin_lock_init(&iss_blk_reglock);
>+
>+ printk(KERN_INFO "ISS Block driver initializing for %d minors\n",
>+ NUM_ISS_BLK_MINOR);
>+
>+ for (i = 0; i < NUM_ISS_BLK_MINOR; i++) {
>+ struct gendisk *disk = alloc_disk(1);
>+ struct request_queue *q;
>+ struct iss_blk *ib = &iss_blks[i];
>+
>+ if (!disk) {
>+ pr_err("issblk%d: Failed to allocate disk\n", i);
>+ break;
>+ }
>+
>+ q = blk_init_queue(iss_blk_do_request, &iss_blk_qlock);
>+ if (q == NULL) {
>+ pr_err("issblk%d: Failed to init queue\n", i);
>+ put_disk(disk);
>+ break;
>+ }
>+ q->queuedata = ib;
>+
>+ ib->disk = disk;
>+ ib->devno = i;
>+ ib->present = 0;
>+ ib->changed = 0;
>+ ib->capacity = 0;
>+ ib->sectsize = 512;
>+
>+ disk->major = MAJOR_NR;
>+ disk->first_minor = i;
>+ disk->fops = &iss_blk_fops;
>+ disk->private_data = &iss_blks[i];
>+ disk->flags = GENHD_FL_REMOVABLE;
>+ disk->queue = q;
>+ sprintf(disk->disk_name, "issblk%d", i);
>+
>+ iss_blk_setup(ib);
>+
>+ add_disk(disk);
>+ }
>+
>+ return 0;
>+}
>+
>+static void __exit iss_blk_exit(void)
>+{
>+ int i;
>+
>+ unregister_blkdev(MAJOR_NR, "iss_blk");
>+
>+ for (i = 0; i < NUM_ISS_BLK_MINOR; i++) {
>+ struct iss_blk *ib = &iss_blks[i];
>+
>+ if (ib->present) {
>+ out_be32(&iss_blk_regs->devno, ib->devno);
>+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_CLOSE);
>+ }
>+ }

Shouldn't you unmap iss_blk_regs at this point?

<snip>

>Index: linux-work/drivers/block/Kconfig
>===================================================================
>--- linux-work.orig/drivers/block/Kconfig 2008-07-17 14:43:58.000000000 +1000
>+++ linux-work/drivers/block/Kconfig 2008-09-23 11:12:03.000000000 +1000
>@@ -357,6 +357,10 @@ config BLK_DEV_XIP
> will prevent RAM block device backing store memory from being
> allocated from highmem (only a problem for highmem systems).
>
>+config BLK_DEV_ISS
>+ bool "Support ISS Simulator Block Device"
>+ default n
>+

Should probably have:

depends on PPC



josh

2008-10-03 12:21:25

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [RFC/PATCH] Block device for the ISS simulator

On Fri, 2008-10-03 at 08:03 -0400, Josh Boyer wrote:
> On Fri, Oct 03, 2008 at 10:08:42AM +1000, Benjamin Herrenschmidt wrote:
> >+static void iss_blk_setup(struct iss_blk *ib)
> >+{
> >+ unsigned long flags;
> >+ u32 stat;
> >+
> >+ pr_debug("iss_blk_setup %d\n", ib->devno);
> >+
> >+ spin_lock_irqsave(&iss_blk_reglock, flags);
> >+ out_8(iss_blk_regs->data, 0);
> >+ out_be32(&iss_blk_regs->devno, ib->devno);
> >+ out_8(&iss_blk_regs->cmd, ISS_BD_CMD_OPEN);
> >+ stat = in_be32(&iss_blk_regs->stat);
>
> Should probably use the ioread/iowrite functions instead of raw out/in.
> Same comment throughout.

Yeah well, old habits :-)

> >+ pr_debug(" -> ending request, rc = %d\n", rc);
> >+ if (rc)
> >+ end_request(req, 0); /* failure */
> >+ else
> >+ end_request(req, 1); /* success */
>
> Could possibly just do:
>
> end_request(req, (!rc));

I knew copy/pasting from a crap driver was going to come back and bite
me :)

> of_find_node_by_path increments the refcount for that node. Need to
> do an of_node_put when you are done.

Yup, suppose so.

> Shouldn't you unmap iss_blk_regs at this point?

Might be a good idea yeah.

Ben.