Skip to contentSkip to conversation

2/2 mtd: Add support for the Dreamcast VMU flash]

PATCH In review Adrian McMenamin 2 participants 10 messages Mar 19, 2008 – Mar 21, 2008 Memory technology devices (MTD)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 2/2 mtd: Add support for the Dreamcast VMU flash]

---

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/Kconfig +14 -0
index 12c2536..c89a610 100644
@@ -590,5 +590,19 @@ config MTD_PLATRAM
 
 	  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
 
drivers/mtd/maps/Makefile +1 -0
index a9cbe80..d5802d2 100644
@@ -68,3 +68,4 @@ obj-$(CONFIG_MTD_PLATRAM)	+= plat-ram.o
 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
drivers/mtd/maps/vmu_flash.c new file +746 -0

746 lines not on this page · raw message

#1 lore raw

Some details I noticed on first glance.

Quoted 24 lines · Adrian McMenamin
drivers/mtd/maps/vmu_flash.c new file +23 -0
+/* 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, locking;
+	void *sendbuf;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+
+	/* wait for the mutex to be available */
+	locking = down_interruptible(&(mdev->mq->sem));
+	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;

drivers/mtd/maps/vmu_flash.c line 160

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+	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);

unsigned long is larger than 32bit on many architectures. Better variant:

	__be32 *sendbuf;
	...
	sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
	sendbuf[1] = cpu_to_be32(partition << 24 | num);

Sparse would have noticed this problem as well. Might be a good idea to

$ make C=1 drivers/mtd/maps/vmu_flash.o
and take a look what other problems show up.

Review

Quoted 1 line
(quoted diff) +1 -0
+	mdev->mq->sendbuf = sendbuf;

Possibly the big-endian annotations need to trickly though the layers here as well.

Quoted 36 lines
drivers/mtd/maps/vmu_flash.c new file +36 -0
+/* 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];

drivers/mtd/maps/vmu_flash.c line 292

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+	kfree(buf);
+	kfree(vblock);
+	return ret;
+
+failed_block:
+	kfree(vblock);
+invalid_block:
+	kfree(buf);
+fail_buffer:
+	return -error;

Yikes. Errnos greater 127 do exist. Better return a signed int and interpret at error if <0.

You could also combine the two sets of kfree() into one, btw.

Quoted 4 lines
drivers/mtd/maps/vmu_flash.c new file +4 -0
+	do {
+		if (pcache->valid &&
+		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
+			vblock = ofs_to_block(from + index, mtd, partition);

drivers/mtd/maps/vmu_flash.c line 334

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+			if (!vblock)
+				return -ENOMEM;
+			if (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);

Five levels of indentation are a strong indication that this code is buggy. I don't understand how it behaves in all corner cases and I bet neither do you.

Quoted 19 lines
drivers/mtd/maps/vmu_flash.c new file +19 -0
+	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;

drivers/mtd/maps/vmu_flash.c line 434

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+		pcache->valid = 0;
+
+		if (retval != card->blocklen) {
+			error = -EIO;
+			goto fail_io;
+		}
+
+		vblock->num++;
+		vblock->ofs = 0;
+	} while (len > index);

And here is an example of essentially the same loop in a fashion mere mortals can understand. Much better. ;)

Quoted 1 line

+MODULE_AUTHOR("Adrian McMenamin, Paul Mundt");

I believe the main use of MODULE_AUTHOR is to figure out who to send bug reports/patches to, not to contain every possible copyright holder. So possibly just your name here would be more appropriate.

Jörn

Signature · 2 lines
A victorious army first wins and then seeks battle.
-- Sun Tzu
#2 lore raw
Quoted 40 lines · Jörn Engel

Some details I noticed on first glance.

Quoted 24 lines · Adrian McMenamin
drivers/mtd/maps/vmu_flash.c new file +23 -0
+/* 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, locking;
+	void *sendbuf;
+
+	mpart = mtd->priv;
+	mdev = mpart->mdev;
+	partition = mpart->partition;
+	card = mdev->private_data;
+
+	/* wait for the mutex to be available */
+	locking = down_interruptible(&(mdev->mq->sem));
+	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;

drivers/mtd/maps/vmu_flash.c line 160

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+	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);

unsigned long is larger than 32bit on many architectures. Better variant:

Quoted 1 line
	__be32 *sendbuf;

No, it's not. This is part of a bus subsystem where different parts of the incoming packets may or may not be in big endian format. Eg for block writes the then the opening blocks may be in big endian format but the data to be written is not.

