From: Joe Perches <joe@perches.com>
To: aimannajjar <aiman.najjar@hurranet.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
Florian Schilhabel <florian.c.schilhabel@googlemail.com>,
devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/5] staging: rtl8712: fix multiline derefernce warning
Date: Sat, 28 Mar 2020 12:17:19 -0700 [thread overview]
Message-ID: <197b261122fc6a751a163545044195f2d98d79dc.camel@perches.com> (raw)
In-Reply-To: <beb6c8c826cdda751f29f985f2b5dedfd2f87914.1585353747.git.aiman.najjar@hurranet.com>
On Fri, 2020-03-27 at 20:08 -0400, aimannajjar wrote:
> This patch fixes the following checkpatch warning in
> rtl8712x_xmit.c:
>
> WARNING: Avoid multiple line dereference - prefer 'psta->sta_xmitpriv.txseq_tid[pattrib->priority]'
> 544: FILE: drivers/staging//rtl8712/rtl871x_xmit.c:544:
> + pattrib->seqnum = psta->sta_xmitpriv.
> + txseq_tid[pattrib->priority];
It's always better to try to make the code clearer than
merely shut up checkpatch bleats.
> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
[]
> @@ -479,70 +479,70 @@ static int make_wlanhdr(struct _adapter *padapter, u8 *hdr,
> __le16 *fctrl = &pwlanhdr->frame_ctl;
>
> memset(hdr, 0, WLANHDR_OFFSET);
> - SetFrameSubType(fctrl, pattrib->subtype);
> - if (pattrib->subtype & WIFI_DATA_TYPE) {
> + SetFrameSubType(fctrl, pattr->subtype);
> + if (pattr->subtype & WIFI_DATA_TYPE) {
>
The whole following block could be outdented one level
by changing this test to
if (!(pattr->subtype & WIFI_DATA_TYPE))
return 0;
> if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> /* to_ds = 1, fr_ds = 0; */
> SetToDs(fctrl);
> memcpy(pwlanhdr->addr1, get_bssid(pmlmepriv),
> ETH_ALEN);
The repetitive call to get_bssid(pmlmepriv) could be saved
by performing it outside this test
u8 bssid = get_bssid(pmlmepriv);
and then using bssid in each memcpy/ether_addr_copy
> - memcpy(pwlanhdr->addr2, pattrib->src, ETH_ALEN);
> - memcpy(pwlanhdr->addr3, pattrib->dst, ETH_ALEN);
> + memcpy(pwlanhdr->addr2, pattr->src, ETH_ALEN);
> + memcpy(pwlanhdr->addr3, pattr->dst, ETH_ALEN);
All of these memcpy could probably use ether_addr_copy if
struct pkt_attrib {
...
u8 dst[ETH_ALEN];
...
};
was changed to
u8 dst[ETH_ALEN] __aligned(2);
so these would be
ether_addr_copy(pwlanhdr->addr2, pattr->src);
and pwlanhdr isn't a particularly valuable name
for an automatic either. It's hungarian like.
So I would suggest something like the below that
avoids any long lines instead and also removes
unnecessary multi-line statements without renaming.
---
drivers/staging/rtl8712/rtl871x_xmit.c | 123 ++++++++++++++++-----------------
drivers/staging/rtl8712/rtl871x_xmit.h | 2 +-
2 files changed, 61 insertions(+), 64 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
index f0b853..3961dae 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -477,75 +477,72 @@ static int make_wlanhdr(struct _adapter *padapter, u8 *hdr,
struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
struct qos_priv *pqospriv = &pmlmepriv->qospriv;
__le16 *fctrl = &pwlanhdr->frame_ctl;
+ u8 *bssid;
memset(hdr, 0, WLANHDR_OFFSET);
SetFrameSubType(fctrl, pattrib->subtype);
- if (pattrib->subtype & WIFI_DATA_TYPE) {
- if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
- /* to_ds = 1, fr_ds = 0; */
- SetToDs(fctrl);
- memcpy(pwlanhdr->addr1, get_bssid(pmlmepriv),
- ETH_ALEN);
- memcpy(pwlanhdr->addr2, pattrib->src, ETH_ALEN);
- memcpy(pwlanhdr->addr3, pattrib->dst, ETH_ALEN);
- } else if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
- /* to_ds = 0, fr_ds = 1; */
- SetFrDs(fctrl);
- memcpy(pwlanhdr->addr1, pattrib->dst, ETH_ALEN);
- memcpy(pwlanhdr->addr2, get_bssid(pmlmepriv),
- ETH_ALEN);
- memcpy(pwlanhdr->addr3, pattrib->src, ETH_ALEN);
- } else if (check_fwstate(pmlmepriv, WIFI_ADHOC_STATE) ||
- check_fwstate(pmlmepriv,
- WIFI_ADHOC_MASTER_STATE)) {
- memcpy(pwlanhdr->addr1, pattrib->dst, ETH_ALEN);
- memcpy(pwlanhdr->addr2, pattrib->src, ETH_ALEN);
- memcpy(pwlanhdr->addr3, get_bssid(pmlmepriv),
- ETH_ALEN);
- } else if (check_fwstate(pmlmepriv, WIFI_MP_STATE)) {
- memcpy(pwlanhdr->addr1, pattrib->dst, ETH_ALEN);
- memcpy(pwlanhdr->addr2, pattrib->src, ETH_ALEN);
- memcpy(pwlanhdr->addr3, get_bssid(pmlmepriv),
- ETH_ALEN);
- } else {
- return -EINVAL;
- }
+ if (!(pattrib->subtype & WIFI_DATA_TYPE))
+ return 0;
- if (pattrib->encrypt)
- SetPrivacy(fctrl);
- if (pqospriv->qos_option) {
- qc = (unsigned short *)(hdr + pattrib->hdrlen - 2);
- if (pattrib->priority)
- SetPriority(qc, pattrib->priority);
- SetAckpolicy(qc, pattrib->ack_policy);
- }
- /* TODO: fill HT Control Field */
- /* Update Seq Num will be handled by f/w */
- {
- struct sta_info *psta;
- bool bmcst = is_multicast_ether_addr(pattrib->ra);
-
- if (pattrib->psta) {
- psta = pattrib->psta;
- } else {
- if (bmcst)
- psta = r8712_get_bcmc_stainfo(padapter);
- else
- psta =
- r8712_get_stainfo(&padapter->stapriv,
- pattrib->ra);
- }
- if (psta) {
- psta->sta_xmitpriv.txseq_tid
- [pattrib->priority]++;
- psta->sta_xmitpriv.txseq_tid[pattrib->priority]
- &= 0xFFF;
- pattrib->seqnum = psta->sta_xmitpriv.
- txseq_tid[pattrib->priority];
- SetSeqNum(hdr, pattrib->seqnum);
- }
+ bssid = get_bssid(pmlmepriv);
+
+ if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
+ /* to_ds = 1, fr_ds = 0; */
+ SetToDs(fctrl);
+ ether_addr_copy(pwlanhdr->addr1, bssid);
+ ether_addr_copy(pwlanhdr->addr2, pattrib->src);
+ ether_addr_copy(pwlanhdr->addr3, pattrib->dst);
+ } else if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
+ /* to_ds = 0, fr_ds = 1; */
+ SetFrDs(fctrl);
+ ether_addr_copy(pwlanhdr->addr1, pattrib->dst);
+ ether_addr_copy(pwlanhdr->addr2, bssid);
+ ether_addr_copy(pwlanhdr->addr3, pattrib->src);
+ } else if (check_fwstate(pmlmepriv, WIFI_ADHOC_STATE) ||
+ check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE)) {
+ ether_addr_copy(pwlanhdr->addr1, pattrib->dst);
+ ether_addr_copy(pwlanhdr->addr2, pattrib->src);
+ ether_addr_copy(pwlanhdr->addr3, bssid);
+ } else if (check_fwstate(pmlmepriv, WIFI_MP_STATE)) {
+ ether_addr_copy(pwlanhdr->addr1, pattrib->dst);
+ ether_addr_copy(pwlanhdr->addr2, pattrib->src);
+ ether_addr_copy(pwlanhdr->addr3, bssid);
+ } else {
+ return -EINVAL;
+ }
+
+ if (pattrib->encrypt)
+ SetPrivacy(fctrl);
+ if (pqospriv->qos_option) {
+ qc = (unsigned short *)(hdr + pattrib->hdrlen - 2);
+ if (pattrib->priority)
+ SetPriority(qc, pattrib->priority);
+ SetAckpolicy(qc, pattrib->ack_policy);
+ }
+ /* TODO: fill HT Control Field */
+ /* Update Seq Num will be handled by f/w */
+ {
+ struct sta_info *psta;
+ bool bmcst = is_multicast_ether_addr(pattrib->ra);
+
+ if (pattrib->psta)
+ psta = pattrib->psta;
+ else if (bmcst)
+ psta = r8712_get_bcmc_stainfo(padapter);
+ else
+ psta = r8712_get_stainfo(&padapter->stapriv,
+ pattrib->ra);
+
+ if (psta) {
+ u16 *txtid = psta->sta_xmitpriv.txseq_tid;
+
+ txtid[pattrib->priority]++;
+ txtid[pattrib->priority] &= 0xFFF;
+ pattrib->seqnum = txtid[pattrib->priority];
+ SetSeqNum(hdr, pattrib->seqnum);
}
}
+
return 0;
}
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.h b/drivers/staging/rtl8712/rtl871x_xmit.h
index f227828..c0c0c7 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.h
+++ b/drivers/staging/rtl8712/rtl871x_xmit.h
@@ -115,7 +115,7 @@ struct pkt_attrib {
u8 icv_len;
unsigned char iv[8];
unsigned char icv[8];
- u8 dst[ETH_ALEN];
+ u8 dst[ETH_ALEN] __aligned(2); /* for ether_addr_copy */
u8 src[ETH_ALEN];
u8 ta[ETH_ALEN];
u8 ra[ETH_ALEN];
next prev parent reply other threads:[~2020-03-28 19:19 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-26 5:56 [PATCH] staging: rtl8712: fix checkpatch warnings Aiman Najjar
2020-03-27 8:04 ` Greg Kroah-Hartman
2020-03-28 0:08 ` [PATCH v2 0/5] staging: rtl8712: fix rtl871x_xmit.c warnings aimannajjar
2020-03-28 0:08 ` [PATCH v2 1/5] staging: rtl8712: fix checkpatch long-line warning aimannajjar
2020-04-02 8:17 ` Dan Carpenter
2020-04-02 8:19 ` Dan Carpenter
2020-03-28 0:08 ` [PATCH v2 2/5] staging: rtl8712: fix long-line checkpatch warning aimannajjar
2020-03-28 0:08 ` [PATCH v2 3/5] staging: rtl8712: fix checkpatch warnings aimannajjar
2020-03-28 0:08 ` [PATCH v2 4/5] staging: rtl8712: fix multiline derefernce warning aimannajjar
2020-03-28 19:17 ` Joe Perches [this message]
2020-03-28 19:30 ` Aiman Najjar
2020-04-02 8:16 ` Dan Carpenter
2020-03-28 0:08 ` [PATCH v2 5/5] staging: rtl8712:fix multiline derefernce warnings aimannajjar
2020-04-02 8:12 ` [PATCH] staging: rtl8712: fix checkpatch warnings Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=197b261122fc6a751a163545044195f2d98d79dc.camel@perches.com \
--to=joe@perches.com \
--cc=Larry.Finger@lwfinger.net \
--cc=aiman.najjar@hurranet.com \
--cc=devel@driverdev.osuosl.org \
--cc=florian.c.schilhabel@googlemail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®