[PATCH] Quieten arch/powerpc in a allmodconfig build.

[PATCH] Quieten arch/powerpc in a allmodconfig build.

[Tony Breeds]

This patch silences all the warnings generated in arch/powerpc for
allmodconfig build.

It does:
 * Where appropriate use the uninitialized_var() macro to help GCC
   understand we know what's going on.
 * Explicitly casts PHYSICAL_START in one printk()
 * Initialise a few variables, as it's "neater" than using uninitialized_var()

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
---
Only compile tested.

 arch/powerpc/kernel/cacheinfo.c           |    4 ++--
 arch/powerpc/kernel/pci_dn.c              |    2 +-
 arch/powerpc/kernel/setup_64.c            |    4 ++--
 arch/powerpc/platforms/cell/axon_msi.c    |    2 +-
 arch/powerpc/platforms/cell/beat_iommu.c  |    2 +-
 arch/powerpc/platforms/iseries/pci.c      |   24 ++++++++++++------------
 arch/powerpc/platforms/powermac/low_i2c.c |    5 ++---
 arch/powerpc/platforms/pseries/msi.c      |    2 +-
 8 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
index bb37b1d..fd6aef9 100644
--- a/arch/powerpc/kernel/cacheinfo.c
+++ b/arch/powerpc/kernel/cacheinfo.c
@@ -510,7 +510,7 @@ static struct cache *index_kobj_to_cache(struct kobject *k)
 
 static ssize_t size_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
 {
-	unsigned int size_kb;
+	unsigned int uninitialized_var(size_kb);
 	struct cache *cache;
 
 	cache = index_kobj_to_cache(k);
@@ -559,7 +559,7 @@ static struct kobj_attribute cache_nr_sets_attr =
 
 static ssize_t associativity_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
 {
-	unsigned int associativity;
+	unsigned int uninitialized_var(associativity);
 	struct cache *cache;
 
 	cache = index_kobj_to_cache(k);
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 1c67de5..b9d66ed 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -83,7 +83,7 @@ void *traverse_pci_devices(struct device_node *start, traverse_func pre,
 		void *data)
 {
 	struct device_node *dn, *nextdn;
-	void *ret;
+	void *uninitialized_var(ret);
 
 	/* We started with a phb, iterate all childs */
 	for (dn = start->child; dn; dn = nextdn) {
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index c410c60..38968f1 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -421,8 +421,8 @@ void __init setup_system(void)
 		printk("htab_address                  = 0x%p\n", htab_address);
 	printk("htab_hash_mask                = 0x%lx\n", htab_hash_mask);
 	if (PHYSICAL_START > 0)
-		printk("physical_start                = 0x%lx\n",
-		       PHYSICAL_START);
+		printk("physical_start                = 0x%llx\n",
+		       (unsigned long long)PHYSICAL_START);
 	printk("-----------------------------------------------------\n");
 
 	DBG(" <- setup_system()\n");
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index 0ce45c2..dae4c7c 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -151,7 +151,7 @@ static struct axon_msic *find_msi_translator(struct pci_dev *dev)
 {
 	struct irq_host *irq_host;
 	struct device_node *dn, *tmp;
-	const phandle *ph;
+	const phandle *uninitialized_var(ph);
 	struct axon_msic *msic = NULL;
 
 	dn = of_node_get(pci_device_to_OF_node(dev));
diff --git a/arch/powerpc/platforms/cell/beat_iommu.c b/arch/powerpc/platforms/cell/beat_iommu.c
index 93b0efd..8230cd8 100644
--- a/arch/powerpc/platforms/cell/beat_iommu.c
+++ b/arch/powerpc/platforms/cell/beat_iommu.c
@@ -57,7 +57,7 @@ static unsigned long celleb_dma_direct_offset;
 static void __init celleb_init_direct_mapping(void)
 {
 	u64 lpar_addr, io_addr;
-	u64 io_space_id, ioid, dma_base, dma_size, io_page_size;
+	u64 io_space_id=0, ioid=0, dma_base=0, dma_size=0, io_page_size=0;
 
 	if (!find_dma_window(&io_space_id, &ioid, &dma_base, &dma_size,
 			     &io_page_size)) {
diff --git a/arch/powerpc/platforms/iseries/pci.c b/arch/powerpc/platforms/iseries/pci.c
index 02a634f..05f047d 100644
--- a/arch/powerpc/platforms/iseries/pci.c
+++ b/arch/powerpc/platforms/iseries/pci.c
@@ -616,8 +616,8 @@ static inline struct device_node *xlate_iomm_address(
  */
 static u8 iseries_readb(const volatile void __iomem *addr)
 {
-	u64 bar_offset;
-	u64 dsa;
+	u64 uninitialized_var(bar_offset);
+	u64 uninitialized_var(dsa);
 	int retry = 0;
 	struct HvCallPci_LoadReturn ret;
 	struct device_node *dn =
@@ -634,8 +634,8 @@ static u8 iseries_readb(const volatile void __iomem *addr)
 
 static u16 iseries_readw_be(const volatile void __iomem *addr)
 {
-	u64 bar_offset;
-	u64 dsa;
+	u64 uninitialized_var(bar_offset);
+	u64 uninitialized_var(dsa);
 	int retry = 0;
 	struct HvCallPci_LoadReturn ret;
 	struct device_node *dn =
@@ -653,8 +653,8 @@ static u16 iseries_readw_be(const volatile void __iomem *addr)
 
 static u32 iseries_readl_be(const volatile void __iomem *addr)
 {
-	u64 bar_offset;
-	u64 dsa;
+	u64 uninitialized_var(bar_offset);
+	u64 uninitialized_var(dsa);
 	int retry = 0;
 	struct HvCallPci_LoadReturn ret;
 	struct device_node *dn =
@@ -676,8 +676,8 @@ static u32 iseries_readl_be(const volatile void __iomem *addr)
  */
 static void iseries_writeb(u8 data, volatile void __iomem *addr)
 {
-	u64 bar_offset;
-	u64 dsa;
+	u64 uninitialized_var(bar_offset);
+	u64 uninitialized_var(dsa);
 	int retry = 0;
 	u64 rc;
 	struct device_node *dn =
@@ -692,8 +692,8 @@ static void iseries_writeb(u8 data, volatile void __iomem *addr)
 
 static void iseries_writew_be(u16 data, volatile void __iomem *addr)
 {
-	u64 bar_offset;
-	u64 dsa;
+	u64 uninitialized_var(bar_offset);
+	u64 uninitialized_var(dsa);
 	int retry = 0;
 	u64 rc;
 	struct device_node *dn =
@@ -708,8 +708,8 @@ static void iseries_writew_be(u16 data, volatile void __iomem *addr)
 
 static void iseries_writel_be(u32 data, volatile void __iomem *addr)
 {
-	u64 bar_offset;
-	u64 dsa;
+	u64 uninitialized_var(bar_offset);
+	u64 uninitialized_var(dsa);
 	int retry = 0;
 	u64 rc;
 	struct device_node *dn =
diff --git a/arch/powerpc/platforms/powermac/low_i2c.c b/arch/powerpc/platforms/powermac/low_i2c.c
index 21226b7..5989427 100644
--- a/arch/powerpc/platforms/powermac/low_i2c.c
+++ b/arch/powerpc/platforms/powermac/low_i2c.c
@@ -1090,7 +1090,7 @@ EXPORT_SYMBOL_GPL(pmac_low_i2c_unlock);
 
 int pmac_i2c_open(struct pmac_i2c_bus *bus, int polled)
 {
-	int rc;
+	int rc = 0;
 
 	mutex_lock(&bus->mutex);
 	bus->polled = polled || pmac_i2c_force_poll;
@@ -1099,9 +1099,8 @@ int pmac_i2c_open(struct pmac_i2c_bus *bus, int polled)
 	if (bus->open && (rc = bus->open(bus)) != 0) {
 		bus->opened = 0;
 		mutex_unlock(&bus->mutex);
-		return rc;
 	}
-	return 0;
+	return rc;
 }
 EXPORT_SYMBOL_GPL(pmac_i2c_open);
 
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index bf2e1ac..d92f593 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -282,7 +282,7 @@ static int msi_quota_for_device(struct pci_dev *dev, int request)
 {
 	struct device_node *pe_dn;
 	struct msi_counts counts;
-	int total;
+	int uninitialized_var(total);
 
 	pr_debug("rtas_msi: calc quota for %s, request %d\n", pci_name(dev),
 		  request);
-- 
1.6.0.6

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 8454 bytes --]

On Wed, 2009-04-08 at 14:36 +1000, Tony Breeds wrote:
> This patch silences all the warnings generated in arch/powerpc for
> allmodconfig build.
> 
> It does:
>  * Where appropriate use the uninitialized_var() macro to help GCC
>    understand we know what's going on.
>  * Explicitly casts PHYSICAL_START in one printk()
>  * Initialise a few variables, as it's "neater" than using uninitialized_var()
> 
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> ---
> Only compile tested.
> 
>  arch/powerpc/kernel/cacheinfo.c           |    4 ++--
>  arch/powerpc/kernel/pci_dn.c              |    2 +-
>  arch/powerpc/kernel/setup_64.c            |    4 ++--
>  arch/powerpc/platforms/cell/axon_msi.c    |    2 +-
>  arch/powerpc/platforms/cell/beat_iommu.c  |    2 +-
>  arch/powerpc/platforms/iseries/pci.c      |   24 ++++++++++++------------
>  arch/powerpc/platforms/powermac/low_i2c.c |    5 ++---
>  arch/powerpc/platforms/pseries/msi.c      |    2 +-
>  8 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
> index bb37b1d..fd6aef9 100644
> --- a/arch/powerpc/kernel/cacheinfo.c
> +++ b/arch/powerpc/kernel/cacheinfo.c
> @@ -510,7 +510,7 @@ static struct cache *index_kobj_to_cache(struct kobject *k)
>  
>  static ssize_t size_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
>  {
> -	unsigned int size_kb;
> +	unsigned int uninitialized_var(size_kb);
>  	struct cache *cache;
>  
>  	cache = index_kobj_to_cache(k);
> @@ -559,7 +559,7 @@ static struct kobj_attribute cache_nr_sets_attr =
>  
>  static ssize_t associativity_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
>  {
> -	unsigned int associativity;
> +	unsigned int uninitialized_var(associativity);
>  	struct cache *cache;

The getter routines in here could really multiplex their return values
with a negative error code, which I generally prefer, but this works I
guess.


> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index 1c67de5..b9d66ed 100644
> --- a/arch/powerpc/kernel/pci_dn.c
> +++ b/arch/powerpc/kernel/pci_dn.c
> @@ -83,7 +83,7 @@ void *traverse_pci_devices(struct device_node *start, traverse_func pre,
>  		void *data)
>  {
>  	struct device_node *dn, *nextdn;
> -	void *ret;
> +	void *uninitialized_var(ret);

The code causing this one is a little ugly anyway, I think we should
change it to be:

-               if (pre && ((ret = pre(dn, data)) != NULL))
-                       return ret;
+               if (pre) {
+                       ret = pre(dn, data);
+                       if (ret != NULL)
+                               return ret;
+               }


> diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
> index 0ce45c2..dae4c7c 100644
> --- a/arch/powerpc/platforms/cell/axon_msi.c
> +++ b/arch/powerpc/platforms/cell/axon_msi.c
> @@ -151,7 +151,7 @@ static struct axon_msic *find_msi_translator(struct pci_dev *dev)
>  {
>  	struct irq_host *irq_host;
>  	struct device_node *dn, *tmp;
> -	const phandle *ph;
> +	const phandle *uninitialized_var(ph);
>  	struct axon_msic *msic = NULL;

Freakin gcc. This is just:

if (!dn)
	return;

for (; dn; ..)
	ph = ..

if (!ph)

And it can't work out that it's never used uninitialised?

> diff --git a/arch/powerpc/platforms/cell/beat_iommu.c b/arch/powerpc/platforms/cell/beat_iommu.c
> index 93b0efd..8230cd8 100644
> --- a/arch/powerpc/platforms/cell/beat_iommu.c
> +++ b/arch/powerpc/platforms/cell/beat_iommu.c
> @@ -57,7 +57,7 @@ static unsigned long celleb_dma_direct_offset;
>  static void __init celleb_init_direct_mapping(void)
>  {
>  	u64 lpar_addr, io_addr;
> -	u64 io_space_id, ioid, dma_base, dma_size, io_page_size;
> +	u64 io_space_id=0, ioid=0, dma_base=0, dma_size=0, io_page_size=0;

Do you need a new spacebar? :D

> diff --git a/arch/powerpc/platforms/iseries/pci.c b/arch/powerpc/platforms/iseries/pci.c
> index 02a634f..05f047d 100644
> --- a/arch/powerpc/platforms/iseries/pci.c
> +++ b/arch/powerpc/platforms/iseries/pci.c
> @@ -616,8 +616,8 @@ static inline struct device_node *xlate_iomm_address(
>   */
>  static u8 iseries_readb(const volatile void __iomem *addr)
>  {
> -	u64 bar_offset;
> -	u64 dsa;
> +	u64 uninitialized_var(bar_offset);
> +	u64 uninitialized_var(dsa);
>  	int retry = 0;
>  	struct HvCallPci_LoadReturn ret;
>  	struct device_node *dn =
> @@ -634,8 +634,8 @@ static u8 iseries_readb(const volatile void __iomem *addr)
>  
>  static u16 iseries_readw_be(const volatile void __iomem *addr)
>  {
> -	u64 bar_offset;
> -	u64 dsa;
> +	u64 uninitialized_var(bar_offset);
> +	u64 uninitialized_var(dsa);
>  	int retry = 0;
>  	struct HvCallPci_LoadReturn ret;
>  	struct device_node *dn =
> @@ -653,8 +653,8 @@ static u16 iseries_readw_be(const volatile void __iomem *addr)
>  
>  static u32 iseries_readl_be(const volatile void __iomem *addr)
>  {
> -	u64 bar_offset;
> -	u64 dsa;
> +	u64 uninitialized_var(bar_offset);
> +	u64 uninitialized_var(dsa);
>  	int retry = 0;
>  	struct HvCallPci_LoadReturn ret;
>  	struct device_node *dn =
> @@ -676,8 +676,8 @@ static u32 iseries_readl_be(const volatile void __iomem *addr)
>   */
>  static void iseries_writeb(u8 data, volatile void __iomem *addr)
>  {
> -	u64 bar_offset;
> -	u64 dsa;
> +	u64 uninitialized_var(bar_offset);
> +	u64 uninitialized_var(dsa);
>  	int retry = 0;
>  	u64 rc;
>  	struct device_node *dn =
> @@ -692,8 +692,8 @@ static void iseries_writeb(u8 data, volatile void __iomem *addr)
>  
>  static void iseries_writew_be(u16 data, volatile void __iomem *addr)
>  {
> -	u64 bar_offset;
> -	u64 dsa;
> +	u64 uninitialized_var(bar_offset);
> +	u64 uninitialized_var(dsa);
>  	int retry = 0;
>  	u64 rc;
>  	struct device_node *dn =
> @@ -708,8 +708,8 @@ static void iseries_writew_be(u16 data, volatile void __iomem *addr)
>  
>  static void iseries_writel_be(u32 data, volatile void __iomem *addr)
>  {
> -	u64 bar_offset;
> -	u64 dsa;
> +	u64 uninitialized_var(bar_offset);
> +	u64 uninitialized_var(dsa);
>  	int retry = 0;
>  	u64 rc;
>  	struct device_node *dn =
> diff --git a/arch/powerpc/platforms/powermac/low_i2c.c b/arch/powerpc/platforms/powermac/low_i2c.c
> index 21226b7..5989427 100644
> --- a/arch/powerpc/platforms/powermac/low_i2c.c
> +++ b/arch/powerpc/platforms/powermac/low_i2c.c
> @@ -1090,7 +1090,7 @@ EXPORT_SYMBOL_GPL(pmac_low_i2c_unlock);
>  
>  int pmac_i2c_open(struct pmac_i2c_bus *bus, int polled)
>  {
> -	int rc;
> +	int rc = 0;
>  
>  	mutex_lock(&bus->mutex);
>  	bus->polled = polled || pmac_i2c_force_poll;
> @@ -1099,9 +1099,8 @@ int pmac_i2c_open(struct pmac_i2c_bus *bus, int polled)
>  	if (bus->open && (rc = bus->open(bus)) != 0) {
>  		bus->opened = 0;
>  		mutex_unlock(&bus->mutex);
> -		return rc;
>  	}
> -	return 0;
> +	return rc;

This one is a bug surely?

1092 {
1093         int rc;
1094        
1095         mutex_lock(&bus->mutex);
1096         bus->polled = polled || pmac_i2c_force_poll;
1097         bus->opened = 1;
1098         bus->mode = pmac_i2c_mode_std;
1099         if (bus->open && (rc = bus->open(bus)) != 0) {
1100                 bus->opened = 0;
1101                 mutex_unlock(&bus->mutex);
1102                 return rc;
1103         }
1104         return 0;
1105 }


I can't reproduce it though with a simple test case :?

> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index bf2e1ac..d92f593 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -282,7 +282,7 @@ static int msi_quota_for_device(struct pci_dev *dev, int request)
>  {
>  	struct device_node *pe_dn;
>  	struct msi_counts counts;
> -	int total;
> +	int uninitialized_var(total);
>  
>  	pr_debug("rtas_msi: calc quota for %s, request %d\n", pci_name(dev),
>  		  request);

Gah, gcc sucks. It should just not warn in these cases where it doesn't
know wth is going on.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Tony Breeds]

On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:

> The getter routines in here could really multiplex their return values
> with a negative error code, which I generally prefer, but this works I
> guess.

I was hoping someone would notice and suggest it.  tag you're it!

> Do you need a new spacebar? :D

nowhydoyouask?
 
> This one is a bug surely?

Hmm actually I think you're right.  I dont want to push my luck with the gcc
hackers though

> Gah, gcc sucks. It should just not warn in these cases where it doesn't
> know wth is going on.

I don't think you'll get any arguments.  it only there was a -Wnowarnunused!

Yours Tony

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Tony Breeds]

On Wed, Apr 08, 2009 at 03:51:26PM +1000, Tony Breeds wrote:
 
> Hmm actually I think you're right.  I dont want to push my luck with the gcc
> hackers though

Replying to myself.

Yes this is a gcc bug but one introduced by CONFIG_TRACE_ALL_BRANCHES (or
whatever that's called).

Yours Tony

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On Wed, 2009-04-08 at 16:13 +1000, Tony Breeds wrote:
> On Wed, Apr 08, 2009 at 03:51:26PM +1000, Tony Breeds wrote:
>  
> > Hmm actually I think you're right.  I dont want to push my luck with the gcc
> > hackers though
> 
> Replying to myself.
> 
> Yes this is a gcc bug but one introduced by CONFIG_TRACE_ALL_BRANCHES (or
> whatever that's called).

CONFIG_MAKE_MY_KERNEL_SUCK I think it is.

In that case I don't blame gcc.

If people want to use that option I think they can just deal with the
warnings, we can make -Werror depend on !CONFIG_PROFILE_ALL_BRANCHES.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote:
> On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:
> 
> > The getter routines in here could really multiplex their return values
> > with a negative error code, which I generally prefer, but this works I
> > guess.
> 
> I was hoping someone would notice and suggest it.  tag you're it!

I meant we /could/ change them, but we could also leave them, it's a bit
of a coin-flip which is better. Nathan might have an opinion?

Something like this:

diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
index bb37b1d..9f3a155 100644
--- a/arch/powerpc/kernel/cacheinfo.c
+++ b/arch/powerpc/kernel/cacheinfo.c
@@ -191,7 +191,7 @@ static void cache_cpu_set(struct cache *cache, int cpu)
        }
 }
 
-static int cache_size(const struct cache *cache, unsigned int *ret)
+static int cache_size(const struct cache *cache)
 {
        const char *propname;
        const u32 *cache_size;
@@ -202,19 +202,18 @@ static int cache_size(const struct cache *cache, unsigned 
        if (!cache_size)
                return -ENODEV;
 
-       *ret = *cache_size;
-       return 0;
+       return cache_size;
 }
 
-static int cache_size_kb(const struct cache *cache, unsigned int *ret)
+static int cache_size_kb(const struct cache *cache)
 {
        unsigned int size;
 
-       if (cache_size(cache, &size))
-               return -ENODEV;
+       size = cache_size(cache);
+       if (size < 0)
+               return size;
 
-       *ret = size / 1024;
-       return 0;
+       return size / 1024;
 }
 
 /* not cache_line_size() because that's a macro in include/linux/cache.h */
@@ -515,8 +514,9 @@ static ssize_t size_show(struct kobject *k, struct kobj_attr
 
        cache = index_kobj_to_cache(k);
 
-       if (cache_size_kb(cache, &size_kb))
-               return -ENODEV;
+       size_kb = cache_size_kb(cache);
+       if (size_kb < 0)
+               return size_kb;
 
        return sprintf(buf, "%uK\n", size_kb);
 }


cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Geert Uytterhoeven]

On Wed, 8 Apr 2009, Michael Ellerman wrote:
> On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote:
> > On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:
> > 
> > > The getter routines in here could really multiplex their return values
> > > with a negative error code, which I generally prefer, but this works I
> > > guess.
> > 
> > I was hoping someone would notice and suggest it.  tag you're it!
> 
> I meant we /could/ change them, but we could also leave them, it's a bit
> of a coin-flip which is better. Nathan might have an opinion?
> 
> Something like this:
> 
> diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
> index bb37b1d..9f3a155 100644
> --- a/arch/powerpc/kernel/cacheinfo.c
> +++ b/arch/powerpc/kernel/cacheinfo.c

> -static int cache_size_kb(const struct cache *cache, unsigned int *ret)
> +static int cache_size_kb(const struct cache *cache)
>  {
>         unsigned int size;
          ^^^^^^^^^^^^
`int size', or Roel will come to get you ;-)

>  
> -       if (cache_size(cache, &size))
> -               return -ENODEV;
> +       size = cache_size(cache);
> +       if (size < 0)
              ^^^^^^^^
> +               return size;

I can't check the others, due to lacking context...

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Nathan Lynch]

Michael Ellerman <michael@ellerman.id.au> wrote:

> On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote:
> > On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:
> > 
> > > The getter routines in here could really multiplex their return values
> > > with a negative error code, which I generally prefer, but this works I
> > > guess.
> > 
> > I was hoping someone would notice and suggest it.  tag you're it!
> 
> I meant we /could/ change them, but we could also leave them, it's a bit
> of a coin-flip which is better. Nathan might have an opinion?

I think I had some reason for doing it this way, but I'm drawing a
blank right now.

In the meantime, can someone post the warnings that gcc emits for
cacheinfo.c, as well as the gcc version?  I can't reproduce them with
4.3.2 on Fedora.

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Tony Breeds]

On Wed, Apr 08, 2009 at 01:47:36PM -0500, Nathan Lynch wrote:

> I think I had some reason for doing it this way, but I'm drawing a
> blank right now.
> 
> In the meantime, can someone post the warnings that gcc emits for
> cacheinfo.c, as well as the gcc version?  I can't reproduce them with
> 4.3.2 on Fedora.

---
[tony@thor ~]$ egrep cacheinfo tmp/build.log 
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'associativity_show':
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: warning: 'associativity' may be used uninitialized in this function
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'size_show':
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: warning: 'size_kb' may be used uninitialized in this function
tony@Sprygo:~/scratch$ /opt/crosstool/gcc-4.4.0-20090216-nolibc/powerpc-linux/bin/powerpc-linux-gcc -v
Using built-in specs.
Target: powerpc-linux
Configured with: /home/tony/buildall/src/gcc/configure --target=powerpc-linux --enable-targets=all --prefix=/opt/crosstool/gcc-4.4.0-20090216-nolibc/powerpc-linux --program-prefix=powerpc-linux- --disable-bootstrap --with-mpfr=/usr --enable-languages=c --without-headers --enable-sjlj-exceptions --with-system-libunwind --disable-nls --disable-threads --disable-shared --disable-libmudflap --disable-libssp --disable-libgomp --disable-decimal-float --enable-checking=release
Thread model: single
gcc version 4.4.0 20090216 (experimental) (GCC) 
tony@Sprygo:~/scratch$ 
---

That's the compiler in rawhide

Yours Tony

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Tony Breeds]

On Fri, Apr 10, 2009 at 12:46:06AM +0200, Segher Boessenkool wrote:

> -Wno-unused or -Wno-unused-pparameter and/or -Wno-unused-variable.  But I
> thought this was about uninitialised var warnings?  -Wno-uninitialized
> for that one.

> If you are asking for a GCC option that will warn for all suspect cases
> _except_ for the ones where it is obvious to you there is no problem:
> you could try -Wesp.

Ooops I was asking for -Wno-uninitialized.  I'll try a build with:
	-Wno-uninitialized -Wesp
and see how that goes.

Thanks.

Yours Tony

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Segher Boessenkool]

>> Gah, gcc sucks. It should just not warn in these cases where it  
>> doesn't
>> know wth is going on.
>
> I don't think you'll get any arguments.  it only there was a - 
> Wnowarnunused!

-Wno-unused or -Wno-unused-pparameter and/or -Wno-unused-variable.   
But I
thought this was about uninitialised var warnings?  -Wno-uninitialized
for that one.

If you are asking for a GCC option that will warn for all suspect cases
_except_ for the ones where it is obvious to you there is no problem:
you could try -Wesp.


Segher

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Fri, 10 Apr 2009 08:45:53 +1000 Tony Breeds <tony@bakeyournoodle.com> wrote:
>
> On Fri, Apr 10, 2009 at 12:46:06AM +0200, Segher Boessenkool wrote:
> 
> > -Wno-unused or -Wno-unused-pparameter and/or -Wno-unused-variable.  But I
> > thought this was about uninitialised var warnings?  -Wno-uninitialized
> > for that one.
> 
> Ooops I was asking for -Wno-uninitialized.  I'll try a build with:
> 	-Wno-uninitialized -Wesp
> and see how that goes.

Unfortunately -Wno-uninitialized also suppresses the warnings that point
to real bugs. (but, I guess, the -Wesp might help there :-))

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Segher Boessenkool]

>> -Wno-unused or -Wno-unused-pparameter and/or -Wno-unused- 
>> variable.  But I
>> thought this was about uninitialised var warnings?  -Wno- 
>> uninitialized
>> for that one.
>
>> If you are asking for a GCC option that will warn for all suspect  
>> cases
>> _except_ for the ones where it is obvious to you there is no problem:
>> you could try -Wesp.
>
> Ooops I was asking for -Wno-uninitialized.  I'll try a build with:
> 	-Wno-uninitialized -Wesp
> and see how that goes.

-Wesp was a joke: I was trying to say that the compiler cannot read  
minds.
It doesn't know what potentially unused variables are obviously (to you)
actually used.


Segher

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Segher Boessenkool]

> Unfortunately -Wno-uninitialized also suppresses the warnings that  
> point
> to real bugs.

It's a double-edged sword, yes.  Warnings are always like that:
if the compiler could know that something _is_ wrong for certain,
it wouldn't need a warning (it would use an error, instead -- and
it does do this in certain cases); if it would know something is
not really wrong, it would just shut up.

> (but, I guess, the -Wesp might help there :-))

Yeah, it's the holy grail of compiler architecture.  "Do what I want,
not what I say" :-)


Segher

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Nathan Lynch]

Tony Breeds <tony@bakeyournoodle.com> wrote:

> On Wed, Apr 08, 2009 at 01:47:36PM -0500, Nathan Lynch wrote:
> 
> > I think I had some reason for doing it this way, but I'm drawing a
> > blank right now.
> > 
> > In the meantime, can someone post the warnings that gcc emits for
> > cacheinfo.c, as well as the gcc version?  I can't reproduce them with
> > 4.3.2 on Fedora.
> 
> ---
> [tony@thor ~]$ egrep cacheinfo tmp/build.log 
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'associativity_show':
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: warning: 'associativity' may be used uninitialized in this function
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'size_show':
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: warning: 'size_kb' may be used uninitialized in this function

Thanks.

So I think I've convinced myself that the warnings are incorrect and
that uninitialized use is not possible.

But I find it odd that gcc gives warnings for these sites but not others
in the file that use the same idiom (e.g. line_size_show,
nr_sets_show).  I'd guess that inlining is implicated somehow.  Would I
be justified in worrying that this version of gcc is generating
incorrect code?

If not, then I'm fine with the uninitialized_var() changes, but do
please include the warnings and the compiler version in the changelog.

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Segher Boessenkool]

>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function  
>> 'associativity_show':
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562:  
>> warning: 'associativity' may be used uninitialized in this function
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function  
>> 'size_show':
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513:  
>> warning: 'size_kb' may be used uninitialized in this function
>
> Thanks.
>
> So I think I've convinced myself that the warnings are incorrect and
> that uninitialized use is not possible.

Strictly speaking the warnings aren't incorrect: the variables
"may be used uninitialized", they just never are.  I agree this
isn't very helpful.

> But I find it odd that gcc gives warnings for these sites but not  
> others
> in the file that use the same idiom (e.g. line_size_show,
> nr_sets_show).

Yeah.

> I'd guess that inlining is implicated somehow.  Would I
> be justified in worrying that this version of gcc is generating
> incorrect code?

Not really.  Sub-optimal code perhaps, but not incorrect.

> If not, then I'm fine with the uninitialized_var() changes, but do
> please include the warnings and the compiler version in the changelog.

If this happens with a non-ancient GCC version, can we have a bugreport
please?


Segher

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Scott Wood]

Segher Boessenkool wrote:
>> Unfortunately -Wno-uninitialized also suppresses the warnings that point
>> to real bugs.
> 
> It's a double-edged sword, yes.  Warnings are always like that:
> if the compiler could know that something _is_ wrong for certain,
> it wouldn't need a warning (it would use an error, instead -- and
> it does do this in certain cases); if it would know something is
> not really wrong, it would just shut up.

The problem is that GCC does not give an error (only a warning) even for 
things like this where it should be trivial to detect that the usage 
*is* uninitialized, not just might be:

int foo(void)
{
    int a;

    return a;
}

And further, there is no separation of warning classes into 
might-be-uninitialized and is-uninitialized-compiler-can-tell-for-sure.

In other words, there should be a way to tell the compiler to err on the 
side of not complaining if it's unsure, but still report the obvious 
ones (or make the latter an error but the former a warning).  That's not 
ESP or DWIM.

-Scott

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Andreas Schwab]

Scott Wood <scottwood@freescale.com> writes:

> The problem is that GCC does not give an error (only a warning) even for
> things like this where it should be trivial to detect that the usage *is*
> uninitialized, not just might be:
>
> int foo(void)
> {
>    int a;
>
>    return a;
> }

The compiler must not reject this code, because the undefined behavior
only occurs if executed.  There is no constraint violated.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Scott Wood]

Andreas Schwab wrote:
> Scott Wood <scottwood@freescale.com> writes:
> 
>> The problem is that GCC does not give an error (only a warning) even for
>> things like this where it should be trivial to detect that the usage *is*
>> uninitialized, not just might be:
>>
>> int foo(void)
>> {
>>    int a;
>>
>>    return a;
>> }
> 
> The compiler must not reject this code, because the undefined behavior
> only occurs if executed.  There is no constraint violated.

Fine (though GCC could have something similar to -Werror but more 
limited in scope to the really serious stuff that *should* be illegal 
even if it isn't), but it should at least be a separate warning class.

My point was to counter Segher's assertion that the compiler currently 
gives an error on the obvious stuff.

-Scott

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Segher Boessenkool]

>>> The problem is that GCC does not give an error (only a warning)  
>>> even for
>>> things like this where it should be trivial to detect that the  
>>> usage *is*
>>> uninitialized, not just might be:
>>>
>>> int foo(void)
>>> {
>>>    int a;
>>>
>>>    return a;
>>> }
>> The compiler must not reject this code, because the undefined  
>> behavior
>> only occurs if executed.  There is no constraint violated.
>
> Fine (though GCC could have something similar to -Werror but more  
> limited in scope to the really serious stuff that *should* be  
> illegal even if it isn't), but it should at least be a separate  
> warning class.
>
> My point was to counter Segher's assertion that the compiler  
> currently gives an error on the obvious stuff.

I never said that, or didn't intend to anyway; what I was trying to say
is that the compiler makes a difference between cases where it knows
something is uninitialized vs. cases where it cannot prove either way:

$ cat mm.c
int bork(void)
{
         int a;

         return a;
}

int main(void)
{
         return bork();
}

$ powerpc-linux-gcc -Wall -W -Os -c mm.c
mm.c: In function 'bork':
mm.c:5: warning: 'a' is used uninitialized in this function

Note: _is_ used uninitialized, not "may be" like in cases where the  
compiler
isn't sure.

I don't know why this isn't an error; perhaps GCC does not assume main 
() to
always be executed.  I don't think it could prove much anything to be
executed in non-toy examples, anyway.


Segher

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Segher Boessenkool]

> And further, there is no separation of warning classes into might- 
> be-uninitialized and is-uninitialized-compiler-can-tell-for-sure.

Indeed.  Please file a bug report.


Segher

Re: [PATCH] Quieten arch/powerpc in a allmodconfig build.

[Scott Wood]

Segher Boessenkool wrote:
>> And further, there is no separation of warning classes into 
>> might-be-uninitialized and is-uninitialized-compiler-can-tell-for-sure.
> 
> Indeed.  Please file a bug report.

Done: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39731

-Scott