[PATCH net-next 0/0] fix sparse warnings

[PATCH net-next 0/0] fix sparse warnings

[Alexander Smirnov]

Hello David,

I've received a notification from Fengguang Wu that there are new sparse
warnings are shown up after my patch sets.

Completely forgot about 'sparse', aghhr...

These patches fixes the following sparse warnings:
 - drivers/ieee802154/at86rf230.c:610:2: warning:
   'rc' may be used uninitialized in this function [-Wuninitialized]
 - net/ieee802154/6lowpan.c:127:12: sparse:
   symbol 'flist_lock' was not declared. Should it be static?
 - net/mac802154/mac_cmd.c:58:17: warning:
   symbol 'mac802154_get_phy' was not declared. Should it be static?
 - net/mac802154/mib.c:42:23: warning:
   symbol 'mac802154_slave_get_priv' was not declared. Should it be static?

With best regards,
Alexander Smirnov

Alexander Smirnov (2):
  ieee802154: sparse warnings: make symbols static
  drivers/ieee802154/at231rf230: remove unused return status

 drivers/ieee802154/at86rf230.c |    3 +--
 net/ieee802154/6lowpan.c       |    2 +-
 net/mac802154/mac_cmd.c        |    2 +-
 net/mac802154/mib.c            |    2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

--
1.7.2.3

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

[Alexander Smirnov]

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

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Eric Dumazet]

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

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Eric Dumazet]

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.

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Eric Dumazet]

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

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Alexander Smirnov]

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

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Eric Dumazet]

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

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Eric Dumazet]

On Mon, 2012-07-02 at 09:09 +0200, Eric Dumazet wrote:
> mething...
> 
> - initialize frame->lock

or better, remove lock from struct lowpan_fragment

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Alexander Smirnov]

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

Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static

[Eric Dumazet]

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.

[PATCH net-next 2/2] drivers/ieee802154/at231rf230: remove unused return status

[Alexander Smirnov]

Remove excessive variable used for the return status.

Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
---
 drivers/ieee802154/at86rf230.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/ieee802154/at86rf230.c b/drivers/ieee802154/at86rf230.c
index 4d033d4..902e38b 100644
--- a/drivers/ieee802154/at86rf230.c
+++ b/drivers/ieee802154/at86rf230.c
@@ -585,7 +585,6 @@ err:
 static int at86rf230_rx(struct at86rf230_local *lp)
 {
 	u8 len = 128, lqi = 0;
-	int rc;
 	struct sk_buff *skb;
 
 	skb = alloc_skb(len, GFP_KERNEL);
@@ -607,7 +606,7 @@ static int at86rf230_rx(struct at86rf230_local *lp)
 
 	ieee802154_rx_irqsafe(lp->dev, skb, lqi);
 
-	dev_dbg(&lp->spi->dev, "READ_FBUF: %d %d %x\n", rc, len, lqi);
+	dev_dbg(&lp->spi->dev, "READ_FBUF: %d %x\n", len, lqi);
 
 	return 0;
 err:
-- 
1.7.2.3

Re: [PATCH net-next 2/2] drivers/ieee802154/at231rf230: remove unused return status

[David Miller]

From: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
Date: Mon,  2 Jul 2012 10:18:32 +0400

> Remove excessive variable used for the return status.
> 
> Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>

Applied.