OK, second iteration of this - have added more comments and, I hope,
addressed all or nearly all of people's comments.
This adds support for the flash memory portion of the "visual memory
unit" on the SEGA Dreamcast.
Changelogs on the patches but, in summary:
On the maple bus side:
- adds a mutex to ensure device is only addressed once per maple bus
cycle (and so avoiding corruption of the linked list of packets waiting
for transmission).
- properly uses the existing lock on the waiting packet queue and cleans
up the way the linked lists (of waiting and processed packets) are
handled.
For the device:
- relies on the mutex to ensure block reads and writes are handled
properly - normally the bus driver will unlock the mutex once a waiting
packet has been processed
- implements support for partitions. There are rumours such devices
exist - they are supported in the specification. But I've never seen
one!
There is code to follow (eventually) to implement the FAT based
filesystem used by the Dreamcast to save to these devices, but they
should be usable as small flash devices with other filesystems too.
(See http://mc.pp.se/dc/vms/flashmem.html for more details on the
hardware etc)
Signed-off-by: Adrian McMenamin <[email protected]>
Signed-off-by: Adrian McMenamin <[email protected]>
---
diff -ruN a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
--- a/drivers/mtd/maps/Kconfig 2008-03-22 17:26:24.000000000 +0000
+++ b/drivers/mtd/maps/Kconfig 2008-03-22 17:20:45.000000000 +0000
@@ -590,5 +590,19 @@
This selection automatically selects the map_ram driver.
+config MTD_VMU_FLASH
+ tristate "Dreamcast Visual Memory devices and clones"
+ depends on SH_DREAMCAST
+ select MAPLE
+ help
+ Flash map driver for the SEGA Dreamcast Visual Memory Unit
+ and clones.
+
+ Most Dreamcast users will have one of these and so will want
+ to say Y here.
+
+ To compile this driver as a module choose M here: the module
+ will be called vmu-flash.
+
endmenu
diff -ruN a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
--- a/drivers/mtd/maps/Makefile 2008-03-22 17:26:24.000000000 +0000
+++ b/drivers/mtd/maps/Makefile 2008-03-22 17:20:45.000000000 +0000
@@ -68,3 +68,4 @@
obj-$(CONFIG_MTD_OMAP_NOR) += omap_nor.o
obj-$(CONFIG_MTD_MTX1) += mtx-1_flash.o
obj-$(CONFIG_MTD_INTEL_VR_NOR) += intel_vr_nor.o
+obj-$(CONFIG_MTD_VMU_FLASH) += vmu_flash.o
The SEGA Visual Memory Unit includes 128k of flash memory
which can be read in blocks.
The hardware specification is also capable of supporting
partitions (though it is doubtful such devices exist).
This driver supports block reads and writes, as well as
queries of hardware capabilities, through the maple
bus susbsystem.
(It also implements a caching system so that, for instance, a
read of 60 bytes will take 1/60th - or 1/50th in PAL regions -
of a second instead of a second or 1.2 seconds.)
Signed-off-by: Adrian McMenamin <[email protected]>
---
diff -ruN a/drivers/mtd/maps/vmu_flash.c b/drivers/mtd/maps/vmu_flash.c
--- a/drivers/mtd/maps/vmu_flash.c 1970-01-01 01:00:00.000000000 +0100
+++ b/drivers/mtd/maps/vmu_flash.c 2008-03-22 17:53:50.000000000 +0000
@@ -0,0 +1,763 @@
+/* vmu-flash.c
+ * Driver for SEGA Dreamcast Visual Memory Unit
+ *
+ * Copyright Adrian McMenamin 2008
+ *
+ * Substantial parts based on code:
+ * Copyright Paul Mundt 2001
+ * Copyright Adrian McMenamin 2002
+ *
+ *
+ * Licensed under version 2 of the
+ * GNU General Public Licence
+ */
+#include <linux/init.h>
+#include <linux/maple.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/map.h>
+#include <asm/mach/maple.h>
+
+static DECLARE_WAIT_QUEUE_HEAD(vmu_read);
+static int block_read;
+
+struct vmu_cache {
+ char *buffer; /* Cache */
+ unsigned int block; /* Which block was cached */
+ unsigned long jiffies_atc; /* When was it cached? */
+ int valid;
+};
+
+struct mdev_part {
+ struct maple_device *mdev;
+ int partition;
+};
+
+struct vmupart {
+ u16 user_blocks;
+ u16 root_block;
+ u16 numblocks;
+ char *name;
+ struct vmu_cache *pcache;
+};
+
+struct memcard {
+ u16 tempA;
+ u16 tempB;
+ u32 partitions;
+ u32 blocklen;
+ u32 writecnt;
+ u32 readcnt;
+ u32 removeable;
+ int partition;
+ void *sendbuf;
+ void *blockread;
+ struct vmupart *parts;
+ struct mtd_info *mtd;
+};
+
+struct vmu_block {
+ unsigned int num; /* block number */
+ unsigned int ofs; /* block offset */
+};
+
+static struct vmu_block *ofs_to_block(unsigned long src_ofs,
+ struct mtd_info *mtd, int partition)
+{
+ struct vmu_block *vblock;
+ struct maple_device *mdev;
+ struct memcard *card;
+ struct mdev_part *mpart;
+ int num;
+
+ mpart = mtd->priv;
+ mdev = mpart->mdev;
+ card = mdev->private_data;
+
+ if (src_ofs >= ((card->parts)[partition]).numblocks * card->blocklen)
+ goto failed;
+
+ num = src_ofs / card->blocklen;
+ if (num > ((card->parts)[partition]).numblocks)
+ goto failed;
+
+ vblock = kmalloc(sizeof(struct vmu_block), GFP_KERNEL);
+ if (!vblock)
+ goto failed;
+
+ vblock->num = num;
+ vblock->ofs = src_ofs % card->blocklen;
+ return vblock;
+
+failed:
+ return NULL;
+}
+
+
+/* Maple bus callback function for reads */
+static void vmu_blockread(struct mapleq *mq)
+{
+ struct maple_device *mdev;
+ struct memcard *card;
+ struct mtd_info *mtd;
+ struct vmu_cache *pcache;
+ struct mdev_part *mpart;
+ int partition;
+
+ mdev = mq->dev;
+ card = mdev->private_data;
+ /* copy the read in data */
+ memcpy(card->blockread, mq->recvbuf + 12, card->blocklen);
+ block_read = 1;
+ /* fill the cache for this block */
+ mtd = card->mtd;
+ mpart = mtd->priv;
+ partition = mpart->partition;
+ pcache = (card->parts[partition]).pcache;
+ if (!pcache->buffer)
+ pcache->buffer = kmalloc(card->blocklen, GFP_KERNEL);
+ /* If fail because of ENOMEM - wake up to cause failure */
+ if (!pcache->buffer)
+ goto wakeup;
+ memcpy(pcache->buffer, card->blockread, card->blocklen);
+ pcache->block = ((unsigned char *)mq->recvbuf)[11];
+ pcache->jiffies_atc = jiffies;
+ pcache->valid = 1;
+wakeup:
+ wake_up_interruptible(&vmu_read);
+}
+
+/* Interface with maple bus to read bytes */
+static int maple_vmu_read_block(unsigned int num, unsigned char *buf,
+ struct mtd_info *mtd)
+{
+ struct memcard *card;
+ struct mdev_part *mpart;
+ struct maple_device *mdev;
+ int partition, error = 0, locking;
+ void *sendbuf;
+
+ mpart = mtd->priv;
+ mdev = mpart->mdev;
+ partition = mpart->partition;
+ card = mdev->private_data;
+
+ /***
+ * Maple devices include a mutex to ensure packets injected into
+ * the wait queue are not corrupted via scans for hotplug events etc
+ */
+ locking = mutex_lock_interruptible(&mdev->mq->mutex);
+ if (locking) {
+ printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
+ " aborting read\n", mdev->unit, mdev->port);
+ goto fail_nosendbuf;
+ }
+ mdev->mq->command = MAPLE_COMMAND_BREAD;
+ mdev->mq->length = 2;
+
+ sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
+ if (!sendbuf) {
+ error = -ENOMEM;
+ goto fail_nosendbuf;
+ }
+
+ ((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+ ((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
+
+ mdev->mq->sendbuf = sendbuf;
+ block_read = 0;
+
+ card->blockread = kmalloc(card->blocklen, GFP_KERNEL);
+ if (!card->blockread) {
+ error = -ENOMEM;
+ goto fail_bralloc;
+ }
+
+ maple_getcond_callback(mdev, vmu_blockread, 0, MAPLE_FUNC_MEMCARD);
+ maple_add_packet(mdev->mq);
+ wait_event_interruptible_timeout(vmu_read, block_read, HZ * 4);
+ if (block_read == 0) {
+ printk(KERN_INFO "Maple: VMU read failed on block 0x%X\n", num);
+ error = -EIO;
+ goto fail_blockread;
+ }
+
+ memcpy(buf, card->blockread, card->blocklen);
+ kfree(card->blockread);
+ kfree(sendbuf);
+
+ return 0;
+
+fail_blockread:
+ kfree(card->blockread);
+fail_bralloc:
+ kfree(sendbuf);
+fail_nosendbuf:
+ if (mutex_is_locked(&mdev->mq->mutex))
+ mutex_unlock(&mdev->mq->mutex);
+ return error;
+}
+
+/* communicate with maple bus for phased writing */
+static int maple_vmu_write_block(unsigned int num, const unsigned char *buf,
+ struct mtd_info *mtd)
+{
+ struct memcard *card;
+ struct mdev_part *mpart;
+ struct maple_device *mdev;
+ int partition, error, locking, x, phaselen;
+ void *sendbuf;
+
+ mpart = mtd->priv;
+ mdev = mpart->mdev;
+ partition = mpart->partition;
+ card = mdev->private_data;
+
+ phaselen = card->blocklen/card->writecnt;
+ mdev->mq->command = MAPLE_COMMAND_BWRITE;
+ mdev->mq->length = phaselen / 4 + 2;
+
+ sendbuf = kmalloc(mdev->mq->length * 4, GFP_KERNEL);
+ if (!sendbuf) {
+ error = -ENOMEM;
+ goto fail_nosendbuf;
+ }
+
+ ((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+
+ for (x = 0; x < card->writecnt; x++) {
+ /* take the lock to protect the contents of sendbuf */
+ locking = mutex_lock_interruptible(&mdev->mq->mutex);
+ if (locking) {
+ error = -EBUSY;
+ goto fail_nolock;
+ }
+ ((unsigned long *)sendbuf)[1] =
+ cpu_to_be32(partition << 24 | x << 16 | num);
+ memcpy(sendbuf + 8, buf + phaselen * x, phaselen);
+ mdev->mq->sendbuf = sendbuf;
+ maple_add_packet(mdev->mq);
+ }
+ /***
+ * The Maple bus driver will unlock the mutex once the command
+ * has been processed, so we'll just sleep waiting for the unlock */
+ locking = mutex_lock_interruptible(&mdev->mq->mutex);
+ if (locking) {
+ error = -EBUSY;
+ goto fail_nolock;
+ }
+ mutex_unlock(&mdev->mq->mutex);
+
+ kfree(sendbuf);
+
+ return card->blocklen;
+
+fail_nolock:
+ printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
+ " aborting write\n", mdev->unit, mdev->port);
+ kfree(sendbuf);
+fail_nosendbuf:
+ printk("Maple: VMU (%d, %d): write failed\n", mdev->port, mdev->unit);
+ if (mutex_is_locked(&mdev->mq->mutex))
+ mutex_unlock(&mdev->mq->mutex);
+ return error;
+}
+
+/* mtd function to simulate reading byte by byte */
+static unsigned char vmu_flash_read_char(unsigned long ofs, int *retval,
+ struct mtd_info *mtd)
+{
+ struct vmu_block *vblock;
+ struct memcard *card;
+ struct mdev_part *mpart;
+ struct maple_device *mdev;
+ unsigned char *buf, ret;
+ int partition, error;
+
+ mpart = mtd->priv;
+ mdev = mpart->mdev;
+ partition = mpart->partition;
+ card = mdev->private_data;
+ *retval = 0;
+ buf = kmalloc(card->blocklen, GFP_KERNEL);
+ if (!buf) {
+ *retval = 1;
+ error = -ENOMEM;
+ goto fail_buffer;
+ }
+
+ vblock = ofs_to_block(ofs, mtd, partition);
+ if (!vblock) {
+ *retval = 3;
+ error = -ENOMEM;
+ goto invalid_block;
+ }
+
+ error = maple_vmu_read_block(vblock->num, buf, mtd);
+ if (error) {
+ *retval = 2;
+ goto failed_block;
+ }
+ ret = buf[vblock->ofs];
+ kfree(buf);
+ kfree(vblock);
+ return ret;
+
+failed_block:
+ kfree(vblock);
+invalid_block:
+ kfree(buf);
+fail_buffer:
+ return error;
+}
+
+/* mtd higher order function to read flash */
+static int vmu_flash_read(struct mtd_info *mtd, loff_t from, size_t len,
+ size_t *retlen, u_char *buf)
+{
+ struct maple_device *mdev;
+ struct memcard *card;
+ struct mdev_part *mpart;
+ struct vmu_cache *pcache;
+ struct vmu_block *vblock = NULL;
+ int index = 0, retval, partition, leftover, numblocks;
+ unsigned char cx;
+
+ mpart = mtd->priv;
+ mdev = mpart->mdev;
+ partition = mpart->partition;
+ card = mdev->private_data;
+
+ if (len < 1)
+ return -1;
+ numblocks = card->parts[partition].numblocks;
+ if (from + len > numblocks * card->blocklen)
+ len = numblocks * card->blocklen - from;
+ if (len == 0)
+ return -1;
+ /* Have we cached this bit already? */
+ pcache = (card->parts[partition]).pcache;
+ do {
+ vblock = ofs_to_block(from + index, mtd, partition);
+ if (!vblock)
+ return -ENOMEM;
+
+ /* Have we cached this and is the cache valid and timely? */
+ if (pcache->valid &&
+ time_before(jiffies, pcache->jiffies_atc + HZ) &&
+ (pcache->block == vblock->num)) {
+ /* we have cached it, so do necessary copying */
+ leftover = card->blocklen - vblock->ofs;
+ if (leftover > len - index) {
+ /* only a bit of this block to copy */
+ memcpy(buf + index,
+ pcache->buffer + vblock->ofs,
+ len - index);
+ index = len;
+ kfree(vblock);
+ break;
+ }
+ /* otherwise copy remainder of whole block */
+ memcpy(buf + index, pcache->buffer +
+ vblock->ofs, leftover);
+ index += leftover;
+ } else {
+ /* Not cached so read from the device */
+ cx = vmu_flash_read_char(from + index, &retval, mtd);
+ if (retval) {
+ *retlen = index;
+ return -EIO;
+ }
+ memset(buf + index, cx, 1);
+ index++;
+ }
+ kfree(vblock);
+ vblock = NULL;
+ } while (len > index);
+ *retlen = index;
+
+ return 0;
+}
+
+static int vmu_flash_write(struct mtd_info *mtd, loff_t to, size_t len,
+ size_t *retlen, const u_char *buf)
+{
+ struct maple_device *mdev;
+ struct memcard *card;
+ struct mdev_part *mpart;
+ int index = 0, retval, partition, error = 0, numblocks;
+ struct vmu_cache *pcache;
+ struct vmu_block *vblock;
+ unsigned char *buffer;
+
+ mpart = mtd->priv;
+ mdev = mpart->mdev;
+ partition = mpart->partition;
+ card = mdev->private_data;
+
+ /* simple sanity checks */
+ if (len < 1) {
+ error = -1;
+ goto failed;
+ }
+ numblocks = card->parts[partition].numblocks;
+ if (to + len > numblocks * card->blocklen)
+ len = numblocks * card->blocklen - to;
+ if (len == 0) {
+ error = -1;
+ goto failed;
+ }
+
+ vblock = ofs_to_block(to, mtd, partition);
+ if (!vblock) {
+ error = -ENOMEM;
+ goto failed;
+ }
+
+ buffer = kmalloc(card->blocklen, GFP_KERNEL);
+ if (!buffer) {
+ error = -ENOMEM;
+ goto fail_buffer;
+ }
+
+ do {
+
+ /* Read in the block we are to write to */
+ if (maple_vmu_read_block(vblock->num, buffer, mtd)) {
+ error = -EIO;
+ goto fail_io;
+ }
+
+ do {
+ buffer[vblock->ofs] = buf[index];
+ vblock->ofs++;
+ index++;
+ if (index >= len)
+ break;
+ } while (vblock->ofs < card->blocklen);
+ /* write out new buffer */
+ retval = maple_vmu_write_block(vblock->num, buffer, mtd);
+ /* invalidare the cache */
+ pcache = (card->parts[partition]).pcache;
+ pcache->valid = 0;
+
+ if (retval != card->blocklen) {
+ error = -EIO;
+ goto fail_io;
+ }
+
+ vblock->num++;
+ vblock->ofs = 0;
+ } while (len > index);
+
+ kfree(buffer);
+ *retlen = index;
+ kfree(vblock);
+ return 0;
+
+fail_io:
+ kfree(buffer);
+fail_buffer:
+ kfree(vblock);
+failed:
+ printk(KERN_INFO "Maple: VMU write failing with error %d\n", error);
+ return error;
+}
+
+static int vmu_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
+{
+ int z;
+ erase->state = MTD_ERASING;
+ vmu_flash_write(mtd, erase->addr, erase->len, &z, "\0");
+ erase->state = MTD_ERASE_DONE;
+ if (erase->callback)
+ (erase->callback) (erase);
+ return 0;
+}
+
+static void vmu_flash_sync(struct mtd_info *mtd)
+{
+ /* Do nothing */
+}
+
+/* Maple bus callback function to recursively query hardware details */
+static void vmu_queryblocks(struct mapleq *mq)
+{
+ struct maple_device *mdev;
+ unsigned short *res;
+ struct memcard *card;
+ void *sendbuf;
+ struct vmu_cache *pcache;
+ struct mdev_part *mpart;
+ struct mtd_info *mtd_cur;
+ struct vmupart *part_cur;
+ int error, locking;
+
+ mdev = mq->dev;
+ card = mdev->private_data;
+ res = mq->recvbuf;
+ card->tempA = res[12];
+ card->tempB = res[6];
+
+ printk(KERN_INFO "Maple: VMU device at partition %d has %d user "
+ "blocks with a root block at %d\n", card->partition,
+ card->tempA, card->tempB);
+
+ part_cur = &((card->parts)[card->partition]);
+ part_cur->user_blocks = card->tempA;
+ part_cur->root_block = card->tempB;
+ part_cur->numblocks = card->tempB + 1;
+ part_cur->name = kmalloc(12, GFP_KERNEL);
+ if (!part_cur->name)
+ goto fail_name;
+
+ sprintf(part_cur->name, "vmu%d.%d.%d",
+ mdev->port, mdev->unit, card->partition);
+ mtd_cur = &((card->mtd)[card->partition]);
+ mtd_cur->name = part_cur->name;
+ mtd_cur->type = MTD_NORFLASH;
+ mtd_cur->flags = MTD_CAP_NORFLASH;
+ mtd_cur->size = part_cur->numblocks * card->blocklen;
+ mtd_cur->erasesize = card->blocklen;
+ mtd_cur->write = vmu_flash_write;
+ mtd_cur->read = vmu_flash_read;
+ mtd_cur->erase = vmu_flash_erase;
+ mtd_cur->sync = vmu_flash_sync;
+ mtd_cur->writesize = card->blocklen;
+
+ mpart = kmalloc(sizeof(struct mdev_part), GFP_KERNEL);
+ if (!mpart)
+ goto fail_mpart;
+
+ mpart->mdev = mdev;
+ mpart->partition = card->partition;
+ mtd_cur->priv = mpart;
+ mtd_cur->owner = THIS_MODULE;
+
+ pcache = kmalloc(sizeof(struct vmu_cache), GFP_KERNEL);
+ if (!pcache)
+ goto fail_cache_create;
+ pcache->buffer = NULL;
+ pcache->valid = 0;
+ part_cur->pcache = pcache;
+
+ error = add_mtd_device(mtd_cur);
+ if (error)
+ goto fail_mtd_register;
+
+ kfree(card->sendbuf);
+ maple_getcond_callback(mdev, NULL, 0,
+ MAPLE_FUNC_MEMCARD);
+
+ if (++(card->partition) < card->partitions) {
+ mdev->mq->command = MAPLE_COMMAND_GETMINFO;
+ mdev->mq->length = 2;
+
+ sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
+ if (!sendbuf) {
+ error = -ENOMEM;
+ goto fail_nosendbuf;
+ }
+ card->sendbuf = sendbuf;
+ mdev->mq->sendbuf = sendbuf;
+ maple_getcond_callback(mdev, vmu_queryblocks, 0,
+ MAPLE_FUNC_MEMCARD);
+
+ locking = mutex_lock_interruptible(&(mdev->mq->mutex));
+ if (!locking)
+ maple_add_packet(mdev->mq);
+ }
+
+ return;
+
+fail_mtd_register:
+ printk("mtd: Could not register maple device at (%d, %d)\n",
+ mdev->port, mdev->unit);
+ for (error = 0; error <= card->partition; error++) {
+ kfree(((card->parts)[error]).pcache);
+ ((card->parts)[error]).pcache = NULL;
+ }
+fail_cache_create:
+ kfree(card->sendbuf);
+fail_mpart:
+ for (error = 0; error <= card->partition; error++) {
+ kfree(((card->mtd)[error]).priv);
+ ((card->mtd)[error]).priv = NULL;
+ }
+fail_nosendbuf:
+ maple_getcond_callback(mdev, NULL, 0,
+ MAPLE_FUNC_MEMCARD);
+ kfree(part_cur->name);
+fail_name:
+ return;
+}
+
+static int vmu_connect(struct maple_device *mdev)
+{
+ unsigned long test_flash_data, basic_flash_data;
+ int c, locking, error = 0;
+ struct memcard *card;
+ void *sendbuf;
+
+ test_flash_data = be32_to_cpu(mdev->devinfo.function);
+ /* Need to count how many bits are set - to find out which
+ * function_data element has details of the memory card:
+ * using Brian Kernighan's/Peter Wegner's method */
+ for (c = 0; test_flash_data; c++)
+ test_flash_data &= test_flash_data - 1;
+
+ basic_flash_data = be32_to_cpu(mdev->devinfo.function_data[c - 1]);
+
+ card = kmalloc(sizeof(struct memcard), GFP_KERNEL);
+ if (!card) {
+ error = ENOMEM;
+ goto fail_nomem;
+ }
+
+ card->partitions = ((basic_flash_data >> 24) & 0xFF) + 1;
+ card->blocklen = (((basic_flash_data >> 16) & 0xFF) + 1) << 5;
+ card->writecnt = (basic_flash_data >> 12) & 0xF;
+ card->readcnt = (basic_flash_data >> 8) & 0xF;
+ card->removeable = (basic_flash_data >> 7) & 1;
+
+ card->partition = 0;
+ /* Not sure there are actually any multi-partition devices in the
+ * real world, but the hardware supports them, so, so will we */
+ card->parts = kmalloc(sizeof(struct vmupart) * card->partitions,
+ GFP_KERNEL);
+ if (!card->parts) {
+ error = -ENOMEM;
+ goto fail_partitions;
+ }
+ /*kzalloc this to ensure safe kfree-ing of NULL mparts on error*/
+ card->mtd = kzalloc(sizeof(struct mtd_info) * card->partitions,
+ GFP_KERNEL);
+ if (!card->mtd) {
+ error = -ENOMEM;
+ goto fail_mtd_info;
+ }
+
+ mdev->private_data = card;
+
+ /* Now query the device to find out about blocks */
+ mdev->mq->command = MAPLE_COMMAND_GETMINFO;
+ mdev->mq->length = 2;
+
+ sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
+ if (!sendbuf) {
+ error = -ENOMEM;
+ goto fail_nosendbuf;
+ }
+ card->sendbuf = sendbuf;
+ mdev->mq->sendbuf = sendbuf;
+
+ ((unsigned long *)(mdev->mq->sendbuf))[0] =
+ cpu_to_be32(MAPLE_FUNC_MEMCARD);
+
+ maple_getcond_callback(mdev, vmu_queryblocks, 0,
+ MAPLE_FUNC_MEMCARD);
+
+ locking = mutex_lock_interruptible(&(mdev->mq->mutex));
+ if (!locking)
+ maple_add_packet(mdev->mq);
+
+ return 0;
+
+fail_nosendbuf:
+ kfree(card->mtd);
+fail_mtd_info:
+ kfree(card->parts);
+fail_partitions:
+ kfree(card);
+fail_nomem:
+ return -error;
+}
+
+static void vmu_disconnect(struct maple_device *mdev)
+{
+ struct memcard *card;
+ int x, locking;
+
+ /* Seek lock to ensure smooth removal */
+ locking = mutex_lock_interruptible(&mdev->mq->mutex);
+ if (locking) {
+ printk(KERN_INFO "Maple: Could not disconnect VMU device at:"
+ "(%d, %d)\n", mdev->port, mdev->unit);
+ return;
+ }
+ mdev->callback = NULL;
+ card = mdev->private_data;
+ for (x = 0; x < card->partitions; x++) {
+ del_mtd_device(&((card->mtd)[x]));
+ kfree(((card->parts)[x]).name);
+ }
+ kfree(card->parts);
+ kfree(card->mtd);
+ kfree(card);
+ mutex_unlock(&mdev->mq->mutex);
+
+}
+
+static int probe_maple_vmu(struct device *dev)
+{
+ int error;
+ struct maple_device *mdev = to_maple_dev(dev);
+ struct maple_driver *mdrv = to_maple_driver(dev->driver);
+
+ error = vmu_connect(mdev);
+ if (error)
+ return error;
+
+ mdev->driver = mdrv;
+
+ return 0;
+}
+
+static int remove_maple_vmu(struct device *dev)
+{
+ struct maple_device *mdev = to_maple_dev(dev);
+
+ vmu_disconnect(mdev);
+ return 0;
+}
+
+static struct maple_driver vmu_flash_driver = {
+ .function = MAPLE_FUNC_MEMCARD,
+ .connect = vmu_connect,
+ .disconnect = vmu_disconnect,
+ .drv = {
+ .name = "Dreamcast_visual_memory",
+ .probe = probe_maple_vmu,
+ .remove = remove_maple_vmu,
+ },
+};
+
+static int unplug_vmu_flash(struct device *dev, void *ignored)
+{
+ struct maple_device *mdev;
+
+ mdev = to_maple_dev(dev);
+ if ((mdev->function & MAPLE_FUNC_MEMCARD)
+ && (mdev->driver == &vmu_flash_driver))
+ remove_maple_vmu(dev);
+
+ return 0;
+}
+
+static int __init vmu_flash_map_init(void)
+{
+ maple_driver_register(&vmu_flash_driver.drv);
+ return 0;
+}
+
+static void __exit vmu_flash_map_exit(void)
+{
+ bus_for_each_dev(&maple_bus_type, NULL, NULL, unplug_vmu_flash);
+ driver_unregister(&vmu_flash_driver.drv);
+}
+
+module_init(vmu_flash_map_init);
+module_exit(vmu_flash_map_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adrian McMenamin");
+MODULE_DESCRIPTION("Flash mapping for Sega Dreamcast visual memory");
Changes to allow support of the VMU as well as clean up some of the
horrors that got imported in with the rewrite of the 2.4 driver.
- Adds a mutex to allow packets (eg block reads and writes) to be
injected into the waiting queue with any risk of data corruption.
- Removes unneeded locking of queue of processed packets while properly
locking access to the queue of waiting packets
- Allows proper probing and removal of device drivers rather than
force majure approach based on function supported
- Separate out "rescan" function to make the code easier to read
and maintain.
- properly initialise and clean up waiting and sent queues
Signed-off-by: Adrian McMenamin <[email protected]>
---
diff -ruN a/include/linux/maple.h b/include/linux/maple.h
--- a/include/linux/maple.h 2008-03-22 17:26:48.000000000 +0000
+++ b/include/linux/maple.h 2008-03-22 17:21:03.000000000 +0000
@@ -33,6 +33,7 @@
void *sendbuf, *recvbuf, *recvbufdcsp;
unsigned char length;
enum maple_code command;
+ struct mutex mutex;
};
struct maple_devinfo {
diff -ruN a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
--- a/drivers/sh/maple/maple.c 2008-03-22 17:26:31.000000000 +0000
+++ b/drivers/sh/maple/maple.c 2008-03-22 17:53:50.000000000 +0000
@@ -46,14 +46,15 @@
static LIST_HEAD(maple_waitq);
static LIST_HEAD(maple_sentq);
-static DEFINE_MUTEX(maple_list_lock);
+/* mutex to protect queue of waiting packets */
+static DEFINE_MUTEX(maple_wlist_lock);
static struct maple_driver maple_dummy_driver;
static struct device maple_bus;
static int subdevice_map[MAPLE_PORTS];
static unsigned long *maple_sendbuf, *maple_sendptr, *maple_lastptr;
static unsigned long maple_pnp_time;
-static int started, scanning, liststatus, fullscan;
+static int started, scanning, fullscan;
static struct kmem_cache *maple_queue_cache;
struct maple_device_specify {
@@ -135,9 +136,9 @@
*/
void maple_add_packet(struct mapleq *mq)
{
- mutex_lock(&maple_list_lock);
+ mutex_lock(&maple_wlist_lock);
list_add(&mq->list, &maple_waitq);
- mutex_unlock(&maple_list_lock);
+ mutex_unlock(&maple_wlist_lock);
}
EXPORT_SYMBOL_GPL(maple_add_packet);
@@ -147,17 +148,26 @@
mq = kmalloc(sizeof(*mq), GFP_KERNEL);
if (!mq)
- return NULL;
+ goto failed_nomem;
mq->dev = mdev;
mq->recvbufdcsp = kmem_cache_zalloc(maple_queue_cache, GFP_KERNEL);
mq->recvbuf = (void *) P2SEGADDR(mq->recvbufdcsp);
- if (!mq->recvbuf) {
- kfree(mq);
- return NULL;
- }
+ if (!mq->recvbuf)
+ goto failed_p2;
+ /***
+ * most devices do not need the mutex - but
+ * anything that injects block reads or writes
+ * will rely on it
+ */
+ mutex_init(&mq->mutex);
return mq;
+
+failed_p2:
+ kfree(mq);
+failed_nomem:
+ return NULL;
}
static struct maple_device *maple_alloc_dev(int port, int unit)
@@ -178,7 +188,6 @@
}
mdev->dev.bus = &maple_bus_type;
mdev->dev.parent = &maple_bus;
- mdev->function = 0;
return mdev;
}
@@ -216,7 +225,6 @@
*maple_sendptr++ = PHYSADDR(mq->recvbuf);
*maple_sendptr++ =
mq->command | (to << 8) | (from << 16) | (len << 24);
-
while (len-- > 0)
*maple_sendptr++ = *lsendbuf++;
}
@@ -230,16 +238,22 @@
if (!list_empty(&maple_sentq))
return;
- if (list_empty(&maple_waitq) || !maple_dma_done())
+ mutex_lock(&maple_wlist_lock);
+ if (list_empty(&maple_waitq) || !maple_dma_done()) {
+ mutex_unlock(&maple_wlist_lock);
return;
+ }
+ mutex_unlock(&maple_wlist_lock);
maple_packets = 0;
maple_sendptr = maple_lastptr = maple_sendbuf;
+ mutex_lock(&maple_wlist_lock);
list_for_each_entry_safe(mq, nmq, &maple_waitq, list) {
maple_build_block(mq);
list_move(&mq->list, &maple_sentq);
if (maple_packets++ > MAPLE_MAXPACKETS)
break;
}
+ mutex_unlock(&maple_wlist_lock);
if (maple_packets > 0) {
for (i = 0; i < (1 << MAPLE_DMA_PAGES); i++)
dma_cache_sync(0, maple_sendbuf + i * PAGE_SIZE,
@@ -247,7 +261,8 @@
}
}
-static int attach_matching_maple_driver(struct device_driver *driver,
+/* check if there is a driver registered likely to match this device */
+static int check_matching_maple_driver(struct device_driver *driver,
void *devptr)
{
struct maple_driver *maple_drv;
@@ -255,12 +270,8 @@
mdev = devptr;
maple_drv = to_maple_driver(driver);
- if (mdev->devinfo.function & be32_to_cpu(maple_drv->function)) {
- if (maple_drv->connect(mdev) == 0) {
- mdev->driver = maple_drv;
- return 1;
- }
- }
+ if (mdev->devinfo.function & cpu_to_be32(maple_drv->function))
+ return 1;
return 0;
}
@@ -268,11 +279,6 @@
{
if (!mdev)
return;
- if (mdev->driver) {
- if (mdev->driver->disconnect)
- mdev->driver->disconnect(mdev);
- }
- mdev->driver = NULL;
device_unregister(&mdev->dev);
mdev = NULL;
}
@@ -328,8 +334,8 @@
mdev->port, mdev->unit, function);
matched =
- bus_for_each_drv(&maple_bus_type, NULL, mdev,
- attach_matching_maple_driver);
+ bus_for_each_drv(&maple_bus_type, NULL, mdev,
+ check_matching_maple_driver);
if (matched == 0) {
/* Driver does not exist yet */
@@ -377,53 +383,62 @@
if ((maple_dev->interval > 0)
&& time_after(jiffies, maple_dev->when)) {
+ /***
+ * Can only queue one command per device at a time
+ * so if cannot aquire the mutex, just return
+ */
+ if (!mutex_trylock(&maple_dev->mq->mutex))
+ goto setup_finished;
maple_dev->when = jiffies + maple_dev->interval;
maple_dev->mq->command = MAPLE_COMMAND_GETCOND;
maple_dev->mq->sendbuf = &maple_dev->function;
maple_dev->mq->length = 1;
maple_add_packet(maple_dev->mq);
- liststatus++;
} else {
if (time_after(jiffies, maple_pnp_time)) {
+ if (!mutex_trylock(&maple_dev->mq->mutex))
+ goto setup_finished;
maple_dev->mq->command = MAPLE_COMMAND_DEVINFO;
maple_dev->mq->length = 0;
maple_add_packet(maple_dev->mq);
- liststatus++;
}
}
-
+setup_finished:
return 0;
}
/* VBLANK bottom half - implemented via workqueue */
static void maple_vblank_handler(struct work_struct *work)
{
- if (!maple_dma_done())
- return;
if (!list_empty(&maple_sentq))
return;
+ if (!maple_dma_done())
+ return;
ctrl_outl(0, MAPLE_ENABLE);
- liststatus = 0;
bus_for_each_dev(&maple_bus_type, NULL, NULL,
setup_maple_commands);
if (time_after(jiffies, maple_pnp_time))
maple_pnp_time = jiffies + MAPLE_PNP_INTERVAL;
- if (liststatus && list_empty(&maple_sentq)) {
- INIT_LIST_HEAD(&maple_sentq);
+ mutex_lock(&maple_wlist_lock);
+ if (!list_empty(&maple_waitq) && list_empty(&maple_sentq)) {
+ mutex_unlock(&maple_wlist_lock);
maple_send();
+ } else {
+ mutex_unlock(&maple_wlist_lock);
}
+
maplebus_dma_reset();
}
/* handle devices added via hotplugs - placing them on queue for DEVINFO*/
static void maple_map_subunits(struct maple_device *mdev, int submask)
{
- int retval, k, devcheck;
+ int retval, k, devcheck, locking;
struct maple_device *mdev_add;
struct maple_device_specify ds;
+ ds.port = mdev->port;
for (k = 0; k < 5; k++) {
- ds.port = mdev->port;
ds.unit = k + 1;
retval =
bus_for_each_dev(&maple_bus_type, NULL, &ds,
@@ -437,9 +452,15 @@
mdev_add = maple_alloc_dev(mdev->port, k + 1);
if (!mdev_add)
return;
+ /* Use trylock again to avoid corrupting
+ * any already queued commands */
+ locking = mutex_trylock(&mdev_add->mq->mutex);
+ if (locking == 0)
+ return;
mdev_add->mq->command = MAPLE_COMMAND_DEVINFO;
mdev_add->mq->length = 0;
maple_add_packet(mdev_add->mq);
+ /* mark that we are checking sub devices */
scanning = 1;
}
submask = submask >> 1;
@@ -505,6 +526,35 @@
}
}
+static void maple_port_rescan(void)
+{
+ int i, locking;
+ struct maple_device *mdev;
+
+ fullscan = 1;
+ for (i = 0; i < MAPLE_PORTS; i++) {
+ if (checked[i] == false) {
+ fullscan = 0;
+ mdev = baseunits[i];
+ /**
+ * Use trylock in case scan has failed
+ * but device is still locked
+ */
+ locking = mutex_trylock(&mdev->mq->mutex);
+ if (locking == 0) {
+ mutex_unlock(&mdev->mq->mutex);
+ locking = mutex_lock_interruptible
+ (&mdev->mq->mutex);
+ if (locking)
+ continue;
+ }
+ mdev->mq->command = MAPLE_COMMAND_DEVINFO;
+ mdev->mq->length = 0;
+ maple_add_packet(mdev->mq);
+ }
+ }
+}
+
/* maple dma end bottom half - implemented via workqueue */
static void maple_dma_handler(struct work_struct *work)
{
@@ -512,7 +562,6 @@
struct maple_device *dev;
char *recvbuf;
enum maple_code code;
- int i;
if (!maple_dma_done())
return;
@@ -521,7 +570,10 @@
list_for_each_entry_safe(mq, nmq, &maple_sentq, list) {
recvbuf = mq->recvbuf;
code = recvbuf[0];
+ if (mutex_is_locked(&mq->mutex))
+ mutex_unlock(&mq->mutex);
dev = mq->dev;
+ list_del_init(&mq->list);
switch (code) {
case MAPLE_RESPONSE_NONE:
maple_response_none(dev, mq);
@@ -558,26 +610,16 @@
break;
}
}
- INIT_LIST_HEAD(&maple_sentq);
+ /* if scanning is 1 then we have subdevices to check */
if (scanning == 1) {
maple_send();
scanning = 2;
} else
scanning = 0;
-
- if (!fullscan) {
- fullscan = 1;
- for (i = 0; i < MAPLE_PORTS; i++) {
- if (checked[i] == false) {
- fullscan = 0;
- dev = baseunits[i];
- dev->mq->command =
- MAPLE_COMMAND_DEVINFO;
- dev->mq->length = 0;
- maple_add_packet(dev->mq);
- }
- }
- }
+ /*check if we have actually tested all ports yet */
+ if (!fullscan)
+ maple_port_rescan();
+ /* mark that we have been through the first scan */
if (started == 0)
started = 1;
}
@@ -631,7 +673,7 @@
if (maple_dev->devinfo.function == 0xFFFFFFFF)
return 0;
else if (maple_dev->devinfo.function &
- be32_to_cpu(maple_drv->function))
+ cpu_to_be32(maple_drv->function))
return 1;
return 0;
}
@@ -713,14 +755,19 @@
if (!maple_queue_cache)
goto cleanup_bothirqs;
+ INIT_LIST_HEAD(&maple_waitq);
+ INIT_LIST_HEAD(&maple_sentq);
+
/* setup maple ports */
for (i = 0; i < MAPLE_PORTS; i++) {
checked[i] = false;
mdev[i] = maple_alloc_dev(i, 0);
baseunits[i] = mdev[i];
- if (!mdev[i]) {
- while (i-- > 0)
+ if (!mdev[i] || mutex_lock_interruptible(&mdev[i]->mq->mutex)) {
+ while (i-- > 0) {
+ mutex_unlock(&mdev[i]->mq->mutex);
maple_free_dev(mdev[i]);
+ }
goto cleanup_cache;
}
mdev[i]->mq->command = MAPLE_COMMAND_DEVINFO;
On Sat, 22 March 2008 18:03:55 +0000, Adrian McMenamin wrote:
>
> The SEGA Visual Memory Unit includes 128k of flash memory
> which can be read in blocks.
>
> The hardware specification is also capable of supporting
> partitions (though it is doubtful such devices exist).
>
> This driver supports block reads and writes, as well as
> queries of hardware capabilities, through the maple
> bus susbsystem.
>
> (It also implements a caching system so that, for instance, a
> read of 60 bytes will take 1/60th - or 1/50th in PAL regions -
> of a second instead of a second or 1.2 seconds.)
Without a doubt, buffering is useful. However I question how useful it
is to implement this in individual device drivers instead of once in
mtd_core.c.
> Signed-off-by: Adrian McMenamin <[email protected]>
Given that you have ignored most of my previous comments, NAK. I don't
mind merging code that isn't up to our standards yet. But I have a bad
feeling about a maintainer that does not understand review comments.
Since you had similar problems understanding Andrew, part of the blame
may sit on your side.
Jörn
--
The key to performance is elegance, not battalions of special cases.
-- Jon Bentley and Doug McIlroy
On Sat, 2008-03-22 at 19:32 +0100, Jörn Engel wrote:
> On Sat, 22 March 2008 18:03:55 +0000, Adrian McMenamin wrote:
> >
> Without a doubt, buffering is useful. However I question how useful it
> is to implement this in individual device drivers instead of once in
> mtd_core.c.
Well, it's obviously useful to this device. Are you making best the
enemy of better?
>
> Given that you have ignored most of my previous comments, NAK.
Not good enough, frankly. What comments have I ignored? I didn't
implement your suggestion that a void* memory type become be32* because
it was totally inappropriate for a type that is passed in both be32 and
le32 and as u8.
> I don't
> mind merging code that isn't up to our standards yet. But I have a bad
> feeling about a maintainer that does not understand review comments.
> Since you had similar problems understanding Andrew, part of the blame
> may sit on your side.
>
I'm sorry that you feel that way, but as you took the hump when I said
that this:
"Possibly the big-endian annotations need to trickly though the layers
here as well."
Isn't good english (and it's not) and asked you - twice - to explain
what you meant. I cannot accept your summary.
Adrian
On Sat, 22 March 2008 18:39:07 +0000, Adrian McMenamin wrote:
> On Sat, 2008-03-22 at 19:32 +0100, Jörn Engel wrote:
>
> > Given that you have ignored most of my previous comments, NAK.
>
> Not good enough, frankly. What comments have I ignored? I didn't
> implement your suggestion that a void* memory type become be32* because
> it was totally inappropriate for a type that is passed in both be32 and
> le32 and as u8.
Your argument would make sense if referring to a transport layer where
some structure is just passed along as payload data. But you are
dereferencing a structure. And instead of declaring a structure, you
still do this:
((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
If that is a valid description of "appropriate" in your dictionary then
we clearly don't speak the same language. Even your own suggestion to
change the cast to (u32*) would have been an improvement.
> I'm sorry that you feel that way, but as you took the hump when I said
> that this:
>
> "Possibly the big-endian annotations need to trickly though the layers
> here as well."
>
> Isn't good english (and it's not) and asked you - twice - to explain
> what you meant. I cannot accept your summary.
s/trickly/trickle/
I tried to help you by a) reviewing the code and b) even converting the
function myself after deciding that you don't understand what I meant.
Clearly this didn't work. And maybe the problem is me, either because
my English is insufficient or I am a d***head. Quite possible.
But even if I am the problem, you should be able to understand
somebody's review comments and act accordingly. If not, I have a really
bad feeling about this. And I hope someone else has more patience than
me, because this code still needs work.
Jörn
--
It is better to die of hunger having lived without grief and fear,
than to live with a troubled spirit amid abundance.
-- Epictetus
On Sat, Mar 22, 2008 at 06:39:07PM +0000, Adrian McMenamin wrote:
> On Sat, 2008-03-22 at 19:32 +0100, J??rn Engel wrote:
> > Without a doubt, buffering is useful. However I question how useful it
> > is to implement this in individual device drivers instead of once in
> > mtd_core.c.
>
> Well, it's obviously useful to this device. Are you making best the
> enemy of better?
>
Adrian, relax. These are helpful comments, and you should be taking them
under consideration even if you have no immediate plans to implement them
in the current driver.
Buffering is useful for this device, but the suggestion is that it's a
useful capability to have for other parts also, and so it should be done
in the generic layer. Doing this requires a bit more work on your part,
but it's in no way removing the ability to do buffering in the VMU MTD
driver (the logic is just moved up a level). You seem to be implying
that the options are having buffering in the driver as it is today, or
not at all, which is a position that doesn't even factor in the initial
comment. This is not helpful.
While it's certainly possible to do all of your work off in your own
corner, it tends to be detrimental to the subsystem if you have stuff
that can and should be done generically. Likewise, it's a give and take,
if you can't be bothered trying to do this incrementally in the subsystem
itself now, there's not much motivation for you to do so once the driver
has been merged either.
> > Given that you have ignored most of my previous comments, NAK.
>
> Not good enough, frankly. What comments have I ignored? I didn't
> implement your suggestion that a void* memory type become be32* because
> it was totally inappropriate for a type that is passed in both be32 and
> le32 and as u8.
>
While I haven't seen the previous comments, the implication seems to be
that there are more than one outstanding.
> > I don't mind merging code that isn't up to our standards yet. But I
> > have a bad feeling about a maintainer that does not understand
> > review comments. Since you had similar problems understanding
> > Andrew, part of the blame may sit on your side.
>
> I'm sorry that you feel that way, but as you took the hump when I said
> that this:
>
> "Possibly the big-endian annotations need to trickly though the layers
> here as well."
>
> Isn't good english (and it's not) and asked you - twice - to explain
> what you meant. I cannot accept your summary.
>
Just as there's no motivation for others to accept merging your code. You
really need to work on handling feedback in a less oppositional way if
you ever want to have your changes merged.
As far as "good" english goes, this is l-k, hang your degree at the
door and get off your high horse. Even the native speakers perform
horrendous atrocities against the english language, that's life, deal
with it. In technical discussions this doesn't tend to matter at all, as
long as the meaning is obvious -- which in this case it certainly seems
to be. If you're speaking a different language, perhaps it's a
miscommunication or misinterpretation of technical terms rather than
anything else. Do not automatically assume the blame lies with people who
are trying to help you.
On Sat, Mar 22, 2008 at 07:56:01PM +0100, J??rn Engel wrote:
> Your argument would make sense if referring to a transport layer where
> some structure is just passed along as payload data. But you are
> dereferencing a structure. And instead of declaring a structure, you
> still do this:
> ((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
> ((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
>
> If that is a valid description of "appropriate" in your dictionary then
> we clearly don't speak the same language. Even your own suggestion to
> change the cast to (u32*) would have been an improvement.
>
Agreed, this stuff is very much a mess, and making the structure more
explicit would help quite a bit in terms of readability. This is legacy
stuff that's been hanging around for close to a decade now, it would be
nice to see this cleaned up and looking more like code.
> I tried to help you by a) reviewing the code and b) even converting the
> function myself after deciding that you don't understand what I meant.
> Clearly this didn't work. And maybe the problem is me, either because
> my English is insufficient or I am a d***head. Quite possible.
>
> But even if I am the problem, you should be able to understand
> somebody's review comments and act accordingly. If not, I have a really
> bad feeling about this. And I hope someone else has more patience than
> me, because this code still needs work.
>
I've hit the same issues with Adrian's patches in the past, but I've
weighed this against a couple of factors:
- Is it reasonably isolated.
- Does it break anything else.
- Is it more likely to get pointed at and fixed in-tree or out.
The merging of less-than-desirable code is certainly a factor here, but
it's incredibly unlikely that any of this code would get cleaned up if it
hadn't been merged. This has certainly been the case for maple, and the
related peripherals. It still has a long way to go, but it's in much
better shape these days. Adrian has done some good work in this area,
even if his bizarre oppositional development model flies in the face of
all conventional wisdom. It's slow progress, but I wouldn't have merged
the maple stuff at all if it didn't seem liks the pros outweighed the
cons. Most of the rest of us that were working in this area 7-8 years ago
have neither the time or inclination to do anything with it today, so
without these bits getting merged, it's unlikely they ever will. The fact
there's still someone hacking on this stuff today is a good indicator
that he'll be around in the future to keep it running also, hopefully
figuring out how to work with subsystem maintainers and folks doing
thankless code review in the process.
With that said, the current code obviously needs quite a bit of work
before it can be merged. I'm obviously coming in to this thread late, so
it would be helpful to have a pointer to the initial review comments. The
comments made by Andrew and myself on the maple bits I assume are being
worked on and will be respun at some point. I'll attempt to work with
Adrian on getting the rest of these issues addressed.
On Sat, Mar 22, 2008 at 05:56:32PM +0000, Adrian McMenamin wrote:
> Signed-off-by: Adrian McMenamin <[email protected]>
> ---
> diff -ruN a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> --- a/drivers/mtd/maps/Kconfig 2008-03-22 17:26:24.000000000 +0000
> +++ b/drivers/mtd/maps/Kconfig 2008-03-22 17:20:45.000000000 +0000
> @@ -590,5 +590,19 @@
>
> This selection automatically selects the map_ram driver.
>
> +config MTD_VMU_FLASH
> + tristate "Dreamcast Visual Memory devices and clones"
> + depends on SH_DREAMCAST
> + select MAPLE
> + help
> + Flash map driver for the SEGA Dreamcast Visual Memory Unit
> + and clones.
> +
> + Most Dreamcast users will have one of these and so will want
> + to say Y here.
> +
> + To compile this driver as a module choose M here: the module
> + will be called vmu-flash.
> +
> endmenu
>
1/3 and 2/3 should be merged, they're completely tied to each other and
are useless on their own.
On Sat, Mar 22, 2008 at 06:16:26PM +0000, Adrian McMenamin wrote:
> Changes to allow support of the VMU as well as clean up some of the
> horrors that got imported in with the rewrite of the 2.4 driver.
>
> - Adds a mutex to allow packets (eg block reads and writes) to be
> injected into the waiting queue with any risk of data corruption.
>
> - Removes unneeded locking of queue of processed packets while properly
> locking access to the queue of waiting packets
>
> - Allows proper probing and removal of device drivers rather than
> force majure approach based on function supported
>
> - Separate out "rescan" function to make the code easier to read
> and maintain.
>
> - properly initialise and clean up waiting and sent queues
>
> Signed-off-by: Adrian McMenamin <[email protected]>
>
Getting better, though still some issues..
> + if (!mq->recvbuf)
> + goto failed_p2;
> + /***
> + * most devices do not need the mutex - but
> + * anything that injects block reads or writes
> + * will rely on it
> + */
> + mutex_init(&mq->mutex);
>
> return mq;
The commenting style here is highly irregular.
> @@ -377,53 +383,62 @@
>
> if ((maple_dev->interval > 0)
> && time_after(jiffies, maple_dev->when)) {
> + /***
> + * Can only queue one command per device at a time
> + * so if cannot aquire the mutex, just return
> + */
> + if (!mutex_trylock(&maple_dev->mq->mutex))
> + goto setup_finished;
Here also. Not only is the commenting weird, so is the indentation.
Also, unrelated to this patch, why is the && on the left of the second
line instead of the right of the first one? Please do not invent new
styles.
> + /* Use trylock again to avoid corrupting
> + * any already queued commands */
This one is almost sane.
> + locking = mutex_trylock(&mdev_add->mq->mutex);
> + if (locking == 0)
> + return;
The variable here is pretty pointless, since you don't really use it for
anything.
> + fullscan = 1;
> + for (i = 0; i < MAPLE_PORTS; i++) {
> + if (checked[i] == false) {
> + fullscan = 0;
> + mdev = baseunits[i];
> + /**
> + * Use trylock in case scan has failed
> + * but device is still locked
> + */
> + locking = mutex_trylock(&mdev->mq->mutex);
> + if (locking == 0) {
> + mutex_unlock(&mdev->mq->mutex);
> + locking = mutex_lock_interruptible
> + (&mdev->mq->mutex);
> + if (locking)
> + continue;
> + }
So here we have the same issue as in the previous patch, but with the
mutex API instead. The entire point of adding a comment for clarity is
that it becomes obvious what this is trying to accomplish, which it still
isn't. Maple shouldn't require a supplemental document to detail its
locking strategy in a way that doesn't induce blindness.
The mutex_unlock() here looks very suspicious. You first try to grab the
lock via the trylock, if it's contended, then you try to unlock it and
then grab it again for yourself before continuing on. This sort of
juggling looks really racy. Under what conditions will this lock be
contended, and under what conditions is it released? If you have a
transfer in place, you contend on the lock, and then this code suddenly
unlocks, what happens to your queue? It seems like you are trying to lock
down the mq for the duration of its lifetime, in addition to having a
separate list lock for guarding against the list getting mangled from
underneath you.
It looks like you are trying to roll your own complex queuing mechanism
in a fairly non-obvious fashion. Have you considered using things like
the block layer qeueing for dealing with a lot of this for you? This is
what we ended up using for OMAP mailboxes and it worked out pretty well
(arch/arm/plat-omap/mailbox.c) there.
This sort of obscure locking is going to cause you nothing but trouble in
the long run.
On Mon, 24 March 2008 11:08:32 +0900, Paul Mundt wrote:
>
> I've hit the same issues with Adrian's patches in the past, but I've
> weighed this against a couple of factors:
>
> - Is it reasonably isolated.
> - Does it break anything else.
> - Is it more likely to get pointed at and fixed in-tree or out.
I tend to agree with one condition: There needs to be a prospect of
improvement. My two preferred variants would be one person doing
everything or two people, one improving the code and the other testing
on hardware to detect and prevent regressions.
For a moment I was contemplating just taking the mtd driver and doing
the work myself. But two things stopped me short. For one, I didn't
find the referenced header anywhere. Secondly I lack any hardware to
test the result.
But more to the point, I cannot come to terms with Adrian. So either
one of us has to be replaced - and I cannot see a lot of volunteers - or
we are currently lacking the prospect of improvement. Hence my bad
feeling about this code.
Hopefully I just caught Adrian on the wrong foot.
> With that said, the current code obviously needs quite a bit of work
> before it can be merged. I'm obviously coming in to this thread late, so
> it would be helpful to have a pointer to the initial review comments.
Thread starts here:
http://lists.infradead.org/pipermail/linux-mtd/2008-March/020880.html
Jörn
--
Those who come seeking peace without a treaty are plotting.
-- Sun Tzu
[Recipient list cut somewhat]
On Mon, 2008-03-24 at 11:08 +0900, Paul Mundt wrote:
> On Sat, Mar 22, 2008 at 06:39:07PM +0000, Adrian McMenamin wrote:
> > On Sat, 2008-03-22 at 19:32 +0100, J??rn Engel wrote:
> > > I don't mind merging code that isn't up to our standards yet. But I
> > > have a bad feeling about a maintainer that does not understand
> > > review comments. Since you had similar problems understanding
> > > Andrew, part of the blame may sit on your side.
> >
> > I'm sorry that you feel that way, but as you took the hump when I said
> > that this:
> >
> > "Possibly the big-endian annotations need to trickly though the layers
> > here as well."
> >
> > Isn't good english (and it's not) and asked you - twice - to explain
> > what you meant. I cannot accept your summary.
> >
> Just as there's no motivation for others to accept merging your code. You
> really need to work on handling feedback in a less oppositional way if
> you ever want to have your changes merged.
>
> As far as "good" english goes, this is l-k, hang your degree at the
> door and get off your high horse. Even the native speakers perform
> horrendous atrocities against the english language, that's life, deal
> with it. In technical discussions this doesn't tend to matter at all, as
> long as the meaning is obvious -- which in this case it certainly seems
> to be. If you're speaking a different language, perhaps it's a
> miscommunication or misinterpretation of technical terms rather than
> anything else. Do not automatically assume the blame lies with people who
> are trying to help you.
I have no desire to have a flame war over this. But I didn't understand
the sentence, was told twice that I should act on it and asked twice
simply what it meant. Eg the second time...
"I'm sorry, but the comment above <snap> isn't good english. What do you
mean?"
Not unreasonable I think. After the second time I was told all
communication would cease. And, yes, that did annoy me, especially when
the next thing I get is someone telling me I didn't listen to them.
I certainly don't expect people to be flawless speakers or writers of
English. But I don't think it is wrong to ask for clarity when something
that doesn't make sense is written.
Bluntly I almost always think the blame lies with me when people point
out mistakes or imperfections in my code - because it usually is. But I
don't think it is unreasonable to ask what people mean.
On Mon, 24 March 2008 12:06:24 +0000, Adrian McMenamin wrote:
>
> I have no desire to have a flame war over this. But I didn't understand
> the sentence, was told twice that I should act on it and asked twice
> simply what it meant. Eg the second time...
>
> "I'm sorry, but the comment above <snap> isn't good english. What do you
> mean?"
Just for reference, I had no bloody idea what the "<snap>" in your
sentence meant. Or what the comment as a whole was referring to. In
other words, we could't communicate.
Probably more important is the thing you didn't ask about. The cast is
- to use a techinical term - crap. And when someone ignores such
things, but instead makes a comment about my English language skills, my
natural response is to suspect a troll. Trolls tend to replace "I don't
understand <foo>" with "you are an idiot" in some disguise like a
comment about hairstyle or language skills.
You ignored other things as well. In general, the things you ignored
are a lot more important than the things you didn't ignore. Which again
is a good recipe for fruitless discussions, if not flamewars. As is a
metadiscussion about previous discussions - like this one.
> Not unreasonable I think. After the second time I was told all
> communication would cease. And, yes, that did annoy me, especially when
> the next thing I get is someone telling me I didn't listen to them.
Then maybe we can end the digressions and get back to the code and
improving it? I would welcome that. Would you mind if I sent you
patches against your code? Might be a way to avoid metadiscussions as
much as possible.
Jörn
--
Doubt is not a pleasant condition, but certainty is an absurd one.
-- Voltaire
On Mon, 2008-03-24 at 14:21 +0100, Jörn Engel wrote:
> Then maybe we can end the digressions and get back to the code and
> improving it? I would welcome that. Would you mind if I sent you
> patches against your code? Might be a way to avoid metadiscussions as
> much as possible.
>
Tempting as it is to continue the war, discretion will be the better
part of valour here and I will give you the last word.
Of course I don't mind you sending patches. Indeed it would be very
helpful and generous of you to do so.
On Mon, 24 March 2008 13:58:40 +0000, Adrian McMenamin wrote:
>
> Tempting as it is to continue the war, discretion will be the better
> part of valour here and I will give you the last word.
>
> Of course I don't mind you sending patches. Indeed it would be very
> helpful and generous of you to do so.
Good. The first four shouldn't change any behaviour. Number five flips
positive error returns into negative ones. I believe the old behaviour
is a bug. Worth a second look to make sure.
All five patches are attached. Hope that doesn't cause any problems.
Jörn
--
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare
On Mon, 2008-03-24 at 15:10 +0100, Jörn Engel wrote:
> On Mon, 24 March 2008 13:58:40 +0000, Adrian McMenamin wrote:
> >
> > Tempting as it is to continue the war, discretion will be the better
> > part of valour here and I will give you the last word.
> >
> > Of course I don't mind you sending patches. Indeed it would be very
> > helpful and generous of you to do so.
>
> Good. The first four shouldn't change any behaviour. Number five flips
> positive error returns into negative ones. I believe the old behaviour
> is a bug. Worth a second look to make sure.
>
> All five patches are attached. Hope that doesn't cause any problems.
>
Well, I haven't got round to applying them and testing them yet, though
they look ok, obviously, except for one thing: I'm pretty much where you
are on the goto versus return thing, but got pretty clear instructions
on a previous patch from Andrew Morton that using gotos to ensure
functions limit the number of return points is the way to go.
I've added him back in now we aren't exchanging flames, so maybe he can
pronouce ex cathedra.
On Mon, Mar 24, 2008 at 02:43:16PM +0000, Adrian McMenamin wrote:
> On Mon, 2008-03-24 at 15:10 +0100, J??rn Engel wrote:
> > On Mon, 24 March 2008 13:58:40 +0000, Adrian McMenamin wrote:
> > >
> > > Tempting as it is to continue the war, discretion will be the better
> > > part of valour here and I will give you the last word.
> > >
> > > Of course I don't mind you sending patches. Indeed it would be very
> > > helpful and generous of you to do so.
> >
> > Good. The first four shouldn't change any behaviour. Number five flips
> > positive error returns into negative ones. I believe the old behaviour
> > is a bug. Worth a second look to make sure.
> >
> > All five patches are attached. Hope that doesn't cause any problems.
>
> Well, I haven't got round to applying them and testing them yet, though
> they look ok, obviously, except for one thing: I'm pretty much where you
> are on the goto versus return thing, but got pretty clear instructions
> on a previous patch from Andrew Morton that using gotos to ensure
> functions limit the number of return points is the way to go.
>
The gotos help clean up the error path when you have something to do. In
this case there's nothing going on but a NULL return, so it really
doesn't matter one way or the other. Most people tend to avoid the goto
if you really have nothing to do in an error path.
On Mon, 24 March 2008 12:33:44 +0900, Paul Mundt wrote:
>
> So here we have the same issue as in the previous patch, but with the
> mutex API instead. The entire point of adding a comment for clarity is
> that it becomes obvious what this is trying to accomplish, which it still
> isn't. Maple shouldn't require a supplemental document to detail its
> locking strategy in a way that doesn't induce blindness.
>
> The mutex_unlock() here looks very suspicious. You first try to grab the
> lock via the trylock, if it's contended, then you try to unlock it and
> then grab it again for yourself before continuing on. This sort of
> juggling looks really racy. Under what conditions will this lock be
> contended, and under what conditions is it released? If you have a
> transfer in place, you contend on the lock, and then this code suddenly
> unlocks, what happens to your queue? It seems like you are trying to lock
> down the mq for the duration of its lifetime, in addition to having a
> separate list lock for guarding against the list getting mangled from
> underneath you.
>
> It looks like you are trying to roll your own complex queuing mechanism
> in a fairly non-obvious fashion. Have you considered using things like
> the block layer qeueing for dealing with a lot of this for you? This is
> what we ended up using for OMAP mailboxes and it worked out pretty well
> (arch/arm/plat-omap/mailbox.c) there.
>
> This sort of obscure locking is going to cause you nothing but trouble in
> the long run.
As a general rule, locks should be taken and released in the same
function. That allows the locking to be easily reviewed and errors
are easy to spot for most readers.
int foo(void)
{
int err;
err = bar();
if (err)
return err;
mutex_lock(&foo_lock);
err = baz();
if (err)
return err;
mutex_unlock(&foo_lock);
return bumble();
}
In the above example, one doesn't have to think twice, the bugfix
becomes almost mechanical. But as soon as locks are taken in one
function and released in another, things get hairy.
int foo(void)
{
int err;
err = bar();
if (err)
return err;
mutex_lock(&foo_lock);
err = baz();
/* baz implicitly unlocks the mutex */
if (err)
return err;
return bumble();
}
Even with the comment, one cannot be sure.
int baz(void)
{
int err;
err = bee();
if (err)
return err;
mutex_unlock(&foo_lock);
return boo();
}
One always has to check all functions involved. If bee() returns an
error, neither foo() nor baz() will unlock the mutex. And after
changing baz() to always unlock the mutex, one next needs to check all
callers whether any of them unlock on errors. Under some circumstances
those would have been correct before, but not after. Unless...
In short, verifying the locking is about the least pleasurable thing to
do once locks are taken and released in seperate functions.
Jörn
--
He who knows others is wise.
He who knows himself is enlightened.
-- Lao Tsu
On Mon, 24 March 2008 14:43:16 +0000, Adrian McMenamin wrote:
>
> Well, I haven't got round to applying them and testing them yet, though
> they look ok, obviously, except for one thing: I'm pretty much where you
> are on the goto versus return thing, but got pretty clear instructions
> on a previous patch from Andrew Morton that using gotos to ensure
> functions limit the number of return points is the way to go.
>
> I've added him back in now we aren't exchanging flames, so maybe he can
> pronouce ex cathedra.
It is a matter of personal taste. Having a single return statement is
nice when using a debugger. One can set a single breakpoint instead of
ten. I guess that is why Andrew prefers it.
I personally don't care much either way and usually pick whatever needs
fewer lines. Main purpose of the patch was to merge the error path with
the good path. In several functions you have
free_this;
free_that;
return 0;
error1:
free_this;
error2:
free_that;
error3:
return error;
Those paths are nearly identical and should be combined. As always,
duplicated code has a tendency to diverge, so in most cases the two
copies have subtle differences. So far I just tackled the easy cases.
Jörn
--
Joern's library part 8:
http://citeseer.ist.psu.edu/plank97tutorial.html
On Mon, 2008-03-24 at 15:46 +0100, Jörn Engel wrote:
> On Mon, 24 March 2008 12:33:44 +0900, Paul Mundt wrote:
> >
> > So here we have the same issue as in the previous patch, but with the
> > mutex API instead. The entire point of adding a comment for clarity is
> > that it becomes obvious what this is trying to accomplish, which it still
> > isn't. Maple shouldn't require a supplemental document to detail its
> > locking strategy in a way that doesn't induce blindness.
> >
> > The mutex_unlock() here looks very suspicious. You first try to grab the
> > lock via the trylock, if it's contended, then you try to unlock it and
> > then grab it again for yourself before continuing on. This sort of
> > juggling looks really racy. Under what conditions will this lock be
> > contended, and under what conditions is it released? If you have a
> > transfer in place, you contend on the lock, and then this code suddenly
> > unlocks, what happens to your queue? It seems like you are trying to lock
> > down the mq for the duration of its lifetime, in addition to having a
> > separate list lock for guarding against the list getting mangled from
> > underneath you.
It would be contended if the initial (on boot) scans of the ports has
failed. This seems to happen when the bus has a lot of devices attached
eg all four ports and some subdevices.
A status command is queued but not processed, and the result is the
device remains locked and can never be accessed.
Here we check whether the queued status command for that port has been
processed. Then trylock - if we take the lock then great, process away,
if we cannot then the unlock the previously taken redundant lock and
reacquire the lock.
OK, it could just use trylock and not give a stuff about the result -
because all we want is a locked queue...but that didn't seem right.
> >
> > It looks like you are trying to roll your own complex queuing mechanism
> > in a fairly non-obvious fashion. Have you considered using things like
> > the block layer qeueing for dealing with a lot of this for you? This is
> > what we ended up using for OMAP mailboxes and it worked out pretty well
> > (arch/arm/plat-omap/mailbox.c) there.
> >
> > This sort of obscure locking is going to cause you nothing but trouble in
> > the long run.
>
> As a general rule, locks should be taken and released in the same
> function. That allows the locking to be easily reviewed and errors
> are easy to spot for most readers.
>
> int foo(void)
> {
> int err;
>
> err = bar();
> if (err)
> return err;
> mutex_lock(&foo_lock);
> err = baz();
> if (err)
> return err;
> mutex_unlock(&foo_lock);
> return bumble();
> }
>
> In the above example, one doesn't have to think twice, the bugfix
> becomes almost mechanical. But as soon as locks are taken in one
> function and released in another, things get hairy.
>
> int foo(void)
> {
> int err;
>
> err = bar();
> if (err)
> return err;
> mutex_lock(&foo_lock);
> err = baz();
> /* baz implicitly unlocks the mutex */
> if (err)
> return err;
> return bumble();
> }
>
> Even with the comment, one cannot be sure.
>
> int baz(void)
> {
> int err;
>
> err = bee();
> if (err)
> return err;
> mutex_unlock(&foo_lock);
> return boo();
> }
>
> One always has to check all functions involved. If bee() returns an
> error, neither foo() nor baz() will unlock the mutex. And after
> changing baz() to always unlock the mutex, one next needs to check all
> callers whether any of them unlock on errors. Under some circumstances
> those would have been correct before, but not after. Unless...
>
> In short, verifying the locking is about the least pleasurable thing to
> do once locks are taken and released in seperate functions.
>
I don't think I have any alternative here. The problem is that the bus
checks periodically (every HZ ticks but that could be changed) if a
device remains connected to the system by queueing a status command. (If
the status command is replied to with a "No response" then that
indicates to the bus that the device has been removed.
But if there is already a command queued for that device (eg a block
read or a block write) then all that would happen is that the commands
get corrupted and an oops ensures.
If I lock the queue for each device then that cannot happen - the bus
driver bounces the status query.
Maple reads and writes are asynchronous of the command being queued - so
the locks are freed when the maple processing is done (ie in another
function). However there is a common unlock point for them all.
On Mon, 24 March 2008 15:06:18 +0000, Adrian McMenamin wrote:
>
> I don't think I have any alternative here. The problem is that the bus
> checks periodically (every HZ ticks but that could be changed) if a
> device remains connected to the system by queueing a status command. (If
> the status command is replied to with a "No response" then that
> indicates to the bus that the device has been removed.
Where does this queueing happen? Somewhere in the kernel where we can
control it or in hardware?
Jörn
--
I don't understand it. Nobody does.
-- Richard P. Feynman
On Mon, 2008-03-24 at 16:29 +0100, Jörn Engel wrote:
> On Mon, 24 March 2008 15:06:18 +0000, Adrian McMenamin wrote:
> >
> > I don't think I have any alternative here. The problem is that the bus
> > checks periodically (every HZ ticks but that could be changed) if a
> > device remains connected to the system by queueing a status command. (If
> > the status command is replied to with a "No response" then that
> > indicates to the bus that the device has been removed.
>
> Where does this queueing happen? Somewhere in the kernel where we can
> control it or in hardware?
Well, the queuing of the status command is under software control.
On Mon, 24 March 2008 15:51:40 +0000, Adrian McMenamin wrote:
> On Mon, 2008-03-24 at 16:29 +0100, Jörn Engel wrote:
> > On Mon, 24 March 2008 15:06:18 +0000, Adrian McMenamin wrote:
> > >
> > > I don't think I have any alternative here. The problem is that the bus
> > > checks periodically (every HZ ticks but that could be changed) if a
> > > device remains connected to the system by queueing a status command. (If
> > > the status command is replied to with a "No response" then that
> > > indicates to the bus that the device has been removed.
> >
> > Where does this queueing happen? Somewhere in the kernel where we can
> > control it or in hardware?
>
> Well, the queuing of the status command is under software control.
Then we should be fine. I'll try to beat the code into submission.
Jörn
--
Joern's library part 7:
http://www.usenix.org/publications/library/proceedings/neworl/full_papers/mckusick.a
On Mon, 24 March 2008 17:04:29 +0100, Jörn Engel wrote:
>
> Then we should be fine. I'll try to beat the code into submission.
And here go two more interesting patches. The first is removing all
locking from the mtd driver. Since the existing locking code is nearly
impossibly to verify, I'd rather have something simple and wrong than
something complicated and wrong.
The second rearranges the list locking a bit. Previously it was
possible to touch maple_waitq or maple_sentq without holding the lock.
With my limited understanding of the driver, the second patch may
already be enough to prevent the type of corruption you've been seeing.
Jörn
--
If a problem has a hardware solution, and a software solution,
do it in software.
-- Arnd Bergmann
On Mon, 2008-03-24 at 18:07 +0100, Jörn Engel wrote:
> On Mon, 24 March 2008 17:04:29 +0100, Jörn Engel wrote:
> >
> > Then we should be fine. I'll try to beat the code into submission.
>
> And here go two more interesting patches. The first is removing all
> locking from the mtd driver. Since the existing locking code is nearly
> impossibly to verify, I'd rather have something simple and wrong than
> something complicated and wrong.
>
> The second rearranges the list locking a bit. Previously it was
> possible to touch maple_waitq or maple_sentq without holding the lock.
> With my limited understanding of the driver, the second patch may
> already be enough to prevent the type of corruption you've been seeing.
>
> Jörn
>
These will fail.
Removing the locks just about guarantees memory corruption and
maple_waitq is not the issue - it is the objects held in maple_waitq
that are the issue.
I have an idea though, so let me hack at it
On Mon, 24 March 2008 18:07:07 +0100, Jörn Engel wrote:
>
> The second rearranges the list locking a bit. Previously it was
> possible to touch maple_waitq or maple_sentq without holding the lock.
> With my limited understanding of the driver, the second patch may
> already be enough to prevent the type of corruption you've been seeing.
Limited understanding indeed. The problem in the mtd driver is that it
has no concept of ownership. For example maple_vmu_read_block() does
the following:
mdev->mq->sendbuf = sendbuf;
...
maple_add_packet(mdev->mq);
It accesses some field in mdev->mq, then sends the structure off to
maple_add_packet(). From this point on, mdev->mq belongs to the bus
driver - until it calls the callback, vmu_blockread() in this case.
But a second thread can call into maple_vmu_read_block() again and
access mdev->mq while it rightfully belongs to the bus driver. Bad
things will happen.
So these two patches try to close the race windows. Please note the
FIXME in the write patch - I didn't do all the work. Real life calls
for attention, so these will be the last patches for a while.
Jörn
--
The only good bug is a dead bug.
-- Starship Troopers
On Mon, 2008-03-24 at 18:38 +0100, Jörn Engel wrote:
> On Mon, 24 March 2008 18:07:07 +0100, Jörn Engel wrote:
> >
> > The second rearranges the list locking a bit. Previously it was
> > possible to touch maple_waitq or maple_sentq without holding the lock.
> > With my limited understanding of the driver, the second patch may
> > already be enough to prevent the type of corruption you've been seeing.
>
> Limited understanding indeed. The problem in the mtd driver is that it
> has no concept of ownership. For example maple_vmu_read_block() does
> the following:
> mdev->mq->sendbuf = sendbuf;
> ...
> maple_add_packet(mdev->mq);
>
> It accesses some field in mdev->mq, then sends the structure off to
> maple_add_packet(). From this point on, mdev->mq belongs to the bus
> driver - until it calls the callback, vmu_blockread() in this case.
>
> But a second thread can call into maple_vmu_read_block() again and
> access mdev->mq while it rightfully belongs to the bus driver. Bad
> things will happen.
>
> So these two patches try to close the race windows. Please note the
> FIXME in the write patch - I didn't do all the work. Real life calls
> for attention, so these will be the last patches for a while.
>
> Jörn
>
The problem is that the hardware does not support another callback. In
any case while you are right that the function might be called as second
time, in the original code it will sleep while waiting for the lock,
which is allocated per device.
On the second patch - aiui completions do an uninterruptible wait, that
means they are surely not a good choice for this - especially as the
device could be unplugged at any time. (Admittedly my documentation
migght be of date here)
On Mon, 24 Mar 2008 15:54:33 +0100
J__rn Engel <[email protected]> wrote:
> On Mon, 24 March 2008 14:43:16 +0000, Adrian McMenamin wrote:
> >
> > Well, I haven't got round to applying them and testing them yet, though
> > they look ok, obviously, except for one thing: I'm pretty much where you
> > are on the goto versus return thing, but got pretty clear instructions
> > on a previous patch from Andrew Morton that using gotos to ensure
> > functions limit the number of return points is the way to go.
> >
> > I've added him back in now we aren't exchanging flames, so maybe he can
> > pronouce ex cathedra.
>
> It is a matter of personal taste. Having a single return statement is
> nice when using a debugger. One can set a single breakpoint instead of
> ten. I guess that is why Andrew prefers it.
Experience has shown that over time, the multiple-return-point approach
leads to locking errors, resource leaks and to much duplicated
unlocking/freeing code.
Because when people add new locking and more dynamic allocations to the function
they need to hunt down each `return' and add unlocking/freeing code to it (this
is bad). And sometimes they miss one (this is worse).
On Mon, 24 March 2008 17:55:51 +0000, Adrian McMenamin wrote:
>
> The problem is that the hardware does not support another callback. In
> any case while you are right that the function might be called as second
> time, in the original code it will sleep while waiting for the lock,
> which is allocated per device.
>
> On the second patch - aiui completions do an uninterruptible wait, that
> means they are surely not a good choice for this - especially as the
> device could be unplugged at any time. (Admittedly my documentation
> migght be of date here)
Neither of those two problem should be visible for the mtd driver. The
usual way how bus driver are implemented is something like this:
struct foo_request {
void *data;
loff_t ofs;
size_t len;
void (*complete)(struct foo_request);
void *private;
};
int do_request(struct foo_request *rq)
{
/* deal with hardware somehow */
rq->complete(rq);
}
With such infrastructure your mtd driver could look roughly like this:
static void read_complete(struct foo_request *rq)
{
complete(rq->private);
}
static int foo_read(struct mtd_info *mtd, loff_t ofs, size_t len, size_t *retlen, u_char *buf)
{
struct foo_dev = mtd->private;
struct foo_request *rq;
DECLARE_COMPLETION_ONSTACK(complete);
int err;
rq = kmalloc(sizeof(*rq), GFP_KERNEL);
if (!rq)
return -ENOMEM;
/* fill in appropriate values for rq->data, rq->ofs and rq->len */
rq->private = &complete;
rq->complete = read_complete;
err = do_request(rq);
if (err)
goto out;
wait_for_completion(&complete);
/* copy read data into buf, set retlen */
out:
kfree(rq);
return err;
}
Throw in a loop if you need several hardware accesses to fulfil large
reads. But that's it. You don't need any locking in the mtd driver at
all. Ownership is clearly defined. Between kmalloc() and do_request()
rq belongs to foo_read(). Between do_request() and the call to
read_complete() it belongs to the bus driver. After completion it
belongs to foo_read() again. No race window, no complicated locking.
If your hardware can only handle a finite number of requests at a time,
you can queue requests in do_request(). The caller should not know or
care about it - apart from declaring the completion variable and waiting
on it.
The goal of my patches was to move your code towards such a design.
Alas, my time is limited and my girlfriend already unhappy as it is -
you will have to continue without me. If in doubt, take a look at the
usb code and try to mimic that. drivers/mtd/nand/alauda.c uses usb to
talk to the hardware and roughly follows what I described above.
Jörn
--
Chance favors only the prepared mind.
-- Louis Pasteur
On Mon, 2008-03-24 at 15:10 +0100, Jörn Engel wrote:
> On Mon, 24 March 2008 13:58:40 +0000, Adrian McMenamin wrote:
> >
> > Tempting as it is to continue the war, discretion will be the better
> > part of valour here and I will give you the last word.
> >
> > Of course I don't mind you sending patches. Indeed it would be very
> > helpful and generous of you to do so.
>
> Good. The first four shouldn't change any behaviour. Number five flips
> positive error returns into negative ones. I believe the old behaviour
> is a bug. Worth a second look to make sure.
>
> All five patches are attached. Hope that doesn't cause any problems.
Just to keep you informed...
I have patched and tested four of these without any problem.
I haven't patched with the return/goto one as I think that needs more
work.
Other patches (the ones that have a chance of working) will be tested in
due course.