On Mon, 23 Jul 2007 16:51:05 +0200
Adrian Bunk <[email protected]> wrote:
> If !mem_node we did already return -ENOMEM above in the function.
>
> Spotted by the Coverity checker.
>
> Signed-off-by: Adrian Bunk <[email protected]>
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;
>
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 <[email protected]> wrote:
>
> > If !mem_node we did already return -ENOMEM above in the function.
> >
> > Spotted by the Coverity checker.
> >
> > Signed-off-by: Adrian Bunk <[email protected]>
>
> 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
On Thu, 9 Aug 2007 15:24:27 -0700
Greg KH <[email protected]> wrote:
[email protected]
> 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 <[email protected]> wrote:
> >
> > > If !mem_node we did already return -ENOMEM above in the function.
> > >
> > > Spotted by the Coverity checker.
> > >
> > > Signed-off-by: Adrian Bunk <[email protected]>
> >
> > 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.
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
> 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
On Fri, 10 Aug 2007 01:04:36 +0200
Adrian Bunk <[email protected]> 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.
On Fri, 10 Aug 2007 01:04:36 +0200
Adrian Bunk <[email protected]> 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).
> 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
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 <[email protected]> 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
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.
On Fri, 10 Aug 2007 01:39:10 +0200
Adrian Bunk <[email protected]> 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
On Sat, 11 Aug 2007 04:42:07 +0100
Christoph Hellwig <[email protected]> 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.