Date:   Mon, 25 Mar 2019 14:34:39 +0000
From:   Brian Norris <briannorris@chromium.org>
To:     yhchuang@realtek.com
Cc:     kvalo@codeaurora.org, johannes@sipsolutions.net,
        linux-wireless@vger.kernel.org, gregkh@linuxfoundation.org,
        sgruszka@redhat.com, pkshih@realtek.com, tehuang@realtek.com,
        Larry.Finger@lwfinger.net
Subject: Re: [PATCH v8 03/14] rtw88: hci files
Message-ID: <20190325143439.pxe2sppqssdad4in@penguin>
References: <1552450443-351-1-git-send-email-yhchuang@realtek.com>
 <1552450443-351-4-git-send-email-yhchuang@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1552450443-351-4-git-send-email-yhchuang@realtek.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi,

On Wed, Mar 13, 2019 at 12:13:52PM +0800, yhchuang@realtek.com wrote:
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> new file mode 100644
> index 0000000..cf3bffb
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -0,0 +1,1211 @@
...
> +static u8 ac_to_hwq[] = {
> +	[0] = RTW_TX_QUEUE_VO,
> +	[1] = RTW_TX_QUEUE_VI,
> +	[2] = RTW_TX_QUEUE_BE,
> +	[3] = RTW_TX_QUEUE_BK,
> +};

I don't want to dig up and repeat the details of my comments, but I
think you missed my earlier suggestion here: you should use the
ieee80211_ac_numbers enum instead of bare 0-3 indexing.

> +static u8 rtw_hw_queue_mapping(struct sk_buff *skb)
> +{
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> +	__le16 fc = hdr->frame_control;
> +	u8 q_mapping = skb_get_queue_mapping(skb);
> +	u8 queue;
> +
> +	if (unlikely(ieee80211_is_beacon(fc)))
> +		queue = RTW_TX_QUEUE_BCN;
> +	else if (unlikely(ieee80211_is_mgmt(fc) || ieee80211_is_ctl(fc)))
> +		queue = RTW_TX_QUEUE_MGMT;
> +	else
> +		queue = ac_to_hwq[q_mapping];

It also seems wise to be defensive about the values of 'q_mapping', so
we don't accidentally overstep the array if for some reason the
implementation details of mac80211 change some day. e.g.:

	else if (WARN_ON_ONCE(q_mapping >= ARRAY_SIZE(ac_to_hwq)))
		// Pick a default.

Brian

> +
> +	return queue;
> +}
> +