Patchwork [00/29] PCI: use pci host bridge to loop pci root bus

login
register
mail settings
Submitter Bjorn Helgaas
Date Sept. 25, 2012, 5:37 p.m.
Message ID <CAErSpo6NOhTV0HX1dw5-mRkUtfO7XH_iCGVwiddBPpQvPZL=7g@mail.gmail.com>
Download mbox | patch
Permalink /patch/186871/
State Changes Requested
Headers show

Comments

Bjorn Helgaas - Sept. 25, 2012, 5:37 p.m.
On Tue, Sep 25, 2012 at 10:29 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Sep 25, 2012 at 9:23 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Sep 25, 2012 at 2:26 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> could remove pci_root_buses list.
>>>
> ...
>>>   PCI: Add dummy bus_type for pci_host_bridge
> ...
>>>   PCI: Add for_each_pci_host_bridge() and pci_get_next_host_bridge
>>
>> I'm not thrilled about adding a new iterator for all host bridges.
>>
>> The iterator design pattern does not work for collections that can
>> change over time.  In this case, it looks like you're adding a safer
>> way to iterate through all host bridges we know about at this time.
>> But we still have the problem of the host bridge that's added
>> tomorrow.
>>
>> I'd prefer a design where the PCI core provides an interface that
>> means "call this function for every host bridge we know about now
>> *and* for every one that's added in the future."
>
> yes, that is the point to add pci_root_bridge_bus_type. We can register
> bus notifier on that.

I guess I missed your point.  In the patch below (20/29 from your
series), you're still iterating through all the host bridges, so there
would have to be something else to handle hot-added host bridges.

Are you saying you plan future patches to change this again to
something using a bus notifier?


                        node = pbus->self->dev.of_node;
