[PATCH 0/5] Fix set but unused variable warnings

[PATCH 0/5] Fix set but unused variable warnings

[Justin P. Mattock]

Here is another set of patches fixing some warning messages generated 
when building the kernel when using gcc 4.6.0.

This set just focuses in on the variables warnings that are reported
to be unused. Keep in mind there are still lots more warning messages
that I'm seeing, and will try my hardest to see if I can come up with
a fix, but if things become too difficult and so forth then a bug should
be filled etc..
 
And lastly I want to Thank the people who helped me out on the last patch
set I had sent out and so forth.
Please look if you have the time, and let me know.

Thanks!!

Justin P. Mattock

[PATCH 1/5]wireless:hostap_main.c warning: variable 'iface' set but not used

[Justin P. Mattock]

The patch below fixes a warning message Im seeing with gcc 4.6.0
 CC [M]  drivers/net/wireless/hostap/hostap_main.o
drivers/net/wireless/hostap/hostap_main.c: In function 'hostap_set_multicast_list_queue':
drivers/net/wireless/hostap/hostap_main.c:744:27: warning: variable 'iface' set but not used

 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/net/wireless/hostap/hostap_main.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/hostap/hostap_main.c b/drivers/net/wireless/hostap/hostap_main.c
index eb57d1e..eaee84b 100644
--- a/drivers/net/wireless/hostap/hostap_main.c
+++ b/drivers/net/wireless/hostap/hostap_main.c
@@ -741,9 +741,7 @@ void hostap_set_multicast_list_queue(struct work_struct *work)
 	local_info_t *local =
 		container_of(work, local_info_t, set_multicast_list_queue);
 	struct net_device *dev = local->dev;
-	struct hostap_interface *iface;
 
