On Tue, Oct 20, 2020 at 08:17:48AM -0700, Elena Afanasova wrote:
> Reported by checkpatch.pl
>
If I were trying to clean up this driver I would probably take a
different approach.
Just send a patch that introduces line breaks for RT_TRACE() printks.
The RT_TRACE() printks are super ugly, and if you add newlines to them,
it can't make it worse than it already is. Do not change the RT_TRACE()
output.
- ("=== curfragnum=%d, pframe = 0x%.2x, 0x%.2x, 0x%.2x, 0x%.2x, 0x%.2x, 0x%.2x, 0x%.2x, 0x%.2x,!!!\n",
+ ("curfrnum=%d: %.2x-%.2x-%.2x-%.2x-%.2x-%.2x-%.2x-%.2x\n",
Your change here is an improvement but it requires thinking and
reviewers don't want to look at RT_TRACE(). Make your patches as
uncontroversial as possible.
Then for the other things there are some changes which are good, but in
other places the correct thing is to introduce new functions. So what
I would want instead is a series of patches which do small clean ups
which make the code obviously better, and are not just to make checkpatch
happy.
> Signed-off-by: Elena Afanasova <eafanasova@xxxxxxxxx>
> ---
> drivers/staging/rtl8188eu/core/rtw_xmit.c | 118 +++++++++++++++-------
> 1 file changed, 81 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c b/drivers/staging/rtl8188eu/core/rtw_xmit.c
> index 317355f830cb..51769f2ca910 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
> @@ -44,7 +44,9 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> u32 max_xmit_extbuf_size = MAX_XMIT_EXTBUF_SZ;
> u32 num_xmit_extbuf = NR_XMIT_EXTBUFF;
>
> - /* We don't need to memset padapter->XXX to zero, because adapter is allocated by vzalloc(). */
> + /* We don't need to memset padapter->XXX to zero,
> + * because adapter is allocated by vzalloc().
> + */
This comment is totally pointless. Kernel devs are expected to
understand what the "z" in vzalloc() means. Just delete it.
>
> spin_lock_init(&pxmitpriv->lock);
>
> @@ -127,7 +129,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> res = rtw_os_xmit_resource_alloc(pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> if (res == _FAIL) {
> msleep(10);
> - res = rtw_os_xmit_resource_alloc(pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> + res = rtw_os_xmit_resource_alloc(pxmitbuf, (MAX_XMITBUF_SZ +
> + XMITBUF_ALIGN_SZ));
Align it like this:
res = rtw_os_xmit_resource_alloc(pxmitbuf,
MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ);
The rtw_os_xmit_resource_alloc() function is poorly named.
> if (res == _FAIL)
> goto exit;
> }
> @@ -441,7 +444,9 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
> ((tmp[21] == 67) && (tmp[23] == 68))) {
> /* 68 : UDP BOOTP client */
> /* 67 : UDP BOOTP server */
> - RT_TRACE(_module_rtl871x_xmit_c_, _drv_err_, ("====================== %s: get DHCP Packet\n", __func__));
> + RT_TRACE(_module_rtl871x_xmit_c_, _drv_err_,
> + ("====================== %s: get DHCP Packet\n",
> + __func__));
What does this message really mean? The "===" characters don't add
anything. It's probably better to just delete this printk.
> /* Use low rate to send DHCP packet. */
> pattrib->dhcp_pkt = 1;
> }
> @@ -455,7 +460,9 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
> rtw_set_scan_deny(padapter, 3000);
>
> /* If EAPOL , ARP , OR DHCP packet, driver must be in active mode. */
> - if ((pattrib->ether_type == ETH_P_ARP) || (pattrib->ether_type == ETH_P_PAE) || (pattrib->dhcp_pkt == 1))
> + if ((pattrib->ether_type == ETH_P_ARP) ||
> + (pattrib->ether_type == ETH_P_PAE) ||
> + (pattrib->dhcp_pkt == 1))
This change is good.
> rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_SPECIAL_PACKET, 1);
>
> mcast = is_multicast_ether_addr(pattrib->ra);
> @@ -466,7 +473,9 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
> } else {
> psta = rtw_get_stainfo(pstapriv, pattrib->ra);
> if (!psta) { /* if we cannot get psta => drrp the pkt */
> - RT_TRACE(_module_rtl871x_xmit_c_, _drv_alert_, ("\nupdate_attrib => get sta_info fail, ra: %pM\n", (pattrib->ra)));
> + RT_TRACE(_module_rtl871x_xmit_c_, _drv_alert_,
> + ("\nupdate_attrib => get sta_info fail, ra: %pM\n",
> + (pattrib->ra)));
I guess this is fine. All the RT_TRACE output just looks so terrible...
Why does it sometimes have a newline at the start and sometimes not?
In one place you improved the RT_TRACE() output which is good but
probably should be done in a separate patch...
Anyway, I'm done thinking about RT_TRACE() because it makes me want to
poke out my own eyeballs like the really gruesome scene from Game of
Throwns where the guys head explodes.
[ snip ]
> @@ -836,11 +859,13 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> if (SN_LESS(pattrib->seqnum, tx_seq)) {
> pattrib->ampdu_en = false;/* AGG BK */
> } else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> - psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq + 1) & 0xfff;
> + psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> + (tx_seq + 1) & 0xfff;
>
> pattrib->ampdu_en = true;/* AGG EN */
> } else {
> - psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (pattrib->seqnum + 1) & 0xfff;
> + psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> + (pattrib->seqnum + 1) & 0xfff;
> pattrib->ampdu_en = true;/* AGG EN */
I would argue that this is less readable (worse) than before. The
way to fix this is to introduce new functions.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
