Re: [RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
On Wed, Dec 26, 2007 at 08:26:27AM +1100, Benjamin Herrenschmidt wrote: > > > This is exactly what's supposed to be happening, but the code is buggy > > and nobody noticed :-) (I'm mixing up IORESOURCE_* flags and > > PCI_COMMAND_* flags). Thanks for reviewing ! > > Note that this patch isn't in the series Greg queued up anyway. The > powerpc specific bits will be going in via Paulus an I already asked him > to drop that specific one until I send a fixed version. Ah okbut I was mostly worried about the use of PPC bits as an example. Anyway, in general it looks like the right direction to me too. cheers, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
> This is exactly what's supposed to be happening, but the code is buggy > and nobody noticed :-) (I'm mixing up IORESOURCE_* flags and > PCI_COMMAND_* flags). Thanks for reviewing ! Note that this patch isn't in the series Greg queued up anyway. The powerpc specific bits will be going in via Paulus an I already asked him to drop that specific one until I send a fixed version. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
On Mon, 2007-12-24 at 00:23 -0700, Grant Grundler wrote:
> On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote:
> > This patch changes the PowerPC PCI code to disable IO and/or Memory
> > decoding on a PCI device when a resource of that type failed to be
> > allocated. This is done to avoid having unallocated dangling BARs enabled
> > that might try to decode on top of other devices.
> >
> > If a proper resource is assigned later on, then pci_enable_device{,_io,_mem}
> > will take care of re-enabling decoding.
> >
> > Signed-off-by: Benjamin Herrenschmidt <[EMAIL PROTECTED]>
>
> > @@ -1062,8 +1065,12 @@ static void __init pcibios_allocate_reso
> > disabled = !(command & PCI_COMMAND_IO);
> > else
> > disabled = !(command & PCI_COMMAND_MEMORY);
> > - if (pass == disabled)
> > - alloc_resource(dev, idx);
> > + if (pass == disabled && alloc_resource(dev, idx)) {
> > + command &= ~(r->flags & (IORESOURCE_IO |
> > +IORESOURCE_MEM));
>
> While this may be ok for PPC, in general, wouldn't we want to only disable
> which ever type of resource that couldn't be allocated?
This is exactly what's supposed to be happening, but the code is buggy
and nobody noticed :-) (I'm mixing up IORESOURCE_* flags and
PCI_COMMAND_* flags). Thanks for reviewing !
> ie make two calls: alloc_resource_io() and alloc_resource_mem() and disable
> the respective flag if the alloc call fails?
No need for 2 calls, just disable whatever type the resource is, but
using the right bits instead of what my code incorrectly does.
> Thus a device which was enable and programmed by BIOS could remain functional
> despite one resource not being allocated.
Yes, that's what intended by the above code, if I didn't have mixed up
the flags.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote:
> This patch changes the PowerPC PCI code to disable IO and/or Memory
> decoding on a PCI device when a resource of that type failed to be
> allocated. This is done to avoid having unallocated dangling BARs enabled
> that might try to decode on top of other devices.
>
> If a proper resource is assigned later on, then pci_enable_device{,_io,_mem}
> will take care of re-enabling decoding.
>
> Signed-off-by: Benjamin Herrenschmidt <[EMAIL PROTECTED]>
> @@ -1062,8 +1065,12 @@ static void __init pcibios_allocate_reso
> disabled = !(command & PCI_COMMAND_IO);
> else
> disabled = !(command & PCI_COMMAND_MEMORY);
> - if (pass == disabled)
> - alloc_resource(dev, idx);
> + if (pass == disabled && alloc_resource(dev, idx)) {
> + command &= ~(r->flags & (IORESOURCE_IO |
> + IORESOURCE_MEM));
While this may be ok for PPC, in general, wouldn't we want to only disable
which ever type of resource that couldn't be allocated?
ie make two calls: alloc_resource_io() and alloc_resource_mem() and disable
the respective flag if the alloc call fails?
Thus a device which was enable and programmed by BIOS could remain functional
despite one resource not being allocated.
(e.g. MMIO was allocated successfully while IO Port space was not)
Again, this is just a hypothetical question since I have no example (yet)
off hand where this is true. ISTR, the original discussion around
pci_enable_device_bars() suggested some machines already have this issue.
cheers,
grant
> + pci_write_config_word(dev,
> + PCI_COMMAND, command);
> + }
> }
> if (pass)
> continue;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
On Tue, 2007-12-18 at 12:56 +0300, Ivan Kokshaysky wrote: > On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote: > > @@ -1040,7 +1040,10 @@ static inline void __devinit alloc_resou > > r->flags |= IORESOURCE_UNSET; > > r->end -= r->start; > > r->start = 0; > > Perhaps we should use IORESOURCE_UNSET universally... It's a lot better > than clearing r->start which is in fact architecture dependent thing > and in the end just destroys information for no purpose. I'm not totally sure. I was actually tempted to switch powerpc to get rid of it lately :-) The problem is that a resource is never "unset" on PCI... the BAR always contains something... the question is whether that something is going to be problematic or not, or rather, whether that something can be "allocated" (ie. fitted into the resource tree in a free spot or not). And we have a way of knowing that already which is ... resource->parent being NULL or not. That also happens to be what pci_assign_unassigned_resources() uses and I think the various arch pcibios_enable_device() should use (in fact I have a fix to make PowerPC use that) instead of testing res->start or even IORESOURCE_UNSET. Anything that isn't in the resource tree is potentially stale and thus mustn't be enabled (ie. IO/MEM decoding must not be enabled on a device that has a BAR whose res->parent is NULL). The reason why we introduced use of IORESOURCE_UNSET on powerpc goes back from when we had our own resource assignment code there that was somewhat a half-assed version of what pci_assign_unassigned_resources(). We decided to introduce IORESOURCE_UNSET rather than testing res->start because it could cope with a value of 0 being valid, and on machines with no legacy ISA stuff at all, 0 is a valid BAR value for some things, and we weren't smart enough to reassign those things in all circumstances. Nowadays, that doesn't seem necessary anymore, and in fact, I think we can still support those valid 0 BARs -and- not have IORESOURCE_UNSET by standardizing on the idea that res->parent is the only indicator of a resource validity... unless I'm missing something which is quite possible :-) Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote: > @@ -1040,7 +1040,10 @@ static inline void __devinit alloc_resou > r->flags |= IORESOURCE_UNSET; > r->end -= r->start; > r->start = 0; Perhaps we should use IORESOURCE_UNSET universally... It's a lot better than clearing r->start which is in fact architecture dependent thing and in the end just destroys information for no purpose. Ivan. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC/PATCH 4/4] [POWERPC] pci: Disable IO/Mem on a device when resources can't be allocated
This patch changes the PowerPC PCI code to disable IO and/or Memory
decoding on a PCI device when a resource of that type failed to be
allocated. This is done to avoid having unallocated dangling BARs enabled
that might try to decode on top of other devices.
If a proper resource is assigned later on, then pci_enable_device{,_io,_mem}
will take care of re-enabling decoding.
Signed-off-by: Benjamin Herrenschmidt <[EMAIL PROTECTED]>
---
NOTE: This patch doesn't apply on current upstream, but rather on top
of a serie that merges 32 and 64 bits PowerPC PCI resource handling, and
which will be in 2.6.25. I post it here mostly to show what I think should
be done. A similar patch will have to be done for other architectures.
arch/powerpc/kernel/pci-common.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
--- linux-work.orig/arch/powerpc/kernel/pci-common.c2007-12-18
09:37:52.0 +1100
+++ linux-work/arch/powerpc/kernel/pci-common.c 2007-12-18 09:39:27.0
+1100
@@ -1016,7 +1016,7 @@ static void __init pcibios_allocate_bus_
}
}
-static inline void __devinit alloc_resource(struct pci_dev *dev, int idx)
+static inline int __devinit alloc_resource(struct pci_dev *dev, int idx)
{
struct resource *pr, *r = &dev->resource[idx];
@@ -1040,7 +1040,10 @@ static inline void __devinit alloc_resou
r->flags |= IORESOURCE_UNSET;
r->end -= r->start;
r->start = 0;
+
+ return -EBUSY;
}
+ return 0;
}
static void __init pcibios_allocate_resources(int pass)
@@ -1062,8 +1065,12 @@ static void __init pcibios_allocate_reso
disabled = !(command & PCI_COMMAND_IO);
else
disabled = !(command & PCI_COMMAND_MEMORY);
- if (pass == disabled)
- alloc_resource(dev, idx);
+ if (pass == disabled && alloc_resource(dev, idx)) {
+ command &= ~(r->flags & (IORESOURCE_IO |
+IORESOURCE_MEM));
+ pci_write_config_word(dev,
+ PCI_COMMAND, command);
+ }
}
if (pass)
continue;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
