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

[Josh Boyer]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/