From: Greg KH <gregkh@linuxfoundation.org> To: Atul Gopinathan <atulgopinathan@gmail.com> 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 Date: Mon, 22 Feb 2021 16:26:33 +0100 [thread overview] Message-ID: <YDPNKTHZqaS37XPe@kroah.com> (raw) In-Reply-To: <20210221165721.GA10040@atulu-nitro> On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > 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; Yes. > 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? It changes the location of the [1] operation to point to a different place in memory from what I can tell, as you changed the type of that array. thanks, greg k-h
WARNING: multiple messages have this Message-ID (diff)
From: Greg KH <gregkh@linuxfoundation.org> To: Atul Gopinathan <atulgopinathan@gmail.com> 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 Date: Mon, 22 Feb 2021 16:26:33 +0100 [thread overview] Message-ID: <YDPNKTHZqaS37XPe@kroah.com> (raw) In-Reply-To: <20210221165721.GA10040@atulu-nitro> On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > 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; Yes. > 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? It changes the location of the [1] operation to point to a different place in memory from what I can tell, as you changed the type of that array. thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2021-02-22 15:34 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-20 18:21 [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer Atul Gopinathan 2021-02-20 18:21 ` Atul Gopinathan 2021-02-20 18:21 ` [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 Atul Gopinathan 2021-02-20 18:21 ` Atul Gopinathan 2021-02-21 13:08 ` Greg KH 2021-02-21 13:08 ` Greg KH 2021-02-21 16:57 ` Atul Gopinathan 2021-02-21 16:57 ` Atul Gopinathan 2021-02-22 15:26 ` Greg KH [this message] 2021-02-22 15:26 ` Greg KH 2021-02-22 17:23 ` Atul Gopinathan 2021-02-22 17:23 ` Atul Gopinathan 2021-03-02 14:38 ` Greg KH 2021-03-02 14:38 ` Greg KH 2021-03-02 16:53 ` Atul Gopinathan 2021-03-02 16:53 ` Atul Gopinathan 2021-02-20 18:34 ` [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer Gustavo A. R. Silva 2021-02-21 5:21 ` Atul Gopinathan 2021-02-21 5:21 ` Atul Gopinathan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YDPNKTHZqaS37XPe@kroah.com \ --to=gregkh@linuxfoundation.org \ --cc=atulgopinathan@gmail.com \ --cc=devel@driverdev.osuosl.org \ --cc=gustavo@embeddedor.com \ --cc=linux-kernel@vger.kernel.org \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.