| Message ID | 1341209912-6030-2-git-send-email-alex.bluesman.smirnov@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2012-07-02 at 10:18 +0400, Alexander Smirnov wrote: > Make symbols static to avoid the following warning shown up > by sparse: > > warning: symbol ... was not declared. Should it be static? > > Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> > --- > net/ieee802154/6lowpan.c | 2 +- > net/mac802154/mac_cmd.c | 2 +- > net/mac802154/mib.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c > index cd5007f..17ad28f 100644 > --- a/net/ieee802154/6lowpan.c > +++ b/net/ieee802154/6lowpan.c > @@ -124,7 +124,7 @@ struct lowpan_fragment { > > static unsigned short fragment_tag; > static LIST_HEAD(lowpan_fragments); > -spinlock_t flist_lock; > +static spinlock_t flist_lock; > static DEFINE_SPINLOCK(flist_lock); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-02 at 08:37 +0200, Eric Dumazet wrote: > On Mon, 2012-07-02 at 10:18 +0400, Alexander Smirnov wrote: > > Make symbols static to avoid the following warning shown up > > by sparse: > > > > warning: symbol ... was not declared. Should it be static? > > > > Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> > > --- > > net/ieee802154/6lowpan.c | 2 +- > > net/mac802154/mac_cmd.c | 2 +- > > net/mac802154/mib.c | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c > > index cd5007f..17ad28f 100644 > > --- a/net/ieee802154/6lowpan.c > > +++ b/net/ieee802154/6lowpan.c > > @@ -124,7 +124,7 @@ struct lowpan_fragment { > > > > static unsigned short fragment_tag; > > static LIST_HEAD(lowpan_fragments); > > -spinlock_t flist_lock; > > +static spinlock_t flist_lock; > > > > static DEFINE_SPINLOCK(flist_lock); and of course commit 768f7c7c121e80f4 (6lowpan: add missing spin_lock_init() ) must be reverted. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-02 at 08:44 +0200, Eric Dumazet wrote: > On Mon, 2012-07-02 at 08:37 +0200, Eric Dumazet wrote: > > On Mon, 2012-07-02 at 10:18 +0400, Alexander Smirnov wrote: > > > Make symbols static to avoid the following warning shown up > > > by sparse: > > > > > > warning: symbol ... was not declared. Should it be static? > > > > > > Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> > > > --- > > > net/ieee802154/6lowpan.c | 2 +- > > > net/mac802154/mac_cmd.c | 2 +- > > > net/mac802154/mib.c | 2 +- > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c > > > index cd5007f..17ad28f 100644 > > > --- a/net/ieee802154/6lowpan.c > > > +++ b/net/ieee802154/6lowpan.c > > > @@ -124,7 +124,7 @@ struct lowpan_fragment { > > > > > > static unsigned short fragment_tag; > > > static LIST_HEAD(lowpan_fragments); > > > -spinlock_t flist_lock; > > > +static spinlock_t flist_lock; > > > > > > > static DEFINE_SPINLOCK(flist_lock); > > > and of course commit 768f7c7c121e80f4 (6lowpan: add missing > spin_lock_init() ) must be reverted. You should validate this code with LOCKDEP lowpan_dellink() does a spin_lock(&flist_lock); while same lock can be taken by lowpan_fragment_timer_expired() from timer irq, -> deadlock. del_timer() probably needs a del_timer_sync() too -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Eric, >> > static DEFINE_SPINLOCK(flist_lock); >> >> >> and of course commit 768f7c7c121e80f4 (6lowpan: add missing >> spin_lock_init() ) must be reverted. > > You should validate this code with LOCKDEP > > lowpan_dellink() does a spin_lock(&flist_lock); > while same lock can be taken by lowpan_fragment_timer_expired() from > timer irq, -> deadlock. > > del_timer() probably needs a del_timer_sync() too > Thanks a lot for the hints! Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-02 at 10:53 +0400, Alexander Smirnov wrote: > Dear Eric, > > >> > static DEFINE_SPINLOCK(flist_lock); > >> > >> > >> and of course commit 768f7c7c121e80f4 (6lowpan: add missing > >> spin_lock_init() ) must be reverted. > > > > You should validate this code with LOCKDEP > > > > lowpan_dellink() does a spin_lock(&flist_lock); > > while same lock can be taken by lowpan_fragment_timer_expired() from > > timer irq, -> deadlock. > > > > del_timer() probably needs a del_timer_sync() too > > > > Thanks a lot for the hints! While you are changing this code, please add in lowpan_alloc_new_frame() : - use netdev_alloc_skb_ip_align() instead of alloc_skb() to get some extra headroom in case we need to forward this frame in a tunnel or something... - initialize frame->lock -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-02 at 09:09 +0200, Eric Dumazet wrote: > mething... > > - initialize frame->lock or better, remove lock from struct lowpan_fragment -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Eric, just a several questions: >> > >> > static DEFINE_SPINLOCK(flist_lock); >> >> >> and of course commit 768f7c7c121e80f4 (6lowpan: add missing >> spin_lock_init() ) must be reverted. Do I need to create 2 separate patches: one for revert and second to initialize spinlock correctly, or I can combine these changes in one patch? > > You should validate this code with LOCKDEP Nothing was shown by LOCKDEP for 6lowpan. :-( I've selected the following options: -*- Spinlock and rw-lock debugging: basic checks -*- Mutex debugging: basic checks -*- Lock debugging: detect incorrect freeing of live locks [*] Lock usage statistics [*] Lock dependency engine debugging > > lowpan_dellink() does a spin_lock(&flist_lock); > while same lock can be taken by lowpan_fragment_timer_expired() from > timer irq, -> deadlock. What would be the best way to solve this context mismatch? Can I do something like following: 1. create some 6lowpan internal workqueue 2. replace lowpan_fragment_timer_expired() body by queue_work() with current list_deleting routine 3. when 6lowpan is going to be deleted - I'll flush the queue and remove all the timers and respective fragments Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-07-04 at 17:38 +0400, Alexander Smirnov wrote: > Do I need to create 2 separate patches: one for revert and second to > initialize spinlock correctly, or I can combine these changes in one > patch? > you can combine patch > > > > You should validate this code with LOCKDEP > > Nothing was shown by LOCKDEP for 6lowpan. :-( > Because path was not hit ( fragment expire ) You would have to simulate a drop or something to trigger the lockdep splat, when lowpan_fragment_timer_expired() fires. > I've selected the following options: > > -*- Spinlock and rw-lock debugging: basic checks > -*- Mutex debugging: basic checks > -*- Lock debugging: detect incorrect freeing of live locks > [*] Lock usage statistics > [*] Lock dependency engine debugging > > > > > lowpan_dellink() does a spin_lock(&flist_lock); > > while same lock can be taken by lowpan_fragment_timer_expired() from > > timer irq, -> deadlock. > > What would be the best way to solve this context mismatch? Can I do > something like following: > 1. create some 6lowpan internal workqueue > 2. replace lowpan_fragment_timer_expired() body by queue_work() with > current list_deleting routine > 3. when 6lowpan is going to be deleted - I'll flush the queue and > remove all the timers and respective fragments > Just use the spin_lock_bh() variant to disable BH, so that timer doesnt deadlock with you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c index cd5007f..17ad28f 100644 --- a/net/ieee802154/6lowpan.c +++ b/net/ieee802154/6lowpan.c @@ -124,7 +124,7 @@ struct lowpan_fragment { static unsigned short fragment_tag; static LIST_HEAD(lowpan_fragments); -spinlock_t flist_lock; +static spinlock_t flist_lock; static inline struct lowpan_dev_info *lowpan_dev_info(const struct net_device *dev) diff --git a/net/mac802154/mac_cmd.c b/net/mac802154/mac_cmd.c index 7f5403e..e6659fa 100644 --- a/net/mac802154/mac_cmd.c +++ b/net/mac802154/mac_cmd.c @@ -55,7 +55,7 @@ static int mac802154_mlme_start_req(struct net_device *dev, return 0; } -struct wpan_phy *mac802154_get_phy(const struct net_device *dev) +static struct wpan_phy *mac802154_get_phy(const struct net_device *dev) { struct mac802154_sub_if_data *priv = netdev_priv(dev); diff --git a/net/mac802154/mib.c b/net/mac802154/mib.c index 380829d..9cfa5f3 100644 --- a/net/mac802154/mib.c +++ b/net/mac802154/mib.c @@ -39,7 +39,7 @@ struct hw_addr_filt_notify_work { unsigned long changed; }; -struct mac802154_priv *mac802154_slave_get_priv(struct net_device *dev) +static struct mac802154_priv *mac802154_slave_get_priv(struct net_device *dev) { struct mac802154_sub_if_data *priv = netdev_priv(dev);
Make symbols static to avoid the following warning shown up by sparse: warning: symbol ... was not declared. Should it be static? Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> --- net/ieee802154/6lowpan.c | 2 +- net/mac802154/mac_cmd.c | 2 +- net/mac802154/mib.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)