Re: [PATCH 4/4] ath6kl: Add UAPSD support in tx path.

[Kalle Valo <kvalo@qca.qualcomm.com>]

On 01/09/2012 09:45 AM, Thirumalai wrote:
> If tx is not because of uAPSD trigger and if power save is
> enabled then queue the packet uAPSD queue.
> 
> Signed-off-by: Thirumalai <tpachamu@qca.qualcomm.com>

[...]

> +static bool ath6kl_process_uapsdq(struct ath6kl_sta *conn,
> +				struct ath6kl_vif *vif,
> +				struct sk_buff *skb,
> +				u32 *flags)
> +{
> +	struct ath6kl *ar = vif->ar;
> +	bool ps_queued = false, is_apsdq_empty = false;
> +	struct ethhdr *datap = (struct ethhdr *) skb->data;
> +	u8 up = 0;
> +	u8 traffic_class;
> +	u16 ether_type;
> +	u8 *ip_hdr;
> +	struct ath6kl_llc_snap_hdr *llc_hdr;

There's a lot unnecessarily indented code here which is makes it harder
to read than it really is. A good rule to follow is that always try to
indent code only for error paths, the main code flow should not be indented.

For example, you could do it like this and you can get rid of ps_queued
variable as well:

if (conn->sta_flags & STA_PS_APSD_TRIGGER) {
	...
	return false;
} else if (!conn->apsd_info) {
	return false;
}

....

if ((conn->apsd_info & (1 << traffic_class) == 0)
	return false;

...

return true;

> +		if (test_bit(WMM_ENABLED, &vif->flags)) {
> +			ether_type = datap->h_proto;
> +			if (is_ethertype(be16_to_cpu(ether_type))) {
> +				/* packet is in DIX format  */
> +				ip_hdr = (u8 *)(datap + 1);
> +			} else {
> +				/* packet is in 802.3 format */
> +				llc_hdr = (struct ath6kl_llc_snap_hdr *)
> +							(datap + 1);
> +				ether_type = llc_hdr->eth_type;
> +				ip_hdr = (u8 *)(llc_hdr + 1);
> +			}
> +
> +			if (ether_type == cpu_to_be16(IP_ETHERTYPE))
> +				up = ath6kl_wmi_determine_user_priority(
> +						ip_hdr, 0);
> +		}

And from earlier:

+	u8 up = 0;

So up will be zero if wmm is not enabled? Maybe you could make it more
obvious by not initialising the variable in the beginning of function
but instead adding an else branch and initialising it to zero there.

> +static bool ath6kl_process_psq(struct ath6kl_sta *conn,
> +				struct ath6kl_vif *vif,
> +				struct sk_buff *skb,
> +				u32 *flags)
> +{
> +	bool ps_queued = false, is_psq_empty = false;
> +	struct ath6kl *ar = vif->ar;
> +
> +	if (conn->sta_flags & STA_PS_POLLED) {
> +		spin_lock_bh(&conn->psq_lock);
> +		if (!skb_queue_empty(&conn->psq))
> +			*flags |= WMI_DATA_HDR_FLAGS_MORE;
> +		spin_unlock_bh(&conn->psq_lock);
> +	} else {
> +		/* Queue the frames if the STA is sleeping */
> +		spin_lock_bh(&conn->psq_lock);
> +		is_psq_empty = skb_queue_empty(&conn->psq);
> +		skb_queue_tail(&conn->psq, skb);
> +		spin_unlock_bh(&conn->psq_lock);
> +
> +		/*
> +		 * If this is the first pkt getting queued
> +		 * for this STA, update the PVB for this
> +		 * STA.
> +		 */
> +		if (is_psq_empty)
> +			ath6kl_wmi_set_pvb_cmd(ar->wmi,
> +					vif->fw_vif_idx,
> +					conn->aid, 1);
> +		ps_queued = true;
> +	}
> +	return ps_queued;
> +}

Similar comment about code flow. If you do it like this:

if (conn->sta_flags & STA_PS_POLLED) {
	...
	return false;
}

....

return true;

it will be easier to read and ps_queued is not needed.

Kalle