Authenticated-By: 
From:   Tony Chuang <yhchuang@realtek.com>
To:     Brian Norris <briannorris@chromium.org>
CC:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "sgruszka@redhat.com" <sgruszka@redhat.com>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>
Subject: RE: [PATCH v8 03/14] rtw88: hci files
Thread-Topic: [PATCH v8 03/14] rtw88: hci files
Thread-Index: AQHU2VNRV2ab8RA6e0q0FRHPGCHboaYb9v2AgAMLpqA=
Date:   Wed, 27 Mar 2019 05:07:17 +0000
Message-ID: <F7CD281DE3E379468C6D07993EA72F84D17CB987@RTITMBSVM04.realtek.com.tw>
References: <1552450443-351-1-git-send-email-yhchuang@realtek.com>
 <1552450443-351-4-git-send-email-yhchuang@realtek.com>
 <20190325143439.pxe2sppqssdad4in@penguin>
In-Reply-To: <20190325143439.pxe2sppqssdad4in@penguin>
Accept-Language: zh-TW, en-US
Content-Language: zh-TW
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
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.

Fixed, thanks. Will be in the next patches.

> 
> > +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.

Yeah, should check if q_mapping does not fit into the array.
Thanks.

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

Yan-Hsuan