[Fwd: Re: [PATCH] 2/2 mtd: Add support for the Dreamcast VMU flash]

[Jörn Engel]

Some details I noticed on first glance.

On Wed, 19 March 2008 21:20:41 +0000, Adrian McMenamin wrote:
> 
> +/* 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;
> +	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.

> +	mdev->mq->sendbuf = sendbuf;

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

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

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.

> +	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) {
> +					/* 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.

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

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

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

-- 
A victorious army first wins and then seeks battle.
-- Sun Tzu