If you insist on leaving open the possibility that someone might decide to build the electronics to attach these devices to a 64 bit machine (unlikely, I'd suggest) then u32 is the way to go - but are you really concerned that might happen?

Quoted 6 lines

drivers/mtd/maps/vmu_flash.c line 292

Quoted 2 lines
drivers/mtd/maps/vmu_flash.c new file +2 -0
+fail_buffer:
+	return -error;

Yikes. Errnos greater 127 do exist. Better return a signed int and interpret at error if <0.

There is an error here, actually, but not as you describe. However as the errors defined in errno-base are all positive integers I don't see how your proposal works.

Quoted 12 lines

You could also combine the two sets of kfree() into one, btw.

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+	do {
+		if (pcache->valid &&
+		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
+			vblock = ofs_to_block(from + index, mtd, partition);
+			if (!vblock)
+				return -ENOMEM;
+			if (pcache->block == vblock->num) {
+				/*we have cached it, so do necessary copying */
+				leftover = card->blocklen - vblock->ofs;
+				if (leftover > len - index) {
Quoted 9 lines

drivers/mtd/maps/vmu_flash.c line 334

Quoted 4 lines
drivers/mtd/maps/vmu_flash.c new file +4 -0
+					/* only a bit of this block to copy */
+					memcpy(buf + index,
+						pcache->buffer + vblock->ofs,
+						len - index);

Five levels of indentation are a strong indication that this code is buggy. I don't understand how it behaves in all corner cases and I bet neither do you.

It's not buggy - and it seems pretty clear to me what is happening here. What's the issue?

Quoted 24 lines
Quoted 24 lines
drivers/mtd/maps/vmu_flash.c new file +24 -0
+	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;
Quoted 8 lines

drivers/mtd/maps/vmu_flash.c line 434

Quoted 5 lines
drivers/mtd/maps/vmu_flash.c new file +5 -0
+		}
+
+		vblock->num++;
+		vblock->ofs = 0;
+	} while (len > index);

And here is an example of essentially the same loop in a fashion mere mortals can understand. Much better. ;)

That's because the writes, for fairly obvious reasons (ie once you've written to a block you've changed it), are not cached in the same way.

Quoted 8 lines
Quoted 2 lines

+MODULE_AUTHOR("Adrian McMenamin, Paul Mundt");

I believe the main use of MODULE_AUTHOR is to figure out who to send bug reports/patches to, not to contain every possible copyright holder. So possibly just your name here would be more appropriate.

fair enough.

#3 lore raw
Quoted 9 lines · Adrian McMenamin
Quoted 7 lines

drivers/mtd/maps/vmu_flash.c line 160

Quoted 3 lines
drivers/mtd/maps/vmu_flash.c new file +3 -0
+
+	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);

unsigned long is larger than 32bit on many architectures. Better variant:

	__be32 *sendbuf;
Quoted 4 lines

No, it's not. This is part of a bus subsystem where different parts of the incoming packets may or may not be in big endian format. Eg for block writes the then the opening blocks may be in big endian format but the data to be written is not.

Data to be written is usually treated as opaque void* pointer. You don't need to know what's in it, just the pointer and the length. But once you dereference the data, it is no longer opaque and you better know its endianness.

So why do you use void* for data you actually dereference?

Quoted 4 lines

If you insist on leaving open the possibility that someone might decide to build the electronics to attach these devices to a 64 bit machine (unlikely, I'd suggest) then u32 is the way to go - but are you really concerned that might happen?

Not too much, but there are examples where such things did happen. emac3 is one.

Quoted 10 lines
Quoted 6 lines

drivers/mtd/maps/vmu_flash.c line 292

Quoted 2 lines
drivers/mtd/maps/vmu_flash.c new file +2 -0
+fail_buffer:
+	return -error;

Yikes. Errnos greater 127 do exist. Better return a signed int and interpret at error if <0.

There is an error here, actually, but not as you describe. However as the errors defined in errno-base are all positive integers I don't see how your proposal works.

A couple of lines down you have "return -ENOMEM;". Notice the "-"? As a general rule, negative return values in the kernel are errors, positive or zero are non-errors. Exceptions exist, but they tend to surprise users and cause subtle bugs.

Quoted 21 lines
Quoted 12 lines

You could also combine the two sets of kfree() into one, btw.

Quoted 10 lines
drivers/mtd/maps/vmu_flash.c new file +10 -0
+	do {
+		if (pcache->valid &&
+		   time_before(jiffies, pcache->jiffies_atc + HZ)) {
+			vblock = ofs_to_block(from + index, mtd, partition);
+			if (!vblock)
+				return -ENOMEM;
+			if (pcache->block == vblock->num) {
+				/*we have cached it, so do necessary copying */
+				leftover = card->blocklen - vblock->ofs;
+				if (leftover > len - index) {
Quoted 8 lines

drivers/mtd/maps/vmu_flash.c line 334

Quoted 4 lines
drivers/mtd/maps/vmu_flash.c new file +4 -0
+					/* only a bit of this block to copy */
+					memcpy(buf + index,
+						pcache->buffer + vblock->ofs,
+						len - index);

Five levels of indentation are a strong indication that this code is buggy. I don't understand how it behaves in all corner cases and I bet neither do you.

Quoted 2 lines

It's not buggy - and it seems pretty clear to me what is happening here. What's the issue?

Every single time I've had to deal with such levels of indentation before I have found a bug. No exceptions. And every single time it was a _lot_ of work because the code was too complicated for my little brain to completely understand. My preferred method is to rework the code until it is simple enough even for me.

But as long as you are willing to maintain this beast and deal with any bug reports, I don't mind either way.

Jörn

Signature · 2 lines
The only good bug is a dead bug.
-- Starship Troopers
#4 lore raw
Quoted 16 lines · Jörn Engel
Quoted 9 lines · Adrian McMenamin
Quoted 7 lines

drivers/mtd/maps/vmu_flash.c line 160

Quoted 3 lines
drivers/mtd/maps/vmu_flash.c new file +3 -0
+
+	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);

unsigned long is larger than 32bit on many architectures. Better variant:

	__be32 *sendbuf;
Quoted 5 lines

No, it's not. This is part of a bus subsystem where different parts of the
incoming packets may or may not be in big endian format. Eg for block writes the then the opening blocks may be in big endian format but the data to be written is not.

Quoted 6 lines

Data to be written is usually treated as opaque void* pointer. You don't need to know what's in it, just the pointer and the length. But once you dereference the data, it is no longer opaque and you better know its endianness.

So why do you use void* for data you actually dereference?

Because for other devices data can be passed as 16 bit structures as well as 32 bit ones.

Quoted 7 lines

.

A couple of lines down you have "return -ENOMEM;". Notice the "-"? As a general rule, negative return values in the kernel are errors, positive or zero are non-errors. Exceptions exist, but they tend to surprise users and cause subtle bugs.

Ah. I understand your point better now. I'd basically saw that too - but I didn't realise that is what you meant.

#5 lore raw
Quoted 9 lines · Adrian McMenamin
Quoted 5 lines · Jörn Engel
Quoted 3 lines · Adrian McMenamin
Quoted 3 lines

drivers/mtd/maps/vmu_flash.c line 160

Quoted 3 lines
drivers/mtd/maps/vmu_flash.c new file +3 -0
+
+	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);

So why do you use void* for data you actually dereference?

Because for other devices data can be passed as 16 bit structures as well as 32 bit ones.

Hogwash! If you ever deal with 16bit structures here you have a bug. cpu_to_be32() only makes sense when actually dealing with __be32. Either your variable is and should be called __be32, or it isn't and you have a bug right in front of you.

Jörn

Signature · 2 lines
It does not matter how slowly you go, so long as you do not stop.
-- Confucius
#6 lore raw
Quoted 8 lines · Jörn Engel
Quoted 7 lines · Adrian McMenamin
Quoted 5 lines · Jörn Engel
Quoted 3 lines · Adrian McMenamin
Quoted 3 lines

drivers/mtd/maps/vmu_flash.c line 160

Quoted 3 lines
drivers/mtd/maps/vmu_flash.c new file +3 -0
+
+	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
+	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);

So why do you use void* for data you actually dereference?

Quoted 7 lines
Quoted 2 lines

Because for other devices data can be passed as 16 bit structures as well as 32 bit ones.

Hogwash! If you ever deal with 16bit structures here you have a bug. cpu_to_be32() only makes sense when actually dealing with __be32. Either your variable is and should be called __be32, or it isn't and you have a bug right in front of you.

You don't seem to have grasped that this is one type of device on a proprietary bus which has other (types of) devices. I keep the code consistent so that the API runs the same across the different devices - makes the code easier to understand and maintain.

And given that this is also the variable that points to the memory block that also includes the 8 bit based date that gets read in for block writes I it makes perfect sense to have this as a void*.

#7 lore raw
Quoted 9 lines · Adrian McMenamin

You don't seem to have grasped that this is one type of device on a proprietary bus which has other (types of) devices. I keep the code consistent so that the API runs the same across the different devices - makes the code easier to understand and maintain.

And given that this is also the variable that points to the memory block that also includes the 8 bit based date that gets read in for block writes I it makes perfect sense to have this as a void*.

Maybe this is a misunderstanding. If you are arguing about this bit several mails back:
---<snip>---

Quoted 1 line

+ mdev->mq->sendbuf = sendbuf;

Possibly the big-endian annotations need to trickly though the layers here as well.
---<snap>---
Then I agree. For a bus driver that only takes opaque data and moves it between device driver and hardware, void* is the type to choose. But if you are arguing about the actual device driver, I couldn't disagree more.

Please take a look at the function below, maybe it becomes clearer then.

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, locking;
       __be32 *sendbuf;

       mpart = mtd->priv;
       mdev = mpart->mdev;
       partition = mpart->partition;
       card = mdev->private_data;

       /* wait for the mutex to be available */
       locking = down_interruptible(&(mdev->mq->sem));
       if (locking) {
               printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
                       " aborting read\n", mdev->unit, mdev->port);
               return -EIO;
       }
       mdev->mq->command = MAPLE_COMMAND_BREAD;
       mdev->mq->length = 2;

       sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
       if (!sendbuf)
               return -ENOMEM;
       
       sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
       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);
	       error = -EIO;
               goto out;
       }
       /* FIXME: we kfree a data structure that was allocated elsewhere.
        * Either move allocation and freeing to the same function or
	* thoroughly document this to avoid subtle bugs after future
	* code changes.
	*/
       memcpy(buf, card->blockread, card->blocklen);
       kfree(card->blockread);

       error = 0;
out:
       kfree(sendbuf);
       return error;
}

And after doing these changes a couple more problems stuck out like sore thumbs, f.e. the FIXME above. But we can discuss those later.

Jörn

Signature · 3 lines
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack
#8 lore raw
Quoted 16 lines · Jörn Engel
Quoted 9 lines · Adrian McMenamin

You don't seem to have grasped that this is one type of device on a proprietary bus which has other (types of) devices. I keep the code consistent so that the API runs the same across the different devices - makes the code easier to understand and maintain.

And given that this is also the variable that points to the memory block that also includes the 8 bit based date that gets read in for block writes I it makes perfect sense to have this as a void*.

Maybe this is a misunderstanding. If you are arguing about this bit several mails back:
---<snip>---

Quoted 1 line

+ mdev->mq->sendbuf = sendbuf;

Quoted 7 lines

Possibly the big-endian annotations need to trickly though the layers here as well.
---<snap>---
Then I agree. For a bus driver that only takes opaque data and moves it between device driver and hardware, void* is the type to choose. But if you are arguing about the actual device driver, I couldn't disagree more.

I'm sorry, but the comment above <snap> isn't good english. What do you mean?

Quoted 59 lines

Please take a look at the function below, maybe it becomes clearer then.

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, locking;
       __be32 *sendbuf;

       mpart = mtd->priv;
       mdev = mpart->mdev;
       partition = mpart->partition;
       card = mdev->private_data;

       /* wait for the mutex to be available */
       locking = down_interruptible(&(mdev->mq->sem));
       if (locking) {
               printk(KERN_INFO "Maple: VMU at (%d, %d) is locked -"
                       " aborting read\n", mdev->unit, mdev->port);
               return -EIO;
       }
       mdev->mq->command = MAPLE_COMMAND_BREAD;
       mdev->mq->length = 2;

       sendbuf = kzalloc(mdev->mq->length * 4, GFP_KERNEL);
       if (!sendbuf)
               return -ENOMEM;
       
       sendbuf[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
       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);
	       error = -EIO;
               goto out;
       }
       /* FIXME: we kfree a data structure that was allocated elsewhere.
        * Either move allocation and freeing to the same function or
	* thoroughly document this to avoid subtle bugs after future
	* code changes.
	*/
       memcpy(buf, card->blockread, card->blocklen);
       kfree(card->blockread);

       error = 0;
out:
       kfree(sendbuf);
       return error;
}
Quoted 2 lines

And after doing these changes a couple more problems stuck out like sore thumbs, f.e. the FIXME above. But we can discuss those later.

What is the issue? That the code is wrong or the documentation is inadequate?

#9 lore raw
Quoted 3 lines · Adrian McMenamin

I'm sorry, but the comment above <snap> isn't good english. What do you mean?

Ok, I give up. There are far too many misunderstandings between us for any sort of useful discussion. No matter who is to blame, it would be pointless to continue.

Maybe someone else has better luck than me.

Jörn

Signature · 2 lines
Eighty percent of success is showing up.
-- Woody Allen