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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/