[rt2x00-users] [RFC 2/8] rt2x00: Introduce an atomic beacon update method

[Johannes Stezenbach]

On Thu, Dec 09, 2010 at 12:52:23PM +0100, Helmut Schaa wrote:
>  /**
> - * rt2x00queue_update_beacon - Send new beacon from mac80211 to hardware
> + * rt2x00queue_update_beacon_nonatomic - Send new beacon from mac80211
> + *	to hardware in non atomic context.
>   * @rt2x00dev: Pointer to &struct rt2x00_dev.
>   * @vif: Interface for which the beacon should be updated.
>   */
> -int rt2x00queue_update_beacon(struct rt2x00_dev *rt2x00dev,
> -			      struct ieee80211_vif *vif);
> +int rt2x00queue_update_beacon_nonatomic(struct rt2x00_dev *rt2x00dev,
> +					struct ieee80211_vif *vif);
> +
> +/**
> + * rt2x00queue_update_beacon_atomic - Send new beacon from mac80211
> + *	to hardware in atomic context. If process context is currently
> + *	updating the beacon this beacon update request will get dropped.
> + * @rt2x00dev: Pointer to &struct rt2x00_dev.
> + * @vif: Interface for which the beacon should be updated.
> + */
> +int rt2x00queue_update_beacon_atomic(struct rt2x00_dev *rt2x00dev,
> +				     struct ieee80211_vif *vif);

I think the function names should follow the kernel
tradition

rt2x00queue_update_beacon()   // takes lock itself

__rt2x00queue_update_beacon() or
rt2x00queue_update_beacon_locked() // locking must be done by caller

"atomic" suggests it does something similar to atomic_inc(),
which is misleading.

> +int rt2x00queue_update_beacon_atomic(struct rt2x00_dev *rt2x00dev,
> +				     struct ieee80211_vif *vif)
> +{
> +	struct rt2x00_intf *intf = vif_to_intf(vif);
> +
> +	/*
> +	 * If a beacon update is happening currently in process context
> +	 * just drop this update request.
> +	 */
> +	if (mutex_is_locked(&intf->beacon_skb_mutex))
> +		return 0;
> +
> +	return rt2x00queue_update_beacon(rt2x00dev, vif);
> +}
> +
> +int rt2x00queue_update_beacon_nonatomic(struct rt2x00_dev *rt2x00dev,
> +					struct ieee80211_vif *vif)
> +{
> +	struct rt2x00_intf *intf = vif_to_intf(vif);
> +	int ret;
> +
> +	mutex_lock(&intf->beacon_skb_mutex);
> +	ret = rt2x00queue_update_beacon(rt2x00dev, vif);
>  	mutex_unlock(&intf->beacon_skb_mutex);
>  
> -	return 0;
> +	return ret;
>  }

On SMP, irq or tasklet can run on one CPU while workqueue
can run on another, right?  If so then this is racy.


Johannes