Re: [PATCH v2 4/5] staging: rtl8712: fix multiline derefernce warning

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux