Messages in this thread | | | Date | Thu, 05 Oct 2006 12:43:50 +0200 | From | Takashi Iwai <> | Subject | Re: [Alsa-devel] [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2 |
| |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |