Re: [PATCH] staging: rtl8723bs: Use shared header constants

From: Pascal Terjan
Date: Wed May 27 2020 - 16:33:23 EST


On Wed, 27 May 2020 at 20:48, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Sat, May 23, 2020 at 10:29:19PM +0100, Pascal Terjan wrote:
> > This is one of the 9 drivers redefining rfc1042_header.
> >
>
> This is how the patch looks like in my email client:
>
> https://marc.info/?l=linux-driver-devel&m=159026973821890&w=2
>
> Do you see how the subject is far away from the body of the commit
> message? I normally only read the subject or the body when I'm
> reviewing patches so it's good if the body is clear on its own. Maybe
> write something like:
>
> "This driver creates a local definitions of "rtw_rfc1042_header" and
> "rtw_bridge_tunnel_header" but it should just use the standard definitions
> from cfg80211.h."

Thanks, I see both together when writing the commit message and need
to remember they are actually separate.

> > void _rtw_init_sta_recv_priv(struct sta_recv_priv *psta_recvpriv)
> > @@ -1625,11 +1622,11 @@ sint wlanhdr_to_ethhdr(union recv_frame *precvframe)
> > psnap_type = ptr+pattrib->hdrlen + pattrib->iv_len+SNAP_SIZE;
> > /* convert hdr + possible LLC headers into Ethernet header */
> > /* eth_type = (psnap_type[0] << 8) | psnap_type[1]; */
> > - if ((!memcmp(psnap, rtw_rfc1042_header, SNAP_SIZE) &&
> > - (memcmp(psnap_type, SNAP_ETH_TYPE_IPX, 2)) &&
> > - (memcmp(psnap_type, SNAP_ETH_TYPE_APPLETALK_AARP, 2))) ||
> > - /* eth_type != ETH_P_AARP && eth_type != ETH_P_IPX) || */
> > - !memcmp(psnap, rtw_bridge_tunnel_header, SNAP_SIZE)) {
> > + if ((!memcmp(psnap, rfc1042_header, SNAP_SIZE) &&
> > + memcmp(psnap_type, SNAP_ETH_TYPE_IPX, 2) &&
> > + memcmp(psnap_type, SNAP_ETH_TYPE_APPLETALK_AARP, 2)) ||
> > + /* eth_type != ETH_P_AARP && eth_type != ETH_P_IPX) || */
> > + !memcmp(psnap, bridge_tunnel_header, SNAP_SIZE)) {
> > /* remove RFC1042 or Bridge-Tunnel encapsulation and replace EtherType */
> > bsnaphdr = true;
>
> Your indenting is correct, but I would probably do that in a separate
> patch. It makes it harder to review. Also probably delete the
> commented out code. Do you see how if we don't touch the indenting then
> it doesn't raise the question about if we should delete the comments as
> well?

I initially didn't want to change it but checkpatch was sad which
makes me sad, maybe I should have cleaned up this area in a first
trivial patch before touching that line.