0/2 Add support for maple "Visual Memory Unit" on SEGA Dreamcast

This builds on (though I essentially rewrote most of it) a driver Paul Mundt and I wrote way back when to support the memory component of the SEGA Dreamcast's Visual Memory Unit.

The device is accessed via the Dreamcast's maple bus.

...

drivers/mtd/maps/vmu_flash.c new file +7 -0

+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;

drivers/mtd/maps/vmu_flash.c new file +30 -0

+	vblock->num = src_ofs / card->blocklen;
+
+	if (vblock->num > ((card->parts)[partition]).numblocks)
+		goto failed;
+
+	vblock->ofs = src_ofs % card->blocklen;
+	return vblock;
+
+failed:
+	kfree(vblock);
+simple_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;
+	card->blockread = kmalloc(card->blocklen, GFP_KERNEL);
+	if (!card->blockread)
+		return;

drivers/mtd/maps/vmu_flash.c new file +12 -0

+	pcache->jiffies_atc = jiffies;
+	pcache->valid = 1;
+	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;

drivers/mtd/maps/vmu_flash.c new file +15 -0

+	((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;
+
+	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);
+		goto fail_blockread;
+	}
+
+	memcpy(buf, card->blockread, card->blocklen);

drivers/mtd/maps/vmu_flash.c new file +24 -0

+/* 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;
+	}

drivers/mtd/maps/vmu_flash.c new file +22 -0

+	}
+	/* wait until command is processed */
+	locking = down_interruptible(&mdev->mq->sem);
+	if (locking) {
+		error = -EBUSY;
+		goto fail_nolock;
+	}
+	up(&mdev->mq->sem);
+
+	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);
+	return error;
+}
+

If this error is taken, vmu_flash_write() (at least) will not call maple_vmu_write_block() and hence mdev->mq->sem never gets up()ed. As far as I can tell.

Also, when this function is called from vmu_flash_read_char() I can't see where mdev->mq->sem gets up()ed ever. But I didn't look too hard.

drivers/mtd/maps/vmu_flash.c line 218

Quoted 8 lines

drivers/mtd/maps/vmu_flash.c new file +8 -0

