[PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Bhanusree Pola]

-Use 'u8' with bitfield instead of 'bool' to preserve space.
-Also bool size can be 1 or sizeof(int) depending on the macro
 definition given to bool
-inorder to avoid alignment issues the u8 bitfield is placed as last
 member of the structure.
-removes checkpatch.pl check:
 CHECK: Avoid using bool structure members because of possible alignment
 issues

Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
---
 drivers/staging/rtl8192e/dot11d.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
index 6d2b93acfa43..a1d879d316ed 100644
--- a/drivers/staging/rtl8192e/dot11d.h
+++ b/drivers/staging/rtl8192e/dot11d.h
@@ -33,8 +33,6 @@ enum dot11d_state {
  */
 
 struct rt_dot11d_info {
-	bool enabled;
-
 	u16 country_len;
 	u8  country_buffer[MAX_IE_LEN];
 	u8  country_src_addr[6];
@@ -44,6 +42,7 @@ struct rt_dot11d_info {
 	u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
 
 	enum dot11d_state state;
+	u8 enabled:1;
 };
 
 static inline void copy_mac_addr(unsigned char *des, unsigned char *src)
-- 
2.17.1

Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Greg Kroah-Hartman]

On Thu, Feb 21, 2019 at 01:17:29PM +0530, Bhanusree Pola wrote:
> -Use 'u8' with bitfield instead of 'bool' to preserve space.
> -Also bool size can be 1 or sizeof(int) depending on the macro
>  definition given to bool
> -inorder to avoid alignment issues the u8 bitfield is placed as last
>  member of the structure.
> -removes checkpatch.pl check:
>  CHECK: Avoid using bool structure members because of possible alignment
>  issues
> 
> Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> ---
>  drivers/staging/rtl8192e/dot11d.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
> index 6d2b93acfa43..a1d879d316ed 100644
> --- a/drivers/staging/rtl8192e/dot11d.h
> +++ b/drivers/staging/rtl8192e/dot11d.h
> @@ -33,8 +33,6 @@ enum dot11d_state {
>   */
>  
>  struct rt_dot11d_info {
> -	bool enabled;
> -
>  	u16 country_len;
>  	u8  country_buffer[MAX_IE_LEN];
>  	u8  country_src_addr[6];
> @@ -44,6 +42,7 @@ struct rt_dot11d_info {
>  	u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
>  
>  	enum dot11d_state state;
> +	u8 enabled:1;

Again, no, this is not ok at all.  I thought I said so the last time you
sent this patch.  checkpatch.pl is a "hint", you do not always have to
follow everything it says to do.

thanks,

greg k-h

Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Bhanusree Mahesh]

Ok, thanks.

On Thu, 21 Feb 2019 at 14:07, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Feb 21, 2019 at 01:17:29PM +0530, Bhanusree Pola wrote:
> > -Use 'u8' with bitfield instead of 'bool' to preserve space.
> > -Also bool size can be 1 or sizeof(int) depending on the macro
> >  definition given to bool
> > -inorder to avoid alignment issues the u8 bitfield is placed as last
> >  member of the structure.
> > -removes checkpatch.pl check:
> >  CHECK: Avoid using bool structure members because of possible alignment
> >  issues
> >
> > Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> > ---
> >  drivers/staging/rtl8192e/dot11d.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
> > index 6d2b93acfa43..a1d879d316ed 100644
> > --- a/drivers/staging/rtl8192e/dot11d.h
> > +++ b/drivers/staging/rtl8192e/dot11d.h
> > @@ -33,8 +33,6 @@ enum dot11d_state {
> >   */
> >
> >  struct rt_dot11d_info {
> > -     bool enabled;
> > -
> >       u16 country_len;
> >       u8  country_buffer[MAX_IE_LEN];
> >       u8  country_src_addr[6];
> > @@ -44,6 +42,7 @@ struct rt_dot11d_info {
> >       u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
> >
> >       enum dot11d_state state;
> > +     u8 enabled:1;
>
> Again, no, this is not ok at all.  I thought I said so the last time you
> sent this patch.  checkpatch.pl is a "hint", you do not always have to
> follow everything it says to do.
>
> thanks,
>
> greg k-h

Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Vaishali Thakkar]

On Thu, Feb 21, 2019 at 2:21 PM Bhanusree Mahesh
<bhanusreemahesh@gmail.com> wrote:
>
> Ok, thanks.

No top-posting, please!

> On Thu, 21 Feb 2019 at 14:07, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Feb 21, 2019 at 01:17:29PM +0530, Bhanusree Pola wrote:
> > > -Use 'u8' with bitfield instead of 'bool' to preserve space.
> > > -Also bool size can be 1 or sizeof(int) depending on the macro
> > >  definition given to bool
> > > -inorder to avoid alignment issues the u8 bitfield is placed as last
> > >  member of the structure.
> > > -removes checkpatch.pl check:
> > >  CHECK: Avoid using bool structure members because of possible alignment
> > >  issues
> > >
> > > Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> > > ---
> > >  drivers/staging/rtl8192e/dot11d.h | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
> > > index 6d2b93acfa43..a1d879d316ed 100644
> > > --- a/drivers/staging/rtl8192e/dot11d.h
> > > +++ b/drivers/staging/rtl8192e/dot11d.h
> > > @@ -33,8 +33,6 @@ enum dot11d_state {
> > >   */
> > >
> > >  struct rt_dot11d_info {
> > > -     bool enabled;
> > > -
> > >       u16 country_len;
> > >       u8  country_buffer[MAX_IE_LEN];
> > >       u8  country_src_addr[6];
> > > @@ -44,6 +42,7 @@ struct rt_dot11d_info {
> > >       u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
> > >
> > >       enum dot11d_state state;
> > > +     u8 enabled:1;
> >
> > Again, no, this is not ok at all.  I thought I said so the last time you
> > sent this patch.  checkpatch.pl is a "hint", you do not always have to
> > follow everything it says to do.
> >
> > thanks,
> >
> > greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAB0Mk-NDubg4tFZe8kzqhL3p31KQzPf7bJ3Pc%3DWsqnpwPisvJA%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Bhanusree Mahesh]

On Thu, 21 Feb 2019 at 14:32, Vaishali Thakkar
<vthakkar@vaishalithakkar.in> wrote:
>
> On Thu, Feb 21, 2019 at 2:21 PM Bhanusree Mahesh
> <bhanusreemahesh@gmail.com> wrote:
> >
> > Ok, thanks.
>
> No top-posting, please!
>
> > On Thu, 21 Feb 2019 at 14:07, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Feb 21, 2019 at 01:17:29PM +0530, Bhanusree Pola wrote:
> > > > -Use 'u8' with bitfield instead of 'bool' to preserve space.
> > > > -Also bool size can be 1 or sizeof(int) depending on the macro
> > > >  definition given to bool
> > > > -inorder to avoid alignment issues the u8 bitfield is placed as last
> > > >  member of the structure.
> > > > -removes checkpatch.pl check:
> > > >  CHECK: Avoid using bool structure members because of possible alignment
> > > >  issues
> > > >
> > > > Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> > > > ---
> > > >  drivers/staging/rtl8192e/dot11d.h | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
> > > > index 6d2b93acfa43..a1d879d316ed 100644
> > > > --- a/drivers/staging/rtl8192e/dot11d.h
> > > > +++ b/drivers/staging/rtl8192e/dot11d.h
> > > > @@ -33,8 +33,6 @@ enum dot11d_state {
> > > >   */
> > > >
> > > >  struct rt_dot11d_info {
> > > > -     bool enabled;
> > > > -
> > > >       u16 country_len;
> > > >       u8  country_buffer[MAX_IE_LEN];
> > > >       u8  country_src_addr[6];
> > > > @@ -44,6 +42,7 @@ struct rt_dot11d_info {
> > > >       u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
> > > >
> > > >       enum dot11d_state state;
> > > > +     u8 enabled:1;
> > >
> > > Again, no, this is not ok at all.  I thought I said so the last time you
> > > sent this patch.  checkpatch.pl is a "hint", you do not always have to
> > > follow everything it says to do.

This clearly says that use unsigned bitfields instead of bool in a structure.
https://lkml.org/lkml/2017/11/21/384
Please, let me know why exactly am I not supposed to change bool
inside a structure?

> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAB0Mk-NDubg4tFZe8kzqhL3p31KQzPf7bJ3Pc%3DWsqnpwPisvJA%40mail.gmail.com.
> > For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Greg Kroah-Hartman]

On Fri, Feb 22, 2019 at 01:24:05PM +0530, Bhanusree Mahesh wrote:
> On Thu, 21 Feb 2019 at 14:32, Vaishali Thakkar
> <vthakkar@vaishalithakkar.in> wrote:
> >
> > On Thu, Feb 21, 2019 at 2:21 PM Bhanusree Mahesh
> > <bhanusreemahesh@gmail.com> wrote:
> > >
> > > Ok, thanks.
> >
> > No top-posting, please!
> >
> > > On Thu, 21 Feb 2019 at 14:07, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Feb 21, 2019 at 01:17:29PM +0530, Bhanusree Pola wrote:
> > > > > -Use 'u8' with bitfield instead of 'bool' to preserve space.
> > > > > -Also bool size can be 1 or sizeof(int) depending on the macro
> > > > >  definition given to bool
> > > > > -inorder to avoid alignment issues the u8 bitfield is placed as last
> > > > >  member of the structure.
> > > > > -removes checkpatch.pl check:
> > > > >  CHECK: Avoid using bool structure members because of possible alignment
> > > > >  issues
> > > > >
> > > > > Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> > > > > ---
> > > > >  drivers/staging/rtl8192e/dot11d.h | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
> > > > > index 6d2b93acfa43..a1d879d316ed 100644
> > > > > --- a/drivers/staging/rtl8192e/dot11d.h
> > > > > +++ b/drivers/staging/rtl8192e/dot11d.h
> > > > > @@ -33,8 +33,6 @@ enum dot11d_state {
> > > > >   */
> > > > >
> > > > >  struct rt_dot11d_info {
> > > > > -     bool enabled;
> > > > > -
> > > > >       u16 country_len;
> > > > >       u8  country_buffer[MAX_IE_LEN];
> > > > >       u8  country_src_addr[6];
> > > > > @@ -44,6 +42,7 @@ struct rt_dot11d_info {
> > > > >       u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
> > > > >
> > > > >       enum dot11d_state state;
> > > > > +     u8 enabled:1;
> > > >
> > > > Again, no, this is not ok at all.  I thought I said so the last time you
> > > > sent this patch.  checkpatch.pl is a "hint", you do not always have to
> > > > follow everything it says to do.
> 
> This clearly says that use unsigned bitfields instead of bool in a structure.
> https://lkml.org/lkml/2017/11/21/384

No, it says that "maybe" the developer should use a bitfield instead of
a bool for that structure.  And that structure being talked about is a
very core, and important one, where every byte and access counts.  That
is not the case here at all :)

> Please, let me know why exactly am I not supposed to change bool
> inside a structure?

Linus's complaint is that "bool" is an unknown size of a variable, so
for structures that really matter as to the layout and speed of access,
using it can be "dangerous" as you do not really know what the compiler
is going to pick to use for it.

So, if your really do care about the size, use something like "u8" which
you know the size and accessing it should be simple and quick.

When you use a bitfield, you too know the size, but accessing it is not
always fast at all (the compiler has to mask off the other bits).  So it
all depends on what is needed to be done.

For a structure like this one, nothing needs to be done.  There are no
size requirements, no speed requirements, and nothing important at all.
So I recommend just leaving it alone and ignoring the checkpatch
"complaint".

Again, remember that checkpatch provides hints as to what might be done
to the code to make it better, it is not always right, and sometimes,
flat out wrong.  You always have to use your brain when using it :)

hope this helps,

greg k-h

Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members

[Bhanusree Mahesh]

On Fri, 22 Feb 2019 at 13:34, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 22, 2019 at 01:24:05PM +0530, Bhanusree Mahesh wrote:
> > On Thu, 21 Feb 2019 at 14:32, Vaishali Thakkar
> > <vthakkar@vaishalithakkar.in> wrote:
> > >
> > > On Thu, Feb 21, 2019 at 2:21 PM Bhanusree Mahesh
> > > <bhanusreemahesh@gmail.com> wrote:
> > > >
> > > > Ok, thanks.
> > >
> > > No top-posting, please!
> > >
> > > > On Thu, 21 Feb 2019 at 14:07, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Feb 21, 2019 at 01:17:29PM +0530, Bhanusree Pola wrote:
> > > > > > -Use 'u8' with bitfield instead of 'bool' to preserve space.
> > > > > > -Also bool size can be 1 or sizeof(int) depending on the macro
> > > > > >  definition given to bool
> > > > > > -inorder to avoid alignment issues the u8 bitfield is placed as last
> > > > > >  member of the structure.
> > > > > > -removes checkpatch.pl check:
> > > > > >  CHECK: Avoid using bool structure members because of possible alignment
> > > > > >  issues
> > > > > >
> > > > > > Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> > > > > > ---
> > > > > >  drivers/staging/rtl8192e/dot11d.h | 3 +--
> > > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/rtl8192e/dot11d.h b/drivers/staging/rtl8192e/dot11d.h
> > > > > > index 6d2b93acfa43..a1d879d316ed 100644
> > > > > > --- a/drivers/staging/rtl8192e/dot11d.h
> > > > > > +++ b/drivers/staging/rtl8192e/dot11d.h
> > > > > > @@ -33,8 +33,6 @@ enum dot11d_state {
> > > > > >   */
> > > > > >
> > > > > >  struct rt_dot11d_info {
> > > > > > -     bool enabled;
> > > > > > -
> > > > > >       u16 country_len;
> > > > > >       u8  country_buffer[MAX_IE_LEN];
> > > > > >       u8  country_src_addr[6];
> > > > > > @@ -44,6 +42,7 @@ struct rt_dot11d_info {
> > > > > >       u8  max_tx_power_list[MAX_CHANNEL_NUMBER + 1];
> > > > > >
> > > > > >       enum dot11d_state state;
> > > > > > +     u8 enabled:1;
> > > > >
> > > > > Again, no, this is not ok at all.  I thought I said so the last time you
> > > > > sent this patch.  checkpatch.pl is a "hint", you do not always have to
> > > > > follow everything it says to do.
> >
> > This clearly says that use unsigned bitfields instead of bool in a structure.
> > https://lkml.org/lkml/2017/11/21/384
>
> No, it says that "maybe" the developer should use a bitfield instead of
> a bool for that structure.  And that structure being talked about is a
> very core, and important one, where every byte and access counts.  That
> is not the case here at all :)
>
> > Please, let me know why exactly am I not supposed to change bool
> > inside a structure?
>
> Linus's complaint is that "bool" is an unknown size of a variable, so
> for structures that really matter as to the layout and speed of access,
> using it can be "dangerous" as you do not really know what the compiler
> is going to pick to use for it.
>
> So, if your really do care about the size, use something like "u8" which
> you know the size and accessing it should be simple and quick.
>
> When you use a bitfield, you too know the size, but accessing it is not
> always fast at all (the compiler has to mask off the other bits).  So it
> all depends on what is needed to be done.
>
Thanks for the helpful information. I understand the difference.

> For a structure like this one, nothing needs to be done.  There are no
> size requirements, no speed requirements, and nothing important at all.
> So I recommend just leaving it alone and ignoring the checkpatch
> "complaint".
>
> Again, remember that checkpatch provides hints as to what might be done
> to the code to make it better, it is not always right, and sometimes,
> flat out wrong.  You always have to use your brain when using it :)

Will definitely remember this while checking out. :)