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

[Pascal Terjan]

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.