[2.6 patch] cpqphp_ctrl.c: remove dead code

[2.6 patch] cpqphp_ctrl.c: remove dead code

[Adrian Bunk]

If !mem_node we did already return -ENOMEM above in the function.

Spotted by the Coverity checker.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

 drivers/pci/hotplug/cpqphp_ctrl.c |   28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

--- linux-2.6.22-rc6-mm1/drivers/pci/hotplug/cpqphp_ctrl.c.old	2007-07-23 15:13:27.000000000 +0200
+++ linux-2.6.22-rc6-mm1/drivers/pci/hotplug/cpqphp_ctrl.c	2007-07-23 15:15:22.000000000 +0200
@@ -2558,29 +2558,15 @@ static int configure_new_function(struct
 			hold_IO_node = NULL;
 		}
 
-		/* If we have memory resources copy them and fill in the
-		 * bridge's memory range registers.  Otherwise, fill in the
-		 * range registers with values that disable them. */
-		if (mem_node) {
-			memcpy(hold_mem_node, mem_node, sizeof(struct pci_resource));
-			mem_node->next = NULL;
-
-			/* set Mem base and Limit registers */
-			temp_word = mem_node->base >> 16;
-			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_BASE, temp_word);
+		memcpy(hold_mem_node, mem_node, sizeof(struct pci_resource));
+		mem_node->next = NULL;
 
-			temp_word = (mem_node->base + mem_node->length - 1) >> 16;
-			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_LIMIT, temp_word);
-		} else {
-			temp_word = 0xFFFF;
-			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_BASE, temp_word);
-
-			temp_word = 0x0000;
-			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_LIMIT, temp_word);
+		/* set Mem base and Limit registers */
+		temp_word = mem_node->base >> 16;
+		rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_BASE, temp_word);
 
-			kfree(hold_mem_node);
-			hold_mem_node = NULL;
-		}
+		temp_word = (mem_node->base + mem_node->length - 1) >> 16;
+		rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_LIMIT, temp_word);
 
 		memcpy(hold_p_mem_node, p_mem_node, sizeof(struct pci_resource));
 		p_mem_node->next = NULL;

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Kristen Carlson Accardi]

On Mon, 23 Jul 2007 16:51:05 +0200
Adrian Bunk <bunk@stusta.de> wrote:

> If !mem_node we did already return -ENOMEM above in the function.
> 
> Spotted by the Coverity checker.
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>

Greg - you are listed as the maintainer for this driver.  Can you either
point me to someone who can review this patch or review it yourself?  
Looking at the code, it looks like it's possible that the driver writer
wanted this code patch to be able to be taken if it got IO resources
and not MEM resources, and if they didn't there's other cleanups that
should be done for the no iomem case.

Thanks,
Kristen


> 
> ---
> 
>  drivers/pci/hotplug/cpqphp_ctrl.c |   28 +++++++---------------------
>  1 file changed, 7 insertions(+), 21 deletions(-)
> 
> --- linux-2.6.22-rc6-mm1/drivers/pci/hotplug/cpqphp_ctrl.c.old	2007-07-23 15:13:27.000000000 +0200
> +++ linux-2.6.22-rc6-mm1/drivers/pci/hotplug/cpqphp_ctrl.c	2007-07-23 15:15:22.000000000 +0200
> @@ -2558,29 +2558,15 @@ static int configure_new_function(struct
>  			hold_IO_node = NULL;
>  		}
>  
> -		/* If we have memory resources copy them and fill in the
> -		 * bridge's memory range registers.  Otherwise, fill in the
> -		 * range registers with values that disable them. */
> -		if (mem_node) {
> -			memcpy(hold_mem_node, mem_node, sizeof(struct pci_resource));
> -			mem_node->next = NULL;
> -
> -			/* set Mem base and Limit registers */
> -			temp_word = mem_node->base >> 16;
> -			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_BASE, temp_word);
> +		memcpy(hold_mem_node, mem_node, sizeof(struct pci_resource));
> +		mem_node->next = NULL;
>  
> -			temp_word = (mem_node->base + mem_node->length - 1) >> 16;
> -			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_LIMIT, temp_word);
> -		} else {
> -			temp_word = 0xFFFF;
> -			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_BASE, temp_word);
> -
> -			temp_word = 0x0000;
> -			rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_LIMIT, temp_word);
> +		/* set Mem base and Limit registers */
> +		temp_word = mem_node->base >> 16;
> +		rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_BASE, temp_word);
>  
> -			kfree(hold_mem_node);
> -			hold_mem_node = NULL;
> -		}
> +		temp_word = (mem_node->base + mem_node->length - 1) >> 16;
> +		rc = pci_bus_write_config_word(pci_bus, devfn, PCI_MEMORY_LIMIT, temp_word);
>  
>  		memcpy(hold_p_mem_node, p_mem_node, sizeof(struct pci_resource));
>  		p_mem_node->next = NULL;
> 

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Greg KH]

