Re: [PATCH 1/2] mac80211: Add TXQ scheduling API

[Johannes Berg <johannes@sipsolutions.net>]

>  	/* protects shared structure data */
>  	spinlock_t data_lock;
> -	/* protects: ar->txqs, artxq->list */
> -	spinlock_t txqs_lock;
>  
> -	struct list_head txqs;

I don't see you removing the artxq->list member, but surely it can't be
used any more now, without a list?

[snip] that's about all I can comment on ath*k :)

> +/**
> + * ieee80211_schedule_txq - add txq to scheduling loop
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface

or more likely next_txq()? :)

> + * Returns true if the txq was actually added to the scheduling,
> + * false otherwise.

Perhaps use %true/%false.

>  	if (sta->sta.txq[0]) {
> +		bool wake = false;

please add a blank line after this new line

>  		for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
>  			if (!txq_has_queue(sta->sta.txq[i]))
>  				continue;
>  
> -			drv_wake_tx_queue(local, to_txq_info(sta->sta.txq[i]));
> +			wake = wake || ieee80211_schedule_txq(&local->hw, sta->sta.txq[i]);
>  		}
> +		if (wake)
> +			drv_wake_tx_queue(local);
>  	}

Are you sure you want to skip the call to ieee80211_schedule_txq() if
wake is true? I think you don't, but boolean short-circuit evaluation
would lead to that, afaict?

So it better be written as

  if (ieee80211_schedule_txq(...))
    wake = true;

No need to even check wake anyway, since it can only ever go from false
to true.

(Also, that line is getting a bit long)

>  	skb_queue_head_init(&pending);
> diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
> index 3d9ac17af407..531c0e7f2358 100644
> --- a/net/mac80211/trace.h
> +++ b/net/mac80211/trace.h
> @@ -2550,33 +2550,20 @@ TRACE_EVENT(drv_tdls_recv_channel_switch,
>  );
>  
>  TRACE_EVENT(drv_wake_tx_queue,
> -	TP_PROTO(struct ieee80211_local *local,
> -		 struct ieee80211_sub_if_data *sdata,
> -		 struct txq_info *txq),
> +	TP_PROTO(struct ieee80211_local *local),

You should just replace all of this with

DEFINE_EVENT(local_only_evt, drv_wake_tx_queue,
        TP_PROTO(struct ieee80211_local *local),
        TP_ARGS(local)
);

now.

> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
> +			     struct ieee80211_txq *txq)
> +{
> +	struct ieee80211_local *local = hw_to_local(hw);
> +	struct txq_info *txqi = to_txq_info(txq);
> +	int ret = 0;

bool/false/true please.

> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw)
> +{
> +	struct ieee80211_local *local = hw_to_local(hw);
> +	struct txq_info *txqi = NULL;
> +
> +	spin_lock_bh(&local->active_txq_lock);
> +
> +	if (list_empty(&local->active_txqs))
> +		goto out;
> +
> +	txqi = list_first_entry(&local->active_txqs, struct txq_info, schedule_order);

that line seems pretty long, but I haven't counted :)

johannes