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

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

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

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]

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