[GIT PATCH] More PCI patches for 2.6.13

[GIT PATCH] More PCI patches for 2.6.13

[Greg KH]

Here are some more PCI patches against your latest git tree.  Most of
them were just not applied to the last pci git pull, due to me messing
up the mbox that I applied to the tree.  They have been in the -mm tree
for a while.

The other two patches are a moving around of the pci probe functions so
that ppc has an easier time of future work, and I've sent in a pci quirk
that has been in the -mm tree for a while.

Please pull from:
	rsync://rsync.kernel.org/pub/scm/linux/kernel/git/gregkh/pci-2.6.git/
or if master.kernel.org hasn't synced up yet:
	master.kernel.org:/pub/scm/linux/kernel/git/gregkh/pci-2.6.git/

The full patches will be sent to the linux-pci mailing lists, if anyone
wants to see them.

thanks,

greg k-h

 drivers/pci/hotplug.c               |   53 ++++++++++++++----------------------
 drivers/pci/hotplug/pciehprm_acpi.c |    8 ++---
 drivers/pci/pci.h                   |    1 
 drivers/pci/probe.c                 |   50 ++++++++++++++++++++++-----------
 drivers/pci/quirks.c                |   12 ++++++++
 include/linux/pci.h                 |   23 ++++++++-------
 6 files changed, 83 insertions(+), 64 deletions(-)


Dave Jones:
  must_check attributes for PCI layer.

Greg Kroah-Hartman:
  PCI: move pci core to use add_hotplug_env_var()

Paul Mackerras:
  PCI: Small rearrangement of PCI probing code

Rajesh Shah:
  PCI: Fix PCI bus mastering enable problem in pciehp

Rumen Ivanov Zarev:
  PCI: Unhide SMBus on Compaq Evo N620c

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Fri, 9 Sep 2005, Greg KH wrote:
> 
> Dave Jones:
>   must_check attributes for PCI layer.

Why?

This only clutters up the compile, hiding real errors.

I think all those compile warnings are totally bogus. Who really cares? 
Are they going to be fixed, or were they added just to irritate people?

We should have a strict rule: anybody who adds things like "must_check"
and "deprecated" had better also be ready and willing to fix all the new
warnings they cause - you're not allowed to just assume that "somebody
else will fix it".

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Greg KH]

On Fri, Sep 09, 2005 at 03:37:16PM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 9 Sep 2005, Greg KH wrote:
> > 
> > Dave Jones:
> >   must_check attributes for PCI layer.
> 
> Why?

This is something that David and Arjan wanted in.  Guys?

> This only clutters up the compile, hiding real errors.
> 
> I think all those compile warnings are totally bogus. Who really cares? 
> Are they going to be fixed, or were they added just to irritate people?

I fixed up all of the PCI core and USB drivers that were flagged by
these warnings already.  Biggest area left is network drivers that I
saw.

> We should have a strict rule: anybody who adds things like "must_check"
> and "deprecated" had better also be ready and willing to fix all the new
> warnings they cause - you're not allowed to just assume that "somebody
> else will fix it".

Fair enough.  Dave and Arjan, want to fix up the rest of the tree?

thanks,

greg k-h

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Fri, 9 Sep 2005, Greg KH wrote:
> 
> I fixed up all of the PCI core and USB drivers that were flagged by
> these warnings already.  Biggest area left is network drivers that I
> saw.

The reason I really dislike patches like these is that it causes people to 
do questionable things.

For example, there may be perfectly valid reasons why somebody doesn't
care about the result. I don't see much point in forcing people to check
the return value of "pci_enable_wake()" for example. There's really no
real reason to ever care, as far as I can tell - if it fails, there's 
nothing you can really do about it anyway.

Also, in general, the fact is that things like "pci_set_power_state()" 
might fail in _theory_, but we just don't care. A driver that doesn't 
check the return value is in practice as good a driver as one that does, 
and forcing people to add code that is totally useless in reality - or 
look at a warning that is irritating - is just not very productive.

There are functions where it is really _important_ to check the error 
return, because they return errors often enough - and the error case is 
something you have to do something about - that it's good to force people 
to be aware.

But "pci_set_power_state()"?

