From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754286AbbCLMaH (ORCPT ); Thu, 12 Mar 2015 08:30:07 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:18458 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932261AbbCLM3z (ORCPT ); Thu, 12 Mar 2015 08:29:55 -0400 Message-ID: <55018330.5050002@huawei.com> Date: Thu, 12 Mar 2015 20:14:40 +0800 From: Yijing Wang User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.0.1 MIME-Version: 1.0 To: Bjorn Helgaas CC: Jiang Liu , , Yinghai Lu , , Marc Zyngier , , Russell King , , Thomas Gleixner , Benjamin Herrenschmidt , Rusty Russell , Tony Luck , , "David S. Miller" , "Guan Xuetao" , , , Liviu Dudau , "Arnd Bergmann" , Geert Uytterhoeven Subject: Re: [PATCH v6 06/30] PCI: Combine PCI domain and bus number in u32 arg References: <1425868467-9667-1-git-send-email-wangyijing@huawei.com> <1425868467-9667-7-git-send-email-wangyijing@huawei.com> <20150312012946.GA10949@google.com> In-Reply-To: <20150312012946.GA10949@google.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.27.212] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown, refid=x_failed, ip=unknown, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32, mode=multiengine X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A0C0203.550186B2.015B,ss=1,re=0.001,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: a9546abd265d07bb149a4dd67b2c9f49 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015/3/12 9:29, Bjorn Helgaas wrote: > On Mon, Mar 09, 2015 at 10:34:03AM +0800, Yijing Wang wrote: >> Currently, we use int type for bus number in >> pci_create_root_bus(), pci_scan_root_bus() and >> pci_scan_bus_legacy. Because PCI bus number >> always <= 255, so we could change the bus number >> argument type to u32, and combine PCI domain and >> bus number in one. > > This makes no sense. Or rather, it only states the obvious: an 8-bit value > and a 16-bit value will both fit in a 32-bit value. But it doesn't say > *why* you think it's a good idea to pass a single value that contains both > domain and bus numbers. The obvious thing is to pass two separate values, > and you don't say why passing a single combined value is better. Sorry for my poor description for this patch, I combined the domain and bus, because I think now we have too many args at pci_scan_root_bus() or other scan functions, struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources) Now we have five args yet, plus the new introduced domain and pci_host_bridge_ops, it will become 7. I thought introduced a new structure which contain the necessary info to scan root bus/ host bridge, E.g struct pci_scan_info { int bus; struct device *parent; struct pci_ops *ops; void *sysdata; struct list_head *resource; int domain; struct pci_host_bridge_ops; } Do you like this one or keep it like now ? pci_scan_root_bus(struct device *parent, int domain, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources, struct pci_host_bridge_ops *ops) > > Also see whitespace mistakes below. I get a little testy when people don't > follow the whitespace convention of neighboring lines because if people are > sloppy about whitespace I assume they're sloppy about the code. > Sorry, I would be more careful. >> int error; >> @@ -1901,6 +1901,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> resource_size_t offset; >> char bus_addr[64]; >> char *fmt; >> + u8 bus = PCI_BUSNUM(db); > > Whitespace botch. > >> >> b = pci_alloc_bus(NULL); >> if (!b) >> @@ -1921,6 +1922,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> if (!bridge) >> goto err_out; >> >> + bridge->domain = PCI_DOMAIN(db); >> bridge->dev.parent = parent; >> bridge->dev.release = pci_release_host_bridge_dev; >> dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); >> @@ -2058,7 +2060,7 @@ void pci_bus_release_busn_res(struct pci_bus *b) >> res, ret ? "can not be" : "is"); >> } >> >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >> struct pci_ops *ops, void *sysdata, struct list_head *resources) >> { >> struct resource_entry *window; >> @@ -2072,15 +2074,15 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> break; >> } >> >> - b = pci_create_root_bus(parent, bus, ops, sysdata, resources); >> + b = pci_create_root_bus(parent, db, ops, sysdata, resources); >> if (!b) >> return NULL; >> >> if (!found) { >> dev_info(&b->dev, >> "No busn resource found for root bus, will use [bus %02x-ff]\n", >> - bus); >> - pci_bus_insert_busn_res(b, bus, 255); >> + PCI_BUSNUM(db)); >> + pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >> } >> >> max = pci_scan_child_bus(b); >> @@ -2092,7 +2094,7 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> } >> EXPORT_SYMBOL(pci_scan_root_bus); >> >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> +struct pci_bus *pci_scan_bus(u32 db, struct pci_ops *ops, >> void *sysdata) >> { >> LIST_HEAD(resources); >> @@ -2101,7 +2103,7 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> pci_add_resource(&resources, &ioport_resource); >> pci_add_resource(&resources, &iomem_resource); >> pci_add_resource(&resources, &busn_resource); >> - b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources); >> + b = pci_create_root_bus(NULL, db, ops, sysdata, &resources); >> if (b) { >> pci_scan_child_bus(b); >> } else { >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 36effb8..8b3d245 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -401,6 +401,7 @@ static inline int pci_channel_offline(struct pci_dev *pdev) >> } >> >> struct pci_host_bridge { >> + u16 domain; > > Please follow the existing whitespace convention, i.e., use a space, not a > tab, between "u16 domain". > >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> struct list_head windows; /* resource_entry */ >> @@ -769,14 +770,14 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res, >> void pcibios_scan_specific_bus(int busn); >> struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); >> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_bus(u32 dombus, struct pci_ops *ops, void *sysdata); >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); >> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); >> void pci_bus_release_busn_res(struct pci_bus *b); >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h >> index 3c292bc..89c6f15 100644 >> --- a/include/uapi/linux/pci.h >> +++ b/include/uapi/linux/pci.h >> @@ -30,6 +30,9 @@ >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) >> #define PCI_FUNC(devfn) ((devfn) & 0x07) >> +#define PCI_DOMAIN(db) ((db) >> 16) >> +#define PCI_BUSNUM(db) ((db) & 0xffff) >> +#define PCI_DOMBUS(domain, bus) (((domain) << 16) | (bus)) >> >> /* Ioctls for /proc/bus/pci/X/Y nodes. */ >> #define PCIIOC_BASE ('P' << 24 | 'C' << 16 | 'I' << 8) >> -- >> 1.7.1 >> >> -- >> 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 > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <55018330.5050002@huawei.com> Date: Thu, 12 Mar 2015 20:14:40 +0800 From: Yijing Wang MIME-Version: 1.0 To: Bjorn Helgaas CC: Jiang Liu , , Yinghai Lu , , Marc Zyngier , , Russell King , , Thomas Gleixner , Benjamin Herrenschmidt , Rusty Russell , Tony Luck , , "David S. Miller" , "Guan Xuetao" , , , Liviu Dudau , "Arnd Bergmann" , Geert Uytterhoeven Subject: Re: [PATCH v6 06/30] PCI: Combine PCI domain and bus number in u32 arg References: <1425868467-9667-1-git-send-email-wangyijing@huawei.com> <1425868467-9667-7-git-send-email-wangyijing@huawei.com> <20150312012946.GA10949@google.com> In-Reply-To: <20150312012946.GA10949@google.com> Content-Type: text/plain; charset="ISO-8859-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: On 2015/3/12 9:29, Bjorn Helgaas wrote: > On Mon, Mar 09, 2015 at 10:34:03AM +0800, Yijing Wang wrote: >> Currently, we use int type for bus number in >> pci_create_root_bus(), pci_scan_root_bus() and >> pci_scan_bus_legacy. Because PCI bus number >> always <= 255, so we could change the bus number >> argument type to u32, and combine PCI domain and >> bus number in one. > > This makes no sense. Or rather, it only states the obvious: an 8-bit value > and a 16-bit value will both fit in a 32-bit value. But it doesn't say > *why* you think it's a good idea to pass a single value that contains both > domain and bus numbers. The obvious thing is to pass two separate values, > and you don't say why passing a single combined value is better. Sorry for my poor description for this patch, I combined the domain and bus, because I think now we have too many args at pci_scan_root_bus() or other scan functions, struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources) Now we have five args yet, plus the new introduced domain and pci_host_bridge_ops, it will become 7. I thought introduced a new structure which contain the necessary info to scan root bus/ host bridge, E.g struct pci_scan_info { int bus; struct device *parent; struct pci_ops *ops; void *sysdata; struct list_head *resource; int domain; struct pci_host_bridge_ops; } Do you like this one or keep it like now ? pci_scan_root_bus(struct device *parent, int domain, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources, struct pci_host_bridge_ops *ops) > > Also see whitespace mistakes below. I get a little testy when people don't > follow the whitespace convention of neighboring lines because if people are > sloppy about whitespace I assume they're sloppy about the code. > Sorry, I would be more careful. >> int error; >> @@ -1901,6 +1901,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> resource_size_t offset; >> char bus_addr[64]; >> char *fmt; >> + u8 bus = PCI_BUSNUM(db); > > Whitespace botch. > >> >> b = pci_alloc_bus(NULL); >> if (!b) >> @@ -1921,6 +1922,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> if (!bridge) >> goto err_out; >> >> + bridge->domain = PCI_DOMAIN(db); >> bridge->dev.parent = parent; >> bridge->dev.release = pci_release_host_bridge_dev; >> dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); >> @@ -2058,7 +2060,7 @@ void pci_bus_release_busn_res(struct pci_bus *b) >> res, ret ? "can not be" : "is"); >> } >> >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >> struct pci_ops *ops, void *sysdata, struct list_head *resources) >> { >> struct resource_entry *window; >> @@ -2072,15 +2074,15 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> break; >> } >> >> - b = pci_create_root_bus(parent, bus, ops, sysdata, resources); >> + b = pci_create_root_bus(parent, db, ops, sysdata, resources); >> if (!b) >> return NULL; >> >> if (!found) { >> dev_info(&b->dev, >> "No busn resource found for root bus, will use [bus %02x-ff]\n", >> - bus); >> - pci_bus_insert_busn_res(b, bus, 255); >> + PCI_BUSNUM(db)); >> + pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >> } >> >> max = pci_scan_child_bus(b); >> @@ -2092,7 +2094,7 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> } >> EXPORT_SYMBOL(pci_scan_root_bus); >> >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> +struct pci_bus *pci_scan_bus(u32 db, struct pci_ops *ops, >> void *sysdata) >> { >> LIST_HEAD(resources); >> @@ -2101,7 +2103,7 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> pci_add_resource(&resources, &ioport_resource); >> pci_add_resource(&resources, &iomem_resource); >> pci_add_resource(&resources, &busn_resource); >> - b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources); >> + b = pci_create_root_bus(NULL, db, ops, sysdata, &resources); >> if (b) { >> pci_scan_child_bus(b); >> } else { >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 36effb8..8b3d245 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -401,6 +401,7 @@ static inline int pci_channel_offline(struct pci_dev *pdev) >> } >> >> struct pci_host_bridge { >> + u16 domain; > > Please follow the existing whitespace convention, i.e., use a space, not a > tab, between "u16 domain". > >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> struct list_head windows; /* resource_entry */ >> @@ -769,14 +770,14 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res, >> void pcibios_scan_specific_bus(int busn); >> struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); >> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_bus(u32 dombus, struct pci_ops *ops, void *sysdata); >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); >> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); >> void pci_bus_release_busn_res(struct pci_bus *b); >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h >> index 3c292bc..89c6f15 100644 >> --- a/include/uapi/linux/pci.h >> +++ b/include/uapi/linux/pci.h >> @@ -30,6 +30,9 @@ >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) >> #define PCI_FUNC(devfn) ((devfn) & 0x07) >> +#define PCI_DOMAIN(db) ((db) >> 16) >> +#define PCI_BUSNUM(db) ((db) & 0xffff) >> +#define PCI_DOMBUS(domain, bus) (((domain) << 16) | (bus)) >> >> /* Ioctls for /proc/bus/pci/X/Y nodes. */ >> #define PCIIOC_BASE ('P' << 24 | 'C' << 16 | 'I' << 8) >> -- >> 1.7.1 >> >> -- >> 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 > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 From: wangyijing@huawei.com (Yijing Wang) Date: Thu, 12 Mar 2015 20:14:40 +0800 Subject: [PATCH v6 06/30] PCI: Combine PCI domain and bus number in u32 arg In-Reply-To: <20150312012946.GA10949@google.com> References: <1425868467-9667-1-git-send-email-wangyijing@huawei.com> <1425868467-9667-7-git-send-email-wangyijing@huawei.com> <20150312012946.GA10949@google.com> Message-ID: <55018330.5050002@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2015/3/12 9:29, Bjorn Helgaas wrote: > On Mon, Mar 09, 2015 at 10:34:03AM +0800, Yijing Wang wrote: >> Currently, we use int type for bus number in >> pci_create_root_bus(), pci_scan_root_bus() and >> pci_scan_bus_legacy. Because PCI bus number >> always <= 255, so we could change the bus number >> argument type to u32, and combine PCI domain and >> bus number in one. > > This makes no sense. Or rather, it only states the obvious: an 8-bit value > and a 16-bit value will both fit in a 32-bit value. But it doesn't say > *why* you think it's a good idea to pass a single value that contains both > domain and bus numbers. The obvious thing is to pass two separate values, > and you don't say why passing a single combined value is better. Sorry for my poor description for this patch, I combined the domain and bus, because I think now we have too many args at pci_scan_root_bus() or other scan functions, struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources) Now we have five args yet, plus the new introduced domain and pci_host_bridge_ops, it will become 7. I thought introduced a new structure which contain the necessary info to scan root bus/ host bridge, E.g struct pci_scan_info { int bus; struct device *parent; struct pci_ops *ops; void *sysdata; struct list_head *resource; int domain; struct pci_host_bridge_ops; } Do you like this one or keep it like now ? pci_scan_root_bus(struct device *parent, int domain, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources, struct pci_host_bridge_ops *ops) > > Also see whitespace mistakes below. I get a little testy when people don't > follow the whitespace convention of neighboring lines because if people are > sloppy about whitespace I assume they're sloppy about the code. > Sorry, I would be more careful. >> int error; >> @@ -1901,6 +1901,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> resource_size_t offset; >> char bus_addr[64]; >> char *fmt; >> + u8 bus = PCI_BUSNUM(db); > > Whitespace botch. > >> >> b = pci_alloc_bus(NULL); >> if (!b) >> @@ -1921,6 +1922,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> if (!bridge) >> goto err_out; >> >> + bridge->domain = PCI_DOMAIN(db); >> bridge->dev.parent = parent; >> bridge->dev.release = pci_release_host_bridge_dev; >> dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); >> @@ -2058,7 +2060,7 @@ void pci_bus_release_busn_res(struct pci_bus *b) >> res, ret ? "can not be" : "is"); >> } >> >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >> struct pci_ops *ops, void *sysdata, struct list_head *resources) >> { >> struct resource_entry *window; >> @@ -2072,15 +2074,15 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> break; >> } >> >> - b = pci_create_root_bus(parent, bus, ops, sysdata, resources); >> + b = pci_create_root_bus(parent, db, ops, sysdata, resources); >> if (!b) >> return NULL; >> >> if (!found) { >> dev_info(&b->dev, >> "No busn resource found for root bus, will use [bus %02x-ff]\n", >> - bus); >> - pci_bus_insert_busn_res(b, bus, 255); >> + PCI_BUSNUM(db)); >> + pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >> } >> >> max = pci_scan_child_bus(b); >> @@ -2092,7 +2094,7 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> } >> EXPORT_SYMBOL(pci_scan_root_bus); >> >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> +struct pci_bus *pci_scan_bus(u32 db, struct pci_ops *ops, >> void *sysdata) >> { >> LIST_HEAD(resources); >> @@ -2101,7 +2103,7 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> pci_add_resource(&resources, &ioport_resource); >> pci_add_resource(&resources, &iomem_resource); >> pci_add_resource(&resources, &busn_resource); >> - b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources); >> + b = pci_create_root_bus(NULL, db, ops, sysdata, &resources); >> if (b) { >> pci_scan_child_bus(b); >> } else { >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 36effb8..8b3d245 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -401,6 +401,7 @@ static inline int pci_channel_offline(struct pci_dev *pdev) >> } >> >> struct pci_host_bridge { >> + u16 domain; > > Please follow the existing whitespace convention, i.e., use a space, not a > tab, between "u16 domain". > >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> struct list_head windows; /* resource_entry */ >> @@ -769,14 +770,14 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res, >> void pcibios_scan_specific_bus(int busn); >> struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); >> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_bus(u32 dombus, struct pci_ops *ops, void *sysdata); >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); >> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); >> void pci_bus_release_busn_res(struct pci_bus *b); >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h >> index 3c292bc..89c6f15 100644 >> --- a/include/uapi/linux/pci.h >> +++ b/include/uapi/linux/pci.h >> @@ -30,6 +30,9 @@ >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) >> #define PCI_FUNC(devfn) ((devfn) & 0x07) >> +#define PCI_DOMAIN(db) ((db) >> 16) >> +#define PCI_BUSNUM(db) ((db) & 0xffff) >> +#define PCI_DOMBUS(domain, bus) (((domain) << 16) | (bus)) >> >> /* Ioctls for /proc/bus/pci/X/Y nodes. */ >> #define PCIIOC_BASE ('P' << 24 | 'C' << 16 | 'I' << 8) >> -- >> 1.7.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >> the body of a message to majordomo at vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yijing Wang Date: Thu, 12 Mar 2015 12:14:40 +0000 Subject: Re: [PATCH v6 06/30] PCI: Combine PCI domain and bus number in u32 arg Message-Id: <55018330.5050002@huawei.com> List-Id: References: <1425868467-9667-1-git-send-email-wangyijing@huawei.com> <1425868467-9667-7-git-send-email-wangyijing@huawei.com> <20150312012946.GA10949@google.com> In-Reply-To: <20150312012946.GA10949@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Bjorn Helgaas Cc: Jiang Liu , linux-pci@vger.kernel.org, Yinghai Lu , linux-kernel@vger.kernel.org, Marc Zyngier , linux-arm-kernel@lists.infradead.org, Russell King , x86@kernel.org, Thomas Gleixner , Benjamin Herrenschmidt , Rusty Russell , Tony Luck , linux-ia64@vger.kernel.org, "David S. Miller" , Guan Xuetao , linux-alpha@vger.kernel.org, linux-m68k@vger.kernel.org, Liviu Dudau , Arnd Bergmann , Geert Uytterhoeven On 2015/3/12 9:29, Bjorn Helgaas wrote: > On Mon, Mar 09, 2015 at 10:34:03AM +0800, Yijing Wang wrote: >> Currently, we use int type for bus number in >> pci_create_root_bus(), pci_scan_root_bus() and >> pci_scan_bus_legacy. Because PCI bus number >> always <= 255, so we could change the bus number >> argument type to u32, and combine PCI domain and >> bus number in one. > > This makes no sense. Or rather, it only states the obvious: an 8-bit value > and a 16-bit value will both fit in a 32-bit value. But it doesn't say > *why* you think it's a good idea to pass a single value that contains both > domain and bus numbers. The obvious thing is to pass two separate values, > and you don't say why passing a single combined value is better. Sorry for my poor description for this patch, I combined the domain and bus, because I think now we have too many args at pci_scan_root_bus() or other scan functions, struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources) Now we have five args yet, plus the new introduced domain and pci_host_bridge_ops, it will become 7. I thought introduced a new structure which contain the necessary info to scan root bus/ host bridge, E.g struct pci_scan_info { int bus; struct device *parent; struct pci_ops *ops; void *sysdata; struct list_head *resource; int domain; struct pci_host_bridge_ops; } Do you like this one or keep it like now ? pci_scan_root_bus(struct device *parent, int domain, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources, struct pci_host_bridge_ops *ops) > > Also see whitespace mistakes below. I get a little testy when people don't > follow the whitespace convention of neighboring lines because if people are > sloppy about whitespace I assume they're sloppy about the code. > Sorry, I would be more careful. >> int error; >> @@ -1901,6 +1901,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> resource_size_t offset; >> char bus_addr[64]; >> char *fmt; >> + u8 bus = PCI_BUSNUM(db); > > Whitespace botch. > >> >> b = pci_alloc_bus(NULL); >> if (!b) >> @@ -1921,6 +1922,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> if (!bridge) >> goto err_out; >> >> + bridge->domain = PCI_DOMAIN(db); >> bridge->dev.parent = parent; >> bridge->dev.release = pci_release_host_bridge_dev; >> dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); >> @@ -2058,7 +2060,7 @@ void pci_bus_release_busn_res(struct pci_bus *b) >> res, ret ? "can not be" : "is"); >> } >> >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >> struct pci_ops *ops, void *sysdata, struct list_head *resources) >> { >> struct resource_entry *window; >> @@ -2072,15 +2074,15 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> break; >> } >> >> - b = pci_create_root_bus(parent, bus, ops, sysdata, resources); >> + b = pci_create_root_bus(parent, db, ops, sysdata, resources); >> if (!b) >> return NULL; >> >> if (!found) { >> dev_info(&b->dev, >> "No busn resource found for root bus, will use [bus %02x-ff]\n", >> - bus); >> - pci_bus_insert_busn_res(b, bus, 255); >> + PCI_BUSNUM(db)); >> + pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >> } >> >> max = pci_scan_child_bus(b); >> @@ -2092,7 +2094,7 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> } >> EXPORT_SYMBOL(pci_scan_root_bus); >> >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> +struct pci_bus *pci_scan_bus(u32 db, struct pci_ops *ops, >> void *sysdata) >> { >> LIST_HEAD(resources); >> @@ -2101,7 +2103,7 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> pci_add_resource(&resources, &ioport_resource); >> pci_add_resource(&resources, &iomem_resource); >> pci_add_resource(&resources, &busn_resource); >> - b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources); >> + b = pci_create_root_bus(NULL, db, ops, sysdata, &resources); >> if (b) { >> pci_scan_child_bus(b); >> } else { >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 36effb8..8b3d245 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -401,6 +401,7 @@ static inline int pci_channel_offline(struct pci_dev *pdev) >> } >> >> struct pci_host_bridge { >> + u16 domain; > > Please follow the existing whitespace convention, i.e., use a space, not a > tab, between "u16 domain". > >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> struct list_head windows; /* resource_entry */ >> @@ -769,14 +770,14 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res, >> void pcibios_scan_specific_bus(int busn); >> struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); >> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_bus(u32 dombus, struct pci_ops *ops, void *sysdata); >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); >> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); >> void pci_bus_release_busn_res(struct pci_bus *b); >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h >> index 3c292bc..89c6f15 100644 >> --- a/include/uapi/linux/pci.h >> +++ b/include/uapi/linux/pci.h >> @@ -30,6 +30,9 @@ >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) >> #define PCI_FUNC(devfn) ((devfn) & 0x07) >> +#define PCI_DOMAIN(db) ((db) >> 16) >> +#define PCI_BUSNUM(db) ((db) & 0xffff) >> +#define PCI_DOMBUS(domain, bus) (((domain) << 16) | (bus)) >> >> /* Ioctls for /proc/bus/pci/X/Y nodes. */ >> #define PCIIOC_BASE ('P' << 24 | 'C' << 16 | 'I' << 8) >> -- >> 1.7.1 >> >> -- >> 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 > > . > -- Thanks! Yijing From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yijing Wang Subject: Re: [PATCH v6 06/30] PCI: Combine PCI domain and bus number in u32 arg Date: Thu, 12 Mar 2015 20:14:40 +0800 Message-ID: <55018330.5050002@huawei.com> References: <1425868467-9667-1-git-send-email-wangyijing@huawei.com> <1425868467-9667-7-git-send-email-wangyijing@huawei.com> <20150312012946.GA10949@google.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150312012946.GA10949@google.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii" To: Bjorn Helgaas Cc: Jiang Liu , linux-pci@vger.kernel.org, Yinghai Lu , linux-kernel@vger.kernel.org, Marc Zyngier , linux-arm-kernel@lists.infradead.org, Russell King , x86@kernel.org, Thomas Gleixner , Benjamin Herrenschmidt , Rusty Russell , Tony Luck , linux-ia64@vger.kernel.org, "David S. Miller" , Guan Xuetao , linux-alpha@vger.kernel.org, linux-m68k@lists.linux-m68k.org, Liviu Dudau , Arnd Bergmann , Geert Uytterhoeven On 2015/3/12 9:29, Bjorn Helgaas wrote: > On Mon, Mar 09, 2015 at 10:34:03AM +0800, Yijing Wang wrote: >> Currently, we use int type for bus number in >> pci_create_root_bus(), pci_scan_root_bus() and >> pci_scan_bus_legacy. Because PCI bus number >> always <= 255, so we could change the bus number >> argument type to u32, and combine PCI domain and >> bus number in one. > > This makes no sense. Or rather, it only states the obvious: an 8-bit value > and a 16-bit value will both fit in a 32-bit value. But it doesn't say > *why* you think it's a good idea to pass a single value that contains both > domain and bus numbers. The obvious thing is to pass two separate values, > and you don't say why passing a single combined value is better. Sorry for my poor description for this patch, I combined the domain and bus, because I think now we have too many args at pci_scan_root_bus() or other scan functions, struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources) Now we have five args yet, plus the new introduced domain and pci_host_bridge_ops, it will become 7. I thought introduced a new structure which contain the necessary info to scan root bus/ host bridge, E.g struct pci_scan_info { int bus; struct device *parent; struct pci_ops *ops; void *sysdata; struct list_head *resource; int domain; struct pci_host_bridge_ops; } Do you like this one or keep it like now ? pci_scan_root_bus(struct device *parent, int domain, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources, struct pci_host_bridge_ops *ops) > > Also see whitespace mistakes below. I get a little testy when people don't > follow the whitespace convention of neighboring lines because if people are > sloppy about whitespace I assume they're sloppy about the code. > Sorry, I would be more careful. >> int error; >> @@ -1901,6 +1901,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> resource_size_t offset; >> char bus_addr[64]; >> char *fmt; >> + u8 bus = PCI_BUSNUM(db); > > Whitespace botch. > >> >> b = pci_alloc_bus(NULL); >> if (!b) >> @@ -1921,6 +1922,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> if (!bridge) >> goto err_out; >> >> + bridge->domain = PCI_DOMAIN(db); >> bridge->dev.parent = parent; >> bridge->dev.release = pci_release_host_bridge_dev; >> dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); >> @@ -2058,7 +2060,7 @@ void pci_bus_release_busn_res(struct pci_bus *b) >> res, ret ? "can not be" : "is"); >> } >> >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >> struct pci_ops *ops, void *sysdata, struct list_head *resources) >> { >> struct resource_entry *window; >> @@ -2072,15 +2074,15 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> break; >> } >> >> - b = pci_create_root_bus(parent, bus, ops, sysdata, resources); >> + b = pci_create_root_bus(parent, db, ops, sysdata, resources); >> if (!b) >> return NULL; >> >> if (!found) { >> dev_info(&b->dev, >> "No busn resource found for root bus, will use [bus %02x-ff]\n", >> - bus); >> - pci_bus_insert_busn_res(b, bus, 255); >> + PCI_BUSNUM(db)); >> + pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >> } >> >> max = pci_scan_child_bus(b); >> @@ -2092,7 +2094,7 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> } >> EXPORT_SYMBOL(pci_scan_root_bus); >> >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> +struct pci_bus *pci_scan_bus(u32 db, struct pci_ops *ops, >> void *sysdata) >> { >> LIST_HEAD(resources); >> @@ -2101,7 +2103,7 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, >> pci_add_resource(&resources, &ioport_resource); >> pci_add_resource(&resources, &iomem_resource); >> pci_add_resource(&resources, &busn_resource); >> - b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources); >> + b = pci_create_root_bus(NULL, db, ops, sysdata, &resources); >> if (b) { >> pci_scan_child_bus(b); >> } else { >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 36effb8..8b3d245 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -401,6 +401,7 @@ static inline int pci_channel_offline(struct pci_dev *pdev) >> } >> >> struct pci_host_bridge { >> + u16 domain; > > Please follow the existing whitespace convention, i.e., use a space, not a > tab, between "u16 domain". > >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> struct list_head windows; /* resource_entry */ >> @@ -769,14 +770,14 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res, >> void pcibios_scan_specific_bus(int busn); >> struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> -struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); >> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_bus(u32 dombus, struct pci_ops *ops, void *sysdata); >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); >> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); >> void pci_bus_release_busn_res(struct pci_bus *b); >> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, >> +struct pci_bus *pci_scan_root_bus(struct device *parent, u32 dombus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h >> index 3c292bc..89c6f15 100644 >> --- a/include/uapi/linux/pci.h >> +++ b/include/uapi/linux/pci.h >> @@ -30,6 +30,9 @@ >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) >> #define PCI_FUNC(devfn) ((devfn) & 0x07) >> +#define PCI_DOMAIN(db) ((db) >> 16) >> +#define PCI_BUSNUM(db) ((db) & 0xffff) >> +#define PCI_DOMBUS(domain, bus) (((domain) << 16) | (bus)) >> >> /* Ioctls for /proc/bus/pci/X/Y nodes. */ >> #define PCIIOC_BASE ('P' << 24 | 'C' << 16 | 'I' << 8) >> -- >> 1.7.1 >> >> -- >> 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 > > . > -- Thanks! Yijing