Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 21 Feb 2021 22:27:21 +0530
From:   Atul Gopinathan <atulgopinathan@gmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     tiwai@suse.de, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org, gustavo@embeddedor.com
Subject: Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16
 to u8
Message-ID: <20210221165721.GA10040@atulu-nitro>
References: <20210220182154.9457-1-atulgopinathan@gmail.com>
 <20210220182154.9457-2-atulgopinathan@gmail.com>
 <YDJbSgqTpBpIsbVB@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YDJbSgqTpBpIsbVB@kroah.com>
Precedence: bulk

On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote:
> On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote:
> > The "CcxRmState" field in struct "rtllib_network" is defined
> > as a u16 array of size 2 (so, 4 bytes in total).
> > 
> > But the operations performed on this array throughout the code
> > base (in rtl8192e/) are all in byte size 2 indicating that this
> > array's type was defined wrongly.
> > 
> > There are two situation were u16 type of this field could yield
> > incorrect behaviour:
> > 
> > 1. In rtllib_rx.c:1970:
> > memcpy(network->CcxRmState, &info_element->data[4], 2);
> > 
> > Here last 2 bytes (index 4 and 5) from the info_element->data[]
> > array are meant to be copied into CcxRmState[].
> > Note that "data" array here is an array of type u8.
> > 
> > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c:
> > memcpy(dst->CcxRmState, src->CcxRmState, 2);
> > 
> > Here again, only 2 bytes are copied from the source state to
> > destination state.
> > 
> > There are no instances of "CcxRmState" requiring u16 data type.
> > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/
> > directory for reviewing:
> > 
> > rtllib_rx.c:1970:			memcpy(network->CcxRmState, &info_element->data[4], 2);
> > rtllib_rx.c:1971:			if (network->CcxRmState[0] != 0)
> > rtllib_rx.c:1975:			network->MBssidMask = network->CcxRmState[1] & 0x07;
> > rtllib_rx.c:2520:	memcpy(dst->CcxRmState, src->CcxRmState, 2);
> > rtllib.h:1108:	u8	CcxRmState[2];
> 
> You just changed the logic in line 1975 in that file, right?  Are you
> _SURE_ that is ok?  Do you have a device to test this on?

I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean
the following line?:

network->MBssidMask = network->CcxRmState[1] & 0x07;

network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as
seen above). I believe my patch doesn't change the logic of an "&" operation
being performed on it with 0x07, right?

(I'm sorry if I'm missing something quite obvious, could kindly elaborate
the change in logic that you are referring to?)

Many thanks for the reviewing it during this time!
Atul