I don't think so.

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Fri, 9 Sep 2005, Linus Torvalds wrote:
> 
> There are functions where it is really _important_ to check the error 
> return, because they return errors often enough - and the error case is 
> something you have to do something about - that it's good to force people 
> to be aware.
> 
> But "pci_set_power_state()"?
> 
> I don't think so.

Btw, a perfect example of this is

	pci_set_power_state(pdev, 0);

which is a very common thing to do in a driver init routine. And it has
absolutely _no_ valid return values: it either succeeds, or it doesn't,
and the only reason it wouldn't succeed is because the device doesn't
support power management in the first place (in which case it already
effectively is in state 0).

In other words, there's nothing you can or should do about it. Testing the 
return value is pointless. And thus adding a "must_check" is really really 
wrong: it might make people do

	if (pci_set_power_state(pdev, 0))
		return -ENODEV

which is actually actively the _wrong_ thing to do, and would just cause 
old revisions of the chip that might not support PM capabilities to no 
longer work.

The problem with warnings is that people may take them too seriously, and 
generate bugs when trying to "fix" them.

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Fri, 9 Sep 2005, Greg KH wrote:
> > 
> > I fixed up all of the PCI core and USB drivers that were flagged by
> > these warnings already.  Biggest area left is network drivers that I
> > saw.
> 
> The reason I really dislike patches like these is that it causes people to 
> do questionable things.
> 
> For example, there may be perfectly valid reasons why somebody doesn't
> care about the result. I don't see much point in forcing people to check
> the return value of "pci_enable_wake()" for example. There's really no
> real reason to ever care, as far as I can tell - if it fails, there's 
> nothing you can really do about it anyway.
> 
> Also, in general, the fact is that things like "pci_set_power_state()" 
> might fail in _theory_, but we just don't care. A driver that doesn't 
> check the return value is in practice as good a driver as one that does, 
> and forcing people to add code that is totally useless in reality - or 
> look at a warning that is irritating - is just not very productive.
> 
> There are functions where it is really _important_ to check the error 
> return, because they return errors often enough - and the error case is 
> something you have to do something about - that it's good to force people 
> to be aware.
> 
> But "pci_set_power_state()"?
> 
> I don't think so.
> 

If something like a PCI power management function fails then it will likely
cause suspend or resume to malfunction, and we have a lot of such problems.

So we-the-developers do need to hear about it when such functions fail.  So
either a) each and every driver has to blurt a printk (dumb) or b) we stick
a warning and a backtrace in the failing function (better).

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Fri, 9 Sep 2005, Andrew Morton wrote:
> 
> If something like a PCI power management function fails then it will likely
> cause suspend or resume to malfunction, and we have a lot of such problems.

No, for several reasons.

First off, some of those functions can't fail in normal usage. Thus 
telling people that they have to check the return code is insane.

Secondly, at least some of the suspend failures have historically been
because drivers returned errors for no good reason. Adding yet another 
broken reason to return error is not going to help.

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Fri, 9 Sep 2005, Andrew Morton wrote:
> 
>>If something like a PCI power management function fails then it will likely
>>cause suspend or resume to malfunction, and we have a lot of such problems.
> 
> 
> No, for several reasons.
> 
> First off, some of those functions can't fail in normal usage. Thus 
> telling people that they have to check the return code is insane.
> 
> Secondly, at least some of the suspend failures have historically been
> because drivers returned errors for no good reason. Adding yet another 
> broken reason to return error is not going to help.

Agreed.

	Jeff

Re: [GIT PATCH] More PCI patches for 2.6.13

[Arjan van de Ven]

On Fri, 2005-09-09 at 15:54 -0700, Greg KH wrote:
> On Fri, Sep 09, 2005 at 03:37:16PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Fri, 9 Sep 2005, Greg KH wrote:
> > > 
> > > Dave Jones:
> > >   must_check attributes for PCI layer.
> > 
> > Why?
> 
> This is something that David and Arjan wanted in.  Guys?

The one I cared about is pci_enable_device(); the others I care about a
lot less to not at all. 

Re: [GIT PATCH] More PCI patches for 2.6.13

[John W. Linville]

On Fri, Sep 09, 2005 at 04:22:25PM -0700, Linus Torvalds wrote:

