Re: [PATCH] lttng: cleanup one-bit signed bitfields

* Greg KH (greg <at> kroah.com) wrote:
> On Thu, Dec 01, 2011 at 09:31:18AM -0500, Mathieu Desnoyers wrote:
> > commit 93db362daaaef88fed672256fa2948cf0efc11fb
> > Author: Mathieu Desnoyers <mathieu.desnoyers <at> efficios.com>
> > Date:   Thu Dec 1 09:25:40 2011 -0500
> > 
> >     lttng: cleanup one-bit signed bitfields
> 
> Ick, I can edit this up, but please, make it in a format that I can
> apply it in (i.e. git send-mail output).

OK, will do next time, sorry,

Thanks!

Mathieu

> 
> thanks,
> 
> greg k-h

--

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] lttng: cleanup one-bit signed bitfields

On Thu, Dec 01, 2011 at 09:31:18AM -0500, Mathieu Desnoyers wrote:
> commit 93db362daaaef88fed672256fa2948cf0efc11fb
> Author: Mathieu Desnoyers <mathieu.desnoyers <at> efficios.com>
> Date:   Thu Dec 1 09:25:40 2011 -0500
> 
>     lttng: cleanup one-bit signed bitfields

Ick, I can edit this up, but please, make it in a format that I can
apply it in (i.e. git send-mail output).

thanks,

greg k-h

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

As we add more than one of those "flags", not using bitfields will grow
the memory footprint of these structures, so I don't think this is
advised here.

Thanks,

Mathieu

* walter harms (wharms <at> bfs.de) wrote:
> Hello Mathieu,
> nice to hear someone is concerned about space.
> since you plan to go for uint perhaps we can drop that bitfield stuff at all ?
> 
> re,
>  wh
> 
> 
> Am 01.12.2011 15:20, schrieb Mathieu Desnoyers:
> > * walter harms (wharms <at> bfs.de) wrote:
> >> hi,
> >> This patch looks ok to me but this design is ugly by itself.
> >> It should be replaced by an uchar uint whatever or use a
> >> real bool (obviously not preferred by this programmes).
> > 
> > bool :1, uchar :1 or uint :1 could make sense. uchar:1/bool:1 won't save
> > any space here, because the surrounding fields are either uint or
> > pointers, so alignment will just add padding.
> > 
> > I try to use int/uint whenever possible because x86 CPUs tend to get
> > less register false-dependencies when using instructions modifying the

Re: [PATCH] lttng: cleanup one-bit signed bitfields

Acked-by: Dan Carpenter <dan.carpenter <at> oracle.com>

regards,
dan carpenter

_______________________________________________
devel mailing list
devel <at> linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

Hello Mathieu,
nice to hear someone is concerned about space.
since you plan to go for uint perhaps we can drop that bitfield stuff at all ?

re,
 wh

Am 01.12.2011 15:20, schrieb Mathieu Desnoyers:
> * walter harms (wharms <at> bfs.de) wrote:
>> hi,
>> This patch looks ok to me but this design is ugly by itself.
>> It should be replaced by an uchar uint whatever or use a
>> real bool (obviously not preferred by this programmes).
> 
> bool :1, uchar :1 or uint :1 could make sense. uchar:1/bool:1 won't save
> any space here, because the surrounding fields are either uint or
> pointers, so alignment will just add padding.
> 
> I try to use int/uint whenever possible because x86 CPUs tend to get
> less register false-dependencies when using instructions modifying the
> whole register (generated by using int/uint types) rather than only part
> of it (uchar/char/bool). I only use char/uchar/bool when there is a
> clear wanted space gain.
> 
> The reason why I never use the bool type within a structure when I want
> a compact representation is that bool takes a whole byte just to
> represent one bit:
> 
> #include <stdio.h>
> #include <stdbool.h>

[PATCH] lttng: cleanup one-bit signed bitfields

commit 93db362daaaef88fed672256fa2948cf0efc11fb
Author: Mathieu Desnoyers <mathieu.desnoyers <at> efficios.com>
Date:   Thu Dec 1 09:25:40 2011 -0500

    lttng: cleanup one-bit signed bitfields

    * Dan Carpenter <dan.carpenter <at> oracle.com> wrote:
    > Sparse complains that these signed bitfields look "dubious".  The
    > problem is that instead of being either 0 or 1 like people would expect,
    > signed one bit variables like this are either 0 or -1.  It doesn't cause
    > a problem in this case but it's ugly so lets fix them.

    * walter harms (wharms <at> bfs.de) wrote:
    > hi,
    > This patch looks ok to me but this design is ugly by itself.
    > It should be replaced by an uchar uint whatever or use a
    > real bool (obviously not preferred by this programmes).

    bool :1, uchar :1 or uint :1 could make sense. uchar:1/bool:1 won't save
    any space here, because the surrounding fields are either uint or
    pointers, so alignment will just add padding.

    I try to use int/uint whenever possible because x86 CPUs tend to get
    less register false-dependencies when using instructions modifying the
    whole register (generated by using int/uint types) rather than only part
    of it (uchar/char/bool). I only use char/uchar/bool when there is a
    clear wanted space gain.

    The reason why I never use the bool type within a structure when I want
    a compact representation is that bool takes a whole byte just to

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

* walter harms (wharms <at> bfs.de) wrote:
> hi,
> This patch looks ok to me but this design is ugly by itself.
> It should be replaced by an uchar uint whatever or use a
> real bool (obviously not preferred by this programmes).

bool :1, uchar :1 or uint :1 could make sense. uchar:1/bool:1 won't save
any space here, because the surrounding fields are either uint or
pointers, so alignment will just add padding.