+
+	for (x = 0; x < card->writecnt; x++) {
+		/* take the lock to protect the contents of sendbuf */
+		locking = down_interruptible(&mdev->mq->sem);
+		if (locking) {
+			error = -EBUSY;
+			goto fail_nolock;
+		}

Confused. Where within this loop does the up(&mdev->mq->sem) happen?

drivers/mtd/maps/vmu_flash.c line 223

Quoted 5 lines

drivers/mtd/maps/vmu_flash.c new file +5 -0

+		((unsigned long *)sendbuf)[1] > > +			cpu_to_be32(partition << 24 | x << 16 | num);
+		memcpy(sendbuf + 8, buf + phaselen * x, phaselen);
+		mdev->mq->sendbuf = sendbuf;
+		/* wait for the mutex to be available */
+		maple_add_packet(mdev->mq);

Within here, I guess, via a workqueue or timer function?

These patches update the maple bus driver with a semaphore to allow other maple drivers to inject arbitrary packets (eg block reads and writes) into the maple bus packet stream - so allowing support for the VMU (and, eventually, other maple devices).

The patches also ensure that the maple bus driver properly supports the device model through probing.

...

drivers/sh/maple/maple.c +2 -6

@@ -255,12 +267,8 @@ static int attach_matching_maple_driver(struct device_driver *driver,
 
 	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 +276,6 @@ static void maple_detach_driver(struct maple_device *mdev)
 {

drivers/sh/maple/maple.c +2 -1

@@ -377,18 +380,20 @@ static int setup_maple_commands(struct device *device, void *ignored)
 
 	if ((maple_dev->interval > 0)
 	    && time_after(jiffies, maple_dev->when)) {
+		if (down_trylock(&maple_dev->mq->sem))
+			return 0;
 		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++;

drivers/sh/maple/maple.c +5 -4

@@ -398,32 +403,38 @@ static int setup_maple_commands(struct device *device, void *ignored)
 /* 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)) {
+	mutex_lock(&maple_wlist_lock);
+	if (!list_empty(&maple_waitq) && list_empty(&maple_sentq)) {
+		mutex_unlock(&maple_wlist_lock);

drivers/sh/maple/maple.c +2 -2

 /* 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,

drivers/sh/maple/maple.c +7 -2

 			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;

@@ -512,15 +529,17 @@ static void maple_dma_handler(struct work_struct *work)
 	struct maple_device *dev;
 	char *recvbuf;
 	enum maple_code code;
-	int i;
+	int i, locking;
 
 	if (!maple_dma_done())
 		return;
 	ctrl_outl(0, MAPLE_ENABLE);
+	mutex_lock(&maple_list_lock);
 	if (!list_empty(&maple_sentq)) {
 		list_for_each_entry_safe(mq, nmq, &maple_sentq, list) {
 			recvbuf = mq->recvbuf;
 			code = recvbuf[0];
+			up(&mq->sem);
 			dev = mq->dev;
 			switch (code) {
 			case MAPLE_RESPONSE_NONE:

@@ -559,18 +578,26 @@ static void maple_dma_handler(struct work_struct *work)
 			}
 		}
 		INIT_LIST_HEAD(&maple_sentq);
+		mutex_unlock(&maple_list_lock);
+		/* if scanning is 1 then we have subdevices to check */
 		if (scanning = 1) {
 			maple_send();
 			scanning = 2;
 		} else
 			scanning = 0;
-
+		/*check if we have actually tested all ports yet */
 		if (!fullscan) {
 			fullscan = 1;

drivers/sh/maple/maple.c +7 -1

 					dev->mq->command >  						MAPLE_COMMAND_DEVINFO;
 					dev->mq->length = 0;

@@ -578,8 +605,11 @@ static void maple_dma_handler(struct work_struct *work)
 				}
 			}
 		}
+		/* mark that we have been through the first scan */
 		if (started = 0)
 			started = 1;
+	} else {
+		mutex_unlock(&maple_list_lock);
 	}
 	maplebus_dma_reset();
 }

