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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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