I try to use int/uint whenever possible because x86 CPUs tend to get
less register false-dependencies when using instructions modifying the
whole register (generated by using int/uint types) rather than only part
of it (uchar/char/bool). I only use char/uchar/bool when there is a
clear wanted space gain.

The reason why I never use the bool type within a structure when I want
a compact representation is that bool takes a whole byte just to
represent one bit:

#include <stdio.h>
#include <stdbool.h>

struct usebitfield {
        int a;
        unsigned int f:1, g:1, h:1, i:1, j:1;
        int b;
};

struct usebool {

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

hi,
This patch looks ok to me but this design is ugly by itself.
It should be replaced by an uchar uint whatever or use a
real bool (obviously not preferred by this programmes).

re,
 wh

Am 01.12.2011 10:37, schrieb Dan Carpenter:
> Sparse complains that these signed bitfields look "dubious".  The
> problem is that instead of being either 0 or 1 like people would expect,
> signed one bit variables like this are either 0 or -1.  It doesn't cause
> a problem in this case but it's ugly so lets fix them.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter <at> oracle.com>
> ---
> I just did this against linux next but it applies fine on top of
> Mathieu's recent patches.
> 
> diff --git a/drivers/staging/lttng/lib/ringbuffer/backend_types.h b/drivers/staging/lttng/lib/ringbuffer/backend_types.h
> index 1d301de..019929a 100644
> --- a/drivers/staging/lttng/lib/ringbuffer/backend_types.h
> +++ b/drivers/staging/lttng/lib/ringbuffer/backend_types.h
> @@ -65,7 +65,7 @@ struct channel_backend {
>  					 * for writer.
>  					 */
>  	unsigned int buf_size_order;	/* Order of buffer size */
> -	int extra_reader_sb:1;		/* Bool: has extra reader subbuffer */
> +	unsigned int extra_reader_sb:1;	/* Bool: has extra reader subbuffer */
>  	struct lib_ring_buffer *buf;	/* Channel per-cpu buffers */

[patch] Staging: lttng: dubious one-bit signed bitfields

Sparse complains that these signed bitfields look "dubious".  The
problem is that instead of being either 0 or 1 like people would expect,
signed one bit variables like this are either 0 or -1.  It doesn't cause
a problem in this case but it's ugly so lets fix them.

Signed-off-by: Dan Carpenter <dan.carpenter <at> oracle.com>
---
I just did this against linux next but it applies fine on top of
Mathieu's recent patches.

diff --git a/drivers/staging/lttng/lib/ringbuffer/backend_types.h b/drivers/staging/lttng/lib/ringbuffer/backend_types.h
index 1d301de..019929a 100644
--- a/drivers/staging/lttng/lib/ringbuffer/backend_types.h
+++ b/drivers/staging/lttng/lib/ringbuffer/backend_types.h
@@ -65,7 +65,7 @@ struct channel_backend {
 					 * for writer.
 					 */
 	unsigned int buf_size_order;	/* Order of buffer size */
-	int extra_reader_sb:1;		/* Bool: has extra reader subbuffer */
+	unsigned int extra_reader_sb:1;	/* Bool: has extra reader subbuffer */
 	struct lib_ring_buffer *buf;	/* Channel per-cpu buffers */

 	unsigned long num_subbuf;	/* Number of sub-buffers for writer */
diff --git a/drivers/staging/lttng/lib/ringbuffer/frontend_types.h b/drivers/staging/lttng/lib/ringbuffer/frontend_types.h
index 5c7437f..9086c58 100644
--- a/drivers/staging/lttng/lib/ringbuffer/frontend_types.h
+++ b/drivers/staging/lttng/lib/ringbuffer/frontend_types.h
@@ -60,8 +60,8 @@ struct channel {
 	struct notifier_block cpu_hp_notifier;	/* CPU hotplug notifier */
 	struct notifier_block tick_nohz_notifier; /* CPU nohz notifier */

[PATCH] ALSA: snd-atmel-abdac: test wrong variable

After clk_get() pclk is checked second time instead of sample_clk check.

Signed-off-by: Vasiliy Kulikov <segoon <at> openwall.com>
---
 Compile tested only.

 sound/atmel/abdac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/atmel/abdac.c b/sound/atmel/abdac.c
index f2f41c8..4e47baa 100644
--- a/sound/atmel/abdac.c
+++ b/sound/atmel/abdac.c
@@ -420,7 +420,7 @@ static int __devinit atmel_abdac_probe(struct platform_device *pdev)
 		return PTR_ERR(pclk);
 	}
 	sample_clk = clk_get(&pdev->dev, "sample_clk");
-	if (IS_ERR(pclk)) {
+	if (IS_ERR(sample_clk)) {
 		dev_dbg(&pdev->dev, "no sample clock\n");
 		retval = PTR_ERR(pclk);
 		goto out_put_pclk;
--

-- 
1.7.0.4

[PATCH] ASoC: atmel: test wrong variable

After clk_get() mclk is checked second time instead of pllb check.

Signed-off-by: Vasiliy Kulikov <segoon <at> openwall.com>
---
 Cannot compile this driver, so it is not tested.

 sound/soc/atmel/sam9g20_wm8731.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/atmel/sam9g20_wm8731.c b/sound/soc/atmel/sam9g20_wm8731.c
index da9c303..68072a2 100644
--- a/sound/soc/atmel/sam9g20_wm8731.c
+++ b/sound/soc/atmel/sam9g20_wm8731.c
@@ -223,7 +223,7 @@ static int __init at91sam9g20ek_init(void)
 	}

 	pllb = clk_get(NULL, "pllb");
-	if (IS_ERR(mclk)) {
+	if (IS_ERR(pllb)) {
 		printk(KERN_ERR "ASoC: Failed to get PLLB\n");
 		ret = PTR_ERR(mclk);
 		goto err_mclk;
--

-- 
1.7.0.4