In fact, the big error I see in the module with all your comments is that I've forgot to free char *msg.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
The other errors are coding style and bad implementation choices. I'll correct all these this evening.
But semaphores are needed I'm pretty sure !:p
-- Kernelnewbies: Help each other learn about the Linux kernel. Archive: http://mail.nl.linux.org/kernelnewbies/ FAQ: http://kernelnewbies.org/faq/
