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

["Jörn Engel" <joern@logfs.org>]

On Thu, 20 March 2008 20:11:01 +0000, Adrian McMenamin wrote:
> 
> 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>---
> +     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

-- 
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack