Re: [Alsa-devel] [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai <tiwai@suse.de>]

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