Patchwork [net-next,1/2] ieee802154: sparse warnings: make symbols static

login
register
mail settings
Submitter alex.bluesman.smirnov@gmail.com
Date July 2, 2012, 6:18 a.m.
Message ID <1341209912-6030-2-git-send-email-alex.bluesman.smirnov@gmail.com>
Download mbox | patch
Permalink /patch/168499/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

alex.bluesman.smirnov@gmail.com - July 2, 2012, 6:18 a.m.
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(-)
Eric Dumazet - July 2, 2012, 6:37 a.m.
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
Eric Dumazet - July 2, 2012, 6:44 a.m.
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
Eric Dumazet - July 2, 2012, 6:49 a.m.
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
alex.bluesman.smirnov@gmail.com - July 2, 2012, 6:53 a.m.
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
Eric Dumazet - July 2, 2012, 7:09 a.m.
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
Eric Dumazet - July 2, 2012, 7:13 a.m.
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
alex.bluesman.smirnov@gmail.com - July 4, 2012, 1:38 p.m.
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
Eric Dumazet - July 4, 2012, 1:45 p.m.
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

Patch

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);