On Thu, Apr 21, 2005 at 11:01:22PM +0200, Tyler wrote:
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/fs.h>
> #include <asm/uaccess.h>
> #include <linux/mm.h>
> #include <linux/slab.h>
> #include <linux/devfs_fs_kernel.h>
> #include <asm/semaphore.h>
> #include <linux/poll.h>
> #include <linux/errno.h>
> #include <linux/proc_fs.h>
The <asm/foo.h> includes should be at the end. And why do you have them
at all? They are probably not needed. I don't think the devfs header
file is needed either.
> #define DRIVER_AUTHOR "tyler@xxxxxxxx"
> #define DRIVER_DESC "Hello World Driver"
> #define DEVICE_NAME "hello"
>
>
> int loading(void) ;
> void unloading(void) ;
These should be static.
Don't put a space after the ) and before the ;
> static int hello_open(struct inode *, struct file *) ;
> static int hello_release(struct inode *, struct file *) ;
> static ssize_t hello_read(struct inode *, char *, size_t, loff_t) ;
> static ssize_t hello_write(struct inode *, const char *, size_t, loff_t) ;
> static int hello_ioctl(struct inode *, struct file *, unsigned int , unsigned long);
> static unsigned int hello_poll(struct file *, poll_table *) ;
> static loff_t hello_llseek(struct file *, loff_t) ;
> static int proc_hello_read(char *,char **,off_t,int,int *,void *) ;
> static int proc_hello_write(struct file *,const char *,unsigned long,void *) ;
Some people like function prototypes, others hate them. I hate them,
try to rearange your code so you don't need them at all, that's
generally nicer.
>
> static struct file_operations fops = {
> read : hello_read,
> write : hello_write,
> open : hello_open,
> release : hello_release,
> ioctl : hello_ioctl,
> poll : hello_poll,
> llseek : hello_llseek,
> };
Try lineing these up like:
static struct file_operations fops = {
read: hello_read,
write: hello_write,
open: hello_open,
release: hello_release,
ioctl: hello_ioctl,
poll: hello_poll,
llseek: hello_llseek,
};
> static int Major=0 ;
Don't use uppercase in a variable.
> static int count=0 ;
Uninitalized static variables are all initialized to 0 automatically.
Do not do it yourself, as it makes for bigger code.
Again, fix the ; and space issue (goes for the whole file).
> /*
> * Loading function
> */
> int loading(void)
Comments that are obvious should not be there.
> {
> Major = register_chrdev(Major,DEVICE_NAME,&fops) ;
Should be:
Major = register_chrdev(Major, DEVICE_NAME, &fops);
>
> if (Major<0) {
Should be:
if (Major < 0) {
> printk(KERN_ERR "Cannot allocate a major number.\n");
> return Major ;
> }
>
> printk("<1>Major number %d has been assigned to device %s.\n",Major,DEVICE_NAME);
Don't use <1> directly. Use the proper KERN_ level.
> sema_init(&sem,1) ;
> init_waitqueue_head(&attente) ;
> init_waitqueue_head(&inq) ;
>
> hello_dir = proc_mkdir("hello",NULL) ;
> if(hello_dir==NULL){
Should be:
if (hello_dir == NULL) {
or:
if (!hello_dir) {
> unregister_chrdev(Major,DEVICE_NAME) ;
> return -ENOMEM ;
> }
>
> hello = create_proc_entry("hello",0644,hello_dir) ;
> if(hello==NULL){
> remove_proc_entry("hello",NULL) ;
> unregister_chrdev(Major,DEVICE_NAME) ;
> return -ENOMEM ;
> }
>
> hello->read_proc = proc_hello_read ;
> hello->write_proc = proc_hello_write ;
And don't use proc at all. No new files should _ever_ be created in
/proc unless they specifically deal with processes. So for a driver,
this means never.
If you want to create a file somewhere, use debugfs. Or sysfs. But
never proc.
> /*
> * Hello open function
> */
> static int hello_open(struct inode *inode, struct file *file)
> {
> down_interruptible(&sem) ;
Why interruptible?
> if(count==0){
> msg=(char *)kmalloc(9*sizeof(char),GFP_KERNEL) ;
cast is not needed.
You did not check the return value of kmalloc.
> sprintf(msg,"Nothing\n") ;
> }
> count++ ;
> up(&sem) ;
> msgPtr=msg ;
> return 0 ;
> }
What happens when your device node is opened multiple times? You will
leak memory.
>
> /*
> * Hello close function
> */
> static int hello_release(struct inode *inode, struct file *file)
> {
> return 0 ;
Where do you free the msgPtr?
> }
>
> /*
> * Hello read function :
> * returns the current string stored in the device
> * if there is no data in the device, the funcion blocks until there is some data
> * written
> */
> static ssize_t hello_read(struct inode *inode, char *buf, size_t len, loff_t offset)
"char *buf" should be "char __user *buf"
> {
> int bytes=0 ;
>
> int minor = MINOR(inode->i_rdev) ;
>
> printk(KERN_ERR "*************************************************%d**",minor) ;
What's with the ******?
>
> down_interruptible(&sem) ;
Again, why interruptible? Why a semaphore at all?
> while(strncmp(msg,"Nothing\n",8)==0){
> up(&sem) ;
> printk("<1> Sleep on\n") ;
> interruptible_sleep_on(&attente) ;
> down_interruptible(&sem) ;
What are you doing here?
> }
>
> while(len && *msgPtr){
> put_user(*(msgPtr++),buf++) ;
> len-- ;
> bytes++ ;
> }
use copy_to_user() instead.
> up(&sem) ;
> return bytes ;
> }
>
> /*
> * Hello write function
> * Change the string stored in the hello device and wakes up
> * the eventual sleeping read's
> */
> static ssize_t hello_write(struct inode *inode, const char *buf, size_t len, loff_t offset)
Again, buf is the wrong type.
> {
> int bytes=0 ;
> const char *bufPtr = buf ;
> char *msgPtr = msg ;
>
> down_interruptible(&sem) ;
> if(msg){
> kfree(msg) ;
> msg=(char *)kmalloc((len+1)*sizeof(char),GFP_KERNEL) ;
> }
>
> while(len && *bufPtr){
> get_user(*(msgPtr++),bufPtr++) ;
> len-- ;
> bytes++ ;
> }
> *msgPtr='\0' ;
> up(&sem) ;
> wake_up_interruptible(&attente) ;
> return bytes ;
> }
>
> /*
> * Implementation of an IOCTL to reset the device
> */
> static int hello_ioctl(struct inode *inode, struct file *filp, unsigned int ioctl_num, unsigned long ioctl_param)
Don't ever add a new ioctl. use debugfs or sysfs instead.
> {
> switch(ioctl_num){
> case 0 :/* Reseting the device */
> down_interruptible(&sem) ;
> if(msg){
> kfree(msg) ;
> msg=(char *)kmalloc(9*sizeof(char),GFP_KERNEL) ;
> sprintf(msg,"Nothing\n") ;
> }
> up(&sem) ;
> break ;
> default:
> printk(KERN_ERR "Erreur : ioctl non implemente.\n");
> return -ENODEV ;
> break ;
> }
> return 0 ;
> }
>
> /*
> * Polling on the hello device
> */
> static unsigned int hello_poll(struct file *filp, poll_table *wait)
> {
> unsigned int mask = 0 ;
>
> poll_wait(filp,&inq,wait) ;
>
> if(strncmp(msg,"Nothing\n",8)!=0)
> mask |= POLLIN | POLLRDNORM ;
>
> mask |= POLLOUT | POLLWRNORM ;
>
> return mask ;
> }
>
> /*
> * Seek the device
> */
> static loff_t hello_llseek(struct file *filp, loff_t off)
> {
> loff_t new_pos ;
>
> new_pos = filp->f_pos + off ;
>
> if(new_pos<0)
> return -EINVAL ;
>
> return new_pos ;
Why have a llseek callback at all if you don't do anything?
thanks,
greg k-h
--
Kernelnewbies: Help each other learn about the Linux kernel.
Archive: http://mail.nl.linux.org/kernelnewbies/
FAQ: http://kernelnewbies.org/faq/
