| Message ID | 20090402155812.5C9A.27C06F64@necst.nec.co.jp (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hello Yuji-san, * Yuji Shimada <shimada-yxb@necst.nec.co.jp>: > This patch setups disabled bridges even if buses have already been > added. > > pci_assign_unassigned_resources is called after buses are added. > pci_assign_unassigned_resources calls pci_bus_assign_resources. > pci_bus_assign_resources calls pci_setup_bridge to configure BARs of > bridges. > > Currently pci_setup_bridge returns immediately if the bus have already > been added. So pci_assign_unassigned_resources can't configure BARs of > bridge. Ok, I agree with your analysis. > The patch fixes the issue. > > On logical hot-add, we need to prevent the kernel from re-initializing > bridges that have already been initialized. To achieve this, > pci_setup_bridge returns immediately if the bridge have already been > enabled. > > Thanks, > -- > Yuji Shimada > > > Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp> > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 334285a..ab27831 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -144,7 +144,7 @@ static void pci_setup_bridge(struct pci_bus *bus) > struct pci_bus_region region; > u32 l, bu, lu, io_upper16; > > - if (!pci_is_root_bus(bus) && bus->is_added) > + if (bus->self == NULL || atomic_read(&bus->self->enable_cnt) != 0) Doesn't this mean that we will return early on a non-materialized root bus (aka, a bus without PCI config space)? There are definitely some platforms like that. I think you want: if (!pci_is_root_bus(bus) && atomic_read(&bus->self->enable_cnt)) which says: - always allow pci_setup_bridge on a root bus (which cannot be logically hot-plugged, so don't have to worry about multiple-initializations) - on a p2p bridge, return early if already enabled A non-root bus will always have a valid ->self, so that pointer dereference should be safe. Right? Thanks. /ac > return; > > dev_info(&bridge->dev, "PCI bridge, secondary bus %04x:%02x\n", > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2 Apr 2009 15:51:19 -0600 Alex Chiang <achiang@hp.com> wrote: > Hello Yuji-san, > > * Yuji Shimada <shimada-yxb@necst.nec.co.jp>: > > This patch setups disabled bridges even if buses have already been > > added. > > > > pci_assign_unassigned_resources is called after buses are added. > > pci_assign_unassigned_resources calls pci_bus_assign_resources. > > pci_bus_assign_resources calls pci_setup_bridge to configure BARs of > > bridges. > > > > Currently pci_setup_bridge returns immediately if the bus have already > > been added. So pci_assign_unassigned_resources can't configure BARs of > > bridge. > > Ok, I agree with your analysis. > > > The patch fixes the issue. > > > > On logical hot-add, we need to prevent the kernel from re-initializing > > bridges that have already been initialized. To achieve this, > > pci_setup_bridge returns immediately if the bridge have already been > > enabled. > > > > Thanks, > > -- > > Yuji Shimada > > > > > > Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp> > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index 334285a..ab27831 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -144,7 +144,7 @@ static void pci_setup_bridge(struct pci_bus *bus) > > struct pci_bus_region region; > > u32 l, bu, lu, io_upper16; > > > > - if (!pci_is_root_bus(bus) && bus->is_added) > > + if (bus->self == NULL || atomic_read(&bus->self->enable_cnt) != 0) > > Doesn't this mean that we will return early on a non-materialized > root bus (aka, a bus without PCI config space)? There are > definitely some platforms like that. Yes, we will return. pci_setup_bridge can't setup a root bus, because a root bus does not have a bridge(bus->self). I find pci_setup_bridge is not called on a root bus. if pci_setup_bridge is called on a root bus pci_setup_bridge will access a NULL pointer. So "bus->self == NULL" is not needed. > I think you want: > > if (!pci_is_root_bus(bus) && atomic_read(&bus->self->enable_cnt)) > > which says: > - always allow pci_setup_bridge on a root bus (which > cannot be logically hot-plugged, so don't have to worry > about multiple-initializations) I think pci_setup_bridge should return early on a root bus, because pci_setup_bridge can't setup a root bus. But we don't need to check whether a bus is a root bus or not as mentioned above. > - on a p2p bridge, return early if already enabled > > A non-root bus will always have a valid ->self, so that pointer > dereference should be safe. > > Right? Yes. pci_setup_bridge is not called on a root bus, so we can assume ->self is a valid pointer. Thanks, -- Yuji Shimada -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 334285a..ab27831 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -144,7 +144,7 @@ static void pci_setup_bridge(struct pci_bus *bus) struct pci_bus_region region; u32 l, bu, lu, io_upper16; - if (!pci_is_root_bus(bus) && bus->is_added) + if (bus->self == NULL || atomic_read(&bus->self->enable_cnt) != 0) return; dev_info(&bridge->dev, "PCI bridge, secondary bus %04x:%02x\n",
This patch setups disabled bridges even if buses have already been added. pci_assign_unassigned_resources is called after buses are added. pci_assign_unassigned_resources calls pci_bus_assign_resources. pci_bus_assign_resources calls pci_setup_bridge to configure BARs of bridges. Currently pci_setup_bridge returns immediately if the bus have already been added. So pci_assign_unassigned_resources can't configure BARs of bridge. The patch fixes the issue. On logical hot-add, we need to prevent the kernel from re-initializing bridges that have already been initialized. To achieve this, pci_setup_bridge returns immediately if the bridge have already been enabled. Thanks, -- Yuji Shimada Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html