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

[Jeff Garzik <jgarzik@pobox.com>]

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