| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <546D57D8.4060708@huawei.com>
Date: Thu, 20 Nov 2014 10:54:16 +0800
From: Yijing Wang <wangyijing@...wei.com>
To: Liviu Dudau <Liviu.Dudau@....com>
CC: Bjorn Helgaas <bhelgaas@...gle.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"huxinwei@...wei.com" <huxinwei@...wei.com>,
Wuyun <wuyun.wu@...wei.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Russell King <linux@....linux.org.uk>,
"x86@...nel.org" <x86@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
Tony Luck <tony.luck@...el.com>,
"linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>,
Thierry Reding <thierry.reding@...il.com>,
Liviu Dudau <liviu@...au.co.uk>,
"suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
Yijing Wang <wangyijing0307@...il.com>
Subject: Re: [RFC PATCH 08/16] PCI: Introduce pci_scan_host_bridge() and pci_host_info
>> No, in this patch, host drivers pass a pci host bridge resources init hook
>> in pci_host_info *info, and we call this info->init_res() in pci_create_host_bridge().
>>
>> +struct pci_host_info {
>> + u8 res_type;
>> + void *arg;
>> + struct list_head *resources; /*just for build, will clean up later */
>> + int (*init_res)(struct pci_host_bridge *host,
>> + struct pci_host_info *info);
>> +};
>> +
>
> That's not what I've asked! Your code does:
>
> if (info->res_type != PCI_HOST_RES_DEFAULT)
> ....
> else /* info->res_type == PCI_HOST_RES_DEFAULT)
> info->res_type = PCI_HOST_RES_DEFAULT;
>
> info->res_type is already == PCI_HOST_RES_DEFAULT in the else side, assignment is a NOP?
Hmmm, I wanted pci_create_host_bridge() to process the default res later(add default res),
It's ugly code, I will rework it.
>
>
>>
>>>
>>>> +
>>>> + return 0;
>>>> +}
>> ...
>>>> @@ -2038,8 +2057,13 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>>>> bool found = false;
>>>> struct pci_host_bridge *host;
>>>> int max;
>>>> + struct pci_host_info info;
>>>> +
>>>> + info.arg = sysdata;
>>>> + info.resources = resources;
>>>> + info.init_res = pci_default_init_res;
>>>
>>> I have mixed feelings about this patch. While it is heading in the right direction
>>> of moving pci_host_bridge relevant information towards the right user, I don't think
>>> you picked up the right set to move. The resource list is going to be copied into
>>> internal pci_host_bridge list anyway, keeping another copy is not helpful *and*
>>> you have increased the code size.
>>>
>>> I think for now we should aim to get the *missing* data into pci_host_bridge: MSI
>>> controllers and PCI domain/segment. Then we can do more cleanup.
>>
>> Hi Liviu, I agree with you here, the changes to resources stuff seems not a perfect
>> solution. In my patch 6, we could pass pci domain nr by u32 PCI_DOMBUS(domain, bus) argument,
>> and store it in pci_host_bridge. For msi controller, we couldn't save the msi_controller
>> in pci_host_bridge. Before we assume one pci host bridge only had one msi_controller,
>> but now something changes, Jiang introduce hierarchy irq domain in x86, and now
>> one pci host bridge may has more than one msi_controller. So I prefer to add a
>> function to pci_host_bridge something like
>>
>> struct msi_controller *pci_get_msi_controller(struct pci_dev *dev)
>
> Yes, good idea.
>
>>
>>>
>>>>
>>>> - host = pci_create_host_bridge(parent, db, ops, sysdata, resources);
>>>> + host = pci_create_host_bridge(parent, db, ops, &info);
>>>> if (!host)
>>>> return NULL;
>>>>
>>>> @@ -2070,6 +2094,47 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>>>> }
>>>> EXPORT_SYMBOL(pci_scan_root_bus);
>>>>
>>>> +struct pci_host_bridge *pci_scan_host_bridge(
>>>> + struct device *parent, u32 db, struct pci_ops *ops,
>>>> + struct pci_host_info *info)
>>>> +{
>>>> + struct pci_host_bridge_window *window;
>>>> + bool found = false;
>>>> + struct pci_host_bridge *host;
>>>> + int max;
>>>> +
>>>> + host = pci_create_host_bridge(parent, db, ops, info);
>>>> + if (!host)
>>>> + return NULL;
>>>> +
>>>> + list_for_each_entry(window, &host->windows, list)
>>>> + if (window->res->flags & IORESOURCE_BUS) {
>>>> + found = true;
>>>> + break;
>>>> + }
>>>> +
>>>> + host->bus = __pci_create_root_bus(host);
>>>> + if (!host->bus) {
>>>> + pci_free_host_bridge(host);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + if (!found) {
>>>> + dev_info(&host->bus->dev,
>>>> + "No busn resource found for root bus, will use [bus %02x-ff]\n",
>>>> + host->busnum);
>>>> + pci_bus_insert_busn_res(host->bus, host->busnum, 255);
>>>> + }
>>>> +
>>>> + max = pci_scan_child_bus(host->bus);
>>>> + if (!found)
>>>> + pci_bus_update_busn_res_end(host->bus, max);
>>>> +
>>>> + return host;
>>>> +
>>>> +}
>>>> +EXPORT_SYMBOL(pci_scan_host_bridge);
>>>> +
>>>> /**
>>>> * pci_rescan_bus_bridge_resize - scan a PCI bus for devices.
>>>> * @bridge: PCI bridge for the bus to scan
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index daa7f40..a51f5f5 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -412,6 +412,21 @@ struct pci_host_bridge {
>>>> void *release_data;
>>>> };
>>>>
>>>> +struct pci_host_info {
>>>> + u8 res_type;
>>>> + void *arg;
>>>> + struct list_head *resources; /*just for build, will clean up later */
>>>> + int (*init_res)(struct pci_host_bridge *host,
>>>> + struct pci_host_info *info);
>>>> +};
>>>> +
>>>> +static inline void init_pci_host_info(struct pci_host_info *info)
>>>> +{
>>>> + memset(info, 0 , sizeof(*info));
>>>> +}
>>>
>>> Where is this used?
>>
>> Host driver uses it to init pci_host_info.
>
> Might be worth adding it that patch rather than here.
OK
>
> Best regards,
> Liviu
>
>>
>>>
>>>> +
>>>> +#define PCI_HOST_RES_DEFAULT 0x2
>>>> +
>>>
>>> Magic number?
>>
>> Hmmm, I will rework pci host bridge resources stuff.
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>> #define to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
>>>> void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>>> void (*release_fn)(struct pci_host_bridge *),
>>>> @@ -420,7 +435,7 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
>>>> struct pci_host_bridge *pci_create_host_bridge(
>>>> struct device *parent, u32 db, struct pci_ops *ops,
>>>> - void *sys, struct list_head *resources);
>>>> + struct pci_host_info *info);
>>>> /*
>>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for
>>>> @@ -785,6 +800,9 @@ void pci_bus_release_busn_res(struct pci_bus *b);
>>>> struct pci_bus *pci_scan_root_bus(struct device *parent, u32 bus,
>>>> struct pci_ops *ops, void *sysdata,
>>>> struct list_head *resources);
>>>> +struct pci_host_bridge *pci_scan_host_bridge(struct device *parent,
>>>> + u32 db, struct pci_ops *ops,
>>>> + struct pci_host_info *info);
>>>> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
>>>> int busnr);
>>>> void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
>>>> --
>>>> 1.7.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>> the body of a message to majordomo@...r.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists