From: Aiman Najjar <aiman.najjar@hurranet.com>
To: Joe Perches <joe@perches.com>
Cc: devel@driverdev.osuosl.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Florian Schilhabel <florian.c.schilhabel@googlemail.com>,
linux-kernel@vger.kernel.org,
Larry Finger <Larry.Finger@lwfinger.net>
Subject: Re: [PATCH v2 4/5] staging: rtl8712: fix multiline derefernce warning
Date: Sat, 28 Mar 2020 15:30:10 -0400 [thread overview]
Message-ID: <20200328193008.GA5108@kernel-dev> (raw)
In-Reply-To: <197b261122fc6a751a163545044195f2d98d79dc.camel@perches.com>
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.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-03-29 0:18 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
2020-03-28 19:30 ` Aiman Najjar [this message]
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=20200328193008.GA5108@kernel-dev \
--to=aiman.najjar@hurranet.com \
--cc=Larry.Finger@lwfinger.net \
--cc=devel@driverdev.osuosl.org \
--cc=florian.c.schilhabel@googlemail.com \
--cc=gregkh@linuxfoundation.org \
--cc=joe@perches.com \
--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;
as well as URLs for NNTP newsgroup(s).