> the return value of "pci_enable_wake()" for example. There's really no
> real reason to ever care, as far as I can tell - if it fails, there's 
> nothing you can really do about it anyway.
> 
> Also, in general, the fact is that things like "pci_set_power_state()" 
> might fail in _theory_, but we just don't care. A driver that doesn't 

But, aren't these arguments for changing the functions to return void?
If there is never any point in checking the results, then why have
results at all?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Sat, 10 Sep 2005, John W. Linville wrote:
> 
> But, aren't these arguments for changing the functions to return void?
> If there is never any point in checking the results, then why have
> results at all?

Trying to set other power states than D0 might return interesting values. 
Also, you _can_ use the value to determine whether the device supports PM 
states at all.

There are tons of functions that return values. That doesn't mean that you 
always have to check them.

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Alan Stern]

On Fri, 9 Sep 2005, Linus Torvalds wrote:
> 
> On Fri, 9 Sep 2005, Linus Torvalds wrote:
> > 
> > There are functions where it is really _important_ to check the error 
> > return, because they return errors often enough - and the error case is 
> > something you have to do something about - that it's good to force people 
> > to be aware.
> > 
> > But "pci_set_power_state()"?
> > 
> > I don't think so.
> 
> Btw, a perfect example of this is
> 
> 	pci_set_power_state(pdev, 0);
> 
> which is a very common thing to do in a driver init routine. And it has
> absolutely _no_ valid return values: it either succeeds, or it doesn't,
> and the only reason it wouldn't succeed is because the device doesn't
> support power management in the first place (in which case it already
> effectively is in state 0).
> 
> In other words, there's nothing you can or should do about it. Testing the 
> return value is pointless. And thus adding a "must_check" is really really 
> wrong: it might make people do
> 
> 	if (pci_set_power_state(pdev, 0))
> 		return -ENODEV
> 
> which is actually actively the _wrong_ thing to do, and would just cause 
> old revisions of the chip that might not support PM capabilities to no 
> longer work.

Funny you should say this -- exactly that problem _did_ arise.  See

http://marc.theaimsgroup.com/?l=linux-pci&m=112621842604724&w=2

pci_enable_device_bars() would an error when trying to initialize 
devices without PM support, because it started checking the return value
from pci_set_power_state().

Alan Stern

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Sat, 10 Sep 2005, Alan Stern wrote:

> On Fri, 9 Sep 2005, Linus Torvalds wrote:
> > 
> > In other words, there's nothing you can or should do about it. Testing the 
> > return value is pointless. And thus adding a "must_check" is really really 
> > wrong: it might make people do
> > 
> > 	if (pci_set_power_state(pdev, 0))
> > 		return -ENODEV
> > 
> > which is actually actively the _wrong_ thing to do, and would just cause 
> > old revisions of the chip that might not support PM capabilities to no 
> > longer work.
> 
> Funny you should say this -- exactly that problem _did_ arise.  See
> 
> http://marc.theaimsgroup.com/?l=linux-pci&m=112621842604724&w=2
> 
> pci_enable_device_bars() would an error when trying to initialize 
> devices without PM support, because it started checking the return value
> from pci_set_power_state().

Case closed. 

Bogus warnings are a _bad_ thing. They cause people to write buggy code.

That drivers/pci/pci.c code should be simplified to not look at the error
return from pci_set_power_state() at all. Special-casing EIO is just
another bug waiting to happen.

			Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Jeff Garzik]

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

Linus Torvalds wrote:
> Case closed. 
> 
> Bogus warnings are a _bad_ thing. They cause people to write buggy code.
> 
> That drivers/pci/pci.c code should be simplified to not look at the error
> return from pci_set_power_state() at all. Special-casing EIO is just
> another bug waiting to happen.

As a tangent, the 'foo is deprecated' warnings for pm_register() and 
inter_module_register() annoy me, primarily because they never seem to 
go away.

The only user of inter_module_xxx is CONFIG_MTD -- thus the deprecated 
warning is useless to 90% of us, who will never use MTD.  As for 
pm_register(), there are tons of users remaining.  As such, for the 
forseeable future, we will continue to see pm_register() warnings and 
ignore them -- thus they are nothing but useless build noise.

I've attached a patch, just tested, which addresses inter_module_xxx by 
making its build conditional on the last remaining user.  This solves 
the deprecated warning problem for most of us, and makes the kernel 
smaller for most of us, at the same time.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 985 bytes --]

