[KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Christophe Lucas]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Matthew Wilcox]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Matthew Wilcox]

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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Christophe Lucas]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Greg KH]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Jeff Garzik]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Jeff Garzik]

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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Matthew Wilcox]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Matthew Wilcox]

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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Jeff Garzik]

[-- 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

Re: [KJ] [PATCH] drivers/char/watchdog/* : pci_request_regions

[Jeff Garzik]

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