Re: [PATCH v2 4/5] staging: rtl8712: fix multiline derefernce warning
From: Aiman Najjar
Date: Sat Mar 28 2020 - 20:18:14 EST
On Sat, Mar 28, 2020 at 12:17:19PM -0700, Joe Perches wrote:
> 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];
>
>
Thanks very much for your review and suggestions, Joe! That
all makes sense to me, I will submit a revised patchset.