--
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
Yinghai Lu - Sept. 25, 2012, 6:06 p.m.
On Tue, Sep 25, 2012 at 10:37 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Sep 25, 2012 at 10:29 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>
>>> I'd prefer a design where the PCI core provides an interface that
>>> means "call this function for every host bridge we know about now
>>> *and* for every one that's added in the future."
>>
>> yes, that is the point to add pci_root_bridge_bus_type. We can register
>> bus notifier on that.
>
> I guess I missed your point.  In the patch below (20/29 from your
> series), you're still iterating through all the host bridges, so there
> would have to be something else to handle hot-added host bridges.
>
> Are you saying you plan future patches to change this again to
> something using a bus notifier?
>
> --- a/arch/sparc/kernel/pci.c
> +++ b/arch/sparc/kernel/pci.c
> @@ -997,11 +997,13 @@ static void __devinit pci_bus_slot_names(struct
> device_node *node,
>
>  static int __init of_pci_slot_init(void)
>  {
> -       struct pci_bus *pbus = NULL;
> +       struct pci_host_bridge *host_bridge = NULL;
> +       struct pci_bus *pbus;
>
> -       while ((pbus = pci_find_next_bus(pbus)) != NULL) {
> +       for_each_pci_host_bridge(host_bridge) {
>                 struct device_node *node;
>
> +               pbus = hot_bridge->bus;
>                 if (pbus->self) {
>                         /* PCI->PCI bridge */
>                         node = pbus->self->dev.of_node;

that is for initial booting path.

for hot add/remove notifier add need to be done case by case.

-Yinghai
--
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
Bjorn Helgaas - Sept. 25, 2012, 6:19 p.m.
On Tue, Sep 25, 2012 at 12:06 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Sep 25, 2012 at 10:37 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Sep 25, 2012 at 10:29 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>>
>>>> I'd prefer a design where the PCI core provides an interface that
>>>> means "call this function for every host bridge we know about now
>>>> *and* for every one that's added in the future."
>>>
>>> yes, that is the point to add pci_root_bridge_bus_type. We can register
>>> bus notifier on that.
>>
>> I guess I missed your point.  In the patch below (20/29 from your
>> series), you're still iterating through all the host bridges, so there
>> would have to be something else to handle hot-added host bridges.
>>
>> Are you saying you plan future patches to change this again to
>> something using a bus notifier?
>>
>> --- a/arch/sparc/kernel/pci.c
>> +++ b/arch/sparc/kernel/pci.c
>> @@ -997,11 +997,13 @@ static void __devinit pci_bus_slot_names(struct
>> device_node *node,
>>
>>  static int __init of_pci_slot_init(void)
>>  {
>> -       struct pci_bus *pbus = NULL;
>> +       struct pci_host_bridge *host_bridge = NULL;
>> +       struct pci_bus *pbus;
>>
>> -       while ((pbus = pci_find_next_bus(pbus)) != NULL) {
>> +       for_each_pci_host_bridge(host_bridge) {
>>                 struct device_node *node;
>>
>> +               pbus = hot_bridge->bus;
>>                 if (pbus->self) {
>>                         /* PCI->PCI bridge */
>>                         node = pbus->self->dev.of_node;
>
> that is for initial booting path.
>
> for hot add/remove notifier add need to be done case by case.

Can initial boot be done the same as hot-add?  If we add interfaces
like for_each_pci_host_bridge(), people will just copy that for use at
run-time.  So it would be better to have the same interfaces for use
at boot-time and at hot add-time.
--
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
Yinghai Lu - Sept. 25, 2012, 7:17 p.m.
On Tue, Sep 25, 2012 at 11:19 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Sep 25, 2012 at 12:06 PM, Yinghai Lu <yinghai@kernel.org> wrote:

>> that is for initial booting path.
>>
>> for hot add/remove notifier add need to be done case by case.
>
> Can initial boot be done the same as hot-add?  If we add interfaces
> like for_each_pci_host_bridge(), people will just copy that for use at
> run-time.  So it would be better to have the same interfaces for use
> at boot-time and at hot add-time.

still need to check them case by case. some function may be too early
to be called in work_fn in notifier. that need to find out when to get
that bus notifier get
register.

I have one draft patch that will delay bridge enabling to BUS_ADD for
pci bridge...
that will need to get that register rather later otherwise that bridge
can not be enabled because resources are not reserved/allocated for
initial booting path.
please refer to the attached patch. you should notice that fs_initcall
is used for registration until boot path already have that pci bridges
before.

+static struct notifier_block pci_hp_nb = {
+	.notifier_call = &pci_hp_notifier,
+};
+
+static int __init pci_hp_init(void)
+{
+	return bus_register_notifier(&pci_bus_type, &pci_hp_nb);
+}
+
+fs_initcall(pci_hp_init);

Also using that for_each_pci_host_bridge in run-time is still safe.

-Yinghai
Bjorn Helgaas - Sept. 25, 2012, 7:45 p.m.
On Tue, Sep 25, 2012 at 1:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Sep 25, 2012 at 11:19 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Sep 25, 2012 at 12:06 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
>>> that is for initial booting path.
>>>
>>> for hot add/remove notifier add need to be done case by case.
>>
>> Can initial boot be done the same as hot-add?  If we add interfaces
>> like for_each_pci_host_bridge(), people will just copy that for use at
>> run-time.  So it would be better to have the same interfaces for use
>> at boot-time and at hot add-time.
>
> still need to check them case by case. some function may be too early
> to be called in work_fn in notifier. that need to find out when to get
> that bus notifier get
> register.
>
> I have one draft patch that will delay bridge enabling to BUS_ADD for
> pci bridge...
> that will need to get that register rather later otherwise that bridge
> can not be enabled because resources are not reserved/allocated for
> initial booting path.
> please refer to the attached patch. you should notice that fs_initcall
> is used for registration until boot path already have that pci bridges
> before.
>
> +static struct notifier_block pci_hp_nb = {
> +       .notifier_call = &pci_hp_notifier,
> +};
> +
> +static int __init pci_hp_init(void)
> +{
> +       return bus_register_notifier(&pci_bus_type, &pci_hp_nb);
> +}
> +
> +fs_initcall(pci_hp_init);
>
> Also using that for_each_pci_host_bridge in run-time is still safe.

Sure, it might be *safe*.  But it's not *sufficient*.  If you use
for_each_pci_host_bridge(), you also need to do something else to deal
with hot-added host bridges.  It's hard to make sure that every caller
does both parts correctly:

  1) for_each_pci_host_bridge() for things we've already seen and
  2) some sort of notifier for hot-added bridges

That's why I'd prefer a single interface.
--
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
Yinghai Lu - Sept. 25, 2012, 7:53 p.m.
On Tue, Sep 25, 2012 at 12:45 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> Sure, it might be *safe*.  But it's not *sufficient*.  If you use
> for_each_pci_host_bridge(), you also need to do something else to deal
> with hot-added host bridges.  It's hard to make sure that every caller
> does both parts correctly:
>
>   1) for_each_pci_host_bridge() for things we've already seen and
>   2) some sort of notifier for hot-added bridges

yes.

>
> That's why I'd prefer a single interface.

that will have draw backs too:
1. too much changes to the code of current caller.
2. have to checking system_state back and forth.
3. some variable function call may only need for _init path could not be freed.
    or add annoying _ref_ok etc.

-Yinghai
--
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
Bjorn Helgaas - Sept. 25, 2012, 8:06 p.m.
On Tue, Sep 25, 2012 at 1:53 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Sep 25, 2012 at 12:45 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> Sure, it might be *safe*.  But it's not *sufficient*.  If you use
>> for_each_pci_host_bridge(), you also need to do something else to deal
>> with hot-added host bridges.  It's hard to make sure that every caller
>> does both parts correctly:
>>
>>   1) for_each_pci_host_bridge() for things we've already seen and
>>   2) some sort of notifier for hot-added bridges
>
> yes.
>
>>
>> That's why I'd prefer a single interface.
>
> that will have draw backs too:
> 1. too much changes to the code of current caller.
> 2. have to checking system_state back and forth.
> 3. some variable function call may only need for _init path could not be freed.
>     or add annoying _ref_ok etc.

We already have to make significant changes to the callers.  Your
changes only address part 1.  More changes will be needed for part 2,
and they will look very much like the approach I'm suggesting.

I'm frankly dubious about your fears of system_state and __init
complexity.  PCI enumeration and driver binding already happens late
enough that the system is almost completely initialized.  But I guess
we won't know for sure until we try out  both ideas.
--
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
Yinghai Lu - Sept. 25, 2012, 10:44 p.m.
On Tue, Sep 25, 2012 at 1:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Sep 25, 2012 at 1:53 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Sep 25, 2012 at 12:45 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> Sure, it might be *safe*.  But it's not *sufficient*.  If you use
>>> for_each_pci_host_bridge(), you also need to do something else to deal
>>> with hot-added host bridges.  It's hard to make sure that every caller
>>> does both parts correctly:
>>>
>>>   1) for_each_pci_host_bridge() for things we've already seen and
>>>   2) some sort of notifier for hot-added bridges
>>
>> yes.
>>
>>>
>>> That's why I'd prefer a single interface.
>>
>> that will have draw backs too:
>> 1. too much changes to the code of current caller.
>> 2. have to checking system_state back and forth.
>> 3. some variable function call may only need for _init path could not be freed.
>>     or add annoying _ref_ok etc.
>
> We already have to make significant changes to the callers.  Your
> changes only address part 1.  More changes will be needed for part 2,
> and they will look very much like the approach I'm suggesting.

not that much, We just update the loop function from the caller.

>
> I'm frankly dubious about your fears of system_state and __init
> complexity.  PCI enumeration and driver binding already happens late
> enough that the system is almost completely initialized.  But I guess
> we won't know for sure until we try out  both ideas.

it is all up to the caller to decide it initial path for_ loop is
enough, or they still need
part 2, or later they could even remove part1 and only keep part2 if
the those function can survive the initial boot path.

-Yinghai
--
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

Patch

--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -997,11 +997,13 @@  static void __devinit pci_bus_slot_names(struct
device_node *node,

 static int __init of_pci_slot_init(void)
 {
-       struct pci_bus *pbus = NULL;
+       struct pci_host_bridge *host_bridge = NULL;
+       struct pci_bus *pbus;

-       while ((pbus = pci_find_next_bus(pbus)) != NULL) {
+       for_each_pci_host_bridge(host_bridge) {
                struct device_node *node;

+               pbus = hot_bridge->bus;
                if (pbus->self) {
                        /* PCI->PCI bridge */