Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()

Yijing Wang
2014-11-18T08:32:56Z

>> +LIST_HEAD(pci_host_bridge_list);
>> +DECLARE_RWSEM(pci_host_bridge_sem);
>
> Unless the pci_host_bridge_sem is accessed thousands of times per second,
> it's normally better to use a simple mutex instead.

OK, I will use simple mutex instead.

>
>> +static struct resource busn_resource = {
>> + .name = "PCI busn",
>> + .start = 0,
>> + .end = 255,
>> + .flags = IORESOURCE_BUS,
>> +};
>
> I think it would be better to require callers to pass the bus resource
> down to the function.

Hmm, I think most of caller will provide the bus resource, but some others
will not give any bus resource, extremely, no any resources :(. But we still
need properly configure their resources for compatibility.

>
>> +struct pci_host_bridge *pci_create_host_bridge(
>> + struct device *parent, u32 db,
>> + struct pci_ops *ops, void *sysdata,
>> + struct list_head *resources)
>> +{
>
> Do we still need to pass the 'sysdata' in here? If we are guaranteed to
> have a device pointer, we should always be able to get the driver
> private data from dev_get_drvdata(host->dev->parent).

We need, some platforms pass NULL pointer as host bridge parent.

>
>> + host = kzalloc(sizeof(*host), GFP_KERNEL);
>> + if (!host)
>> + return NULL;
>
> devm_kzalloc maybe?

I don't know much detail about devm_kzalloc(), but we have no pci host driver
here, and I found no devm_kzalloc() uses in core PCI code before.

>
>> + if (!resources) {
>> + /* Use default IO/MEM/BUS resources*/
>> + pci_add_resource(&host->windows, &ioport_resource);
>> + pci_add_resource(&host->windows, &iomem_resource);
>> + pci_add_resource(&host->windows, &busn_resource);
>> + } else {
>> + list_for_each_entry_safe(window, n, resources, list)
>> + list_move_tail(&window->list, &host->windows);
>> + }
>
> I think we should assume that the correct resources are passed. You
> could add a wrapper around this function to convert old platforms
> though.

OK, I will move these code out of pci_create_host_bridge, and add a wrapper
to setup the default resources.

>
>> +EXPORT_SYMBOL(pci_create_host_bridge);
>
> EXPORT_SYMBOL_GPL() maybe?

OK, will update it.

>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 8b11b38..daa7f40 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
>> struct pci_host_bridge {
>> struct device dev;
>> struct pci_bus *bus; /* root bus */
>> + struct list_head list;
>> struct list_head windows; /* pci_host_bridge_windows */
>> + int busnum;
>
> The busnum should already be implied through the bus resource.

Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks!

Thanks!
Yijing.

>
> Arnd
>
> .
>

--
Thanks!
Yijing