1. Patchew
  2. linux
  3. [PATCH v2 0/3] staging: rtl8723bs: Code cleanup patches
  4. View patch

[PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted

Erick Karanja posted 3 patches 1 month ago
[PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Erick Karanja 1 month ago 
Standardize boolean representation by ensuring consistency,
replace instances of 1/0 with true/false where boolean logic is implied,
as some definitions already use true/false.
This improves code clarity and aligns with the kernel’s bool type usage.

Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
index a389ba5ecc6f..fd04dbacb50f 100644
--- a/drivers/staging/rtl8723bs/core/rtw_recv.c
+++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
@@ -1358,7 +1358,7 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
 			u8 *mgmt_DATA;
 			u32 data_len = 0;
 
-			pattrib->bdecrypted = 0;
+			pattrib->bdecrypted = false;
 			pattrib->encrypt = _AES_;
 			pattrib->hdrlen = sizeof(struct ieee80211_hdr_3addr);
 			/* set iv and icv length */
-- 
2.43.0
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Greg KH 1 month ago 
On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> Standardize boolean representation by ensuring consistency,
> replace instances of 1/0 with true/false where boolean logic is implied,
> as some definitions already use true/false.
> This improves code clarity and aligns with the kernel’s bool type usage.
> 
> Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index a389ba5ecc6f..fd04dbacb50f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -1358,7 +1358,7 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
>  			u8 *mgmt_DATA;
>  			u32 data_len = 0;
>  
> -			pattrib->bdecrypted = 0;
> +			pattrib->bdecrypted = false;

but bdecrypted is a u8, not a boolean type.  So setting it to "false"
does not seem correct here, right?

Also, the name of the variable should really be changed.

But, step back, are you _SURE_ you can change this structure at all?
At a quick glance it kind of looks like it comes directly from the
hardware, or am I mis-reading the driver code?

Have you tested this series on an actual device?

thanks,

greg k-h
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Julia Lawall 1 month ago 
On Wed, 2 Apr 2025, Greg KH wrote:

> On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> > Standardize boolean representation by ensuring consistency,
> > replace instances of 1/0 with true/false where boolean logic is implied,
> > as some definitions already use true/false.
> > This improves code clarity and aligns with the kernel’s bool type usage.
> >
> > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > index a389ba5ecc6f..fd04dbacb50f 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > @@ -1358,7 +1358,7 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
> >  			u8 *mgmt_DATA;
> >  			u32 data_len = 0;
> >
> > -			pattrib->bdecrypted = 0;
> > +			pattrib->bdecrypted = false;
>
> but bdecrypted is a u8, not a boolean type.  So setting it to "false"
> does not seem correct here, right?

Is false different than 0?

Elsewhere there is an assignment to true.

julia

>
> Also, the name of the variable should really be changed.
>
> But, step back, are you _SURE_ you can change this structure at all?
> At a quick glance it kind of looks like it comes directly from the
> hardware, or am I mis-reading the driver code?
>
> Have you tested this series on an actual device?
>
> thanks,
>
> greg k-h
>
>
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Greg KH 1 month ago 
On Wed, Apr 02, 2025 at 10:34:22PM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 2 Apr 2025, Greg KH wrote:
> 
> > On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> > > Standardize boolean representation by ensuring consistency,
> > > replace instances of 1/0 with true/false where boolean logic is implied,
> > > as some definitions already use true/false.
> > > This improves code clarity and aligns with the kernel’s bool type usage.
> > >
> > > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > > ---
> > >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > index a389ba5ecc6f..fd04dbacb50f 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > @@ -1358,7 +1358,7 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
> > >  			u8 *mgmt_DATA;
> > >  			u32 data_len = 0;
> > >
> > > -			pattrib->bdecrypted = 0;
> > > +			pattrib->bdecrypted = false;
> >
> > but bdecrypted is a u8, not a boolean type.  So setting it to "false"
> > does not seem correct here, right?
> 
> Is false different than 0?

Does C guarantee that?  I can never remember.  I don't think it
guarantees that a 'bool' will only be 8 bits, or am I mistaken there
too?

> Elsewhere there is an assignment to true.

Was that in the original driver?

If this doesn't come from the hardware, then it's fine to make the
change.  If it does, it needs to be verified that the layout and bit
values are identical.

thanks,

greg k-h
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Erick Karanja 1 month ago 
On Wed, 2025-04-02 at 21:41 +0100, Greg KH wrote:
> On Wed, Apr 02, 2025 at 10:34:22PM +0200, Julia Lawall wrote:
> > 
> > 
> > On Wed, 2 Apr 2025, Greg KH wrote:
> > 
> > > On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> > > > Standardize boolean representation by ensuring consistency,
> > > > replace instances of 1/0 with true/false where boolean logic is
> > > > implied,
> > > > as some definitions already use true/false.
> > > > This improves code clarity and aligns with the kernel’s bool
> > > > type usage.
> > > > 
> > > > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > > > ---
> > > >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > index a389ba5ecc6f..fd04dbacb50f 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > @@ -1358,7 +1358,7 @@ static signed int
> > > > validate_80211w_mgmt(struct adapter *adapter, union recv_frame
> > > >  			u8 *mgmt_DATA;
> > > >  			u32 data_len = 0;
> > > > 
> > > > -			pattrib->bdecrypted = 0;
> > > > +			pattrib->bdecrypted = false;
> > > 
> > > but bdecrypted is a u8, not a boolean type.  So setting it to
> > > "false"
> > > does not seem correct here, right?
> > 
> > Is false different than 0?
> 
> Does C guarantee that?  I can never remember.  I don't think it
> guarantees that a 'bool' will only be 8 bits, or am I mistaken there
> too?
> 
> > Elsewhere there is an assignment to true.
> 
> Was that in the original driver?
> 
> If this doesn't come from the hardware, then it's fine to make the
> change.  If it does, it needs to be verified that the layout and bit
> values are identical.
> 
> thanks,
I have compared the generated assembly code
before and after and I have observed the only
change is the comment below.
-# drivers/staging/rtl8723bs/core/rtw_recv.c:1361:
 			pattrib->bdecrypted = 0;
+# drivers/staging/rtl8723bs/core/rtw_recv.c:1361:
 			pattrib->bdecrypted = false;
There is no change in the assembly instructions.

> 
> greg k-h
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Greg KH 1 month ago 
On Fri, Apr 04, 2025 at 09:58:23AM +0300, Erick Karanja wrote:
> On Wed, 2025-04-02 at 21:41 +0100, Greg KH wrote:
> > On Wed, Apr 02, 2025 at 10:34:22PM +0200, Julia Lawall wrote:
> > > 
> > > 
> > > On Wed, 2 Apr 2025, Greg KH wrote:
> > > 
> > > > On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> > > > > Standardize boolean representation by ensuring consistency,
> > > > > replace instances of 1/0 with true/false where boolean logic is
> > > > > implied,
> > > > > as some definitions already use true/false.
> > > > > This improves code clarity and aligns with the kernel’s bool
> > > > > type usage.
> > > > > 
> > > > > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > > > > ---
> > > > >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > index a389ba5ecc6f..fd04dbacb50f 100644
> > > > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > @@ -1358,7 +1358,7 @@ static signed int
> > > > > validate_80211w_mgmt(struct adapter *adapter, union recv_frame
> > > > >  			u8 *mgmt_DATA;
> > > > >  			u32 data_len = 0;
> > > > > 
> > > > > -			pattrib->bdecrypted = 0;
> > > > > +			pattrib->bdecrypted = false;
> > > > 
> > > > but bdecrypted is a u8, not a boolean type.  So setting it to
> > > > "false"
> > > > does not seem correct here, right?
> > > 
> > > Is false different than 0?
> > 
> > Does C guarantee that?  I can never remember.  I don't think it
> > guarantees that a 'bool' will only be 8 bits, or am I mistaken there
> > too?
> > 
> > > Elsewhere there is an assignment to true.
> > 
> > Was that in the original driver?
> > 
> > If this doesn't come from the hardware, then it's fine to make the
> > change.  If it does, it needs to be verified that the layout and bit
> > values are identical.
> > 
> > thanks,
> I have compared the generated assembly code
> before and after and I have observed the only
> change is the comment below.
> -# drivers/staging/rtl8723bs/core/rtw_recv.c:1361:
>  			pattrib->bdecrypted = 0;
> +# drivers/staging/rtl8723bs/core/rtw_recv.c:1361:
>  			pattrib->bdecrypted = false;
> There is no change in the assembly instructions.

Showing the assembly is key, not just a comment :)
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Erick Karanja 1 month ago 
On Fri, 2025-04-04 at 09:00 +0100, Greg KH wrote:
> On Fri, Apr 04, 2025 at 09:58:23AM +0300, Erick Karanja wrote:
> > On Wed, 2025-04-02 at 21:41 +0100, Greg KH wrote:
> > > On Wed, Apr 02, 2025 at 10:34:22PM +0200, Julia Lawall wrote:
> > > > 
> > > > 
> > > > On Wed, 2 Apr 2025, Greg KH wrote:
> > > > 
> > > > > On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja
> > > > > wrote:
> > > > > > Standardize boolean representation by ensuring consistency,
> > > > > > replace instances of 1/0 with true/false where boolean
> > > > > > logic is
> > > > > > implied,
> > > > > > as some definitions already use true/false.
> > > > > > This improves code clarity and aligns with the kernel’s
> > > > > > bool
> > > > > > type usage.
> > > > > > 
> > > > > > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > > > > > ---
> > > > > >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > > b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > > index a389ba5ecc6f..fd04dbacb50f 100644
> > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > > > @@ -1358,7 +1358,7 @@ static signed int
> > > > > > validate_80211w_mgmt(struct adapter *adapter, union
> > > > > > recv_frame
> > > > > >  			u8 *mgmt_DATA;
> > > > > >  			u32 data_len = 0;
> > > > > > 
> > > > > > -			pattrib->bdecrypted = 0;
> > > > > > +			pattrib->bdecrypted = false;
> > > > > 
> > > > > but bdecrypted is a u8, not a boolean type.  So setting it to
> > > > > "false"
> > > > > does not seem correct here, right?
> > > > 
> > > > Is false different than 0?
> > > 
> > > Does C guarantee that?  I can never remember.  I don't think it
> > > guarantees that a 'bool' will only be 8 bits, or am I mistaken
> > > there
> > > too?
> > > 
> > > > Elsewhere there is an assignment to true.
> > > 
> > > Was that in the original driver?
> > > 
> > > If this doesn't come from the hardware, then it's fine to make
> > > the
> > > change.  If it does, it needs to be verified that the layout and
> > > bit
> > > values are identical.
> > > 
> > > thanks,
> > I have compared the generated assembly code
> > before and after and I have observed the only
> > change is the comment below.
> > -# drivers/staging/rtl8723bs/core/rtw_recv.c:1361:
> >  			pattrib->bdecrypted = 0;
> > +# drivers/staging/rtl8723bs/core/rtw_recv.c:1361:
> >  			pattrib->bdecrypted = false;
> > There is no change in the assembly instructions.
> 
> Showing the assembly is key, not just a comment :)
Thank you for the update. I will take this into consideration.
Erick
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Dan Carpenter 1 month ago 
On Wed, Apr 02, 2025 at 09:41:57PM +0100, Greg KH wrote:
> On Wed, Apr 02, 2025 at 10:34:22PM +0200, Julia Lawall wrote:
> > 
> > 
> > On Wed, 2 Apr 2025, Greg KH wrote:
> > 
> > > On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> > > > Standardize boolean representation by ensuring consistency,
> > > > replace instances of 1/0 with true/false where boolean logic is implied,
> > > > as some definitions already use true/false.
> > > > This improves code clarity and aligns with the kernel’s bool type usage.
> > > >
> > > > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > > > ---
> > > >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > index a389ba5ecc6f..fd04dbacb50f 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > @@ -1358,7 +1358,7 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
> > > >  			u8 *mgmt_DATA;
> > > >  			u32 data_len = 0;
> > > >
> > > > -			pattrib->bdecrypted = 0;
> > > > +			pattrib->bdecrypted = false;
> > >
> > > but bdecrypted is a u8, not a boolean type.  So setting it to "false"
> > > does not seem correct here, right?
> > 
> > Is false different than 0?
> 
> Does C guarantee that?  I can never remember.  I don't think it
> guarantees that a 'bool' will only be 8 bits, or am I mistaken there
> too?
> 

These patches are fine.  This does come from the hardware but the
patches don't change the layout of the struct, just the right
hand side of the assignment.

The C standard doesn't specify the size of _Bool.  It just has to
be unsigned and at least one bit large.  The surprising thing about
_Bool type is that it doesn't truncate anything.  So you can do:
"_Bool x = y & BIT(20);" and it works, but if we use unsigned char
then we would have to add a !!.  "unsigned char x = !!(y & BIT(20));"

Btw, true/false are not keywords in C.  They're defined in
include/linux/stddef.h.

The main review here is if there is a typo where we accidentally type
true instead of false.

regards,
dan carpenter
Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted
Posted by Julia Lawall 1 month ago 
On Wed, 2 Apr 2025, Greg KH wrote:

> On Wed, Apr 02, 2025 at 10:34:22PM +0200, Julia Lawall wrote:
> >
> >
> > On Wed, 2 Apr 2025, Greg KH wrote:
> >
> > > On Wed, Apr 02, 2025 at 08:16:42PM +0300, Erick Karanja wrote:
> > > > Standardize boolean representation by ensuring consistency,
> > > > replace instances of 1/0 with true/false where boolean logic is implied,
> > > > as some definitions already use true/false.
> > > > This improves code clarity and aligns with the kernel’s bool type usage.
> > > >
> > > > Signed-off-by: Erick Karanja <karanja99erick@gmail.com>
> > > > ---
> > > >  drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > index a389ba5ecc6f..fd04dbacb50f 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > > > @@ -1358,7 +1358,7 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
> > > >  			u8 *mgmt_DATA;
> > > >  			u32 data_len = 0;
> > > >
> > > > -			pattrib->bdecrypted = 0;
> > > > +			pattrib->bdecrypted = false;
> > >
> > > but bdecrypted is a u8, not a boolean type.  So setting it to "false"
> > > does not seem correct here, right?
> >
> > Is false different than 0?
>
> Does C guarantee that?  I can never remember.  I don't think it
> guarantees that a 'bool' will only be 8 bits, or am I mistaken there
> too?
>
> > Elsewhere there is an assignment to true.
>
> Was that in the original driver?

Yes:

git blame -L 436,436 drivers/staging/rtl8723bs/core/rtw_recv.c

554c0a3abf216 (Hans de Goede 2017-03-29 19:47:51 +0200 436)
prxattrib->bdecrypted = true;

> If this doesn't come from the hardware, then it's fine to make the
> change.  If it does, it needs to be verified that the layout and bit
> values are identical.

Erick, you can see if the generated assembly code is the same before and
after the same.  You can see that with
make drivers/staging/rtl8723bs/core/rtw_recv.s

julia

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.