On Thu, Aug 09, 2007 at 02:51:40PM -0700, Kristen Carlson Accardi wrote:
> On Mon, 23 Jul 2007 16:51:05 +0200
> Adrian Bunk <bunk@stusta.de> wrote:
> 
> > If !mem_node we did already return -ENOMEM above in the function.
> > 
> > Spotted by the Coverity checker.
> > 
> > Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> Greg - you are listed as the maintainer for this driver.

Not anymore, look at 2.6.23-rc1 :)

> Can you either
> point me to someone who can review this patch or review it yourself?  
> Looking at the code, it looks like it's possible that the driver writer
> wanted this code patch to be able to be taken if it got IO resources
> and not MEM resources, and if they didn't there's other cleanups that
> should be done for the no iomem case.

Hm, I agree that this looks like the way the code was intended to work,
but as this code has been working just fine so far the way it is, I'm
not inclined to change it much, if any.

Especially as I no longer even have the hardware to test it on :(

So, how about we just leave it alone?

thanks,

greg k-h

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Kristen Carlson Accardi]

On Thu, 9 Aug 2007 15:24:27 -0700
Greg KH <gregkh@suse.de> wrote:
pcihpd-discuss@lists.sourceforge.net
> On Thu, Aug 09, 2007 at 02:51:40PM -0700, Kristen Carlson Accardi wrote:
> > On Mon, 23 Jul 2007 16:51:05 +0200
> > Adrian Bunk <bunk@stusta.de> wrote:
> > 
> > > If !mem_node we did already return -ENOMEM above in the function.
> > > 
> > > Spotted by the Coverity checker.
> > > 
> > > Signed-off-by: Adrian Bunk <bunk@stusta.de>
> > 
> > Greg - you are listed as the maintainer for this driver.
> 
> Not anymore, look at 2.6.23-rc1 :)
> 
> > Can you either
> > point me to someone who can review this patch or review it yourself?  
> > Looking at the code, it looks like it's possible that the driver writer
> > wanted this code patch to be able to be taken if it got IO resources
> > and not MEM resources, and if they didn't there's other cleanups that
> > should be done for the no iomem case.
> 
> Hm, I agree that this looks like the way the code was intended to work,
> but as this code has been working just fine so far the way it is, I'm
> not inclined to change it much, if any.
> 
> Especially as I no longer even have the hardware to test it on :(
> 
> So, how about we just leave it alone?
> 
> thanks,
> 
> greg k-h
> 

fine by me - let's NAK this patch (and all future ones for this driver) until 
someone with hardware steps up to maintain this driver.  Eventually it
will just die I guess.

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Adrian Bunk]

On Thu, Aug 09, 2007 at 03:47:02PM -0700, Kristen Carlson Accardi wrote:
> 
> fine by me - let's NAK this patch (and all future ones for this driver) until 
> someone with hardware steps up to maintain this driver.  Eventually it
> will just die I guess.

We have tons of unmaintained drivers and none of them has such a silly 
auto-NAK policy.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Kristen Carlson Accardi]

On Fri, 10 Aug 2007 01:04:36 +0200
Adrian Bunk <bunk@stusta.de> wrote:

> We have tons of unmaintained drivers and none of them has such a silly 
> auto-NAK policy.
> 
> cu
> Adrian

Silly is in the eye of the beholder.  I don't want to take this patch
because it needs to be reviewed by someone who really knows the intent
of the driver.  Seems silly to me to blindly take patches.

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Kristen Carlson Accardi]

On Fri, 10 Aug 2007 01:04:36 +0200
Adrian Bunk <bunk@stusta.de> wrote:

