lkml.org 
[lkml]   [2006]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2006-10-05 12:47    [W:8.129 / U:1.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site