Hello Michal,
Thanks for the review,
On February 29, 2012 2:03 PM Michał Nazarewicz wrote:
<...>
> > data.dev_name = dev_name;
> > - return mount_single(t, flags, &data, ffs_sb_fill);
> > + rv = mount_nodev(t, flags, &data, ffs_sb_fill);
>
> mount_single() -> mount_nodev() change is made because now functionfs
> can
> be mounted multiple times, right? Just asking to make sure I
> understand
> that correctly.
That's right.
<....>
> >-static struct ffs_data *gfs_ffs_data;
> > -static unsigned long gfs_registered;
> > +static DEFINE_MUTEX(gfs_lock);
>
> I actually think that we could go without mutex using bit operations
> and atomic counters. On the other side, mutex is only acquired during
> setup and cleanup, so I won't strongly oppose it.
>
The experiments show something rather contrary - without a mutex there
is a race condition: it is not enough just to atomically set/unset
the "registered" flag, because e.g. in the ready callback, after setting
the flag the control moves on to next statements, so the only difference
in execution between cases that the flag is not set or is set is that
it is either being set in this thread or is not being set because has
already been set; a similar thing happens in the closed callback.
So we need a larger granularity of atomic operations and I think
it is probably better that you keep your word and don't
strongly oppose it ;)
<....>
>-static int functionfs_check_dev_callback(const char *dev_name)
> +static int functionfs_acquire_dev_callback(const char *dev_name)
> {
> struct ffs_data has a private_data field which does not seem to be used at all,
> yet it looks that this new g_ffs could make uses of it.
>
> In particular, we could change functionfs_acquire_dev_callback() return type
> to “void *” allowing any non-error pointers to be returned. This could then
> be passed to a ffs_data created structure. This would allow
> functionfs_{ready,closed}_callback() to use private_data instead of iterating
> over all objects.
>
> Alternatively, functionfs_acquire_dev_callback() could return struct ffs_data *,
> with the following semantics:
> 1. if it returns error pointer (eg. ERR_PTR(-EINVAL)), f_fs aborts mounting,
> 2. if it returns NULL pointer, f_fs continues mounting and allocates a new
> ffs_data structure,
> 3. if it returns non-NULL, non-error pointer, f_fs continues mounting and uses
> returned ffs_data structure.
I like the first alternative better and am willing to change the patch
so that it uses this approach.
<....>
As far as other comments are concerned I generally agree.
Andrzej
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
