Re: [PATCH] Re: NAK new drivers without proper power management?
On Monday, 12 February 2007 00:06, Nigel Cunningham wrote: > Hi. > > On Sun, 2007-02-11 at 19:53 +0100, Rafael J. Wysocki wrote: > > > Having drivers explicitly marked as to whether they are safe is a good > > > kernel > > > feature; what to do if they're not is policy. > > > > That's true, but I assume that the people who opt for doing that are also > > willing to take part in the review of the drivers. :-) > > Absolutely :) > > > Well, I don't think so. Let's estimate the number of drivers that define > > .resume() right now: > > > > $ grep -I -l -r '.resume =' linux-2.6.20/drivers/ | wc > > 102 1024169 > > I think the '.resume =' doesn't help - some have tabs. I ran '\.resume' > and got 351. Ah, good catch. I have searched for ".resume" only and got 612, but this is the number of files, not the number of drivers. And it is not exactly large. ;-) > It would be interesting to see how many struct pci_driver etc instances > lack resume methods. Yes, I'll try to invent a test. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi. On Sun, 2007-02-11 at 19:53 +0100, Rafael J. Wysocki wrote: > > Having drivers explicitly marked as to whether they are safe is a good > > kernel > > feature; what to do if they're not is policy. > > That's true, but I assume that the people who opt for doing that are also > willing to take part in the review of the drivers. :-) Absolutely :) > Well, I don't think so. Let's estimate the number of drivers that define > .resume() right now: > > $ grep -I -l -r '.resume =' linux-2.6.20/drivers/ | wc > 102 1024169 I think the '.resume =' doesn't help - some have tabs. I ran '\.resume' and got 351. It would be interesting to see how many struct pci_driver etc instances lack resume methods. Regards, Nige - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi. On Sun, 2007-02-11 at 12:13 +, Matthew Garrett wrote: > On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: > > > instead of modifying all drivers to explicitly state that they don't support > > it, we should start with a test of the NULL pointer for .suspend which > > should > > mean exactly the same without modifying the drivers. I find it obvious that > > a driver which does provide a suspend function will not support it. And if > > some drivers (eg /dev/null) can support it anyway, it's better to change > > *those* drivers to explicitly mark them as compatible. > > No, that doesn't work. In the absence of suspend/resume methods, the PCI > layer will implement basic PM itself. In some cases, this works. In > others, it doesn't. There's no way to automatically determine which is > which without modifying the drivers. I think we have it backwards there. Power management support for a driver should always start with the driver itself. If there's a generic routine that can be used for the bus, the driver should explicitly set the routine to the generic routine. Regards, Nigel - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi! > > > instead of modifying all drivers to explicitly state that they don't > > > support > > > it, we should start with a test of the NULL pointer for .suspend which > > > should > > > mean exactly the same without modifying the drivers. I find it obvious > > > that > > > a driver which does provide a suspend function will not support it. And if > > > some drivers (eg /dev/null) can support it anyway, it's better to change > > > *those* drivers to explicitly mark them as compatible. > > > > No, that doesn't work. In the absence of suspend/resume methods, the PCI > > layer will implement basic PM itself. In some cases, this works. In > > others, it doesn't. There's no way to automatically determine which is > > which without modifying the drivers. > > Then change the PCI layer to do the basic PM only for known compatible > drivers, and modify only the known-compatible drivers to mark them > explicitly compatible. IMHO, it generally is a bad idea to require that > any driver explicitly states what it *does not* support. It's the reason > why users encounter problem on new features with old drivers. For instance, > do you know if the old ISA NE2000 driver breaks suspend ? I don't know, > but I would at least expect it not to support it by default. It's best > to announce what *is* supported and consider everything unimplemented > otherwise explicitly stated. Actually, ne2k driver is okay. ne2k cards are notoriously buggy, so it responds with ", that damn card has just locked up again, lets reset it". Ok, it takes timeout to realize card is "locked up", so it could be improved... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Rafael J. Wysocki wrote: > - Problem what to do with drivers that work for some people and don't work > for the others (ie. if we don't flag them as known good, we will break the > setups in which they work) And this issue is independent of whether a driver has .suspend and .resume or not. For example, ohci1394 had them for ages but they didn't work as expected before 2.6.20. What's more, ohci1394's resume routine alone is insufficient; an additional facility in ieee1394 was necessary and added for 2.6.21(-rc). This shows that the original authors of ohci1394's .suspend and .resume didn't test with actual external devices or gave up when they figured that the problem reaches into upper subsystem layers. (We still haven't tested interaction with most of the IEEE 1394 high level.) -- Stefan Richter -=-=-=== --=- -=-== http://arcgraph.de/sr/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi! > > Also, I think there are quite some drivers already in the tree that don't > > support suspend/resume explicitly and honestly we should start from adding > > the > > suspend/resume routines to these drivers _before_ we ban new drivers like > > that. > > It'd be relatively quick to modify all the current drivers that don't > explicitly support suspend/resume to explicitly not support it. (Or Well, I think that is more work than you realize, but yes, that patch would really be welcome. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 16:19, Pekka Enberg wrote: > On 2/11/07, Rafael J. Wysocki <[EMAIL PROTECTED]> wrote: > > Unfortunately it has to be done in one shot for all of the known good > > drivers to avoid > > user-observable regressions. > > No you don't. You can make it a config option that defaults to n > during a transition period. Yes, that's possible. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 18:27, Daniel Barkalow wrote: > On Sun, 11 Feb 2007, Rafael J. Wysocki wrote: > > > The problem is it was made implicit long ago. The design is "optimistic", > > so > > to speak, and I think we have the following choices: > > > > 1) Change the design to make the kernel refuse to suspend if there are any > > drivers not explicitly flagged as "suspend/resume-safe". [This looks like a > > lot of work to me, but it is generally doable provided that someone has > > enough > > time to do it. Unfortunately it has to be done in one shot for all of the > > known good drivers to avoid user-observable regressions.] > > The kernel wouldn't necessarily have to refuse to suspend. Well, not from the start, but I think at some point in the fufure it would. > It could just warn (and list the drivers that aren't marked), or could > require some extra insistance from the user. We would have to change the interface for that and I don't want to do it. > It would be good to have it log a message saying something like: "If you can > read this, report that ne2000 seems to be safe for suspend/resume". Sure, it would. > Having drivers explicitly marked as to whether they are safe is a good kernel > feature; what to do if they're not is policy. That's true, but I assume that the people who opt for doing that are also willing to take part in the review of the drivers. :-) > > 2) Require the authors of new drivers to _either_ ensure that their drivers > > will be suspend/resume-safe (and I mean both STR and STD here), _or_ > > explicitly > > flag the drivers as "suspend/resume-unsafe", for example by impelenting > > .suspend() routines returning -ENOSYS. [The existing drivers can be > > modified > > to follow this convention gradually.] > > I don't see any reason not to do (2) regardless of (1). That was (my idea > of) the statement that started this thread: new drivers need to not mess > up on suspend/resume, as a matter of suitability for inclusion. Of course, > we need some way for drivers to indicate that they work fine with the > PCI-layer defaults. And it should probably more machine-readable than the > author telling reviewers that it works. Well, if we change the design to fail by default, the authors of new drivers will only have to flag them if they believe that the drivers are suspend/resume-safe. > > - Problem what to do with drivers that work for some people and don't work > > for the others (ie. if we don't flag them as known good, we will break the > > setups in which they work) > > I think the only interesting case here is when a device resumes fine with > no driver support if the BIOS manages to deal effectively with it, but the > BIOS generally doesn't. Well, I don't think so. Let's estimate the number of drivers that define .resume() right now: $ grep -I -l -r '.resume =' linux-2.6.20/drivers/ | wc 102 1024169 And I think there are much more drivers that really work fine with respect to the suspend/resume. This indicates that in fact many drivers can be marked as known good even though they don't define the .suspend() or .resume() routines. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Matthew Garrett wrote: On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. The only thing that the PCI layer does for PM is the stuff that the driver would normally tell the PCI layer to do as part of a proper suspend/resume implementation: enable/disable the device and save/restore the PCI configuration space (only the standardized part, I believe). This is the bare minimum that's needed on all PCI devices, whether or not they even have a driver loaded. I suspect the number of PCI devices where this is truly all they need, i.e. no state in any IO ports or MMIO registers that need to be reset on resume, is quite low. Maybe in some cases it may appear to work by luck, i.e. the registers happening to be set to the correct values (especially on suspend-to-disk) but this is not a proper implementation. -- Robert Hancock Saskatoon, SK, Canada To email, remove "nospam" from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, 11 Feb 2007, Rafael J. Wysocki wrote: > The problem is it was made implicit long ago. The design is "optimistic", so > to speak, and I think we have the following choices: > > 1) Change the design to make the kernel refuse to suspend if there are any > drivers not explicitly flagged as "suspend/resume-safe". [This looks like a > lot of work to me, but it is generally doable provided that someone has enough > time to do it. Unfortunately it has to be done in one shot for all of the > known good drivers to avoid user-observable regressions.] The kernel wouldn't necessarily have to refuse to suspend. It could just warn (and list the drivers that aren't marked), or could require some extra insistance from the user. It would be good to have it log a message saying something like: "If you can read this, report that ne2000 seems to be safe for suspend/resume". Having drivers explicitly marked as to whether they are safe is a good kernel feature; what to do if they're not is policy. > 2) Require the authors of new drivers to _either_ ensure that their drivers > will be suspend/resume-safe (and I mean both STR and STD here), _or_ > explicitly > flag the drivers as "suspend/resume-unsafe", for example by impelenting > .suspend() routines returning -ENOSYS. [The existing drivers can be modified > to follow this convention gradually.] I don't see any reason not to do (2) regardless of (1). That was (my idea of) the statement that started this thread: new drivers need to not mess up on suspend/resume, as a matter of suitability for inclusion. Of course, we need some way for drivers to indicate that they work fine with the PCI-layer defaults. And it should probably more machine-readable than the author telling reviewers that it works. > - Problem what to do with drivers that work for some people and don't work > for the others (ie. if we don't flag them as known good, we will break the > setups in which they work) I think the only interesting case here is when a device resumes fine with no driver support if the BIOS manages to deal effectively with it, but the BIOS generally doesn't. Otherwise, I think it's only going to work at all if the author put in the effort to make it work (so it should be "known good"), but there may be bugs (firmware, BIOS, driver, etc). But that's true of any functionality. -Daniel *This .sig left intentionally blank* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On 2/11/07, Rafael J. Wysocki <[EMAIL PROTECTED]> wrote: Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions. No you don't. You can make it a config option that defaults to n during a transition period. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 14:57, Willy Tarreau wrote: > On Sun, Feb 11, 2007 at 02:50:48PM +0100, Rafael J. Wysocki wrote: > > On Sunday, 11 February 2007 14:37, Willy Tarreau wrote: > > > On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: > > > > On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: > > > > > > > > > Then change the PCI layer to do the basic PM only for known compatible > > > > > drivers, and modify only the known-compatible drivers to mark them > > > > > explicitly compatible. IMHO, it generally is a bad idea to require > > > > > that > > > > > any driver explicitly states what it *does not* support. It's the > > > > > reason > > > > > why users encounter problem on new features with old drivers. For > > > > > instance, > > > > > do you know if the old ISA NE2000 driver breaks suspend ? I don't > > > > > know, > > > > > but I would at least expect it not to support it by default. It's best > > > > > to announce what *is* supported and consider everything unimplemented > > > > > otherwise explicitly stated. > > > > > > > > This ignores the reality of the situation, which is that many drivers > > > > support suspend and resume despite the lack of any explicit > > > > implementation. Changing things so they're flagged as broken when > > > > they're not would be a regression. > > > > > > Those which are identified as OK should be flagged OK. Only those for > > > which we have no idea should be flagged broken. > > > > I think we don't need to flag the drivers identified as OK. Let's flag only > > the suspicious ones. > > > > Whatever we finally come up with, I'd like to avoid modifying drivers that > > are > > known good. > > I understand your concerns, but the problem is not *current* drivers, but > what will happen to *new* drivers. If we make it implicit that a driver > is compatible, then new drivers will be promoted as good even if nothing > has been done for this. The problem is it was made implicit long ago. The design is "optimistic", so to speak, and I think we have the following choices: 1) Change the design to make the kernel refuse to suspend if there are any drivers not explicitly flagged as "suspend/resume-safe". [This looks like a lot of work to me, but it is generally doable provided that someone has enough time to do it. Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions.] 2) Require the authors of new drivers to _either_ ensure that their drivers will be suspend/resume-safe (and I mean both STR and STD here), _or_ explicitly flag the drivers as "suspend/resume-unsafe", for example by impelenting .suspend() routines returning -ENOSYS. [The existing drivers can be modified to follow this convention gradually.] Now IMO 1) has some disadvantages: - Potential for introducing some regressions (eg. if we omit a known-good driver) - Necessity to review a lot of drivers simultaneously - Problem what to do with drivers that work for some people and don't work for the others (ie. if we don't flag them as known good, we will break the setups in which they work) The advantage of 1), however, is that we can differentiate drivers that are safe only with respect to the suspend to disk from those that are safe with respect to the suspend to RAM too (there are some). I'd prefer 2), because it's simpler to make happen. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 02:50:48PM +0100, Rafael J. Wysocki wrote: > On Sunday, 11 February 2007 14:37, Willy Tarreau wrote: > > On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: > > > On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: > > > > > > > Then change the PCI layer to do the basic PM only for known compatible > > > > drivers, and modify only the known-compatible drivers to mark them > > > > explicitly compatible. IMHO, it generally is a bad idea to require that > > > > any driver explicitly states what it *does not* support. It's the reason > > > > why users encounter problem on new features with old drivers. For > > > > instance, > > > > do you know if the old ISA NE2000 driver breaks suspend ? I don't know, > > > > but I would at least expect it not to support it by default. It's best > > > > to announce what *is* supported and consider everything unimplemented > > > > otherwise explicitly stated. > > > > > > This ignores the reality of the situation, which is that many drivers > > > support suspend and resume despite the lack of any explicit > > > implementation. Changing things so they're flagged as broken when > > > they're not would be a regression. > > > > Those which are identified as OK should be flagged OK. Only those for > > which we have no idea should be flagged broken. > > I think we don't need to flag the drivers identified as OK. Let's flag only > the suspicious ones. > > Whatever we finally come up with, I'd like to avoid modifying drivers that are > known good. I understand your concerns, but the problem is not *current* drivers, but what will happen to *new* drivers. If we make it implicit that a driver is compatible, then new drivers will be promoted as good even if nothing has been done for this. Regards, Willy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 14:37, Willy Tarreau wrote: > On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: > > On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: > > > > > Then change the PCI layer to do the basic PM only for known compatible > > > drivers, and modify only the known-compatible drivers to mark them > > > explicitly compatible. IMHO, it generally is a bad idea to require that > > > any driver explicitly states what it *does not* support. It's the reason > > > why users encounter problem on new features with old drivers. For > > > instance, > > > do you know if the old ISA NE2000 driver breaks suspend ? I don't know, > > > but I would at least expect it not to support it by default. It's best > > > to announce what *is* supported and consider everything unimplemented > > > otherwise explicitly stated. > > > > This ignores the reality of the situation, which is that many drivers > > support suspend and resume despite the lack of any explicit > > implementation. Changing things so they're flagged as broken when > > they're not would be a regression. > > Those which are identified as OK should be flagged OK. Only those for > which we have no idea should be flagged broken. I think we don't need to flag the drivers identified as OK. Let's flag only the suspicious ones. Whatever we finally come up with, I'd like to avoid modifying drivers that are known good. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: > On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: > > > Then change the PCI layer to do the basic PM only for known compatible > > drivers, and modify only the known-compatible drivers to mark them > > explicitly compatible. IMHO, it generally is a bad idea to require that > > any driver explicitly states what it *does not* support. It's the reason > > why users encounter problem on new features with old drivers. For instance, > > do you know if the old ISA NE2000 driver breaks suspend ? I don't know, > > but I would at least expect it not to support it by default. It's best > > to announce what *is* supported and consider everything unimplemented > > otherwise explicitly stated. > > This ignores the reality of the situation, which is that many drivers > support suspend and resume despite the lack of any explicit > implementation. Changing things so they're flagged as broken when > they're not would be a regression. Those which are identified as OK should be flagged OK. Only those for which we have no idea should be flagged broken. It's better than leaving them in the wild waiting for a victim. And given what Nigel would like, they would all have to be reviewed to get .suspend/.resume entries anyway. But at least, we would only have to change the known good instead of all of them. And the remaining ones would not cause trouble to users. Willy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: > Then change the PCI layer to do the basic PM only for known compatible > drivers, and modify only the known-compatible drivers to mark them > explicitly compatible. IMHO, it generally is a bad idea to require that > any driver explicitly states what it *does not* support. It's the reason > why users encounter problem on new features with old drivers. For instance, > do you know if the old ISA NE2000 driver breaks suspend ? I don't know, > but I would at least expect it not to support it by default. It's best > to announce what *is* supported and consider everything unimplemented > otherwise explicitly stated. This ignores the reality of the situation, which is that many drivers support suspend and resume despite the lack of any explicit implementation. Changing things so they're flagged as broken when they're not would be a regression. -- Matthew Garrett | [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 12:13:40PM +, Matthew Garrett wrote: > On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: > > > instead of modifying all drivers to explicitly state that they don't support > > it, we should start with a test of the NULL pointer for .suspend which > > should > > mean exactly the same without modifying the drivers. I find it obvious that > > a driver which does provide a suspend function will not support it. And if > > some drivers (eg /dev/null) can support it anyway, it's better to change > > *those* drivers to explicitly mark them as compatible. > > No, that doesn't work. In the absence of suspend/resume methods, the PCI > layer will implement basic PM itself. In some cases, this works. In > others, it doesn't. There's no way to automatically determine which is > which without modifying the drivers. Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. Regards, Willy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: > instead of modifying all drivers to explicitly state that they don't support > it, we should start with a test of the NULL pointer for .suspend which should > mean exactly the same without modifying the drivers. I find it obvious that > a driver which does provide a suspend function will not support it. And if > some drivers (eg /dev/null) can support it anyway, it's better to change > *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. -- Matthew Garrett | [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. -- Matthew Garrett | [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 12:13:40PM +, Matthew Garrett wrote: On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. Regards, Willy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. This ignores the reality of the situation, which is that many drivers support suspend and resume despite the lack of any explicit implementation. Changing things so they're flagged as broken when they're not would be a regression. -- Matthew Garrett | [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. This ignores the reality of the situation, which is that many drivers support suspend and resume despite the lack of any explicit implementation. Changing things so they're flagged as broken when they're not would be a regression. Those which are identified as OK should be flagged OK. Only those for which we have no idea should be flagged broken. It's better than leaving them in the wild waiting for a victim. And given what Nigel would like, they would all have to be reviewed to get .suspend/.resume entries anyway. But at least, we would only have to change the known good instead of all of them. And the remaining ones would not cause trouble to users. Willy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 14:37, Willy Tarreau wrote: On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. This ignores the reality of the situation, which is that many drivers support suspend and resume despite the lack of any explicit implementation. Changing things so they're flagged as broken when they're not would be a regression. Those which are identified as OK should be flagged OK. Only those for which we have no idea should be flagged broken. I think we don't need to flag the drivers identified as OK. Let's flag only the suspicious ones. Whatever we finally come up with, I'd like to avoid modifying drivers that are known good. Greetings, Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, Feb 11, 2007 at 02:50:48PM +0100, Rafael J. Wysocki wrote: On Sunday, 11 February 2007 14:37, Willy Tarreau wrote: On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. This ignores the reality of the situation, which is that many drivers support suspend and resume despite the lack of any explicit implementation. Changing things so they're flagged as broken when they're not would be a regression. Those which are identified as OK should be flagged OK. Only those for which we have no idea should be flagged broken. I think we don't need to flag the drivers identified as OK. Let's flag only the suspicious ones. Whatever we finally come up with, I'd like to avoid modifying drivers that are known good. I understand your concerns, but the problem is not *current* drivers, but what will happen to *new* drivers. If we make it implicit that a driver is compatible, then new drivers will be promoted as good even if nothing has been done for this. Regards, Willy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 14:57, Willy Tarreau wrote: On Sun, Feb 11, 2007 at 02:50:48PM +0100, Rafael J. Wysocki wrote: On Sunday, 11 February 2007 14:37, Willy Tarreau wrote: On Sun, Feb 11, 2007 at 01:19:57PM +, Matthew Garrett wrote: On Sun, Feb 11, 2007 at 02:09:43PM +0100, Willy Tarreau wrote: Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. This ignores the reality of the situation, which is that many drivers support suspend and resume despite the lack of any explicit implementation. Changing things so they're flagged as broken when they're not would be a regression. Those which are identified as OK should be flagged OK. Only those for which we have no idea should be flagged broken. I think we don't need to flag the drivers identified as OK. Let's flag only the suspicious ones. Whatever we finally come up with, I'd like to avoid modifying drivers that are known good. I understand your concerns, but the problem is not *current* drivers, but what will happen to *new* drivers. If we make it implicit that a driver is compatible, then new drivers will be promoted as good even if nothing has been done for this. The problem is it was made implicit long ago. The design is optimistic, so to speak, and I think we have the following choices: 1) Change the design to make the kernel refuse to suspend if there are any drivers not explicitly flagged as suspend/resume-safe. [This looks like a lot of work to me, but it is generally doable provided that someone has enough time to do it. Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions.] 2) Require the authors of new drivers to _either_ ensure that their drivers will be suspend/resume-safe (and I mean both STR and STD here), _or_ explicitly flag the drivers as suspend/resume-unsafe, for example by impelenting .suspend() routines returning -ENOSYS. [The existing drivers can be modified to follow this convention gradually.] Now IMO 1) has some disadvantages: - Potential for introducing some regressions (eg. if we omit a known-good driver) - Necessity to review a lot of drivers simultaneously - Problem what to do with drivers that work for some people and don't work for the others (ie. if we don't flag them as known good, we will break the setups in which they work) The advantage of 1), however, is that we can differentiate drivers that are safe only with respect to the suspend to disk from those that are safe with respect to the suspend to RAM too (there are some). I'd prefer 2), because it's simpler to make happen. Greetings, Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On 2/11/07, Rafael J. Wysocki [EMAIL PROTECTED] wrote: Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions. No you don't. You can make it a config option that defaults to n during a transition period. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sun, 11 Feb 2007, Rafael J. Wysocki wrote: The problem is it was made implicit long ago. The design is optimistic, so to speak, and I think we have the following choices: 1) Change the design to make the kernel refuse to suspend if there are any drivers not explicitly flagged as suspend/resume-safe. [This looks like a lot of work to me, but it is generally doable provided that someone has enough time to do it. Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions.] The kernel wouldn't necessarily have to refuse to suspend. It could just warn (and list the drivers that aren't marked), or could require some extra insistance from the user. It would be good to have it log a message saying something like: If you can read this, report that ne2000 seems to be safe for suspend/resume. Having drivers explicitly marked as to whether they are safe is a good kernel feature; what to do if they're not is policy. 2) Require the authors of new drivers to _either_ ensure that their drivers will be suspend/resume-safe (and I mean both STR and STD here), _or_ explicitly flag the drivers as suspend/resume-unsafe, for example by impelenting .suspend() routines returning -ENOSYS. [The existing drivers can be modified to follow this convention gradually.] I don't see any reason not to do (2) regardless of (1). That was (my idea of) the statement that started this thread: new drivers need to not mess up on suspend/resume, as a matter of suitability for inclusion. Of course, we need some way for drivers to indicate that they work fine with the PCI-layer defaults. And it should probably more machine-readable than the author telling reviewers that it works. - Problem what to do with drivers that work for some people and don't work for the others (ie. if we don't flag them as known good, we will break the setups in which they work) I think the only interesting case here is when a device resumes fine with no driver support if the BIOS manages to deal effectively with it, but the BIOS generally doesn't. Otherwise, I think it's only going to work at all if the author put in the effort to make it work (so it should be known good), but there may be bugs (firmware, BIOS, driver, etc). But that's true of any functionality. -Daniel *This .sig left intentionally blank* - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Matthew Garrett wrote: On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. The only thing that the PCI layer does for PM is the stuff that the driver would normally tell the PCI layer to do as part of a proper suspend/resume implementation: enable/disable the device and save/restore the PCI configuration space (only the standardized part, I believe). This is the bare minimum that's needed on all PCI devices, whether or not they even have a driver loaded. I suspect the number of PCI devices where this is truly all they need, i.e. no state in any IO ports or MMIO registers that need to be reset on resume, is quite low. Maybe in some cases it may appear to work by luck, i.e. the registers happening to be set to the correct values (especially on suspend-to-disk) but this is not a proper implementation. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 16:19, Pekka Enberg wrote: On 2/11/07, Rafael J. Wysocki [EMAIL PROTECTED] wrote: Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions. No you don't. You can make it a config option that defaults to n during a transition period. Yes, that's possible. Greetings, Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sunday, 11 February 2007 18:27, Daniel Barkalow wrote: On Sun, 11 Feb 2007, Rafael J. Wysocki wrote: The problem is it was made implicit long ago. The design is optimistic, so to speak, and I think we have the following choices: 1) Change the design to make the kernel refuse to suspend if there are any drivers not explicitly flagged as suspend/resume-safe. [This looks like a lot of work to me, but it is generally doable provided that someone has enough time to do it. Unfortunately it has to be done in one shot for all of the known good drivers to avoid user-observable regressions.] The kernel wouldn't necessarily have to refuse to suspend. Well, not from the start, but I think at some point in the fufure it would. It could just warn (and list the drivers that aren't marked), or could require some extra insistance from the user. We would have to change the interface for that and I don't want to do it. It would be good to have it log a message saying something like: If you can read this, report that ne2000 seems to be safe for suspend/resume. Sure, it would. Having drivers explicitly marked as to whether they are safe is a good kernel feature; what to do if they're not is policy. That's true, but I assume that the people who opt for doing that are also willing to take part in the review of the drivers. :-) 2) Require the authors of new drivers to _either_ ensure that their drivers will be suspend/resume-safe (and I mean both STR and STD here), _or_ explicitly flag the drivers as suspend/resume-unsafe, for example by impelenting .suspend() routines returning -ENOSYS. [The existing drivers can be modified to follow this convention gradually.] I don't see any reason not to do (2) regardless of (1). That was (my idea of) the statement that started this thread: new drivers need to not mess up on suspend/resume, as a matter of suitability for inclusion. Of course, we need some way for drivers to indicate that they work fine with the PCI-layer defaults. And it should probably more machine-readable than the author telling reviewers that it works. Well, if we change the design to fail by default, the authors of new drivers will only have to flag them if they believe that the drivers are suspend/resume-safe. - Problem what to do with drivers that work for some people and don't work for the others (ie. if we don't flag them as known good, we will break the setups in which they work) I think the only interesting case here is when a device resumes fine with no driver support if the BIOS manages to deal effectively with it, but the BIOS generally doesn't. Well, I don't think so. Let's estimate the number of drivers that define .resume() right now: $ grep -I -l -r '.resume =' linux-2.6.20/drivers/ | wc 102 1024169 And I think there are much more drivers that really work fine with respect to the suspend/resume. This indicates that in fact many drivers can be marked as known good even though they don't define the .suspend() or .resume() routines. Greetings, Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi! Also, I think there are quite some drivers already in the tree that don't support suspend/resume explicitly and honestly we should start from adding the suspend/resume routines to these drivers _before_ we ban new drivers like that. It'd be relatively quick to modify all the current drivers that don't explicitly support suspend/resume to explicitly not support it. (Or Well, I think that is more work than you realize, but yes, that patch would really be welcome. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Rafael J. Wysocki wrote: - Problem what to do with drivers that work for some people and don't work for the others (ie. if we don't flag them as known good, we will break the setups in which they work) And this issue is independent of whether a driver has .suspend and .resume or not. For example, ohci1394 had them for ages but they didn't work as expected before 2.6.20. What's more, ohci1394's resume routine alone is insufficient; an additional facility in ieee1394 was necessary and added for 2.6.21(-rc). This shows that the original authors of ohci1394's .suspend and .resume didn't test with actual external devices or gave up when they figured that the problem reaches into upper subsystem layers. (We still haven't tested interaction with most of the IEEE 1394 high level.) -- Stefan Richter -=-=-=== --=- -=-== http://arcgraph.de/sr/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi! instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. Then change the PCI layer to do the basic PM only for known compatible drivers, and modify only the known-compatible drivers to mark them explicitly compatible. IMHO, it generally is a bad idea to require that any driver explicitly states what it *does not* support. It's the reason why users encounter problem on new features with old drivers. For instance, do you know if the old ISA NE2000 driver breaks suspend ? I don't know, but I would at least expect it not to support it by default. It's best to announce what *is* supported and consider everything unimplemented otherwise explicitly stated. Actually, ne2k driver is okay. ne2k cards are notoriously buggy, so it responds with , that damn card has just locked up again, lets reset it. Ok, it takes timeout to realize card is locked up, so it could be improved... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi. On Sun, 2007-02-11 at 12:13 +, Matthew Garrett wrote: On Sun, Feb 11, 2007 at 07:54:04AM +0100, Willy Tarreau wrote: instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. No, that doesn't work. In the absence of suspend/resume methods, the PCI layer will implement basic PM itself. In some cases, this works. In others, it doesn't. There's no way to automatically determine which is which without modifying the drivers. I think we have it backwards there. Power management support for a driver should always start with the driver itself. If there's a generic routine that can be used for the bus, the driver should explicitly set the routine to the generic routine. Regards, Nigel - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi. On Sun, 2007-02-11 at 19:53 +0100, Rafael J. Wysocki wrote: Having drivers explicitly marked as to whether they are safe is a good kernel feature; what to do if they're not is policy. That's true, but I assume that the people who opt for doing that are also willing to take part in the review of the drivers. :-) Absolutely :) Well, I don't think so. Let's estimate the number of drivers that define .resume() right now: $ grep -I -l -r '.resume =' linux-2.6.20/drivers/ | wc 102 1024169 I think the '.resume =' doesn't help - some have tabs. I ran '\.resume' and got 351. It would be interesting to see how many struct pci_driver etc instances lack resume methods. Regards, Nige - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Monday, 12 February 2007 00:06, Nigel Cunningham wrote: Hi. On Sun, 2007-02-11 at 19:53 +0100, Rafael J. Wysocki wrote: Having drivers explicitly marked as to whether they are safe is a good kernel feature; what to do if they're not is policy. That's true, but I assume that the people who opt for doing that are also willing to take part in the review of the drivers. :-) Absolutely :) Well, I don't think so. Let's estimate the number of drivers that define .resume() right now: $ grep -I -l -r '.resume =' linux-2.6.20/drivers/ | wc 102 1024169 I think the '.resume =' doesn't help - some have tabs. I ran '\.resume' and got 351. Ah, good catch. I have searched for .resume only and got 612, but this is the number of files, not the number of drivers. And it is not exactly large. ;-) It would be interesting to see how many struct pci_driver etc instances lack resume methods. Yes, I'll try to invent a test. Greetings, Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sat, Feb 10, 2007 at 08:50:27PM +0100, Rafael J. Wysocki wrote: > On Saturday, 10 February 2007 18:52, Daniel Barkalow wrote: > > On Sat, 10 Feb 2007, Rafael J. Wysocki wrote: > > > > > On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: > > > > > > > Well, the original desire was to stop new drivers getting in without > > > > proper power management. > > > > > > I know, but I agree with the argument that having a driver without the > > > suspend/resume support is better than not having the driver at all. > > > > How about if "proper power management" is defined to include the driver > > explicitly preventing suspend? It seems to me like the current problem is > > that driver writers don't think about power management at all, and the > > result is that, after suspend/resume, the system doesn't come back. It > > would be better if driver writers had to think about power management just > > enough to realize that it's not going to work, and make this information > > available to the system. At that point, it's relatively easy for the > > system to do something useful about it. > > Actually, it is easy for the driver authors to do this right now. They can > just make the .suspend() routine always return an error. > > Well, I think this is a good idea: if the device in question requires specific > power management during the suspend/resume, but it is not implemented by the > driver, we should require the author of the driver to define the .suspend() > routine that returns -ENOSYS (preferably, with an explanatory warning in > dmesg). instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. regards, Willy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Saturday, 10 February 2007 18:52, Daniel Barkalow wrote: > On Sat, 10 Feb 2007, Rafael J. Wysocki wrote: > > > On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: > > > > > Well, the original desire was to stop new drivers getting in without > > > proper power management. > > > > I know, but I agree with the argument that having a driver without the > > suspend/resume support is better than not having the driver at all. > > How about if "proper power management" is defined to include the driver > explicitly preventing suspend? It seems to me like the current problem is > that driver writers don't think about power management at all, and the > result is that, after suspend/resume, the system doesn't come back. It > would be better if driver writers had to think about power management just > enough to realize that it's not going to work, and make this information > available to the system. At that point, it's relatively easy for the > system to do something useful about it. Actually, it is easy for the driver authors to do this right now. They can just make the .suspend() routine always return an error. Well, I think this is a good idea: if the device in question requires specific power management during the suspend/resume, but it is not implemented by the driver, we should require the author of the driver to define the .suspend() routine that returns -ENOSYS (preferably, with an explanatory warning in dmesg). Thoughts? > > Also, I think there are quite some drivers already in the tree that don't > > support suspend/resume explicitly and honestly we should start from adding > > the > > suspend/resume routines to these drivers _before_ we ban new drivers like > > that. > > It'd be relatively quick to modify all the current drivers that don't > explicitly support suspend/resume to explicitly not support it. (Or to > explicitly support it trivially; /dev/null obviously doesn't need > anything.) The problem is we have to review quite a lot of drivers for this purpose. Still it looks like we should do this anyway. Greetings, Rafael -- If you don't have the time to read, you don't have the time or the tools to write. - Stephen King - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sat, 10 Feb 2007, Rafael J. Wysocki wrote: > On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: > > > Well, the original desire was to stop new drivers getting in without > > proper power management. > > I know, but I agree with the argument that having a driver without the > suspend/resume support is better than not having the driver at all. How about if "proper power management" is defined to include the driver explicitly preventing suspend? It seems to me like the current problem is that driver writers don't think about power management at all, and the result is that, after suspend/resume, the system doesn't come back. It would be better if driver writers had to think about power management just enough to realize that it's not going to work, and make this information available to the system. At that point, it's relatively easy for the system to do something useful about it. > Also, I think there are quite some drivers already in the tree that don't > support suspend/resume explicitly and honestly we should start from adding the > suspend/resume routines to these drivers _before_ we ban new drivers like > that. It'd be relatively quick to modify all the current drivers that don't explicitly support suspend/resume to explicitly not support it. (Or to explicitly support it trivially; /dev/null obviously doesn't need anything.) -Daniel *This .sig left intentionally blank* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi, On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: > Gidday. > > On Sat, 2007-02-10 at 10:34 +0100, Rafael J. Wysocki wrote: > > On Saturday, 10 February 2007 04:02, Nigel Cunningham wrote: > > > On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote: > > > > It also kind of bothers me that if a driver has no suspend/resume > > > > functions, and you suspend and resume the system, we don't complain > > > > about it even though there's a very good chance that device is not > > > > going > > > > to function properly. How about something in dmesg like: > > > > > > > > Warning: driver for device has no suspend or resume support. > > > > Device may not function properly after resume. > > > > > > > > so that users know who to complain to. Maybe there are some devices > > > > that > > > > truly don't need any handling for suspend, but if so I suspect the > > > > number of those is small enough that adding empty functions would be a > > > > good-enough solution. > > > > > > Here's my current version of a patch to do this, if anyone wants to try > > > it out. It dumps stack with the warning to make it easier to see what > > > the source of the message is: > > > > I have an alternative idea. > > > > There is a test mode of swsusp that's triggered with > > "echo test > /sys/power/disk" and "echo disk > /sys/power/state". We can > > make > > it set a switch that will be used to trigger the warnings in the core. > > > > This way the warnings will only appear in the user's dmesg in the test mode > > and not always. > > > > Would that be acceptable? > > Well, the original desire was to stop new drivers getting in without > proper power management. I know, but I agree with the argument that having a driver without the suspend/resume support is better than not having the driver at all. Moreover, if you _know_ which of the drivers you use have no suspend/resume support, usually you _can_ suspend (at least to disk) and resume anyway - you only need to unload their modules before the suspend. Of course, this is not true for the disk (and related) drivers and for the drivers that cannot be unloaded. [I think it generally is not true for the suspend-to-RAM either.] Also, I think there are quite some drivers already in the tree that don't support suspend/resume explicitly and honestly we should start from adding the suspend/resume routines to these drivers _before_ we ban new drivers like that. > For this to help achieve that aim, one or more > of the following would need to happen: > > * developers of new drivers would have to remember to run the test > after/during writing their driver. Of course if they remember to do > this, they've already remembered that they need to implement power > management; > * you, I or someone else would need to test all these trees before > Andrew and Linus merge them and report problems to the developers before > they get their new drivers merged; > * Andrew or Linus would run it prior to or after merging a whole lot of > stuff. > > I'm afraid I don't like the chances of any of those things happening, so > I'm not sure that it is an acceptable alternative from my perspective. > > Sticking a printk in dmesg doesn't exactly put the problem in flashing > red 36 point characters before the developer either, but I think they're > slightly more likely to see it there if only because they might stick > printks in that they want to read (eg perhaps while debugging the > driver) and therefore see the message when checking output from the > driver being loaded/initialised. > > Maybe another alternative would be to make the warnings compile time > warnings - if that's possible? Well, maybe. Still, I'd like to create standard debugging procedures for suspend/resume, so that if someone reports a problem, we can tell him exactly what to do next. >From this point of view, the idea of having warnings related to the lack of .suspend or .resume routines in drivers that will appear in the user's dmesg in the test mode seems to be a good one. ;-) Greetings, Rafael -- If you don't have the time to read, you don't have the time or the tools to write. - Stephen King - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Gidday. On Sat, 2007-02-10 at 10:34 +0100, Rafael J. Wysocki wrote: > On Saturday, 10 February 2007 04:02, Nigel Cunningham wrote: > > On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote: > > > It also kind of bothers me that if a driver has no suspend/resume > > > functions, and you suspend and resume the system, we don't complain > > > about it even though there's a very good chance that device is not going > > > to function properly. How about something in dmesg like: > > > > > > Warning: driver for device has no suspend or resume support. > > > Device may not function properly after resume. > > > > > > so that users know who to complain to. Maybe there are some devices that > > > truly don't need any handling for suspend, but if so I suspect the > > > number of those is small enough that adding empty functions would be a > > > good-enough solution. > > > > Here's my current version of a patch to do this, if anyone wants to try > > it out. It dumps stack with the warning to make it easier to see what > > the source of the message is: > > I have an alternative idea. > > There is a test mode of swsusp that's triggered with > "echo test > /sys/power/disk" and "echo disk > /sys/power/state". We can make > it set a switch that will be used to trigger the warnings in the core. > > This way the warnings will only appear in the user's dmesg in the test mode > and not always. > > Would that be acceptable? Well, the original desire was to stop new drivers getting in without proper power management. For this to help achieve that aim, one or more of the following would need to happen: * developers of new drivers would have to remember to run the test after/during writing their driver. Of course if they remember to do this, they've already remembered that they need to implement power management; * you, I or someone else would need to test all these trees before Andrew and Linus merge them and report problems to the developers before they get their new drivers merged; * Andrew or Linus would run it prior to or after merging a whole lot of stuff. I'm afraid I don't like the chances of any of those things happening, so I'm not sure that it is an acceptable alternative from my perspective. Sticking a printk in dmesg doesn't exactly put the problem in flashing red 36 point characters before the developer either, but I think they're slightly more likely to see it there if only because they might stick printks in that they want to read (eg perhaps while debugging the driver) and therefore see the message when checking output from the driver being loaded/initialised. Maybe another alternative would be to make the warnings compile time warnings - if that's possible? Regards, Nigel - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi, On Saturday, 10 February 2007 04:02, Nigel Cunningham wrote: > Hi. > > On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote: > > It also kind of bothers me that if a driver has no suspend/resume > > functions, and you suspend and resume the system, we don't complain > > about it even though there's a very good chance that device is not going > > to function properly. How about something in dmesg like: > > > > Warning: driver for device has no suspend or resume support. > > Device may not function properly after resume. > > > > so that users know who to complain to. Maybe there are some devices that > > truly don't need any handling for suspend, but if so I suspect the > > number of those is small enough that adding empty functions would be a > > good-enough solution. > > Here's my current version of a patch to do this, if anyone wants to try > it out. It dumps stack with the warning to make it easier to see what > the source of the message is: I have an alternative idea. There is a test mode of swsusp that's triggered with "echo test > /sys/power/disk" and "echo disk > /sys/power/state". We can make it set a switch that will be used to trigger the warnings in the core. This way the warnings will only appear in the user's dmesg in the test mode and not always. Would that be acceptable? Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sat, Feb 10, 2007 at 08:50:27PM +0100, Rafael J. Wysocki wrote: On Saturday, 10 February 2007 18:52, Daniel Barkalow wrote: On Sat, 10 Feb 2007, Rafael J. Wysocki wrote: On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: Well, the original desire was to stop new drivers getting in without proper power management. I know, but I agree with the argument that having a driver without the suspend/resume support is better than not having the driver at all. How about if proper power management is defined to include the driver explicitly preventing suspend? It seems to me like the current problem is that driver writers don't think about power management at all, and the result is that, after suspend/resume, the system doesn't come back. It would be better if driver writers had to think about power management just enough to realize that it's not going to work, and make this information available to the system. At that point, it's relatively easy for the system to do something useful about it. Actually, it is easy for the driver authors to do this right now. They can just make the .suspend() routine always return an error. Well, I think this is a good idea: if the device in question requires specific power management during the suspend/resume, but it is not implemented by the driver, we should require the author of the driver to define the .suspend() routine that returns -ENOSYS (preferably, with an explanatory warning in dmesg). instead of modifying all drivers to explicitly state that they don't support it, we should start with a test of the NULL pointer for .suspend which should mean exactly the same without modifying the drivers. I find it obvious that a driver which does provide a suspend function will not support it. And if some drivers (eg /dev/null) can support it anyway, it's better to change *those* drivers to explicitly mark them as compatible. regards, Willy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi, On Saturday, 10 February 2007 04:02, Nigel Cunningham wrote: Hi. On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote: It also kind of bothers me that if a driver has no suspend/resume functions, and you suspend and resume the system, we don't complain about it even though there's a very good chance that device is not going to function properly. How about something in dmesg like: Warning: driver for device has no suspend or resume support. Device may not function properly after resume. so that users know who to complain to. Maybe there are some devices that truly don't need any handling for suspend, but if so I suspect the number of those is small enough that adding empty functions would be a good-enough solution. Here's my current version of a patch to do this, if anyone wants to try it out. It dumps stack with the warning to make it easier to see what the source of the message is: I have an alternative idea. There is a test mode of swsusp that's triggered with echo test /sys/power/disk and echo disk /sys/power/state. We can make it set a switch that will be used to trigger the warnings in the core. This way the warnings will only appear in the user's dmesg in the test mode and not always. Would that be acceptable? Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Gidday. On Sat, 2007-02-10 at 10:34 +0100, Rafael J. Wysocki wrote: On Saturday, 10 February 2007 04:02, Nigel Cunningham wrote: On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote: It also kind of bothers me that if a driver has no suspend/resume functions, and you suspend and resume the system, we don't complain about it even though there's a very good chance that device is not going to function properly. How about something in dmesg like: Warning: driver for device has no suspend or resume support. Device may not function properly after resume. so that users know who to complain to. Maybe there are some devices that truly don't need any handling for suspend, but if so I suspect the number of those is small enough that adding empty functions would be a good-enough solution. Here's my current version of a patch to do this, if anyone wants to try it out. It dumps stack with the warning to make it easier to see what the source of the message is: I have an alternative idea. There is a test mode of swsusp that's triggered with echo test /sys/power/disk and echo disk /sys/power/state. We can make it set a switch that will be used to trigger the warnings in the core. This way the warnings will only appear in the user's dmesg in the test mode and not always. Would that be acceptable? Well, the original desire was to stop new drivers getting in without proper power management. For this to help achieve that aim, one or more of the following would need to happen: * developers of new drivers would have to remember to run the test after/during writing their driver. Of course if they remember to do this, they've already remembered that they need to implement power management; * you, I or someone else would need to test all these trees before Andrew and Linus merge them and report problems to the developers before they get their new drivers merged; * Andrew or Linus would run it prior to or after merging a whole lot of stuff. I'm afraid I don't like the chances of any of those things happening, so I'm not sure that it is an acceptable alternative from my perspective. Sticking a printk in dmesg doesn't exactly put the problem in flashing red 36 point characters before the developer either, but I think they're slightly more likely to see it there if only because they might stick printks in that they want to read (eg perhaps while debugging the driver) and therefore see the message when checking output from the driver being loaded/initialised. Maybe another alternative would be to make the warnings compile time warnings - if that's possible? Regards, Nigel - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
Hi, On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: Gidday. On Sat, 2007-02-10 at 10:34 +0100, Rafael J. Wysocki wrote: On Saturday, 10 February 2007 04:02, Nigel Cunningham wrote: On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote: It also kind of bothers me that if a driver has no suspend/resume functions, and you suspend and resume the system, we don't complain about it even though there's a very good chance that device is not going to function properly. How about something in dmesg like: Warning: driver for device has no suspend or resume support. Device may not function properly after resume. so that users know who to complain to. Maybe there are some devices that truly don't need any handling for suspend, but if so I suspect the number of those is small enough that adding empty functions would be a good-enough solution. Here's my current version of a patch to do this, if anyone wants to try it out. It dumps stack with the warning to make it easier to see what the source of the message is: I have an alternative idea. There is a test mode of swsusp that's triggered with echo test /sys/power/disk and echo disk /sys/power/state. We can make it set a switch that will be used to trigger the warnings in the core. This way the warnings will only appear in the user's dmesg in the test mode and not always. Would that be acceptable? Well, the original desire was to stop new drivers getting in without proper power management. I know, but I agree with the argument that having a driver without the suspend/resume support is better than not having the driver at all. Moreover, if you _know_ which of the drivers you use have no suspend/resume support, usually you _can_ suspend (at least to disk) and resume anyway - you only need to unload their modules before the suspend. Of course, this is not true for the disk (and related) drivers and for the drivers that cannot be unloaded. [I think it generally is not true for the suspend-to-RAM either.] Also, I think there are quite some drivers already in the tree that don't support suspend/resume explicitly and honestly we should start from adding the suspend/resume routines to these drivers _before_ we ban new drivers like that. For this to help achieve that aim, one or more of the following would need to happen: * developers of new drivers would have to remember to run the test after/during writing their driver. Of course if they remember to do this, they've already remembered that they need to implement power management; * you, I or someone else would need to test all these trees before Andrew and Linus merge them and report problems to the developers before they get their new drivers merged; * Andrew or Linus would run it prior to or after merging a whole lot of stuff. I'm afraid I don't like the chances of any of those things happening, so I'm not sure that it is an acceptable alternative from my perspective. Sticking a printk in dmesg doesn't exactly put the problem in flashing red 36 point characters before the developer either, but I think they're slightly more likely to see it there if only because they might stick printks in that they want to read (eg perhaps while debugging the driver) and therefore see the message when checking output from the driver being loaded/initialised. Maybe another alternative would be to make the warnings compile time warnings - if that's possible? Well, maybe. Still, I'd like to create standard debugging procedures for suspend/resume, so that if someone reports a problem, we can tell him exactly what to do next. From this point of view, the idea of having warnings related to the lack of .suspend or .resume routines in drivers that will appear in the user's dmesg in the test mode seems to be a good one. ;-) Greetings, Rafael -- If you don't have the time to read, you don't have the time or the tools to write. - Stephen King - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Sat, 10 Feb 2007, Rafael J. Wysocki wrote: On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: Well, the original desire was to stop new drivers getting in without proper power management. I know, but I agree with the argument that having a driver without the suspend/resume support is better than not having the driver at all. How about if proper power management is defined to include the driver explicitly preventing suspend? It seems to me like the current problem is that driver writers don't think about power management at all, and the result is that, after suspend/resume, the system doesn't come back. It would be better if driver writers had to think about power management just enough to realize that it's not going to work, and make this information available to the system. At that point, it's relatively easy for the system to do something useful about it. Also, I think there are quite some drivers already in the tree that don't support suspend/resume explicitly and honestly we should start from adding the suspend/resume routines to these drivers _before_ we ban new drivers like that. It'd be relatively quick to modify all the current drivers that don't explicitly support suspend/resume to explicitly not support it. (Or to explicitly support it trivially; /dev/null obviously doesn't need anything.) -Daniel *This .sig left intentionally blank* - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: NAK new drivers without proper power management?
On Saturday, 10 February 2007 18:52, Daniel Barkalow wrote: On Sat, 10 Feb 2007, Rafael J. Wysocki wrote: On Saturday, 10 February 2007 11:02, Nigel Cunningham wrote: Well, the original desire was to stop new drivers getting in without proper power management. I know, but I agree with the argument that having a driver without the suspend/resume support is better than not having the driver at all. How about if proper power management is defined to include the driver explicitly preventing suspend? It seems to me like the current problem is that driver writers don't think about power management at all, and the result is that, after suspend/resume, the system doesn't come back. It would be better if driver writers had to think about power management just enough to realize that it's not going to work, and make this information available to the system. At that point, it's relatively easy for the system to do something useful about it. Actually, it is easy for the driver authors to do this right now. They can just make the .suspend() routine always return an error. Well, I think this is a good idea: if the device in question requires specific power management during the suspend/resume, but it is not implemented by the driver, we should require the author of the driver to define the .suspend() routine that returns -ENOSYS (preferably, with an explanatory warning in dmesg). Thoughts? Also, I think there are quite some drivers already in the tree that don't support suspend/resume explicitly and honestly we should start from adding the suspend/resume routines to these drivers _before_ we ban new drivers like that. It'd be relatively quick to modify all the current drivers that don't explicitly support suspend/resume to explicitly not support it. (Or to explicitly support it trivially; /dev/null obviously doesn't need anything.) The problem is we have to review quite a lot of drivers for this purpose. Still it looks like we should do this anyway. Greetings, Rafael -- If you don't have the time to read, you don't have the time or the tools to write. - Stephen King - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Re: NAK new drivers without proper power management?
Hi.
On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote:
> It also kind of bothers me that if a driver has no suspend/resume
> functions, and you suspend and resume the system, we don't complain
> about it even though there's a very good chance that device is not going
> to function properly. How about something in dmesg like:
>
> Warning: driver for device has no suspend or resume support.
> Device may not function properly after resume.
>
> so that users know who to complain to. Maybe there are some devices that
> truly don't need any handling for suspend, but if so I suspect the
> number of those is small enough that adding empty functions would be a
> good-enough solution.
Here's my current version of a patch to do this, if anyone wants to try
it out. It dumps stack with the warning to make it easier to see what
the source of the message is:
drivers/base/core.c | 25 +
drivers/pci/pci-driver.c |6 ++
drivers/usb/core/driver.c |5 +
include/linux/device.h|1 +
4 files changed, 37 insertions(+)
diff -ruNp 920-report-no-pm-support.patch-old/drivers/base/core.c
920-report-no-pm-support.patch-new/drivers/base/core.c
--- 920-report-no-pm-support.patch-old/drivers/base/core.c 2007-02-06
14:48:31.0 +1100
+++ 920-report-no-pm-support.patch-new/drivers/base/core.c 2007-02-10
13:36:33.0 +1100
@@ -552,6 +552,30 @@ int device_add(struct device *dev)
class_intf->add_dev(dev, class_intf);
up(>class->sem);
}
+
+#ifdef CONFIG_PM
+ {
+ int nosusp = 0, nores = 0;
+
+ if (!((dev->class && dev->class->suspend) ||
+ (dev->bus && (dev->bus->suspend || dev->bus->suspend_late
+ nosusp = 1;
+
+ if (!((dev->class && dev->class->resume) ||
+ (dev->bus && (dev->bus->resume || dev->bus->resume_early
+ nores = 1;
+
+ if ((nosusp || nores) && !dev->pm_safe) {
+ printk("Device driver %s lacks bus and class support for "
+ "being %s.\n",
+ kobject_name(>kobj),
+ nosusp ? (nores ? "suspended or resumed" :
+ "resumed") : "suspended");
+ dump_stack();
+ }
+ }
+#endif
+
Done:
kfree(class_name);
put_device(dev);
@@ -851,6 +875,7 @@ struct device *device_create(struct clas
dev->class = class;
dev->parent = parent;
dev->release = device_create_release;
+ dev->pm_safe = 1;
va_start(args, fmt);
vsnprintf(dev->bus_id, BUS_ID_SIZE, fmt, args);
diff -ruNp 920-report-no-pm-support.patch-old/drivers/pci/pci-driver.c
920-report-no-pm-support.patch-new/drivers/pci/pci-driver.c
--- 920-report-no-pm-support.patch-old/drivers/pci/pci-driver.c 2007-02-06
14:48:44.0 +1100
+++ 920-report-no-pm-support.patch-new/drivers/pci/pci-driver.c 2007-02-10
14:00:39.0 +1100
@@ -449,6 +449,12 @@ int __pci_register_driver(struct pci_dri
if (error)
driver_unregister(>driver);
+ if (!drv->suspend || !drv->resume)
+ printk("PCI driver %s lacks driver specific %s support.\n",
+ drv->name,
+ !drv->suspend ? (drv->resume ? "suspend" :
+ "suspend and resume") : "resume");
+
return error;
}
diff -ruNp 920-report-no-pm-support.patch-old/drivers/usb/core/driver.c
920-report-no-pm-support.patch-new/drivers/usb/core/driver.c
--- 920-report-no-pm-support.patch-old/drivers/usb/core/driver.c
2007-02-06 14:48:47.0 +1100
+++ 920-report-no-pm-support.patch-new/drivers/usb/core/driver.c
2007-02-10 12:32:57.0 +1100
@@ -709,6 +709,11 @@ int usb_register_device_driver(struct us
pr_info("%s: registered new device driver %s\n",
usbcore_name, new_udriver->name);
usbfs_update_special();
+ if (!new_udriver->suspend || !new_udriver->resume)
+ printk("USB driver %s lacks %s support.\n",
+ new_udriver->name, !new_udriver->suspend ?
+ (new_udriver->resume ? "suspend" :
+"suspend and resume") : "resume");
} else {
printk(KERN_ERR "%s: error %d registering device "
" driver %s\n",
diff -ruNp 920-report-no-pm-support.patch-old/include/linux/device.h
920-report-no-pm-support.patch-new/include/linux/device.h
--- 920-report-no-pm-support.patch-old/include/linux/device.h 2007-02-06
14:48:56.0 +1100
+++ 920-report-no-pm-support.patch-new/include/linux/device.h 2007-02-10
13:36:01.0 +1100
@@ -356,6 +356,7 @@ struct device {
struct kobject kobj;
charbus_id[BUS_ID_SIZE];/*
[PATCH] Re: NAK new drivers without proper power management?
Hi.
On Fri, 2007-02-09 at 19:50 -0600, Robert Hancock wrote:
It also kind of bothers me that if a driver has no suspend/resume
functions, and you suspend and resume the system, we don't complain
about it even though there's a very good chance that device is not going
to function properly. How about something in dmesg like:
Warning: driver for device has no suspend or resume support.
Device may not function properly after resume.
so that users know who to complain to. Maybe there are some devices that
truly don't need any handling for suspend, but if so I suspect the
number of those is small enough that adding empty functions would be a
good-enough solution.
Here's my current version of a patch to do this, if anyone wants to try
it out. It dumps stack with the warning to make it easier to see what
the source of the message is:
drivers/base/core.c | 25 +
drivers/pci/pci-driver.c |6 ++
drivers/usb/core/driver.c |5 +
include/linux/device.h|1 +
4 files changed, 37 insertions(+)
diff -ruNp 920-report-no-pm-support.patch-old/drivers/base/core.c
920-report-no-pm-support.patch-new/drivers/base/core.c
--- 920-report-no-pm-support.patch-old/drivers/base/core.c 2007-02-06
14:48:31.0 +1100
+++ 920-report-no-pm-support.patch-new/drivers/base/core.c 2007-02-10
13:36:33.0 +1100
@@ -552,6 +552,30 @@ int device_add(struct device *dev)
class_intf-add_dev(dev, class_intf);
up(dev-class-sem);
}
+
+#ifdef CONFIG_PM
+ {
+ int nosusp = 0, nores = 0;
+
+ if (!((dev-class dev-class-suspend) ||
+ (dev-bus (dev-bus-suspend || dev-bus-suspend_late
+ nosusp = 1;
+
+ if (!((dev-class dev-class-resume) ||
+ (dev-bus (dev-bus-resume || dev-bus-resume_early
+ nores = 1;
+
+ if ((nosusp || nores) !dev-pm_safe) {
+ printk(Device driver %s lacks bus and class support for
+ being %s.\n,
+ kobject_name(dev-kobj),
+ nosusp ? (nores ? suspended or resumed :
+ resumed) : suspended);
+ dump_stack();
+ }
+ }
+#endif
+
Done:
kfree(class_name);
put_device(dev);
@@ -851,6 +875,7 @@ struct device *device_create(struct clas
dev-class = class;
dev-parent = parent;
dev-release = device_create_release;
+ dev-pm_safe = 1;
va_start(args, fmt);
vsnprintf(dev-bus_id, BUS_ID_SIZE, fmt, args);
diff -ruNp 920-report-no-pm-support.patch-old/drivers/pci/pci-driver.c
920-report-no-pm-support.patch-new/drivers/pci/pci-driver.c
--- 920-report-no-pm-support.patch-old/drivers/pci/pci-driver.c 2007-02-06
14:48:44.0 +1100
+++ 920-report-no-pm-support.patch-new/drivers/pci/pci-driver.c 2007-02-10
14:00:39.0 +1100
@@ -449,6 +449,12 @@ int __pci_register_driver(struct pci_dri
if (error)
driver_unregister(drv-driver);
+ if (!drv-suspend || !drv-resume)
+ printk(PCI driver %s lacks driver specific %s support.\n,
+ drv-name,
+ !drv-suspend ? (drv-resume ? suspend :
+ suspend and resume) : resume);
+
return error;
}
diff -ruNp 920-report-no-pm-support.patch-old/drivers/usb/core/driver.c
920-report-no-pm-support.patch-new/drivers/usb/core/driver.c
--- 920-report-no-pm-support.patch-old/drivers/usb/core/driver.c
2007-02-06 14:48:47.0 +1100
+++ 920-report-no-pm-support.patch-new/drivers/usb/core/driver.c
2007-02-10 12:32:57.0 +1100
@@ -709,6 +709,11 @@ int usb_register_device_driver(struct us
pr_info(%s: registered new device driver %s\n,
usbcore_name, new_udriver-name);
usbfs_update_special();
+ if (!new_udriver-suspend || !new_udriver-resume)
+ printk(USB driver %s lacks %s support.\n,
+ new_udriver-name, !new_udriver-suspend ?
+ (new_udriver-resume ? suspend :
+suspend and resume) : resume);
} else {
printk(KERN_ERR %s: error %d registering device
driver %s\n,
diff -ruNp 920-report-no-pm-support.patch-old/include/linux/device.h
920-report-no-pm-support.patch-new/include/linux/device.h
--- 920-report-no-pm-support.patch-old/include/linux/device.h 2007-02-06
14:48:56.0 +1100
+++ 920-report-no-pm-support.patch-new/include/linux/device.h 2007-02-10
13:36:01.0 +1100
@@ -356,6 +356,7 @@ struct device {
struct kobject kobj;
charbus_id[BUS_ID_SIZE];/* position on parent bus */
unsignedis_registered:1;
+
