Hey greg, thanks for you reply.On Thu, Apr 21, 2005 at 11:01:22PM +0200, Tyler wrote:
I'll correct all the errors and make a nicer module. I'v commented some of your corrections because I don't agree with these.
Have a look please :p
Thx
Well no I don't think so. There is one message for all the users who open the device.#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.
If nobody has opened the device, the message is initialized (at "Nothing\n").
And if someone has already open the device, nothing is done.
But I realize I made something strange with the msgPtr. It should not be used in this function. I'll correct it.
Yeah interruptible is not needed. But semaphore (or locking mechanism) must be used here.
/* * 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?
Because msg is shared between all the users. Am I wrong ?
I check the message buffer. If the message is "Nothing\n", nobody has written in the device (and we consider that there is no data avalaible). So it put the process in sleeping mode. The process will be wake up by the next process which write something in the device.while(strncmp(msg,"Nothing\n",8)==0){ up(&sem) ; printk("<1> Sleep on\n") ; interruptible_sleep_on(&attente) ; //down_interruptible(&sem) ; /* This line sux */
What are you doing here?
Just to see how it works :)
}
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/