-	iface = netdev_priv(dev);
 	if (hostap_set_word(dev, HFA384X_RID_PROMISCUOUSMODE,
 			    local->is_promisc)) {
 		printk(KERN_INFO "%s: %sabling promiscuous mode failed\n",
-- 
1.7.1.rc1.21.gf3bd6

[PATCH 2/5]wireless:hostap_ap.c Fix warning: variable 'fc' set but not used

[Justin P. Mattock]

The below patch fixes a warning message when compiling with gcc 4.6.0
  CC [M]  drivers/net/wireless/hostap/hostap_ap.o
drivers/net/wireless/hostap/hostap_ap.c: In function 'hostap_ap_tx_cb_assoc':
drivers/net/wireless/hostap/hostap_ap.c:691:6: warning: variable 'fc' set but not used

 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/net/wireless/hostap/hostap_ap.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/hostap/hostap_ap.c b/drivers/net/wireless/hostap/hostap_ap.c
index 231dbd7..9cadaa2 100644
--- a/drivers/net/wireless/hostap/hostap_ap.c
+++ b/drivers/net/wireless/hostap/hostap_ap.c
@@ -688,7 +688,7 @@ static void hostap_ap_tx_cb_assoc(struct sk_buff *skb, int ok, void *data)
 	struct ap_data *ap = data;
 	struct net_device *dev = ap->local->dev;
 	struct ieee80211_hdr *hdr;
-	u16 fc, status;
+	u16 status;
 	__le16 *pos;
 	struct sta_info *sta = NULL;
 	char *txt = NULL;
@@ -699,7 +699,6 @@ static void hostap_ap_tx_cb_assoc(struct sk_buff *skb, int ok, void *data)
 	}
 
 	hdr = (struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
 	if ((!ieee80211_is_assoc_resp(hdr->frame_control) &&
 	     !ieee80211_is_reassoc_resp(hdr->frame_control)) ||
 	    skb->len < IEEE80211_MGMT_HDR_LEN + 4) {
-- 
1.7.1.rc1.21.gf3bd6

[PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Justin P. Mattock]

The patch below fixes the warning message I am seeing with gcc 4.6.0
  CC      drivers/pci/bus.o
drivers/pci/bus.c: In function 'pci_enable_bridges':
drivers/pci/bus.c:237:6: warning: variable 'retval' set but not used

 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/pci/bus.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 628ea20..84bdb48 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 void pci_enable_bridges(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
-	int retval;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		if (dev->subordinate) {
 			if (!pci_is_enabled(dev)) {
-				retval = pci_enable_device(dev);
 				pci_set_master(dev);
 			}
 			pci_enable_bridges(dev->subordinate);
-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Julian Calaby]

On Wed, Jun 16, 2010 at 15:33, Justin P. Mattock
<justinmattock@gmail.com> wrote:
> The patch below fixes the warning message I am seeing with gcc 4.6.0
>  CC      drivers/pci/bus.o
> drivers/pci/bus.c: In function 'pci_enable_bridges':
> drivers/pci/bus.c:237:6: warning: variable 'retval' set but not used
>
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
>
> ---
>  drivers/pci/bus.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 628ea20..84bdb48 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>  void pci_enable_bridges(struct pci_bus *bus)
>  {
>        struct pci_dev *dev;
> -       int retval;
>
>        list_for_each_entry(dev, &bus->devices, bus_list) {
>                if (dev->subordinate) {
>                        if (!pci_is_enabled(dev)) {
> -                               retval = pci_enable_device(dev);
>                                pci_set_master(dev);
>                        }
>                        pci_enable_bridges(dev->subordinate);

Er, this appears to be bogus: I'm only guessing, but I'd expect that
the pci_enable_device() call is actually doing something useful, and
removing it is going to break *something* - Have you booted a kernel
with this code enabled and these patches applied?

As for this warning, I think that there is a better solution.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Justin P. Mattock]

On 06/15/2010 10:42 PM, Julian Calaby wrote:
> On Wed, Jun 16, 2010 at 15:33, Justin P. Mattock
> <justinmattock@gmail.com>  wrote:
>> The patch below fixes the warning message I am seeing with gcc 4.6.0
>>   CC      drivers/pci/bus.o
>> drivers/pci/bus.c: In function 'pci_enable_bridges':
>> drivers/pci/bus.c:237:6: warning: variable 'retval' set but not used
>>
>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>   drivers/pci/bus.c |    2 --
>>   1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 628ea20..84bdb48 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>>   void pci_enable_bridges(struct pci_bus *bus)
>>   {
>>         struct pci_dev *dev;
>> -       int retval;
>>
>>         list_for_each_entry(dev,&bus->devices, bus_list) {
>>                 if (dev->subordinate) {
>>                         if (!pci_is_enabled(dev)) {
>> -                               retval = pci_enable_device(dev);
>>                                 pci_set_master(dev);
>>                         }
>>                         pci_enable_bridges(dev->subordinate);
>
> Er, this appears to be bogus: I'm only guessing, but I'd expect that
> the pci_enable_device() call is actually doing something useful, and
> removing it is going to break *something* - Have you booted a kernel
> with this code enabled and these patches applied?
>
> As for this warning, I think that there is a better solution.
>
> Thanks,
>

alright!!

Justin P. Mattock

Re: [PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Junchang Wang]

On Tue, Jun 15, 2010 at 10:33:52PM -0700, Justin P. Mattock wrote:
>@@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
> void pci_enable_bridges(struct pci_bus *bus)
> {
> 	struct pci_dev *dev;
>-	int retval;
> 
> 	list_for_each_entry(dev, &bus->devices, bus_list) {
> 		if (dev->subordinate) {
> 			if (!pci_is_enabled(dev)) {
>-				retval = pci_enable_device(dev);
Hi Justin,

pci_enable_device initializes device before it's used by a driver.

I think you should add check instead of eliminating pci_enable_device.

For example,
retval = pci_enable_device(dev);
if (retval < 0) {
	goto handle_err;
}

--Junchang

Re: [PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Justin P. Mattock]

On 06/15/2010 11:07 PM, Junchang Wang wrote:
> On Tue, Jun 15, 2010 at 10:33:52PM -0700, Justin P. Mattock wrote:
>> @@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>> void pci_enable_bridges(struct pci_bus *bus)
>> {
>> 	struct pci_dev *dev;
>> -	int retval;
>>
>> 	list_for_each_entry(dev,&bus->devices, bus_list) {
>> 		if (dev->subordinate) {
>> 			if (!pci_is_enabled(dev)) {
>> -				retval = pci_enable_device(dev);
> Hi Justin,
>
> pci_enable_device initializes device before it's used by a driver.
>
> I think you should add check instead of eliminating pci_enable_device.
>
> For example,
> retval = pci_enable_device(dev);
> if (retval<  0) {
> 	goto handle_err;
> }
>
> --Junchang
>
>


cool, thanks for the example and info on this. I'll play around with 
this to see if I come up with anything.

cheers,

Justin P. Mattock

Re: [PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Justin P. Mattock]

On 06/15/2010 11:07 PM, Junchang Wang wrote:
> On Tue, Jun 15, 2010 at 10:33:52PM -0700, Justin P. Mattock wrote:
>> @@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>> void pci_enable_bridges(struct pci_bus *bus)
>> {
>> 	struct pci_dev *dev;
>> -	int retval;
>>
>> 	list_for_each_entry(dev,&bus->devices, bus_list) {
>> 		if (dev->subordinate) {
>> 			if (!pci_is_enabled(dev)) {
>> -				retval = pci_enable_device(dev);
> Hi Justin,
>
> pci_enable_device initializes device before it's used by a driver.
>
> I think you should add check instead of eliminating pci_enable_device.
>
> For example,
> retval = pci_enable_device(dev);
> if (retval<  0) {
> 	goto handle_err;
> }
>
> --Junchang
>
>

this gets it to compile with no warning. although not sure if correct.

		if (dev->subordinate) {
			if (!pci_is_enabled(dev)) {
				retval = pci_enable_device(dev);
				if (retval)
					printk("test...\n");
				pci_set_master(dev);
			}
			pci_enable_bridges(dev->subordinate);
		}

Justin P. Mattock

Re: [PATCH 3/5]pci:bus.c Fix variable 'retval' set but not used

[Justin P. Mattock]

On 06/15/2010 11:07 PM, Junchang Wang wrote:
> On Tue, Jun 15, 2010 at 10:33:52PM -0700, Justin P. Mattock wrote:
>> @@ -234,12 +234,10 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>> void pci_enable_bridges(struct pci_bus *bus)
>> {
>> 	struct pci_dev *dev;
>> -	int retval;
>>
>> 	list_for_each_entry(dev,&bus->devices, bus_list) {
>> 		if (dev->subordinate) {
>> 			if (!pci_is_enabled(dev)) {
>> -				retval = pci_enable_device(dev);
> Hi Justin,
>
> pci_enable_device initializes device before it's used by a driver.
>
> I think you should add check instead of eliminating pci_enable_device.
>
> For example,
> retval = pci_enable_device(dev);
> if (retval<  0) {
> 	goto handle_err;
> }
>
> --Junchang
>
>

o.k. I looked into this one, as well as the scsi, please have a look 
when you have time and let me know what you think:


---
  drivers/pci/bus.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 628ea20..dba4c28 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -240,6 +240,9 @@ void pci_enable_bridges(struct pci_bus *bus)
  		if (dev->subordinate) {
  			if (!pci_is_enabled(dev)) {
  				retval = pci_enable_device(dev);
+				if (retval < 0) {
+				dev_err(&dev->dev, "maybe something here explaining something...\n");
+				}
  				pci_set_master(dev);
  			}
  			pci_enable_bridges(dev->subordinate);


Justin P. Mattock

[PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Justin P. Mattock]

The below patch fixes a warning message when using gcc 4.6.0
  CC      drivers/pci/setup-bus.o
drivers/pci/setup-bus.c: In function 'pci_assign_unassigned_bridge_resources':
drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
 
 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/pci/setup-bus.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 19b1113..215590b 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -865,7 +865,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 	struct pci_bus *parent = bridge->subordinate;
 	int tried_times = 0;
 	struct resource_list_x head, *list;
-	int retval;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
 				  IORESOURCE_PREFETCH;
 
@@ -874,7 +873,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 again:
 	pci_bus_size_bridges(parent);
 	__pci_bridge_assign_resources(bridge, &head);
-	retval = pci_reenable_device(bridge);
 	pci_set_master(bridge);
 	pci_enable_bridges(parent);
 
-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Jesse Barnes]

On Tue, 15 Jun 2010 22:33:53 -0700
"Justin P. Mattock" <justinmattock@gmail.com> wrote:

> The below patch fixes a warning message when using gcc 4.6.0
>   CC      drivers/pci/setup-bus.o
> drivers/pci/setup-bus.c: In function 'pci_assign_unassigned_bridge_resources':
> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
>  
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> 
> ---
>  drivers/pci/setup-bus.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 19b1113..215590b 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -865,7 +865,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  	struct pci_bus *parent = bridge->subordinate;
>  	int tried_times = 0;
>  	struct resource_list_x head, *list;
> -	int retval;
>  	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>  				  IORESOURCE_PREFETCH;
>  
> @@ -874,7 +873,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  again:
>  	pci_bus_size_bridges(parent);
>  	__pci_bridge_assign_resources(bridge, &head);
> -	retval = pci_reenable_device(bridge);
>  	pci_set_master(bridge);
>  	pci_enable_bridges(parent);
>  

This has the same problem as your earlier bridge_enable patch; you need
to keep the call to pci_reenable_device.  I should have caught this
issue when Yinghai's patch went in: the right way to silence a warning
about not checking a return value isn't to simply assign it to
something, you really should *do* something as well, at least some
debug output would be nice, but ideally handling the error in a sane
way. 

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Justin P. Mattock]

On 06/18/2010 10:23 AM, Jesse Barnes wrote:
> On Tue, 15 Jun 2010 22:33:53 -0700
> "Justin P. Mattock"<justinmattock@gmail.com>  wrote:
>
>> The below patch fixes a warning message when using gcc 4.6.0
>>    CC      drivers/pci/setup-bus.o
>> drivers/pci/setup-bus.c: In function 'pci_assign_unassigned_bridge_resources':
>> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
>>
>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>   drivers/pci/setup-bus.c |    2 --
>>   1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 19b1113..215590b 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -865,7 +865,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>>   	struct pci_bus *parent = bridge->subordinate;
>>   	int tried_times = 0;
>>   	struct resource_list_x head, *list;
>> -	int retval;
>>   	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>>   				  IORESOURCE_PREFETCH;
>>
>> @@ -874,7 +873,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>>   again:
>>   	pci_bus_size_bridges(parent);
>>   	__pci_bridge_assign_resources(bridge,&head);
>> -	retval = pci_reenable_device(bridge);
>>   	pci_set_master(bridge);
>>   	pci_enable_bridges(parent);
>>
>
> This has the same problem as your earlier bridge_enable patch; you need
> to keep the call to pci_reenable_device.  I should have caught this
> issue when Yinghai's patch went in: the right way to silence a warning
> about not checking a return value isn't to simply assign it to
> something, you really should *do* something as well, at least some
> debug output would be nice, but ideally handling the error in a sane
> way.
>


yeah I was still looking into the scsi patch with adding a printk
then I can look at this as well.

Justin P. Mattock

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Justin P. Mattock]

 From b07231ddb853e9388cea77a82da210e36ab79aad Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Fri, 18 Jun 2010 11:32:20 -0700
Subject: [PATCH 2/2] setup-bus_test
  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
  drivers/pci/setup-bus.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 19b1113..7b57dd0 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -875,6 +875,7 @@ again:
  	pci_bus_size_bridges(parent);
  	__pci_bridge_assign_resources(bridge, &head);
  	retval = pci_reenable_device(bridge);
+	printk(KERN_DEBUG "PCI%d: re-enabling device\n", retval);
  	pci_set_master(bridge);
  	pci_enable_bridges(parent);

-- 
1.7.1.rc1.21.gf3bd6


o.k. I went through this as you had requested to the best of my 
knowledge(bit confused with this, but will try).

let me know if more should be added and/or it's totally wrong
then I'll try again until correct..

Justin P. Mattock

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Yinghai Lu]

On 06/18/2010 10:23 AM, Jesse Barnes wrote:
> On Tue, 15 Jun 2010 22:33:53 -0700
> "Justin P. Mattock" <justinmattock@gmail.com> wrote:
> 
>> The below patch fixes a warning message when using gcc 4.6.0
>>   CC      drivers/pci/setup-bus.o
>> drivers/pci/setup-bus.c: In function 'pci_assign_unassigned_bridge_resources':
>> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
>>  
>>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
>>
>> ---
>>  drivers/pci/setup-bus.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 19b1113..215590b 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -865,7 +865,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>>  	struct pci_bus *parent = bridge->subordinate;
>>  	int tried_times = 0;
>>  	struct resource_list_x head, *list;
>> -	int retval;
>>  	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>>  				  IORESOURCE_PREFETCH;
>>  
>> @@ -874,7 +873,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>>  again:
>>  	pci_bus_size_bridges(parent);
>>  	__pci_bridge_assign_resources(bridge, &head);
>> -	retval = pci_reenable_device(bridge);
>>  	pci_set_master(bridge);
>>  	pci_enable_bridges(parent);
>>  

I sent following patch several weeks ago, can you put that in pci-next?

Subject: [PATCH] pciehp: Enable bridges at last for multiple try assigning

Found one PCIe Module with several bridges "cold" hotadd doesn't work.

the root cause:
1. BIOS assign small range the parent bridges.
2. First try for hotadd only can make some bridges get resource assigned.
3. Second will update parent bridge res, get right sizes for all child bridges
    and devices, but resource for child bridges are not set to HW register.
    Because first try already enable those bridges, so __pci_bridge_assign_resource
    skip the setting step.

So try to move enabling of child bridges to last and only do that one time

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -866,19 +866,16 @@ void pci_assign_unassigned_bridge_resour
 again:
 	pci_bus_size_bridges(parent);
 	__pci_bridge_assign_resources(bridge, &head);
-	retval = pci_reenable_device(bridge);
-	pci_set_master(bridge);
-	pci_enable_bridges(parent);
 
 	tried_times++;
 
 	if (!head.next)
-		return;
+		goto enable_all;
 
 	if (tried_times >= 2) {
 		/* still fail, don't need to try more */
 		free_failed_list(&head);
-		return;
+		goto enable_all;
 	}
 
 	printk(KERN_DEBUG "PCI: No. %d try to assign unassigned res\n",
@@ -911,5 +908,10 @@ again:
 	free_failed_list(&head);
 
 	goto again;
+
+enable_all:
+	retval = pci_reenable_device(bridge);
+	pci_set_master(bridge);
+	pci_enable_bridges(parent);
 }
 EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Jesse Barnes]

On Fri, 18 Jun 2010 11:48:29 -0700
Yinghai Lu <yinghai@kernel.org> wrote:

> On 06/18/2010 10:23 AM, Jesse Barnes wrote:
> > On Tue, 15 Jun 2010 22:33:53 -0700
> > "Justin P. Mattock" <justinmattock@gmail.com> wrote:
> > 
> >> The below patch fixes a warning message when using gcc 4.6.0
> >>   CC      drivers/pci/setup-bus.o
> >> drivers/pci/setup-bus.c: In function 'pci_assign_unassigned_bridge_resources':
> >> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
> >>  
> >>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> >>
> >> ---
> >>  drivers/pci/setup-bus.c |    2 --
> >>  1 files changed, 0 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> >> index 19b1113..215590b 100644
> >> --- a/drivers/pci/setup-bus.c
> >> +++ b/drivers/pci/setup-bus.c
> >> @@ -865,7 +865,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
> >>  	struct pci_bus *parent = bridge->subordinate;
> >>  	int tried_times = 0;
> >>  	struct resource_list_x head, *list;
> >> -	int retval;
> >>  	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> >>  				  IORESOURCE_PREFETCH;
> >>  
> >> @@ -874,7 +873,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
> >>  again:
> >>  	pci_bus_size_bridges(parent);
> >>  	__pci_bridge_assign_resources(bridge, &head);
> >> -	retval = pci_reenable_device(bridge);
> >>  	pci_set_master(bridge);
> >>  	pci_enable_bridges(parent);
> >>  
> 
> I sent following patch several weeks ago, can you put that in pci-next?
> 
> Subject: [PATCH] pciehp: Enable bridges at last for multiple try assigning
> 
> Found one PCIe Module with several bridges "cold" hotadd doesn't work.
> 
> the root cause:
> 1. BIOS assign small range the parent bridges.
> 2. First try for hotadd only can make some bridges get resource assigned.
> 3. Second will update parent bridge res, get right sizes for all child bridges
>     and devices, but resource for child bridges are not set to HW register.
>     Because first try already enable those bridges, so __pci_bridge_assign_resource
>     skip the setting step.
> 
> So try to move enabling of child bridges to last and only do that one time
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Yeah I had a hard time following the changelog, but just looked it
over.  Looks safe, but Justin will still need to check the return value
on pci_reenable_device.

Justin, we don't want a message on every reenable, just on ones that
fail.  So can you protect your printk with if (retval) instead?  You'll
need to refresh it based on my linux-next branch in a few minutes, as
I'm pushing Yinghai's patch now.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Justin P. Mattock]

On 06/18/2010 12:49 PM, Jesse Barnes wrote:
> On Fri, 18 Jun 2010 11:48:29 -0700
> Yinghai Lu<yinghai@kernel.org>  wrote:
>
>> On 06/18/2010 10:23 AM, Jesse Barnes wrote:
>>> On Tue, 15 Jun 2010 22:33:53 -0700
>>> "Justin P. Mattock"<justinmattock@gmail.com>  wrote:
>>>
>>>> The below patch fixes a warning message when using gcc 4.6.0
>>>>    CC      drivers/pci/setup-bus.o
>>>> drivers/pci/setup-bus.c: In function 'pci_assign_unassigned_bridge_resources':
>>>> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
>>>>
>>>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>>>
>>>> ---
>>>>   drivers/pci/setup-bus.c |    2 --
>>>>   1 files changed, 0 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>>> index 19b1113..215590b 100644
>>>> --- a/drivers/pci/setup-bus.c
>>>> +++ b/drivers/pci/setup-bus.c
>>>> @@ -865,7 +865,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>>>>   	struct pci_bus *parent = bridge->subordinate;
>>>>   	int tried_times = 0;
>>>>   	struct resource_list_x head, *list;
>>>> -	int retval;
>>>>   	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>>>>   				  IORESOURCE_PREFETCH;
>>>>
>>>> @@ -874,7 +873,6 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>>>>   again:
>>>>   	pci_bus_size_bridges(parent);
>>>>   	__pci_bridge_assign_resources(bridge,&head);
>>>> -	retval = pci_reenable_device(bridge);
>>>>   	pci_set_master(bridge);
>>>>   	pci_enable_bridges(parent);
>>>>
>>
>> I sent following patch several weeks ago, can you put that in pci-next?
>>
>> Subject: [PATCH] pciehp: Enable bridges at last for multiple try assigning
>>
>> Found one PCIe Module with several bridges "cold" hotadd doesn't work.
>>
>> the root cause:
>> 1. BIOS assign small range the parent bridges.
>> 2. First try for hotadd only can make some bridges get resource assigned.
>> 3. Second will update parent bridge res, get right sizes for all child bridges
>>      and devices, but resource for child bridges are not set to HW register.
>>      Because first try already enable those bridges, so __pci_bridge_assign_resource
>>      skip the setting step.
>>
>> So try to move enabling of child bridges to last and only do that one time
>>
>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>
> Yeah I had a hard time following the changelog, but just looked it
> over.  Looks safe, but Justin will still need to check the return value
> on pci_reenable_device.
>
> Justin, we don't want a message on every reenable, just on ones that
> fail.  So can you protect your printk with if (retval) instead?  You'll
> need to refresh it based on my linux-next branch in a few minutes, as
> I'm pushing Yinghai's patch now.
>


just added this in(as a test), and the retval warning still shows up.
with the last post I just added a printk was that legit, and if so what 
else might be added to it to make it complete and proper?

Justin P. Mattock

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Jesse Barnes]

On Fri, 18 Jun 2010 12:59:32 -0700
"Justin P. Mattock" <justinmattock@gmail.com> wrote:
> just added this in(as a test), and the retval warning still shows up.
> with the last post I just added a printk was that legit, and if so what 
> else might be added to it to make it complete and proper?

What's the full warning?  Seems like printing the value should have
been enough to shut up gcc...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Justin P. Mattock]

On 06/18/2010 01:05 PM, Jesse Barnes wrote:
> On Fri, 18 Jun 2010 12:59:32 -0700
> "Justin P. Mattock"<justinmattock@gmail.com>  wrote:
>> just added this in(as a test), and the retval warning still shows up.
>> with the last post I just added a printk was that legit, and if so what
>> else might be added to it to make it complete and proper?
>
> What's the full warning?  Seems like printing the value should have
> been enough to shut up gcc...
>

this is the warning messg after applying yinghai's patch:

   CC      drivers/pci/setup-bus.o
drivers/pci/setup-bus.c: In function 
'pci_assign_unassigned_bridge_resources':
drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used


if I add a printk then gcc is content.. patch below, but not the best at 
creating printk's(the whole % thing messes me up) but here goes:

 From 48e15b87072c6b4286d943c55bfe2ae26d358795 Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Fri, 18 Jun 2010 13:23:27 -0700
Subject: [PATCH 4/4] bus.c_add_print
  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
  drivers/pci/setup-bus.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 66cb8f4..806b766 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -919,6 +919,7 @@ again:

  enable_all:
  	retval = pci_reenable_device(bridge);
+	printk(KERN_DEBUG "PCI%d: re-enabling device\n", retval);
  	pci_set_master(bridge);
  	pci_enable_bridges(parent);
  }
-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Jesse Barnes]

On Fri, 18 Jun 2010 13:26:32 -0700
"Justin P. Mattock" <justinmattock@gmail.com> wrote:

> On 06/18/2010 01:05 PM, Jesse Barnes wrote:
> > On Fri, 18 Jun 2010 12:59:32 -0700
> > "Justin P. Mattock"<justinmattock@gmail.com>  wrote:
> >> just added this in(as a test), and the retval warning still shows up.
> >> with the last post I just added a printk was that legit, and if so what
> >> else might be added to it to make it complete and proper?
> >
> > What's the full warning?  Seems like printing the value should have
> > been enough to shut up gcc...
> >
> 
> this is the warning messg after applying yinghai's patch:
> 
>    CC      drivers/pci/setup-bus.o
> drivers/pci/setup-bus.c: In function 
> 'pci_assign_unassigned_bridge_resources':
> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used

Right because Yinghai's patch just sets retval but doesn't actually use
it anywhere.

> if I add a printk then gcc is content.. patch below, but not the best at 
> creating printk's(the whole % thing messes me up) but here goes:
> 
>  From 48e15b87072c6b4286d943c55bfe2ae26d358795 Mon Sep 17 00:00:00 2001
> From: Justin P. Mattock <justinmattock@gmail.com>
> Date: Fri, 18 Jun 2010 13:23:27 -0700
> Subject: [PATCH 4/4] bus.c_add_print
>   Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> 
> ---
>   drivers/pci/setup-bus.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 66cb8f4..806b766 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -919,6 +919,7 @@ again:
> 
>   enable_all:
>   	retval = pci_reenable_device(bridge);
> +	printk(KERN_DEBUG "PCI%d: re-enabling device\n", retval);
>   	pci_set_master(bridge);
>   	pci_enable_bridges(parent);
>   }

Again, this doesn't have the if (retval) condition around the printk; I
don't want to see this message everytime regardless.  Also the message
is misleading, it should be something like:
  dev_err(&bridge->dev, "failed to re-enable device: %d\n", retval)
instead.  PCI%d makes it look like we're talking about a specific bus
or something and not an error code.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5]pci:setup_bus.c Fix warning: variable 'retval' set but not used

[Justin P. Mattock]

On 06/18/2010 01:46 PM, Jesse Barnes wrote:
> On Fri, 18 Jun 2010 13:26:32 -0700
> "Justin P. Mattock"<justinmattock@gmail.com>  wrote:
>
>> On 06/18/2010 01:05 PM, Jesse Barnes wrote:
>>> On Fri, 18 Jun 2010 12:59:32 -0700
>>> "Justin P. Mattock"<justinmattock@gmail.com>   wrote:
>>>> just added this in(as a test), and the retval warning still shows up.
>>>> with the last post I just added a printk was that legit, and if so what
>>>> else might be added to it to make it complete and proper?
>>>
>>> What's the full warning?  Seems like printing the value should have
>>> been enough to shut up gcc...
>>>
>>
>> this is the warning messg after applying yinghai's patch:
>>
>>     CC      drivers/pci/setup-bus.o
>> drivers/pci/setup-bus.c: In function
>> 'pci_assign_unassigned_bridge_resources':
>> drivers/pci/setup-bus.c:868:6: warning: variable 'retval' set but not used
>
> Right because Yinghai's patch just sets retval but doesn't actually use
> it anywhere.
>

that's what is confusing..(not being used, but is being used, but gcc 
says it's not used..)  :-)

>> if I add a printk then gcc is content.. patch below, but not the best at
>> creating printk's(the whole % thing messes me up) but here goes:
>>
>>   From 48e15b87072c6b4286d943c55bfe2ae26d358795 Mon Sep 17 00:00:00 2001
>> From: Justin P. Mattock<justinmattock@gmail.com>
>> Date: Fri, 18 Jun 2010 13:23:27 -0700
>> Subject: [PATCH 4/4] bus.c_add_print
>>    Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>    drivers/pci/setup-bus.c |    1 +
>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 66cb8f4..806b766 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -919,6 +919,7 @@ again:
>>
>>    enable_all:
>>    	retval = pci_reenable_device(bridge);
>> +	printk(KERN_DEBUG "PCI%d: re-enabling device\n", retval);
>>    	pci_set_master(bridge);
>>    	pci_enable_bridges(parent);
>>    }
>
> Again, this doesn't have the if (retval) condition around the printk; I
> don't want to see this message everytime regardless.  Also the message
> is misleading, it should be something like:
>    dev_err(&bridge->dev, "failed to re-enable device: %d\n", retval)
> instead.  PCI%d makes it look like we're talking about a specific bus
> or something and not an error code.
>

o.k. I admit I looked at other printk's in this file to get an idea of 
what I might do.. saw PCI%d and figured it would print
"PCI: re-enabling device"
but didnt think it was an error... reason for putting KERN_DEBUG.

here is what the new patch looks like:


 From f910375438be06497d0524bff146c26cafca272b Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Fri, 18 Jun 2010 14:08:37 -0700
Subject: [PATCH 4/4] setup-pci_test
  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
  drivers/pci/setup-bus.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 66cb8f4..2ab5f1e 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -919,6 +919,9 @@ again:

  enable_all:
  	retval = pci_reenable_device(bridge);
+	if (retval) {
+		dev_err(&bridge->dev, "failed to re-enable device: %d\n", retval);
+	}
  	pci_set_master(bridge);
  	pci_enable_bridges(parent);
  }
-- 
1.7.1.rc1.21.gf3bd6


should I have put if (!retval) instead
should I put "failed to re-enable bridge device"
is there an exit code needed?

if not and all is good then I can resend this out..

Justin P. Mattock

[PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Justin P. Mattock]

The below patch fixes a warning message generated by gcc 4.6.0
  CC      drivers/scsi/hosts.o
drivers/scsi/hosts.c: In function 'scsi_host_alloc':
drivers/scsi/hosts.c:328:6: warning: variable 'rval' set but not used
 
 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/scsi/hosts.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 6660fa9..00fd6a4 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 {
 	struct Scsi_Host *shost;
 	gfp_t gfp_mask = GFP_KERNEL;
-	int rval;
 
 	if (sht->unchecked_isa_dma && privsize)
 		gfp_mask |= __GFP_DMA;
@@ -420,7 +419,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->ehandler = kthread_run(scsi_error_handler, shost,
 			"scsi_eh_%d", shost->host_no);
 	if (IS_ERR(shost->ehandler)) {
-		rval = PTR_ERR(shost->ehandler);
 		goto fail_kfree;
 	}
 
-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[James Bottomley]

On Tue, 2010-06-15 at 22:33 -0700, Justin P. Mattock wrote:
> The below patch fixes a warning message generated by gcc 4.6.0
>   CC      drivers/scsi/hosts.o
> drivers/scsi/hosts.c: In function 'scsi_host_alloc':
> drivers/scsi/hosts.c:328:6: warning: variable 'rval' set but not used
>  
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> 
> ---
>  drivers/scsi/hosts.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 6660fa9..00fd6a4 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  {
>  	struct Scsi_Host *shost;
>  	gfp_t gfp_mask = GFP_KERNEL;
> -	int rval;
>  
>  	if (sht->unchecked_isa_dma && privsize)
>  		gfp_mask |= __GFP_DMA;
> @@ -420,7 +419,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  	shost->ehandler = kthread_run(scsi_error_handler, shost,
>  			"scsi_eh_%d", shost->host_no);
>  	if (IS_ERR(shost->ehandler)) {
> -		rval = PTR_ERR(shost->ehandler);
>  		goto fail_kfree;
>  	}

For future reference, this is less stylistically acceptable C: you've
reduced the if clause to a single statement, so the braces need
removing.

However, I don't think we should be ignoring the fact that the eh thread
failed to spawn, so I think some type of printed warning (giving the
error code) would be a much more appropriate replacement.

James

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Justin P. Mattock]

On 06/16/2010 08:34 AM, James Bottomley wrote:
> On Tue, 2010-06-15 at 22:33 -0700, Justin P. Mattock wrote:
>> The below patch fixes a warning message generated by gcc 4.6.0
>>    CC      drivers/scsi/hosts.o
>> drivers/scsi/hosts.c: In function 'scsi_host_alloc':
>> drivers/scsi/hosts.c:328:6: warning: variable 'rval' set but not used
>>
>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>   drivers/scsi/hosts.c |    2 --
>>   1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 6660fa9..00fd6a4 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>   {
>>   	struct Scsi_Host *shost;
>>   	gfp_t gfp_mask = GFP_KERNEL;
>> -	int rval;
>>
>>   	if (sht->unchecked_isa_dma&&  privsize)
>>   		gfp_mask |= __GFP_DMA;
>> @@ -420,7 +419,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>   	shost->ehandler = kthread_run(scsi_error_handler, shost,
>>   			"scsi_eh_%d", shost->host_no);
>>   	if (IS_ERR(shost->ehandler)) {
>> -		rval = PTR_ERR(shost->ehandler);
>>   		goto fail_kfree;
>>   	}
>
> For future reference, this is less stylistically acceptable C: you've
> reduced the if clause to a single statement, so the braces need
> removing.
>
> However, I don't think we should be ignoring the fact that the eh thread
> failed to spawn, so I think some type of printed warning (giving the
> error code) would be a much more appropriate replacement.
>
> James
>
>
>


o.k. I'll give a try at that.. as a  test I did this(below) seemed to 
compile clean, but not sure if this is what you're asking for though:


 From 8a4d6e793e0f92d180a6f48c53bbf00d2751ad01 Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Wed, 16 Jun 2010 08:58:13 -0700
Subject: [PATCH] test
  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
  drivers/scsi/hosts.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 6660fa9..8d98a46 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
  {
  	struct Scsi_Host *shost;
  	gfp_t gfp_mask = GFP_KERNEL;
-	int rval;

  	if (sht->unchecked_isa_dma && privsize)
  		gfp_mask |= __GFP_DMA;
@@ -420,7 +419,7 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
  	shost->ehandler = kthread_run(scsi_error_handler, shost,
  			"scsi_eh_%d", shost->host_no);
  	if (IS_ERR(shost->ehandler)) {
-		rval = PTR_ERR(shost->ehandler);
+		printk(KERN_WARNING "test.....\n");
  		goto fail_kfree;
  	}

-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Rolf Eike Beer]

[-- Attachment #1: Type: Text/Plain, Size: 1478 bytes --]

Justin P. Mattock wrote:
> On 06/16/2010 08:34 AM, James Bottomley wrote:

> > However, I don't think we should be ignoring the fact that the eh thread
> > failed to spawn, so I think some type of printed warning (giving the
> > error code) would be a much more appropriate replacement.

> o.k. I'll give a try at that.. as a  test I did this(below) seemed to
> compile clean, but not sure if this is what you're asking for though:

> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 6660fa9..8d98a46 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
>   {
>   	struct Scsi_Host *shost;
>   	gfp_t gfp_mask = GFP_KERNEL;
> -	int rval;
> 
>   	if (sht->unchecked_isa_dma && privsize)
>   		gfp_mask |= __GFP_DMA;
> @@ -420,7 +419,7 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
>   	shost->ehandler = kthread_run(scsi_error_handler, shost,
>   			"scsi_eh_%d", shost->host_no);
>   	if (IS_ERR(shost->ehandler)) {
> -		rval = PTR_ERR(shost->ehandler);
> +		printk(KERN_WARNING "test.....\n");
>   		goto fail_kfree;
>   	}

If you want to print anything here I think the error code would be a good 
thing to include as that could give a hint _what_ actually went wrong. In that 
case you would need rval, but IMHO it should be declared inside that if 
clause.

Eike

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

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[James Bottomley]

On Wed, 2010-06-16 at 09:00 -0700, Justin P. Mattock wrote:
> On 06/16/2010 08:34 AM, James Bottomley wrote:
> > On Tue, 2010-06-15 at 22:33 -0700, Justin P. Mattock wrote:
> >> The below patch fixes a warning message generated by gcc 4.6.0
> >>    CC      drivers/scsi/hosts.o
> >> drivers/scsi/hosts.c: In function 'scsi_host_alloc':
> >> drivers/scsi/hosts.c:328:6: warning: variable 'rval' set but not used
> >>
> >>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
> >>
> >> ---
> >>   drivers/scsi/hosts.c |    2 --
> >>   1 files changed, 0 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> >> index 6660fa9..00fd6a4 100644
> >> --- a/drivers/scsi/hosts.c
> >> +++ b/drivers/scsi/hosts.c
> >> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> >>   {
> >>   	struct Scsi_Host *shost;
> >>   	gfp_t gfp_mask = GFP_KERNEL;
> >> -	int rval;
> >>
> >>   	if (sht->unchecked_isa_dma&&  privsize)
> >>   		gfp_mask |= __GFP_DMA;
> >> @@ -420,7 +419,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> >>   	shost->ehandler = kthread_run(scsi_error_handler, shost,
> >>   			"scsi_eh_%d", shost->host_no);
> >>   	if (IS_ERR(shost->ehandler)) {
> >> -		rval = PTR_ERR(shost->ehandler);
> >>   		goto fail_kfree;
> >>   	}
> >
> > For future reference, this is less stylistically acceptable C: you've
> > reduced the if clause to a single statement, so the braces need
> > removing.
> >
> > However, I don't think we should be ignoring the fact that the eh thread
> > failed to spawn, so I think some type of printed warning (giving the
> > error code) would be a much more appropriate replacement.
> >
> > James
> >
> >
> >
> 
> 
> o.k. I'll give a try at that.. as a  test I did this(below) seemed to 
> compile clean, but not sure if this is what you're asking for though:
> 
> 
>  From 8a4d6e793e0f92d180a6f48c53bbf00d2751ad01 Mon Sep 17 00:00:00 2001
> From: Justin P. Mattock <justinmattock@gmail.com>
> Date: Wed, 16 Jun 2010 08:58:13 -0700
> Subject: [PATCH] test
>   Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> 
> ---
>   drivers/scsi/hosts.c |    3 +--
>   1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 6660fa9..8d98a46 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct 
> scsi_host_template *sht, int privsize)
>   {
>   	struct Scsi_Host *shost;
>   	gfp_t gfp_mask = GFP_KERNEL;
> -	int rval;
> 
>   	if (sht->unchecked_isa_dma && privsize)
>   		gfp_mask |= __GFP_DMA;
> @@ -420,7 +419,7 @@ struct Scsi_Host *scsi_host_alloc(struct 
> scsi_host_template *sht, int privsize)
>   	shost->ehandler = kthread_run(scsi_error_handler, shost,
>   			"scsi_eh_%d", shost->host_no);
>   	if (IS_ERR(shost->ehandler)) {
> -		rval = PTR_ERR(shost->ehandler);
> +		printk(KERN_WARNING "test.....\n")

Erm, well, as I said, error code and the fact that the thread failed to
start, so more

printk(KERN_WARNING "scsi%d: error handler thread failed to spawn, error
= %d\n", host->host_no, PTR_ERR(shost->ehandler));

James

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Justin P. Mattock]

On 06/16/2010 10:33 AM, James Bottomley wrote:
> On Wed, 2010-06-16 at 09:00 -0700, Justin P. Mattock wrote:
>> On 06/16/2010 08:34 AM, James Bottomley wrote:
>>> On Tue, 2010-06-15 at 22:33 -0700, Justin P. Mattock wrote:
>>>> The below patch fixes a warning message generated by gcc 4.6.0
>>>>     CC      drivers/scsi/hosts.o
>>>> drivers/scsi/hosts.c: In function 'scsi_host_alloc':
>>>> drivers/scsi/hosts.c:328:6: warning: variable 'rval' set but not used
>>>>
>>>>    Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>>>
>>>> ---
>>>>    drivers/scsi/hosts.c |    2 --
>>>>    1 files changed, 0 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>>> index 6660fa9..00fd6a4 100644
>>>> --- a/drivers/scsi/hosts.c
>>>> +++ b/drivers/scsi/hosts.c
>>>> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>>>    {
>>>>    	struct Scsi_Host *shost;
>>>>    	gfp_t gfp_mask = GFP_KERNEL;
>>>> -	int rval;
>>>>
>>>>    	if (sht->unchecked_isa_dma&&   privsize)
>>>>    		gfp_mask |= __GFP_DMA;
>>>> @@ -420,7 +419,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>>>    	shost->ehandler = kthread_run(scsi_error_handler, shost,
>>>>    			"scsi_eh_%d", shost->host_no);
>>>>    	if (IS_ERR(shost->ehandler)) {
>>>> -		rval = PTR_ERR(shost->ehandler);
>>>>    		goto fail_kfree;
>>>>    	}
>>>
>>> For future reference, this is less stylistically acceptable C: you've
>>> reduced the if clause to a single statement, so the braces need
>>> removing.
>>>
>>> However, I don't think we should be ignoring the fact that the eh thread
>>> failed to spawn, so I think some type of printed warning (giving the
>>> error code) would be a much more appropriate replacement.
>>>
>>> James
>>>
>>>
>>>
>>
>>
>> o.k. I'll give a try at that.. as a  test I did this(below) seemed to
>> compile clean, but not sure if this is what you're asking for though:
>>
>>
>>   From 8a4d6e793e0f92d180a6f48c53bbf00d2751ad01 Mon Sep 17 00:00:00 2001
>> From: Justin P. Mattock<justinmattock@gmail.com>
>> Date: Wed, 16 Jun 2010 08:58:13 -0700
>> Subject: [PATCH] test
>>    Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>    drivers/scsi/hosts.c |    3 +--
>>    1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 6660fa9..8d98a46 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -325,7 +325,6 @@ struct Scsi_Host *scsi_host_alloc(struct
>> scsi_host_template *sht, int privsize)
>>    {
>>    	struct Scsi_Host *shost;
>>    	gfp_t gfp_mask = GFP_KERNEL;
>> -	int rval;
>>
>>    	if (sht->unchecked_isa_dma&&  privsize)
>>    		gfp_mask |= __GFP_DMA;
>> @@ -420,7 +419,7 @@ struct Scsi_Host *scsi_host_alloc(struct
>> scsi_host_template *sht, int privsize)
>>    	shost->ehandler = kthread_run(scsi_error_handler, shost,
>>    			"scsi_eh_%d", shost->host_no);
>>    	if (IS_ERR(shost->ehandler)) {
>> -		rval = PTR_ERR(shost->ehandler);
>> +		printk(KERN_WARNING "test.....\n")
>
> Erm, well, as I said, error code and the fact that the thread failed to
> start, so more
>
> printk(KERN_WARNING "scsi%d: error handler thread failed to spawn, error
> = %d\n", host->host_no, PTR_ERR(shost->ehandler));
>
> James
>
>
>
>

yeah I figured the printk needed more. I was more concerned with making 
sure this is what you are asking for(which looks like it is).
So I added your printk but ended up getting an undeclared error:

   CC      drivers/scsi/hosts.o
drivers/scsi/hosts.c: In function 'scsi_host_alloc':
drivers/scsi/hosts.c:422:85: error: 'host' undeclared (first use in this 
function)
drivers/scsi/hosts.c:422:85: note: each undeclared identifier is 
reported only once for each function it appears in
make[2]: *** [drivers/scsi/hosts.o] Error 1
make[1]: *** [drivers/scsi] Error 2
make: *** [drivers] Error 2


I do see a
shost->host_no
but using this seems to not satisfy the compiler:

drivers/scsi/hosts.c: In function 'scsi_host_alloc':
drivers/scsi/hosts.c:422:3: warning: format '%d' expects type 'int', but 
argument 3 has type 'long int'

I guess it's safe to say my newbieness is getting my a** kicked with 
code(all part of the learning experience..)

Justin P. Mattock

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Justin P. Mattock]

> Erm, well, as I said, error code and the fact that the thread failed to
> start, so more
>
> printk(KERN_WARNING "scsi%d: error handler thread failed to spawn, error
> = %d\n", host->host_no, PTR_ERR(shost->ehandler));
>
> James
>

o.k. I looked at this a bit more and finally got the thing to build 
through without a warning, using what you had sent, but keep in mind I 
still need to add error  = %d\n", host->host_no, to the printk

here's what I have so far:


  drivers/scsi/hosts.c |    3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 6660fa9..c1ff708 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -419,8 +419,9 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)

  	shost->ehandler = kthread_run(scsi_error_handler, shost,
  			"scsi_eh_%d", shost->host_no);
+	rval = PTR_ERR(shost->ehandler);
  	if (IS_ERR(shost->ehandler)) {
-		rval = PTR_ERR(shost->ehandler);
+		printk(KERN_WARNING "scsi%d: error handler thread failed to spawn\n", 
rval);
  		goto fail_kfree;
  	}

I'll continue to look at this today!!

cheers,

Justin P. Mattock

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[James Bottomley]

On Thu, 2010-06-17 at 09:16 -0700, Justin P. Mattock wrote:
> > Erm, well, as I said, error code and the fact that the thread failed to
> > start, so more
> >
> > printk(KERN_WARNING "scsi%d: error handler thread failed to spawn, error
> > = %d\n", host->host_no, PTR_ERR(shost->ehandler));
> >
> > James
> >
> 
> o.k. I looked at this a bit more and finally got the thing to build 
> through without a warning, using what you had sent, but keep in mind I 
> still need to add error  = %d\n", host->host_no, to the printk
> 
> here's what I have so far:
> 
> 
>   drivers/scsi/hosts.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 6660fa9..c1ff708 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -419,8 +419,9 @@ struct Scsi_Host *scsi_host_alloc(struct 
> scsi_host_template *sht, int privsize)
> 
>   	shost->ehandler = kthread_run(scsi_error_handler, shost,
>   			"scsi_eh_%d", shost->host_no);
> +	rval = PTR_ERR(shost->ehandler);
>   	if (IS_ERR(shost->ehandler)) {
> -		rval = PTR_ERR(shost->ehandler);
> +		printk(KERN_WARNING "scsi%d: error handler thread failed to spawn\n", 
> rval);

So this isn't really an improvement over the suggestion.  What you
wanted was shost->host_no, as you deduced, but the %d becomes %ld to
quiet the type warning.

James

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Justin P. Mattock]

On 06/18/2010 12:37 PM, James Bottomley wrote:
> On Thu, 2010-06-17 at 09:16 -0700, Justin P. Mattock wrote:
>>> Erm, well, as I said, error code and the fact that the thread failed to
>>> start, so more
>>>
>>> printk(KERN_WARNING "scsi%d: error handler thread failed to spawn, error
>>> = %d\n", host->host_no, PTR_ERR(shost->ehandler));
>>>
>>> James
>>>
>>
>> o.k. I looked at this a bit more and finally got the thing to build
>> through without a warning, using what you had sent, but keep in mind I
>> still need to add error  = %d\n", host->host_no, to the printk
>>
>> here's what I have so far:
>>
>>
>>    drivers/scsi/hosts.c |    3 ++-
>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 6660fa9..c1ff708 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -419,8 +419,9 @@ struct Scsi_Host *scsi_host_alloc(struct
>> scsi_host_template *sht, int privsize)
>>
>>    	shost->ehandler = kthread_run(scsi_error_handler, shost,
>>    			"scsi_eh_%d", shost->host_no);
>> +	rval = PTR_ERR(shost->ehandler);
>>    	if (IS_ERR(shost->ehandler)) {
>> -		rval = PTR_ERR(shost->ehandler);
>> +		printk(KERN_WARNING "scsi%d: error handler thread failed to spawn\n",
>> rval);
>
> So this isn't really an improvement over the suggestion.  What you
> wanted was shost->host_no, as you deduced, but the %d becomes %ld to
> quiet the type warning.
>
> James

ahh.. was reading the manual but couldn't make sense of the % then 
letter to use.

let me add these in, then if good I'll send it out for review etc..

Justin P. Mattock

Re: [PATCH 5/5]scsi:hosts.c Fix warning: variable 'rval' set but not used

[Justin P. Mattock]

Thanks for the help, and info on this..I just tested and sent out the 
patch let me know if it's legit or not..

Justin P. Mattock

Re: [PATCH 0/5] Fix set but unused variable warnings

[Julian Calaby]

On Wed, Jun 16, 2010 at 15:33, Justin P. Mattock
<justinmattock@gmail.com> wrote:
> Here is another set of patches fixing some warning messages generated
> when building the kernel when using gcc 4.6.0.
>
> This set just focuses in on the variables warnings that are reported
> to be unused. Keep in mind there are still lots more warning messages
> that I'm seeing, and will try my hardest to see if I can come up with
> a fix, but if things become too difficult and so forth then a bug should
> be filled etc..

Given that patches 3, 4 and 5 seem to be a cases of missing error
handling, (3 and 4 in particular seem to be breaking things rather
than fixing them) in my humble opinion, I think this set needs some
work and discussion.

Justin, maybe you'd be better off posting the actual error messages
(split up by subsystem) and letting the lists discuss them, rather
than posting patches which are obviously wrong. (like the ones I've
pointed out)

I'm sure that everyone here is as committed as you are to eliminating
compile warnings and errors and, in my opinion, more good will come
from a healthy discussion of the warnings than maintainers NAKing your
patches out of hand.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH 0/5] Fix set but unused variable warnings

[Justin P. Mattock]

On 06/15/2010 10:52 PM, Julian Calaby wrote:
> On Wed, Jun 16, 2010 at 15:33, Justin P. Mattock
> <justinmattock@gmail.com>  wrote:
>> Here is another set of patches fixing some warning messages generated
>> when building the kernel when using gcc 4.6.0.
>>
>> This set just focuses in on the variables warnings that are reported
>> to be unused. Keep in mind there are still lots more warning messages
>> that I'm seeing, and will try my hardest to see if I can come up with
>> a fix, but if things become too difficult and so forth then a bug should
>> be filled etc..
>
> Given that patches 3, 4 and 5 seem to be a cases of missing error
> handling, (3 and 4 in particular seem to be breaking things rather
> than fixing them) in my humble opinion, I think this set needs some
> work and discussion.
>

alright..

> Justin, maybe you'd be better off posting the actual error messages
> (split up by subsystem) and letting the lists discuss them, rather
> than posting patches which are obviously wrong. (like the ones I've
> pointed out)
>

yeah I'm a newbie!! alright so just file bugs for all these then.

> I'm sure that everyone here is as committed as you are to eliminating
> compile warnings and errors and, in my opinion, more good will come
> from a healthy discussion of the warnings than maintainers NAKing your
> patches out of hand.
>
> Thanks,
>


NAKing is o.k. but having discussions is better..

Justin P. Mattock

Re: [PATCH 0/5] Fix set but unused variable warnings

[Matthew Wilcox]

On Wed, Jun 16, 2010 at 03:52:58PM +1000, Julian Calaby wrote:
> Given that patches 3, 4 and 5 seem to be a cases of missing error
> handling, (3 and 4 in particular seem to be breaking things rather
> than fixing them) in my humble opinion, I think this set needs some
> work and discussion.
> 
> Justin, maybe you'd be better off posting the actual error messages
> (split up by subsystem) and letting the lists discuss them, rather
> than posting patches which are obviously wrong. (like the ones I've
> pointed out)

My first impression of patch 5 was "that can't be right", but upon review,
I concluded that was the best solution.  If we return the PTR_ERR, this
is going to confuse every user of scsi_host_alloc who are currently only
checking for NULL.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 0/5] Fix set but unused variable warnings

[Julian Calaby]

On Wed, Jun 16, 2010 at 21:09, Matthew Wilcox <matthew@wil.cx> wrote:
> On Wed, Jun 16, 2010 at 03:52:58PM +1000, Julian Calaby wrote:
>> Given that patches 3, 4 and 5 seem to be a cases of missing error
>> handling, (3 and 4 in particular seem to be breaking things rather
>> than fixing them) in my humble opinion, I think this set needs some
>> work and discussion.
>>
>> Justin, maybe you'd be better off posting the actual error messages
>> (split up by subsystem) and letting the lists discuss them, rather
>> than posting patches which are obviously wrong. (like the ones I've
>> pointed out)
>
> My first impression of patch 5 was "that can't be right", but upon review,
> I concluded that was the best solution.  If we return the PTR_ERR, this
> is going to confuse every user of scsi_host_alloc who are currently only
> checking for NULL.

I did a double take at patch 4 when skimming over them, then took a
proper look at the lot of them. Whilst I agree that 5 isn't doing
anything particularly bad, it does seem wrong, especially in the
context of 3 and 4.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/