* [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions
@ 2005-02-14 15:01 Christophe Lucas
2005-02-14 15:10 ` Matthew Wilcox
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Christophe Lucas @ 2005-02-14 15:01 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 293 bytes --]
Hi,
description:
If PCI request regions fails, then someone else is using the
hardware we wish to use. For that one case, calling
pci_disable_device() is rather rude.
See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html
Signed-off-by: Christophe Lucas <c.lucas@ifrance.com>
[-- Attachment #2: pci_disable-linux-2.6.11-rc4_drivers_char_watchdog.patch --]
[-- Type: text/plain, Size: 1009 bytes --]
diff -urpN -X /work/users/dontdiff linux-2.6.11-rc4-vanilla/drivers/char/watchdog/pcwd_pci.c linux-2.6.11-rc4/drivers/char/watchdog/pcwd_pci.c
--- linux-2.6.11-rc4-vanilla/drivers/char/watchdog/pcwd_pci.c 2004-12-24 22:33:59.000000000 +0100
+++ linux-2.6.11-rc4/drivers/char/watchdog/pcwd_pci.c 2005-02-14 09:45:07.000000000 +0100
@@ -506,6 +506,7 @@ static int __devinit pcipcwd_card_init(s
int got_fw_rev, fw_rev_major, fw_rev_minor;
char fw_ver_str[20];
char option_switches;
+ int pci_dev_busy = 0 ;
cards_found++;
if (cards_found == 1)
@@ -534,6 +535,7 @@ static int __devinit pcipcwd_card_init(s
printk(KERN_ERR PFX "I/O address 0x%04x already in use\n",
(int) pcipcwd_private.io_addr);
ret = -EIO;
+ pci_dev_busy = 1 ;
goto err_out_disable_device;
}
@@ -621,7 +623,8 @@ err_out_unregister_reboot:
err_out_release_region:
pci_release_regions(pdev);
err_out_disable_device:
- pci_disable_device(pdev);
+ if(!pci_dev_busy)
+ pci_disable_device(pdev);
return ret;
}
[-- Attachment #3: Type: text/plain, Size: 167 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-14 15:01 [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions Christophe Lucas @ 2005-02-14 15:10 ` Matthew Wilcox 2005-02-14 15:12 ` Matthew Wilcox ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2005-02-14 15:10 UTC (permalink / raw) To: kernel-janitors [-- Attachment #1: Type: text/plain, Size: 676 bytes --] On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: Please pay attention to CodingStyle. > + int pci_dev_busy = 0 ; No space before the ; > + pci_dev_busy = 1 ; Ditto. > + if(!pci_dev_busy) Should be a space between if and ( -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-14 15:01 [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions Christophe Lucas @ 2005-02-14 15:12 ` Matthew Wilcox 2005-02-14 15:12 ` Matthew Wilcox ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2005-02-14 15:12 UTC (permalink / raw) To: Christophe Lucas; +Cc: kernel-janitors, Jeff Garzik, linux-kernel [-- Attachment #1: Type: text/plain, Size: 983 bytes --] On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > If PCI request regions fails, then someone else is using the > hardware we wish to use. For that one case, calling > pci_disable_device() is rather rude. > See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html Actually, that isn't necessarily true. If the request_regions call fails, that can mean there's a resource conflict. If so, leaving the device enabled is the worst possible thing to do as we'll now have two devices trying to respond to the same io accesses. -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions @ 2005-02-14 15:12 ` Matthew Wilcox 0 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2005-02-14 15:12 UTC (permalink / raw) To: Christophe Lucas; +Cc: kernel-janitors, Jeff Garzik, linux-kernel On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > If PCI request regions fails, then someone else is using the > hardware we wish to use. For that one case, calling > pci_disable_device() is rather rude. > See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html Actually, that isn't necessarily true. If the request_regions call fails, that can mean there's a resource conflict. If so, leaving the device enabled is the worst possible thing to do as we'll now have two devices trying to respond to the same io accesses. -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-14 15:12 ` Matthew Wilcox @ 2005-02-17 18:49 ` Jeff Garzik -1 siblings, 0 replies; 12+ messages in thread From: Jeff Garzik @ 2005-02-17 18:49 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Christophe Lucas, kernel-janitors, linux-kernel [-- Attachment #1: Type: text/plain, Size: 911 bytes --] Matthew Wilcox wrote: > On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > >>If PCI request regions fails, then someone else is using the >>hardware we wish to use. For that one case, calling >>pci_disable_device() is rather rude. >>See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html > > > Actually, that isn't necessarily true. If the request_regions call fails, > that can mean there's a resource conflict. If so, leaving the device > enabled is the worst possible thing to do as we'll now have two devices > trying to respond to the same io accesses. Incorrect. If request_region() fails, drivers are coded to _not_ touch the hardware. That's the entire purpose of the whole charade: to avoid having two devices responding to the same io accesses. If your driver is talking to the hardware after request_region() fails, it is BROKEN plain and simple. Jeff [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions @ 2005-02-17 18:49 ` Jeff Garzik 0 siblings, 0 replies; 12+ messages in thread From: Jeff Garzik @ 2005-02-17 18:49 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Christophe Lucas, kernel-janitors, linux-kernel Matthew Wilcox wrote: > On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > >>If PCI request regions fails, then someone else is using the >>hardware we wish to use. For that one case, calling >>pci_disable_device() is rather rude. >>See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html > > > Actually, that isn't necessarily true. If the request_regions call fails, > that can mean there's a resource conflict. If so, leaving the device > enabled is the worst possible thing to do as we'll now have two devices > trying to respond to the same io accesses. Incorrect. If request_region() fails, drivers are coded to _not_ touch the hardware. That's the entire purpose of the whole charade: to avoid having two devices responding to the same io accesses. If your driver is talking to the hardware after request_region() fails, it is BROKEN plain and simple. Jeff ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-17 18:49 ` Jeff Garzik @ 2005-02-17 19:04 ` Matthew Wilcox -1 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2005-02-17 19:04 UTC (permalink / raw) To: Jeff Garzik Cc: Matthew Wilcox, Christophe Lucas, kernel-janitors, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2362 bytes --] On Thu, Feb 17, 2005 at 01:49:12PM -0500, Jeff Garzik wrote: > Matthew Wilcox wrote: > >On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > > > >>If PCI request regions fails, then someone else is using the > >>hardware we wish to use. For that one case, calling > >>pci_disable_device() is rather rude. > >>See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html > > > > > >Actually, that isn't necessarily true. If the request_regions call fails, > >that can mean there's a resource conflict. If so, leaving the device > >enabled is the worst possible thing to do as we'll now have two devices > >trying to respond to the same io accesses. > > Incorrect. If request_region() fails, drivers are coded to _not_ touch > the hardware. That's the entire purpose of the whole charade: to avoid > having two devices responding to the same io accesses. > > If your driver is talking to the hardware after request_region() fails, > it is BROKEN plain and simple. I don't think you understood my point. Assume we really do have two devices in the system with clashing resource settings. Yes, I really have seen this happen; it's rare. While the PCI device is disabled, there is no problem. But then we call pci_enable_device(), so now the device is enabled to respond to IO and memory resources in its BARs. At the point we discover this, we need to disable the PCI device again -- exactly the opposite behaviour from your case where we need to leave the PCI device enabled when we have resource conflicts. I think the only way to fix this is have pci_enable_device claim the resources for the BARs before enabling the PCI device to respond to the resources; that way we leave the enable bits the way they currently are. No, this doesn't cure the world's ills, but it does obey "First, do no harm". One way it'll fail is if the other driver loads after the PCI driver that claims this resource. -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions @ 2005-02-17 19:04 ` Matthew Wilcox 0 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2005-02-17 19:04 UTC (permalink / raw) To: Jeff Garzik Cc: Matthew Wilcox, Christophe Lucas, kernel-janitors, linux-kernel On Thu, Feb 17, 2005 at 01:49:12PM -0500, Jeff Garzik wrote: > Matthew Wilcox wrote: > >On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > > > >>If PCI request regions fails, then someone else is using the > >>hardware we wish to use. For that one case, calling > >>pci_disable_device() is rather rude. > >>See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html > > > > > >Actually, that isn't necessarily true. If the request_regions call fails, > >that can mean there's a resource conflict. If so, leaving the device > >enabled is the worst possible thing to do as we'll now have two devices > >trying to respond to the same io accesses. > > Incorrect. If request_region() fails, drivers are coded to _not_ touch > the hardware. That's the entire purpose of the whole charade: to avoid > having two devices responding to the same io accesses. > > If your driver is talking to the hardware after request_region() fails, > it is BROKEN plain and simple. I don't think you understood my point. Assume we really do have two devices in the system with clashing resource settings. Yes, I really have seen this happen; it's rare. While the PCI device is disabled, there is no problem. But then we call pci_enable_device(), so now the device is enabled to respond to IO and memory resources in its BARs. At the point we discover this, we need to disable the PCI device again -- exactly the opposite behaviour from your case where we need to leave the PCI device enabled when we have resource conflicts. I think the only way to fix this is have pci_enable_device claim the resources for the BARs before enabling the PCI device to respond to the resources; that way we leave the enable bits the way they currently are. No, this doesn't cure the world's ills, but it does obey "First, do no harm". One way it'll fail is if the other driver loads after the PCI driver that claims this resource. -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-17 19:04 ` Matthew Wilcox @ 2005-02-17 19:22 ` Jeff Garzik -1 siblings, 0 replies; 12+ messages in thread From: Jeff Garzik @ 2005-02-17 19:22 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Christophe Lucas, kernel-janitors, linux-kernel, Greg KH [-- Attachment #1: Type: text/plain, Size: 2478 bytes --] Matthew Wilcox wrote: > On Thu, Feb 17, 2005 at 01:49:12PM -0500, Jeff Garzik wrote: > >>Matthew Wilcox wrote: >> >>>On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: >>> >>> >>>>If PCI request regions fails, then someone else is using the >>>>hardware we wish to use. For that one case, calling >>>>pci_disable_device() is rather rude. >>>>See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html >>> >>> >>>Actually, that isn't necessarily true. If the request_regions call fails, >>>that can mean there's a resource conflict. If so, leaving the device >>>enabled is the worst possible thing to do as we'll now have two devices >>>trying to respond to the same io accesses. >> >>Incorrect. If request_region() fails, drivers are coded to _not_ touch >>the hardware. That's the entire purpose of the whole charade: to avoid >>having two devices responding to the same io accesses. >> >>If your driver is talking to the hardware after request_region() fails, >>it is BROKEN plain and simple. > > > I don't think you understood my point. Assume we really do have two > devices in the system with clashing resource settings. Yes, I really > have seen this happen; it's rare. > > While the PCI device is disabled, there is no problem. But then we call > pci_enable_device(), so now the device is enabled to respond to IO and > memory resources in its BARs. > > At the point we discover this, we need to disable the PCI device again > -- exactly the opposite behaviour from your case where we need to leave > the PCI device enabled when we have resource conflicts. > I think the only way to fix this is have pci_enable_device claim the > resources for the BARs before enabling the PCI device to respond to the > resources; that way we leave the enable bits the way they currently are. > No, this doesn't cure the world's ills, but it does obey "First, do > no harm". One way it'll fail is if the other driver loads after the PCI > driver that claims this resource. Ok, I agree with this, it echoes what I said in another message in this thread ;-) namely, * the fact that pci_enable_device() does not claim the resources is a problem. pci_request_regions() should not be a separate step. * the fact that pci_disable_device() does not perform the _exact_ opposite of the operations that pci_enable_device() performed is a problem. pci_disable_device() should not just unconditionally stop the decoder bits, then exit. Jeff [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions @ 2005-02-17 19:22 ` Jeff Garzik 0 siblings, 0 replies; 12+ messages in thread From: Jeff Garzik @ 2005-02-17 19:22 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Christophe Lucas, kernel-janitors, linux-kernel, Greg KH Matthew Wilcox wrote: > On Thu, Feb 17, 2005 at 01:49:12PM -0500, Jeff Garzik wrote: > >>Matthew Wilcox wrote: >> >>>On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: >>> >>> >>>>If PCI request regions fails, then someone else is using the >>>>hardware we wish to use. For that one case, calling >>>>pci_disable_device() is rather rude. >>>>See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html >>> >>> >>>Actually, that isn't necessarily true. If the request_regions call fails, >>>that can mean there's a resource conflict. If so, leaving the device >>>enabled is the worst possible thing to do as we'll now have two devices >>>trying to respond to the same io accesses. >> >>Incorrect. If request_region() fails, drivers are coded to _not_ touch >>the hardware. That's the entire purpose of the whole charade: to avoid >>having two devices responding to the same io accesses. >> >>If your driver is talking to the hardware after request_region() fails, >>it is BROKEN plain and simple. > > > I don't think you understood my point. Assume we really do have two > devices in the system with clashing resource settings. Yes, I really > have seen this happen; it's rare. > > While the PCI device is disabled, there is no problem. But then we call > pci_enable_device(), so now the device is enabled to respond to IO and > memory resources in its BARs. > > At the point we discover this, we need to disable the PCI device again > -- exactly the opposite behaviour from your case where we need to leave > the PCI device enabled when we have resource conflicts. > I think the only way to fix this is have pci_enable_device claim the > resources for the BARs before enabling the PCI device to respond to the > resources; that way we leave the enable bits the way they currently are. > No, this doesn't cure the world's ills, but it does obey "First, do > no harm". One way it'll fail is if the other driver loads after the PCI > driver that claims this resource. Ok, I agree with this, it echoes what I said in another message in this thread ;-) namely, * the fact that pci_enable_device() does not claim the resources is a problem. pci_request_regions() should not be a separate step. * the fact that pci_disable_device() does not perform the _exact_ opposite of the operations that pci_enable_device() performed is a problem. pci_disable_device() should not just unconditionally stop the decoder bits, then exit. Jeff ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-14 15:01 [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions Christophe Lucas 2005-02-14 15:10 ` Matthew Wilcox 2005-02-14 15:12 ` Matthew Wilcox @ 2005-02-14 15:50 ` Christophe Lucas 2005-02-14 19:10 ` Greg KH 3 siblings, 0 replies; 12+ messages in thread From: Christophe Lucas @ 2005-02-14 15:50 UTC (permalink / raw) To: kernel-janitors [-- Attachment #1: Type: text/plain, Size: 857 bytes --] Matthew Wilcox (matthew@wil.cx) wrote: > On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > > If PCI request regions fails, then someone else is using the > > hardware we wish to use. For that one case, calling > > pci_disable_device() is rather rude. > > See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html > > Actually, that isn't necessarily true. If the request_regions call fails, > that can mean there's a resource conflict. If so, leaving the device > enabled is the worst possible thing to do as we'll now have two devices > trying to respond to the same io accesses. Ok, if I understand this kj TODO entry is not trusted for now !? Sorry, for CondingStyle, if it could help, I can resend these patch with CodingStyle correction. I stop for now to send patches about this entry. Have a good day. Christophe [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions 2005-02-14 15:01 [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions Christophe Lucas ` (2 preceding siblings ...) 2005-02-14 15:50 ` Christophe Lucas @ 2005-02-14 19:10 ` Greg KH 3 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2005-02-14 19:10 UTC (permalink / raw) To: kernel-janitors [-- Attachment #1: Type: text/plain, Size: 475 bytes --] On Mon, Feb 14, 2005 at 04:01:11PM +0100, Christophe Lucas wrote: > Hi, > > description: > If PCI request regions fails, then someone else is using the > hardware we wish to use. For that one case, calling > pci_disable_device() is rather rude. > See : http://www.ussg.iu.edu/hypermail/linux/kernel/0502.1/1061.html > > Signed-off-by: Christophe Lucas <c.lucas@ifrance.com> Do these devices live on the broken type of devices that are described above? thanks, greg k-h [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2005-02-17 19:35 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2005-02-14 15:01 [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions Christophe Lucas 2005-02-14 15:10 ` Matthew Wilcox 2005-02-14 15:12 ` Matthew Wilcox 2005-02-14 15:12 ` Matthew Wilcox 2005-02-17 18:49 ` Jeff Garzik 2005-02-17 18:49 ` Jeff Garzik 2005-02-17 19:04 ` Matthew Wilcox 2005-02-17 19:04 ` Matthew Wilcox 2005-02-17 19:22 ` Jeff Garzik 2005-02-17 19:22 ` Jeff Garzik 2005-02-14 15:50 ` Christophe Lucas 2005-02-14 19:10 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.