Re: [PATCH 1/7] staging: r8188eu: use ieee80211 define for version check

Martin Kaiser
2022-03-27T18:19:44Z

Thus wrote David Laight (David.Laight@ACULAB.COM):

> From: Martin Kaiser
> > Sent: 23 March 2022 07:49

> > Use the IEEE80211_FCTL_VERS define to check the version number
> > of a received frame.

> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> > drivers/staging/r8188eu/core/rtw_recv.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)

> > diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> > index 8800ea4825ff..524a00345501 100644
> > --- a/drivers/staging/r8188eu/core/rtw_recv.c
> > +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> > @@ -1063,7 +1063,6 @@ static int validate_recv_frame(struct adapter *adapter, struct recv_frame *precv
> > struct rx_pkt_attrib *pattrib = &precv_frame->attrib;
> > u8 *ptr = precv_frame->rx_data;
> > __le16 fc = *(__le16 *)ptr;

> Those two lines are somewhat horrid.
> Casts of pointers to integer types have a nasty habit of being bugs.

The fc is the Frame Control field, which is at the start of an incoming
80211 frame. The existing helper functions that parse the Frame Control
want an fc parameter like this.

I looked at the drawing in

https://einstein.informatik.uni-oldenburg.de/rechnernetze/frame.htm

for the structure that the r8188eu driver is trying to parse (the text
is in German, sorry).

> In any case 'ptr' should probably be 'frame_data'.

I'm trying to remove ptr complety and use existing helper functions for
all components.

> If the first two bytes are some kind of 16 bit id, then what follows?
> Should this be a 'struct' that defines the frame data layout??

I define an fc variable in functions that use only components of Frame
Control. If we need other fields, I use a struct ieee80211_hdr.

I've just sent a v2 of this series where I replaced fc with a struct
ieee80211_hdr in the validate_recv_frame function.

Best regards,
Martin