* [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members @ 2019-02-21 7:47 Bhanusree Pola 2019-02-21 8:37 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Bhanusree Pola @ 2019-02-21 7:47 UTC (permalink / raw) To: outreachy-kernel; +Cc: Greg Kroah-Hartman -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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members 2019-02-21 7:47 [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members Bhanusree Pola @ 2019-02-21 8:37 ` Greg Kroah-Hartman 2019-02-21 8:51 ` Bhanusree Mahesh 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2019-02-21 8:37 UTC (permalink / raw) To: Bhanusree Pola; +Cc: outreachy-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members 2019-02-21 8:37 ` Greg Kroah-Hartman @ 2019-02-21 8:51 ` Bhanusree Mahesh 2019-02-21 9:02 ` [Outreachy kernel] " Vaishali Thakkar 0 siblings, 1 reply; 7+ messages in thread From: Bhanusree Mahesh @ 2019-02-21 8:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: outreachy-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members 2019-02-21 8:51 ` Bhanusree Mahesh @ 2019-02-21 9:02 ` Vaishali Thakkar 2019-02-22 7:54 ` Bhanusree Mahesh 0 siblings, 1 reply; 7+ messages in thread From: Vaishali Thakkar @ 2019-02-21 9:02 UTC (permalink / raw) To: Bhanusree Mahesh; +Cc: Greg Kroah-Hartman, outreachy-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members 2019-02-21 9:02 ` [Outreachy kernel] " Vaishali Thakkar @ 2019-02-22 7:54 ` Bhanusree Mahesh 2019-02-22 8:04 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Bhanusree Mahesh @ 2019-02-22 7:54 UTC (permalink / raw) To: Vaishali Thakkar; +Cc: Greg Kroah-Hartman, outreachy-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members 2019-02-22 7:54 ` Bhanusree Mahesh @ 2019-02-22 8:04 ` Greg Kroah-Hartman 2019-02-22 9:56 ` Bhanusree Mahesh 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2019-02-22 8:04 UTC (permalink / raw) To: Bhanusree Mahesh; +Cc: Vaishali Thakkar, outreachy-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members 2019-02-22 8:04 ` Greg Kroah-Hartman @ 2019-02-22 9:56 ` Bhanusree Mahesh 0 siblings, 0 replies; 7+ messages in thread From: Bhanusree Mahesh @ 2019-02-22 9:56 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Vaishali Thakkar, outreachy-kernel 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. :) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-22 9:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-21 7:47 [PATCH v2 2/2] Staging: rtl8192e: Avoid using bool structure members Bhanusree Pola 2019-02-21 8:37 ` Greg Kroah-Hartman 2019-02-21 8:51 ` Bhanusree Mahesh 2019-02-21 9:02 ` [Outreachy kernel] " Vaishali Thakkar 2019-02-22 7:54 ` Bhanusree Mahesh 2019-02-22 8:04 ` Greg Kroah-Hartman 2019-02-22 9:56 ` Bhanusree Mahesh
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.