Re: [PATCH v2 4/5] staging: rtl8712: fix multiline derefernce warning
From: Joe Perches
Date: Sat Mar 28 2020 - 15:19:18 EST
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];