diff --git a/kernel/Makefile b/kernel/Makefile
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -6,7 +6,7 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    rcupdate.o extable.o params.o posix-timers.o \
 	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o
 
 obj-$(CONFIG_FUTEX) += futex.o
@@ -32,6 +32,13 @@ obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
 obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
 obj-$(CONFIG_SECCOMP) += seccomp.o
 
+ifeq ($(CONFIG_MTD),y)
+obj-y += intermodule.o
+endif
+ifeq ($(CONFIG_MTD),m)
+obj-y += intermodule.o
+endif
+
 ifneq ($(CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
 # needed for x86 only.  Why this used to be enabled for all architectures is beyond

Re: [GIT PATCH] More PCI patches for 2.6.13

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> Linus Torvalds wrote:
> > Case closed. 
> > 
> > Bogus warnings are a _bad_ thing. They cause people to write buggy code.
> > 
> > That drivers/pci/pci.c code should be simplified to not look at the error
> > return from pci_set_power_state() at all. Special-casing EIO is just
> > another bug waiting to happen.
> 
> As a tangent, the 'foo is deprecated' warnings for pm_register() and 
> inter_module_register() annoy me, primarily because they never seem to 
> go away.

Warnings are irritating.  Several times I've received obviously buggy
patches which generated warnings which pointed out the bug quite clearly. 
Presumably the originator simply failed to notice the warnings amongst all
the other noise.

Who do I bug about these longstanding ppc64 warnings, btw ;)

drivers/scsi/sata_svw.c: In function `k2_sata_tf_load':
drivers/scsi/sata_svw.c:111: warning: passing arg 2 of `eeh_writeb' makes pointer from integer without a cast
drivers/scsi/sata_svw.c:116: warning: passing arg 2 of `eeh_writew' makes pointer from integer without a cast
drivers/scsi/sata_svw.c:117: warning: passing arg 2 of `eeh_writew' makes pointer from integer without a cast

And these:

drivers/pci/probe.c: In function `pci_read_bases':
drivers/pci/probe.c:168: warning: large integer implicitly truncated to unsigned type 
drivers/pci/probe.c:218: warning: large integer implicitly truncated to unsigned type

And these:

drivers/char/drm/drm_bufs.c: In function `drm_addmap_ioctl':
drivers/char/drm/drm_bufs.c:309: warning: cast to pointer from integer of different size
drivers/char/drm/drm_bufs.c:309: warning: cast to pointer from integer of different size
drivers/char/drm/drm_bufs.c:309: warning: cast to pointer from integer of different size

> The only user of inter_module_xxx is CONFIG_MTD -- thus the deprecated 
> warning is useless to 90% of us, who will never use MTD.

Suggest we either undeprecate it, or remove the darn functions.

And in future, only deprecate functions after all callers have been
removed.

>  As for pm_register(), there are tons of users remaining.

Well it would kinda help if people knew what to _do_ about pm_register(). 
Documentation/pm.txt cheerfully tells everyone how to use it in new code
and the comment over the pm_register() implementation doesn't say that it's
deprecated and doesn't tell people what to replace it with.

Re: [GIT PATCH] More PCI patches for 2.6.13

[Alistair John Strachan]

On Saturday 10 September 2005 22:58, Jeff Garzik wrote:
> Linus Torvalds wrote:
> > Case closed.
> >
> > Bogus warnings are a _bad_ thing. They cause people to write buggy code.
> >
> > That drivers/pci/pci.c code should be simplified to not look at the error
> > return from pci_set_power_state() at all. Special-casing EIO is just
> > another bug waiting to happen.
>
> As a tangent, the 'foo is deprecated' warnings for pm_register() and
> inter_module_register() annoy me, primarily because they never seem to
> go away.
>
> The only user of inter_module_xxx is CONFIG_MTD -- thus the deprecated
> warning is useless to 90% of us, who will never use MTD.  As for
> pm_register(), there are tons of users remaining.  As such, for the
> forseeable future, we will continue to see pm_register() warnings and
> ignore them -- thus they are nothing but useless build noise.
>
> I've attached a patch, just tested, which addresses inter_module_xxx by
> making its build conditional on the last remaining user.  This solves
> the deprecated warning problem for most of us, and makes the kernel
> smaller for most of us, at the same time.

Though external modules using these functions will be hung out to dry. But 
only if you don't select CONFIG_MTD? That's not particularly intuitive.

It's better to mark it deprecated (which has been done), then officially 
remove it (and all in-tree users) once and for all. Compiling the code 
conditionally just to avoid a few warnings is a silly idea.

-- 
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Sat, 10 Sep 2005, Andrew Morton wrote:
> 
> Who do I bug about these longstanding ppc64 warnings, btw ;)
> 
> drivers/scsi/sata_svw.c: In function `k2_sata_tf_load':
> drivers/scsi/sata_svw.c:111: warning: passing arg 2 of `eeh_writeb' makes pointer from integer without a cast
> drivers/scsi/sata_svw.c:116: warning: passing arg 2 of `eeh_writew' makes pointer from integer without a cast
> drivers/scsi/sata_svw.c:117: warning: passing arg 2 of `eeh_writew' makes pointer from integer without a cast

