Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300013 > unrolled thread

[PATCH] ASoC: cs35l32: avoid uninitialized variable access

Back to article view | Back to linux.kernel

Contents

  [PATCH] ASoC: cs35l32: avoid uninitialized variable access Arnd Bergmann <arnd@arndb.de> - 2016-01-02 00:30 +0100
    Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access Mark Brown <broonie@kernel.org> - 2016-01-02 15:20 +0100
      Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access Arnd Bergmann <arnd@arndb.de> - 2016-01-04 16:20 +0100
        Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-01-04 16:30 +0100
          Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access Arnd Bergmann <arnd@arndb.de> - 2016-01-04 17:50 +0100
            Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-01-04 18:00 +0100
        Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access Mark Brown <broonie@kernel.org> - 2016-01-04 16:50 +0100

#1300013 — [PATCH] ASoC: cs35l32: avoid uninitialized variable access

gcc warns about the possibilty of accessing a property read from
devicetree in cs35l32_i2c_probe() when it has not been initialized
because CONFIG_OF is disabled:

sound/soc/codecs/cs35l32.c: In function 'cs35l32_i2c_probe':
sound/soc/codecs/cs35l32.c:278:2: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]

The code is actually correct because it checks the dev->of_node
variable first and we know this is NULL here, but by adding a
check for IS_ENABLED(CONFIG_OF), we can let the compiler know
as well, and also generate smaller object code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/sound/soc/codecs/cs35l32.c b/sound/soc/codecs/cs35l32.c
index 44c30fe3e315..52ffaa8eb02b 100644
--- a/sound/soc/codecs/cs35l32.c
+++ b/sound/soc/codecs/cs35l32.c
@@ -372,7 +372,7 @@ static int cs35l32_i2c_probe(struct i2c_client *i2c_client,
 			dev_err(&i2c_client->dev, "could not allocate pdata\n");
 			return -ENOMEM;
 		}