> On Thu, Aug 09, 2007 at 03:47:02PM -0700, Kristen Carlson Accardi wrote:
> > 
> > fine by me - let's NAK this patch (and all future ones for this driver) until 
> > someone with hardware steps up to maintain this driver.  Eventually it
> > will just die I guess.
> 
> We have tons of unmaintained drivers and none of them has such a silly 
> auto-NAK policy.
> 
> cu
> Adrian

OK - "all future ones" was too extreme.  I'll take trivial patches (of
which this one is not).

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Alan Cox]

> fine by me - let's NAK this patch (and all future ones for this driver) until 
> someone with hardware steps up to maintain this driver.  Eventually it
> will just die I guess.

If you want to NAK it perhaps you should become maintainer ;)

Alan

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Alan Cox]

> Silly is in the eye of the beholder.  I don't want to take this patch
> because it needs to be reviewed by someone who really knows the intent
> of the driver.  Seems silly to me to blindly take patches.

For unmaintained code we usually work on wackipedia theory ("its probably
right but if not we can revert it/update it cheaply")

Alan

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Adrian Bunk]

On Thu, Aug 09, 2007 at 04:20:01PM -0700, Kristen Carlson Accardi wrote:
> On Fri, 10 Aug 2007 01:04:36 +0200
> Adrian Bunk <bunk@stusta.de> wrote:
> 
> > On Thu, Aug 09, 2007 at 03:47:02PM -0700, Kristen Carlson Accardi wrote:
> > > 
> > > fine by me - let's NAK this patch (and all future ones for this driver) until 
> > > someone with hardware steps up to maintain this driver.  Eventually it
> > > will just die I guess.
> > 
> > We have tons of unmaintained drivers and none of them has such a silly 
> > auto-NAK policy.
> > 
> > cu
> > Adrian
> 
> OK - "all future ones" was too extreme.  I'll take trivial patches (of
> which this one is not).

As I've wrote in the patch description, all it does is to remove an if() 
check that could never be false (which is easily verifyable if you look 
at the source code).

I've also verified that my patch does not change a single bit in the 
object file (after compilation with gcc 4.2.1).

What's your definition of a trivial patch?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Christoph Hellwig]

On Thu, Aug 09, 2007 at 03:47:02PM -0700, Kristen Carlson Accardi wrote:
> fine by me - let's NAK this patch (and all future ones for this driver) until 
> someone with hardware steps up to maintain this driver.  Eventually it
> will just die I guess.

Very bad idea.   For example I sent a patch ages ago to remove kernel_thread
useage from the driver.  We need to get that patch in sooner or later because
the kernel_thread export will have to go away.  We're not going to block that
on this driver.

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Kristen Carlson Accardi]

On Fri, 10 Aug 2007 01:39:10 +0200
Adrian Bunk <bunk@kernel.org> wrote:

> As I've wrote in the patch description, all it does is to remove an if() 
> check that could never be false (which is easily verifyable if you look 
> at the source code).
> 
> I've also verified that my patch does not change a single bit in the 
> object file (after compilation with gcc 4.2.1).

while you are correct that your patch will not change the object,
the reason I think that we should not do this patch is because it appears
to me that this code path was meant to be taken, and perhaps it was a 
mistake to return -ENOMEM further up (the reason the code isn't taken).
It seems to me that the thing to do is to leave the code as is, and then when
someone picks up the code again, they can clean it up after they've determined
they truly don't want that code path taken.

My definition of trivial is a patch that is both simple and obviously the 
right thing to do.
Kristen
 

Re: [2.6 patch] cpqphp_ctrl.c: remove dead code

[Kristen Carlson Accardi]

On Sat, 11 Aug 2007 04:42:07 +0100
Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Aug 09, 2007 at 03:47:02PM -0700, Kristen Carlson Accardi wrote:
> > fine by me - let's NAK this patch (and all future ones for this driver) until 
> > someone with hardware steps up to maintain this driver.  Eventually it
> > will just die I guess.
> 
> Very bad idea.   For example I sent a patch ages ago to remove kernel_thread
> useage from the driver.  We need to get that patch in sooner or later because
> the kernel_thread export will have to go away.  We're not going to block that
> on this driver.
> 

So sorry for the delay with your patch, I am planning to take your patch - 
I just keep losing it in my inbox.