I used to have a patch to fix them, and sent it to Jeff ages ago. At that 
point, he didn't want to use the iomap() functionality, but maybe that has 
changed.

Jeff? It requires making almost all the SATA IO base pointers be iomapped: 
the current

	struct ata_ioports {
	        unsigned long           cmd_addr;
	        unsigned long           data_addr;
		...

would have to become

	struct ata_ioports {
		void __iomem *cmd_addr;
		void __iomem *data_addr;
		...

instead - and initialized with the proper ioport_map() or pci_iomap() as 
appropriate.

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Pavel Machek]

Hi!

> >  As for pm_register(), there are tons of users remaining.
> 
> Well it would kinda help if people knew what to _do_ about pm_register(). 
> Documentation/pm.txt cheerfully tells everyone how to use it in new code
> and the comment over the pm_register() implementation doesn't say that it's
> deprecated and doesn't tell people what to replace it with.

Well, we want people to properly register their devices into sysfs,
and use sysfs suspend()/resume() callbacks.

They do not map too closely to pm_register, however.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [GIT PATCH] More PCI patches for 2.6.13

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Sat, 10 Sep 2005, Andrew Morton wrote:
> 
>>Who do I bug about these longstanding ppc64 warnings, btw ;)
>>
>>drivers/scsi/sata_svw.c: In function `k2_sata_tf_load':
>>drivers/scsi/sata_svw.c:111: warning: passing arg 2 of `eeh_writeb' makes pointer from integer without a cast
>>drivers/scsi/sata_svw.c:116: warning: passing arg 2 of `eeh_writew' makes pointer from integer without a cast
>>drivers/scsi/sata_svw.c:117: warning: passing arg 2 of `eeh_writew' makes pointer from integer without a cast

> I used to have a patch to fix them, and sent it to Jeff ages ago. At that 
> point, he didn't want to use the iomap() functionality, but maybe that has 
> changed.
> 
> Jeff? It requires making almost all the SATA IO base pointers be iomapped: 
> the current

Glad you two asked!  :)

I -do- want to use iomap.  The problem is that no one has yet come up 
with a few that does all the proper resource reservation.  Everybody 
(including myself) did the ioread/iowrite part, but gave up before 
handling all cases of (a) legacy ISA iomap, (b) native PCI IDE iomap, 
and (c) non-standard MMIO iomap.

This "works, but leaks resources" code exists in the 'iomap' branch of
rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git

It languished in this incomplete stage for a while, but very recently 
got moving again:

> commit 374b1873571bf80dc0c1fcceaaad067980f3b9de
> Author: Jeff Garzik <jgarzik@pobox.com>
> Date:   Tue Aug 30 05:42:52 2005 -0400
> 
>     [libata] update several drivers to use pci_iomap()/pci_iounmap()

I actually made a promise to use iomap, since libata will benefit quite 
a bit from ioread/iowrite usage.  I intend to keep that promise... it's 
just taking me a while to get us there.  Now that I figured out the path 
I want to take, we should get there around 2.6.1[56].

	Jeff

Re: [GIT PATCH] More PCI patches for 2.6.13

[Jeff Garzik]