-		if (i2c_client->dev.of_node) {
+		if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {
 			ret = cs35l32_handle_of_data(i2c_client,
 						     &cs35l32->pdata);
 			if (ret != 0)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300073

[Multipart message — attachments visible in raw view] — view raw

On Sat, Jan 02, 2016 at 12:19:52AM +0100, Arnd Bergmann wrote:

> -		if (i2c_client->dev.of_node) {
> +		if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {

This would be a lot nicer if there was an __always_null annotation we
could put on of_node for !OF configurations, that'd Just Work and this
can't be the only case where we have this idiom.

[toc] | [prev] | [next] | [standalone]

#1300802

On Saturday 02 January 2016 14:17:46 Mark Brown wrote:
> On Sat, Jan 02, 2016 at 12:19:52AM +0100, Arnd Bergmann wrote:
> 
> > -             if (i2c_client->dev.of_node) {
> > +             if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {
> 
> This would be a lot nicer if there was an __always_null annotation we
> could put on of_node for !OF configurations, that'd Just Work and this
> can't be the only case where we have this idiom.
> 

How about an inline helper like

static inline struct device_node *dev_of_node(struct device *dev)
{
	if (IS_ENABLED(CONFIG_OF))
		return dev->of_node;	
}

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300825

On Mon, Jan 04, 2016 at 04:17:47PM +0100, Arnd Bergmann wrote:
> On Saturday 02 January 2016 14:17:46 Mark Brown wrote:
> > On Sat, Jan 02, 2016 at 12:19:52AM +0100, Arnd Bergmann wrote:
> > 
> > > -             if (i2c_client->dev.of_node) {
> > > +             if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {
> > 
> > This would be a lot nicer if there was an __always_null annotation we
> > could put on of_node for !OF configurations, that'd Just Work and this
> > can't be the only case where we have this idiom.
> > 
> 
> How about an inline helper like
> 
> static inline struct device_node *dev_of_node(struct device *dev)
> {
> 	if (IS_ENABLED(CONFIG_OF))
> 		return dev->of_node;	

ITYM:

	return IS_ENABLED(CONFIG_OF) ? dev->of_node : NULL;

or

	if (IS_ENABLED(CONFIG_OF))
		return dev->of_node;
	else
		return NULL;

> }
> 
> 	Arnd
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300878

On Monday 04 January 2016 15:20:58 Russell King - ARM Linux wrote:
> On Mon, Jan 04, 2016 at 04:17:47PM +0100, Arnd Bergmann wrote:
> > On Saturday 02 January 2016 14:17:46 Mark Brown wrote:
> > > On Sat, Jan 02, 2016 at 12:19:52AM +0100, Arnd Bergmann wrote:
> > > 
> > > > -             if (i2c_client->dev.of_node) {
> > > > +             if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {
> > > 
> > > This would be a lot nicer if there was an __always_null annotation we
> > > could put on of_node for !OF configurations, that'd Just Work and this
> > > can't be the only case where we have this idiom.
> > > 
> > 
> > How about an inline helper like
> > 
> > static inline struct device_node *dev_of_node(struct device *dev)
> > {
> >       if (IS_ENABLED(CONFIG_OF))
> >               return dev->of_node;    
> 
> ITYM:
> 
>         return IS_ENABLED(CONFIG_OF) ? dev->of_node : NULL;
> 
> or
> 
>         if (IS_ENABLED(CONFIG_OF))
>                 return dev->of_node;
>         else
>                 return NULL;
> 

Right, yes.

That reminds of a different problem that has been bugging me for a
while: We frequently have a pattern like

#ifdef CONFIG_FOO
static int function(void)
{
	...
}
#endif

struct operations = {
	...
#ifdef CONFIG_FOO
	.function = function;
#endif
	...
};

Except that people constantly get it wrong, e.g. by using the
wrong ifdef, forgetting one of the two ifdefs, or by leaving
unused static functions that only get called indirectly from the
other one that is built conditionally.

We could add a macro like

#define COND_PTR(config, ptr) (IS_ENABLED(config) ? (ptr) : NULL)

and then let the compiler figure out that "function" is unused even
without an explicit __maybe_unused annotation.  The function above
can be simplied to

static inline struct device_node *dev_of_node(struct device *dev)
{
	return COND_PTR(CONFIG_OF, dev->of_node);
}

with that, which is another benefit.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300889

On Mon, Jan 04, 2016 at 05:41:05PM +0100, Arnd Bergmann wrote:
> That reminds of a different problem that has been bugging me for a
> while: We frequently have a pattern like
> 
> #ifdef CONFIG_FOO
> static int function(void)
> {
> 	...
> }
> #endif
> 
> struct operations = {
> 	...
> #ifdef CONFIG_FOO
> 	.function = function;
> #endif
> 	...
> };
> 
> Except that people constantly get it wrong, e.g. by using the
> wrong ifdef, forgetting one of the two ifdefs, or by leaving
> unused static functions that only get called indirectly from the
> other one that is built conditionally.

We already have a solution to that.  __maybe_unused against the
function, and use the correct #ifdef in the structure initialiser.
We just need reviewers to be better at picking that up.

> We could add a macro like
> 
> #define COND_PTR(config, ptr) (IS_ENABLED(config) ? (ptr) : NULL)
> 
> and then let the compiler figure out that "function" is unused even
> without an explicit __maybe_unused annotation.  The function above
> can be simplied to
> 
> static inline struct device_node *dev_of_node(struct device *dev)
> {
> 	return COND_PTR(CONFIG_OF, dev->of_node);
> }
> 
> with that, which is another benefit.

You're just inventing another way for people to get it wrong though.
Instead of having mismatched #ifdefs, we can now have a mismatched
#ifdef around the function and the COND_PTR config - and people will
add #ifdef's because they won't realise they don't need them.

You're reliant on reviewers to spotting the pattern, and suggesting
using COND_PTR() without #ifdefs around the function.  It's the same
problem with spotting the existing pattern and suggesting dropping
the #ifdef around the function and annotating the function with
__maybe_unused.

So, I don't see the benefit.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300840

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jan 04, 2016 at 04:17:47PM +0100, Arnd Bergmann wrote:
> On Saturday 02 January 2016 14:17:46 Mark Brown wrote:

> > This would be a lot nicer if there was an __always_null annotation we
> > could put on of_node for !OF configurations, that'd Just Work and this
> > can't be the only case where we have this idiom.

> How about an inline helper like

> static inline struct device_node *dev_of_node(struct device *dev)
> {
> 	if (IS_ENABLED(CONFIG_OF))
> 		return dev->of_node;	
> }

Yeah, that'd work as well (with the correction Russell mentioned) - it's
a bit more typing but we already do similar things for the ID tables and
it looks nicer than the IS_ENABLED() in code does.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web