At Thu, 5 Oct 2006 01:41:47 +0200,
Karsten Wiese wrote:
>
> Am Mittwoch, 4. Oktober 2006 22:15 schrieb Takashi Iwai:
> >
> > This looks like a good optoin. But one thing we have to be careful
> > about is the module counter since the owner is different between the
> > old f_op and disconnect_f_op...
> >
> here is rc1, will test later.
> Feel free to pick it apart ;-)
Any special reason to make it separate instead of patching init.c?
Most of codes (e.g. dummy callbacks) are already in init.c.
> struct snd_disconnected_file {
> struct file *file;
> int (*release) (struct inode *, struct file *);
> struct snd_disconnected_file *next;
We can use a standard list here.
> };
>
> static struct snd_disconnected_file *disconnecting_files;
> static struct file_operations snd_disconnect_f_ops;
> static DEFINE_MUTEX(mutex);
>
> void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> {
> struct snd_disconnected_file *df, **_dfs;
> df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
> if (df == NULL)
> panic("Atomic allocation failed for snd_disconnected_file!");
IIRC, the reason that snd_card_disconnect() uses GFP_ATOMIC is that
(usb-)disconnection was atomic in the earlier time.
You're using mutex here, hence no reason to allocate with GFP_ATOMIC.
> df->file = file;
> df->release = release;
> df->next = NULL;
>
> mutex_lock(&mutex);
> _dfs = &disconnecting_files;
> while (*_dfs != NULL)
> _dfs = &(*_dfs)->next;
> *_dfs = df;
You can add to the item to head :) The order doesn't matter.
> mutex_unlock(&mutex);
>
> {
> const struct file_operations *old_f_op = file->f_op;
> fops_get(&snd_disconnect_f_ops);
> file->f_op = &snd_disconnect_f_ops;
> fops_put(old_f_op);
I wonder whether the old release might be called during this
operation. Then df won't be freed.
> static int snd_disconnect_release(struct inode *inode, struct file *file)
> {
> struct snd_disconnected_file *df, **_dfs, **__dfs;
> int err = 0;
> __dfs = _dfs = &disconnecting_files;
>
> mutex_lock(&mutex);
> while ((df = *_dfs))
> if (df->file == file) {
> *__dfs = df->next;
> break;
> } else {
> __dfs = _dfs;
> _dfs = &df->next;
> }
> mutex_unlock(&mutex);
A standard list would make the code more readable (unless you use too
many underscores ;)
Thanks,
Takashi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
