Re: [PATCH bluetooth-next 2/4] mac802154: remove pib lock

[Alexander Aring <alex.aring@gmail.com>]

On Fri, May 22, 2015 at 02:19:05PM +0200, Stefan Schmidt wrote:

...

> >  	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
> >  		rc = drv_set_promiscuous_mode(local,
> >  					      wpan_dev->promiscuous_mode);
> >@@ -295,11 +292,7 @@ static int mac802154_wpan_open(struct net_device *dev)
> >  			goto out;
> >  	}
> >-	mutex_unlock(&phy->pib_lock);
> >-	return 0;
> 
> Hmm, why did you remove the return 0; here? Is this supposed to fall through
> to out: and use the return rc now?
> 

This function is the netdev open call of an wpan interface. In this call
we do at the moment all MAC dependent settings which are done by phy.
e.g. address filtering, frame reties, etc...

When this call failed the interface is still down. This callback is
usually called on an ifup. If it's successful it's up, if not still down.

The last call is:

        if (local->hw.flags & IEEE802154_HW_FRAME_RETRIES) {
                rc = drv_set_max_frame_retries(local, wpan_dev->frame_retries);
                if (rc < 0)
                        goto out;
        }

Which is the driver callback which have an identically return value
indicator, means -ERRNO if failed, according to the netdev open call.


We can still use the return 0 on successful or the rc. It's a silent
cleanup which I did here (hope that was okay).


btw:
Also I don't know why there is a pib hold, when some of them are mib
attributes. This all makes no sense for me.

To hold the pib_lock will represent the documentation of driver_ops, but
this makes no sense when you hold the pib lock for mib attributes in
this case of callback and when accessing mib attributes over netlink or
such else to hold the mib lock and not the pib lock, something is wrong
there.

Now, we doing everything over rtnl, which can be indicated by [0] flag
to hold this lock while netlink command and the _most_ mib attributes are
readonly while interface up. This will occur that we need don't care
about locking if the interface is up. For attributes like address
filtering this is also "impossible" to set address filtering registers
while the interface is running. The best option here is to set address
filtering while ifup and then don't allo the change the addresses while
having the interface up. This is what we have now.


The dsn/bsn values are special here, this need to be writeable while ifup.
Also we need to think about handling setting of short_address while
assoc with coordinators, but I think this will take some time to support
such feature. I am sure we will find some solution.

- Alex

[0] http://lxr.free-electrons.com/source/net/ieee802154/nl802154.c#L799