Alistair John Strachan wrote:
> On Saturday 10 September 2005 22:58, Jeff Garzik wrote:
> 
>>Linus Torvalds wrote:
>>
>>>Case closed.
>>>
>>>Bogus warnings are a _bad_ thing. They cause people to write buggy code.
>>>
>>>That drivers/pci/pci.c code should be simplified to not look at the error
>>>return from pci_set_power_state() at all. Special-casing EIO is just
>>>another bug waiting to happen.
>>
>>As a tangent, the 'foo is deprecated' warnings for pm_register() and
>>inter_module_register() annoy me, primarily because they never seem to
>>go away.
>>
>>The only user of inter_module_xxx is CONFIG_MTD -- thus the deprecated
>>warning is useless to 90% of us, who will never use MTD.  As for
>>pm_register(), there are tons of users remaining.  As such, for the
>>forseeable future, we will continue to see pm_register() warnings and
>>ignore them -- thus they are nothing but useless build noise.
>>
>>I've attached a patch, just tested, which addresses inter_module_xxx by
>>making its build conditional on the last remaining user.  This solves
>>the deprecated warning problem for most of us, and makes the kernel
>>smaller for most of us, at the same time.
> 
> 
> Though external modules using these functions will be hung out to dry. But 
> only if you don't select CONFIG_MTD? That's not particularly intuitive.
> 
> It's better to mark it deprecated (which has been done), then officially 
> remove it (and all in-tree users) once and for all. Compiling the code 
> conditionally just to avoid a few warnings is a silly idea.


Presumably David Woodhouse (MTD maintainer) would yell if we just ripped 
out the in-tree users.

It is even more silly to continue compiling code that is dead for almost 
everybody.

	Jeff

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Sat, 10 Sep 2005, Jeff Garzik wrote:
> 
> I -do- want to use iomap.  The problem is that no one has yet come up 
> with a few that does all the proper resource reservation.  Everybody 
> (including myself) did the ioread/iowrite part, but gave up before 
> handling all cases of (a) legacy ISA iomap, (b) native PCI IDE iomap, 
> and (c) non-standard MMIO iomap.

It should all be trivial. The only ugly issue in the patch I just sent out 
is that it needs to save the "legacy_mode" bits that were calculated at 
initialization time somewhere in the ap structure. Then the 
release_regions should match the request_regions.

That's a cleanup, the current code is literally buggy. It may end up
releasing IO address 0x1f0 twice, if somebody wasn't marked legacy, but
actually had 0x1f0 in the PCI resource pointers (maybe that doesn't ever
happen, but still.. Relying on the legacy-value of the IO port instead of
relying on whether you did a legacy request_region() is definitely at
least conceptually wrong).

			Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Sat, 10 Sep 2005, Jeff Garzik wrote:
> 
>>I -do- want to use iomap.  The problem is that no one has yet come up 
>>with a few that does all the proper resource reservation.  Everybody 
>>(including myself) did the ioread/iowrite part, but gave up before 
>>handling all cases of (a) legacy ISA iomap, (b) native PCI IDE iomap, 
>>and (c) non-standard MMIO iomap.
> 
> 
> It should all be trivial. The only ugly issue in the patch I just sent out 
> is that it needs to save the "legacy_mode" bits that were calculated at 
> initialization time somewhere in the ap structure. Then the 
> release_regions should match the request_regions.

More ugly issues abound, see below :)


> That's a cleanup, the current code is literally buggy. It may end up
> releasing IO address 0x1f0 twice, if somebody wasn't marked legacy, but
> actually had 0x1f0 in the PCI resource pointers (maybe that doesn't ever

Haven't run into anything yet that trips up the legacy/native detection 
in libata except for mixed mode (1 port legacy, 1 port native).  But 
those bugs aren't in the area of code we're discussing.


> happen, but still.. Relying on the legacy-value of the IO port instead of
> relying on whether you did a legacy request_region() is definitely at
> least conceptually wrong).

Its not that simple.  grep for ____request_region in both libata and the 
PCI quirks code.  libata grabs the SATA port on ICH boxes in combined 
mode... but has to do so before built-in IDE driver grabs them.

	Jeff

Re: [GIT PATCH] More PCI patches for 2.6.13

[Linus Torvalds]

On Sat, 10 Sep 2005, Jeff Garzik wrote:
> 
> > happen, but still.. Relying on the legacy-value of the IO port instead of
> > relying on whether you did a legacy request_region() is definitely at
> > least conceptually wrong).
> 
> Its not that simple.  grep for ____request_region in both libata and the 
> PCI quirks code.  libata grabs the SATA port on ICH boxes in combined 
> mode... but has to do so before built-in IDE driver grabs them.

That's not what I'm talking about.

The _request_ side is fine, and yes, it needs to be done early.

It's the module unload time that is broken - it doesn't remember whether
it requested the legacy mode addresses, so instead it uses the address
_values_ to determine if it did so or not, and that's broken: it is
conceivable at least in theory that a PCI BAR would contain the legacy
mode address value, without the legacy mode bit being set. In that case we 
have _not_ done the legacy-mode "request_region()", but we _will_ do the 
"release_region()".

Exactly because the code checks the wrong thing. That's also the thing 
that makes for problems for iomap. What used to be the wrong thing to test 
now becomes _impossible_ to test.

If the code had just saved the value of "legacy_mode" from the probing 
phase, the release phase wouldn't have any ambiguous cases, and the iomap 
code wouldn't have any issues either..

		Linus

Re: [GIT PATCH] More PCI patches for 2.6.13

[Gabriel A. Devenyi]

Hi Andrew,

I noticed your mail about this bug in a discussion on PCI patches for 2.6.13, I emailed the list with some info
about this bug, so here it is again, perhaps this can clear up what the problem is, sorry I don't have a patch,
I don't understand the kernel well enough for that.