@@ -631,7 +661,7 @@ static int match_maple_bus_driver(struct device *devptr,
 	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,6 +743,8 @@ static int __init maple_bus_init(void)
 	if (!maple_queue_cache)
 		goto cleanup_bothirqs;
 
+	INIT_LIST_HEAD(&maple_waitq);
+
 	/* setup maple ports */
 	for (i = 0; i < MAPLE_PORTS; i++) {
 		checked[i] = false;

@@ -723,6 +755,7 @@ static int __init maple_bus_init(void)
 				maple_free_dev(mdev[i]);
 			goto cleanup_cache;
 		}
+		down_interruptible(&mdev[i]->mq->sem);
 		mdev[i]->mq->command = MAPLE_COMMAND_DEVINFO;
 		mdev[i]->mq->length = 0;
 		maple_add_packet(mdev[i]->mq);

include/linux/maple.h +1 -0

index d31e36e..e40142e 100644
@@ -33,6 +33,7 @@ struct mapleq {
 	void *sendbuf, *recvbuf, *recvbufdcsp;
 	unsigned char length;
 	enum maple_code command;
+	struct semaphore sem;
 };
 
 struct maple_devinfo {

urgh, down_trylock(). And a secret, undocumented one too.

A trylock is always an exceptional thing. How is *any* reader of this code supposed to work out what the heck it's doing there? Convert it into down(), run the code and decrypt the lockdep warnings, I suspect.

<looks>

Nope, I can't see any other lock being held when we call this function.

The trylocks are an utter mystery to me. Please don't write mysterious code.

Quoted 26 lines

drivers/sh/maple/maple.c +9 -4

@@ -398,32 +403,38 @@ static int setup_maple_commands(struct device *device, void *ignored)
 /* 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)) {
+	mutex_lock(&maple_wlist_lock);
+	if (!list_empty(&maple_waitq) && list_empty(&maple_sentq)) {
+		mutex_unlock(&maple_wlist_lock);
+		mutex_lock(&maple_list_lock);
 		INIT_LIST_HEAD(&maple_sentq);
+		mutex_unlock(&maple_list_lock);
 		maple_send();
+	} else {
+		mutex_unlock(&maple_wlist_lock);

drivers/sh/maple/maple.c static int setup_maple_commands(struct device *device, void *ignored) · line 427

Quoted 4 lines

drivers/sh/maple/maple.c +1 -0

 	}
+
 	maplebus_dma_reset();
 }

This locking looks like a mess. I'd suggest that maple_list_lock and maple_wlist_lock now need documentation. Describe what data they protect and what their ranking rules are.

Quoted 9 lines

drivers/sh/maple/maple.c +1 -1

 /* 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;
 

Quoted 6 lines

drivers/sh/maple/maple.c +1 -1

+	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,

drivers/sh/maple/maple.c static void maple_map_subunits(struct maple_device *mdev, int submask) · line 455

Quoted 8 lines

drivers/sh/maple/maple.c +5 -0

@@ -437,9 +448,15 @@ static void maple_map_subunits(struct maple_device *mdev, int submask)
 			mdev_add = maple_alloc_dev(mdev->port, k + 1);
 			if (!mdev_add)
 				return;
+			locking = down_trylock(&mdev_add->mq->sem);
+			if (locking) {
+				up(&mdev_add->mq->sem);
+				down_interruptible(&mdev_add->mq->sem);
+			}

What the heck is that trying to do?!?!?!

And it's buggy. The return value of down_interruptible() is ignored, so the driver has lost track of whether or not the semphore was taken.

Quoted 9 lines

urgh, down_trylock(). And a secret, undocumented one too.

A trylock is always an exceptional thing. How is *any* reader of this code supposed to work out what the heck it's doing there? Convert it into down(), run the code and decrypt the lockdep warnings, I suspect.

<looks>

Quoted 5 lines

Nope, I can't see any other lock being held when we call this function.

The trylocks are an utter mystery to me. Please don't write mysterious code.

OK, I am sure this is my problem but I have no idea why you are describing down_trylock as undocumented

Quoted 15 lines · On Thu, 20 Mar 2008 22:23:17 +0000 Adrian McMenamin…

Quoted 14 lines

urgh, down_trylock(). And a secret, undocumented one too.

A trylock is always an exceptional thing. How is *any* reader of this code supposed to work out what the heck it's doing there? Convert it into down(), run the code and decrypt the lockdep warnings, I suspect.

<looks>

Nope, I can't see any other lock being held when we call this function.

The trylocks are an utter mystery to me. Please don't write mysterious code.

Quoted 2 lines

OK, I am sure this is my problem but I have no idea why you are describing down_trylock as undocumented

I'm describing your use of it! I'm sitting here trying to work out why on earth this code is using the highly unusual (and highly suspicious) trylock idiom and this is far from clear.

Quoted 21 lines · Andrew Morton

Quoted 17 lines · On Thu, 20 Mar 2008 22:23:17 +0000 Adrian McMenamin…

Quoted 7 lines

urgh, down_trylock(). And a secret, undocumented one too.

A trylock is always an exceptional thing. How is *any* reader of this code supposed to work out what the heck it's doing there? Convert it into down(), run the code and decrypt the lockdep warnings, I suspect.

Quoted 7 lines

<looks>

Nope, I can't see any other lock being held when we call this function.

The trylocks are an utter mystery to me. Please don't write mysterious code.

OK, I am sure this is my problem but I have no idea why you are describing down_trylock as undocumented

Quoted 4 lines

I'm describing your use of it! I'm sitting here trying to work out why on earth this code is using the highly unusual (and highly suspicious) trylock idiom and this is far from clear.

OK, I understand your point now. Some comments wouldn't go amiss, for sure.