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