Hey Antonio,
On Thu, Jan 10, 2013 at 04:34:47AM +0200, Antonio Quartulli wrote:
> Hello Simon,
>
> thanks a lot for taking care of this and sorry for my very late reply..
no problem, thanks for your review!
>
>
> Il 31.12.2012 01:40 Simon Wunderlich ha scritto:
> >diff --git a/hard-interface.c b/hard-interface.c
> >index f1d37cd..eb3a12d 100644
> >--- a/hard-interface.c
> >+++ b/hard-interface.c
> >@@ -506,6 +506,17 @@ out:
> > return NULL;
> > }
> >
> >+static void batadv_hardif_remove_interface_finish(struct
> >work_struct *work)
> >+{
> >+ struct batadv_hard_iface *hard_iface;
> >+
> >+ hard_iface = container_of(work, struct batadv_hard_iface,
> >+ cleanup_work);
> >+
> >+ batadv_sysfs_del_hardif(&hard_iface->hardif_obj);
> >+ batadv_hardif_free_ref(hard_iface);
> >+}
> >+
> > static void batadv_hardif_remove_interface(struct batadv_hard_iface
> >*hard_iface)
> > {
> > ASSERT_RTNL();
> >@@ -518,8 +529,9 @@ static void batadv_hardif_remove_interface(struct
> >batadv_hard_iface *hard_iface)
> > return;
> >
> > hard_iface->if_status = BATADV_IF_TO_BE_REMOVED;
> >- batadv_sysfs_del_hardif(&hard_iface->hardif_obj);
> >- batadv_hardif_free_ref(hard_iface);
> >+ INIT_WORK(&hard_iface->cleanup_work,
> >+ batadv_hardif_remove_interface_finish);
> >+ queue_work(batadv_event_workqueue, &hard_iface->cleanup_work);
> > }
> >
>
> why do we need to postpone the invocation of
> batadv_hardif_remove_interface_finish() ? Is it also creating a
> possible
> deadlock? As far as I understood only rtnl_lock() would create the
> problem, but it
> is not invoked in batadv_hardif_remove_interface_finish(). It seems
> I am missing
> something?
batadv_hardif_remove_interface() is called by batadv_hard_if_event(),
which is protected rtnl_lock(). You can find an ASSERT_RTNL() there too.
As batadv_hardif_remove_interface() then removes sysfs, this could
be problematic.
>
> Other than this, remember that the INIT_WORK macro does not need to
> be invoked
> each and every time you want to enqueue the work. It should be
> invoked once only
> (A patch fixing this "issue" for all the delayed works we have in
> the code has
> been recently applied). However it is not a real issue, but we want
> to keep the
> code consistent :)
>
Well, the work item is actually only initialized once, before taking
the interface down. Then the struct is destroyed, so it can't be called
again. But you are right, for consistency I will move the INIT_WORK
macros to positions where the structs are initialized.
> > int batadv_softif_is_valid(const struct net_device *net_dev)
> >diff --git a/types.h b/types.h
> >index d8061ac..a9800ee 100644
> >--- a/types.h
> >+++ b/types.h
> >@@ -63,6 +63,7 @@ struct batadv_hard_iface {
> > struct net_device *soft_iface;
> > struct rcu_head rcu;
> > struct batadv_hard_iface_bat_iv bat_iv;
> >+ struct work_struct cleanup_work;
> > };
> >
>
> kernel doc :)
>
The Mareks kernel doc patch wasn't applied at this time, so I didn't
bother documenting my single new variable, but will do that in the
next revision. :)
Thanks,
Simon
signature.asc
Description: Digital signature
