[Cbe-oss-dev] [PATCH] axonram: 1st version

[Christoph Hellwig]

On Wed, Feb 14, 2007 at 12:06:13AM +0100, Maxim Shchetynin wrote:
> 
> Hello,
> 
> This patch implements access to Axon's DDR2 memory via character and block
> devices.

This looks pretty good.  Comments below:

> +
> +#include <asm/mmu.h>
> +#include <asm/of_device.h>
> +#include <asm/of_platform.h>
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/prom.h>
> +#include <asm/string.h>
> +#include <asm/uaccess.h>
> +#include <linux/bio.h>
> +#include <linux/blkdev.h>

generally please include all asm/ headers after linux/ ones.

> +static int
> +axon_ram_open(struct inode *, struct file *);
> +static int
> +axon_ram_release(struct inode *, struct file *);
> +static loff_t
> +axon_ram_llseek(struct file *, loff_t, int);
> +static ssize_t
> +axon_ram_read(struct file *, char __user *, size_t, loff_t *);

please try to order function so that we don need forward declarations.

> +struct axon_ram_bank {
> +     char              chrdev_name[DEVICE_NAME_SIZE];
> +     char              blkdev_name[DEVICE_NAME_SIZE];
> +     rwlock_t          lock;
> +     phys_addr_t       phys_addr;
> +     void __iomem            *addr;
> +     size_t                  size;
> +     struct gendisk          *disk;
> +     struct miscdevice misc;
> +     struct file_operations  fops;

We shouldn't need per-instance file operations and miscdevice  One global
is enough, like most drivers do.

> +static int
> +axon_ram_open(struct inode *inode, struct file *file)
> +{
> +     struct axon_ram_bank *bank;
> +
> +     /* Prevent access to memory bank using simultaneously both */
> +     /* block and character device methods. */
> +     if (S_ISCHR(inode->i_mode)) {
> +           file->private_data = (void*) file->f_op +
> +                       sizeof(struct file_operations) -
> +                       sizeof(struct axon_ram_bank);

Actually this explains why you per-intance file operations.  I think
this is quite hackish and you'd be better of using a global list
for clarity.

> +           bank = (struct axon_ram_bank*) file->private_data;
> +
> +           /* Allow only a single open of character device. */
> +           /* No access to device if it is already read-locked */
> +           /* (opened at least one time as block device). */
> +           if (!write_trylock(&bank->lock))
> +                 return -EBUSY;
> +     } else if (S_ISBLK(inode->i_mode)) {
> +           bank = (struct axon_ram_bank*)
> +                       inode->i_bdev->bd_disk->private_data;
> +
> +           /* Block device maybe opened multiple times therefore */
> +           /* read-lock. No access to device if it is already */
> +           /* write-locked (opened as character device). */
> +           if (!read_trylock(&bank->lock))
> +                 return -EBUSY;
> +           inode->i_bdev->bd_block_size = 1 << AXON_RAM_BLOCK_SHIFT;
> +     } else {
> +           return -ENODEV;
> +     }
> +
> +     return 0;
> +}

Please don't share methods for char and block drivers.  This just
confuses the reader, especially when as in this case there's no
code shared between them at all.  Btw, why do we need the character
device at all?  We will get the same semantics opening the block device
using O_DIRECT or binding the raw driver to it.

> + * axon_ram_llseek - llseek() method for character device
> + * @file, @offset, @origin: see file_operations method
> + */
> +static loff_t
> +axon_ram_llseek(struct file *file, loff_t offset, int origin)
> +{
> +     struct axon_ram_bank *bank;
> +
> +     bank = (struct axon_ram_bank*) file->private_data;
> +
> +     switch (origin) {
> +     case SEEK_CUR:
> +           offset += file->f_pos;
> +           break;
> +
> +     case SEEK_END:
> +           offset += bank->size;
> +           break;
> +     }
> +
> +     /* No access outside a memory DIMM. */
> +     if (offset < 0 || offset > bank->size)
> +           return -EINVAL;
> +
> +     return file->f_pos = offset;
> +}

Do we need some locking here?

> +/**
> + * axon_ram_read - read() method for character device
> + * @file, @buffer, @size, @offset: see file_operations method
> + */
> +static ssize_t
> +axon_ram_read(struct file *file,
> +           char __user *buffer, size_t size, loff_t *offset)
> +{
> +     struct axon_ram_bank *bank;
> +
> +     bank = (struct axon_ram_bank*) file->private_data;

btw, you don't need casts here as file->private is void *.  The same
holds true for most other casts throughout the code.

> +     if (*offset + size > bank->size)
> +           size = bank->size - *offset;
> +     copy_to_user(__user buffer, __iomem bank->addr + *offset, size);

This is wrong.  You need readl/writel or ioread*/iowrite* accessors
to touch io memory.  Also the __user and __iomem in the above don't
make sense and will stop sparse from running over this code :)

Then again all this would go away if we used just the block device.