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
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
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
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
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
Linus Torvalds <[email protected]> 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).
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
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
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.
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
[email protected]
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
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
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
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 <[email protected]>, the -fno-omit-frame-pointer is
# needed for x86 only. Why this used to be enabled for all architectures is beyond
Jeff Garzik <[email protected]> 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.
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.
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
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
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 <[email protected]>
> 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
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
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
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
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
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
[email protected]
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
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
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.
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
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 <[email protected]>
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