drivers/pci/probe.c: In function `pci_read_bases':
drivers/pci/probe.c:166: warning: large integer implicitly truncated to unsigned type
drivers/pci/probe.c:216: warning: large integer implicitly truncated to unsigned type

I've tracked this down to pci_size, and two #define's in include/linux/pci.h

#define  PCI_BASE_ADDRESS_MEM_MASK      (~0x0fUL)
#define PCI_ROM_ADDRESS_MASK    (~0x7ffUL)

pci_size expects 3 u32 arguments,but from what I can tell, on 64 bit arch's the two above 
defines expand to 64bit values, and are truncated when being passed.

I'm not sure how to go about fixing this, if pci_size should accept a u64 or if the defines should
be changed. Is this bug dangerous? What should be done to fix it?

-- 
Gabriel A. Devenyi
ace@staticwave.ca

Re: [GIT PATCH] More PCI patches for 2.6.13

[Greg KH]

On Sat, Sep 10, 2005 at 11:32:54PM +0100, Alistair John Strachan wrote:
> On Saturday 10 September 2005 22:58, Jeff Garzik wrote:
> > Linus Torvalds wrote:
> > > Case closed.
> > >
> > > Bogus warnings are a _bad_ thing. They cause people to write buggy code.
> > >
> > > That drivers/pci/pci.c code should be simplified to not look at the error
> > > return from pci_set_power_state() at all. Special-casing EIO is just
> > > another bug waiting to happen.
> >
> > As a tangent, the 'foo is deprecated' warnings for pm_register() and
> > inter_module_register() annoy me, primarily because they never seem to
> > go away.
> >
> > The only user of inter_module_xxx is CONFIG_MTD -- thus the deprecated
> > warning is useless to 90% of us, who will never use MTD.  As for
> > pm_register(), there are tons of users remaining.  As such, for the
> > forseeable future, we will continue to see pm_register() warnings and
> > ignore them -- thus they are nothing but useless build noise.
> >
> > I've attached a patch, just tested, which addresses inter_module_xxx by
> > making its build conditional on the last remaining user.  This solves
> > the deprecated warning problem for most of us, and makes the kernel
> > smaller for most of us, at the same time.
> 
> Though external modules using these functions will be hung out to dry.

External modules are always hung out to dry.  Seriously, you don't have
code using those functions, do you?  If so, it's not like you haven't
been warned...

thanks,

greg k-h

Re: [GIT PATCH] More PCI patches for 2.6.13

[Alistair John Strachan]

On Sunday 11 September 2005 05:41, Greg KH wrote:
> On Sat, Sep 10, 2005 at 11:32:54PM +0100, Alistair John Strachan wrote:
> > On Saturday 10 September 2005 22:58, Jeff Garzik wrote:
> > > Linus Torvalds wrote:
> > > > Case closed.
> > > >
> > > > Bogus warnings are a _bad_ thing. They cause people to write buggy
> > > > code.
> > > >
> > > > That drivers/pci/pci.c code should be simplified to not look at the
> > > > error return from pci_set_power_state() at all. Special-casing EIO is
> > > > just another bug waiting to happen.
> > >
> > > As a tangent, the 'foo is deprecated' warnings for pm_register() and
> > > inter_module_register() annoy me, primarily because they never seem to
> > > go away.
> > >
> > > The only user of inter_module_xxx is CONFIG_MTD -- thus the deprecated
> > > warning is useless to 90% of us, who will never use MTD.  As for
> > > pm_register(), there are tons of users remaining.  As such, for the
> > > forseeable future, we will continue to see pm_register() warnings and
> > > ignore them -- thus they are nothing but useless build noise.
> > >
> > > I've attached a patch, just tested, which addresses inter_module_xxx by
> > > making its build conditional on the last remaining user.  This solves
> > > the deprecated warning problem for most of us, and makes the kernel
> > > smaller for most of us, at the same time.
> >
> > Though external modules using these functions will be hung out to dry.
>
> External modules are always hung out to dry.  Seriously, you don't have
> code using those functions, do you?  If so, it's not like you haven't
> been warned...

I don't care, and no I don't. I was just making the point before somebody 
makes the mistake of doing something that isn't 100% clear cut.

Either you remove it, or you don't. You don't leave it there for some distros 
to enable with CONFIG_MTD and some not.

-- 
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [GIT PATCH] More PCI patches for 2.6.13

[David Woodhouse]

On Sat, 2005-09-10 at 19:47 -0400, Jeff Garzik wrote:
> Presumably David Woodhouse (MTD maintainer) would yell if we just
> ripped out the in-tree users.

Not as much as I yelled when the inter_module_crap was added in the
first place. I'll be happy to see the back of it.

The code which uses it is in need of a redesign anyway, and I'd
previously said that I didn't want a patch which _only_ removes the
inter_module_crap; the warnings there were actually a real indication
that something more was needed. But at the Kernel Summit I was persuaded
otherwise -- I'd accept a minimal patch just to kill the warning.

> It is even more silly to continue compiling code that is dead for
> almost everybody.

Agreed. And even though it's deprecated, people don't _really_ stop
using it till it's dead.

-- 
dwmw2

Re: [GIT PATCH] More PCI patches for 2.6.13

[Pavel Machek]

Hi!

> > But, aren't these arguments for changing the functions to return void?
> > If there is never any point in checking the results, then why have
> > results at all?
> 
> Trying to set other power states than D0 might return interesting values. 
> Also, you _can_ use the value to determine whether the device supports PM 
> states at all.

Perhaps we should make pci_set_power_state(pdev, PCI_D0) to succeed in
case of old device not supporting power managment? As you've said, it
is effectively in PCI_D0 anyway ;-).
								Pavel

-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [GIT PATCH] More PCI patches for 2.6.13

[Pavel Machek]

Hi!

> >  As for pm_register(), there are tons of users remaining.
> 
> Well it would kinda help if people knew what to _do_ about pm_register(). 
> Documentation/pm.txt cheerfully tells everyone how to use it in new code
> and the comment over the pm_register() implementation doesn't say that it's
> deprecated and doesn't tell people what to replace it with.

Well, Doc*/pm.txt seems to say this is "not to use"

Driver Interface -- OBSOLETE, DO NOT USE!
----------------*************************

anyway, if you want a comment "what to do with it", here it is:

---

Tell people not to use pm_register().

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/Documentation/pm.txt b/Documentation/pm.txt
--- a/Documentation/pm.txt
+++ b/Documentation/pm.txt
@@ -38,6 +38,12 @@ system the associated daemon will exit g
 
 Driver Interface -- OBSOLETE, DO NOT USE!
 ----------------*************************
+
+Note: pm_register(), pm_access(), pm_dev_idle() and friends are
+obsolete. Please do not use them. Instead you should properly hook
+your driver into the driver model, and use its suspend()/resume()
+callbacks to do this kind of stuff.
+
 If you are writing a new driver or maintaining an old driver, it
 should include power management support.  Without power management
 support, a single driver may prevent a system with power management


-- 
if you have sharp zaurus hardware you don't need... you know my address