| Message ID | 1420684386-5975-18-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Thursday, January 08, 2015 10:33:04 AM Jiang Liu wrote: > Currently ACPI, PCI and pnp all implement the same resource list > management with different data structure. We need to transfer from > one data structure into another when passing resources from one > subsystem into another subsystem. Sp move struct resource_list_entry > from ACPI into resource core, so it could be reused by different > subystems and avoid the data structure conversion. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > drivers/acpi/acpi_lpss.c | 6 +++--- > drivers/acpi/acpi_platform.c | 2 +- > drivers/acpi/resource.c | 13 ++++-------- > drivers/dma/acpi-dma.c | 8 +++---- > include/linux/acpi.h | 6 ------ > include/linux/ioport.h | 25 ++++++++++++++++++++++ > kernel/resource.c | 48 ++++++++++++++++++++++++++++++++++++++++++ > 7 files changed, 85 insertions(+), 23 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index 4f3febf8a589..39b548dba70b 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -333,12 +333,12 @@ static int acpi_lpss_create_device(struct acpi_device *adev, > goto err_out; > > list_for_each_entry(rentry, &resource_list, node) > - if (resource_type(&rentry->res) == IORESOURCE_MEM) { > + if (resource_type(rentry->res) == IORESOURCE_MEM) { > if (dev_desc->prv_size_override) > pdata->mmio_size = dev_desc->prv_size_override; > else > - pdata->mmio_size = resource_size(&rentry->res); > - pdata->mmio_base = ioremap(rentry->res.start, > + pdata->mmio_size = resource_size(rentry->res); > + pdata->mmio_base = ioremap(rentry->res->start, > pdata->mmio_size); > break; > } > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c > index 6ba8beb6b9d2..238e32b9cbb0 100644 > --- a/drivers/acpi/acpi_platform.c > +++ b/drivers/acpi/acpi_platform.c > @@ -71,7 +71,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) > } > count = 0; > list_for_each_entry(rentry, &resource_list, node) > - resources[count++] = rentry->res; > + resources[count++] = *rentry->res; > > acpi_dev_free_resource_list(&resource_list); > } > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index 8ea7c26d6915..c0dc038274d4 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -444,12 +444,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_interrupt); > */ > void acpi_dev_free_resource_list(struct list_head *list) > { > - struct resource_list_entry *rentry, *re; > - > - list_for_each_entry_safe(rentry, re, list, node) { > - list_del(&rentry->node); > - kfree(rentry); > - } > + resource_list_free_list(list); > } > EXPORT_SYMBOL_GPL(acpi_dev_free_resource_list); > > @@ -467,14 +462,14 @@ static acpi_status acpi_dev_new_resource_entry(struct resource *r, > { > struct resource_list_entry *rentry; > > - rentry = kmalloc(sizeof(*rentry), GFP_KERNEL); > + rentry = resource_list_alloc(NULL, 0); > if (!rentry) { > c->error = -ENOMEM; > return AE_NO_MEMORY; > } > - rentry->res = *r; > + *rentry->res = *r; > rentry->offset = offset; > - list_add_tail(&rentry->node, c->list); > + resource_list_insert(c->list, rentry, true); > c->count++; > return AE_OK; > } > diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c > index de361a156b34..1ce84abe0924 100644 > --- a/drivers/dma/acpi-dma.c > +++ b/drivers/dma/acpi-dma.c > @@ -56,10 +56,10 @@ static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp, > return 0; > > list_for_each_entry(rentry, &resource_list, node) { > - if (resource_type(&rentry->res) == IORESOURCE_MEM) > - mem = rentry->res.start; > - else if (resource_type(&rentry->res) == IORESOURCE_IRQ) > - irq = rentry->res.start; > + if (resource_type(rentry->res) == IORESOURCE_MEM) > + mem = rentry->res->start; > + else if (resource_type(rentry->res) == IORESOURCE_IRQ) > + irq = rentry->res->start; > } > > acpi_dev_free_resource_list(&resource_list); > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 1df4a0211ae5..6d7f7edca22c 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -297,12 +297,6 @@ unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable); > bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > struct resource *res); > > -struct resource_list_entry { > - struct list_head node; > - struct resource res; > - resource_size_t offset; > -}; > - > void acpi_dev_free_resource_list(struct list_head *list); > int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list, > int (*preproc)(struct acpi_resource *, void *), > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index 2c5250222278..a6f4841ca40c 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -11,6 +11,7 @@ > #ifndef __ASSEMBLY__ > #include <linux/compiler.h> > #include <linux/types.h> > + > /* > * Resources are tree-like, allowing > * nesting etc.. > @@ -255,6 +256,30 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2) > return (r1->start <= r2->end && r1->end >= r2->start); > } > > +/* > + * Common resource list management data structure and interfaces to support > + * ACPI, PNP and PCI host bridge etc. > + */ > +struct resource_list_entry { > + struct list_head node; > + struct resource *res; /* In master (CPU) address space */ > + resource_size_t offset; /* Translation offset for bridge */ > + struct resource __res; /* Default storage for res */ > +}; > + > +extern struct resource_list_entry *resource_list_alloc(struct resource *res, > + size_t extra_size); > +extern void resource_list_free(struct resource_list_entry *entry); > +extern void resource_list_insert(struct list_head *head, > + struct resource_list_entry *entry, bool tail); > +extern void resource_list_delete(struct resource_list_entry *entry); > +extern void resource_list_free_list(struct list_head *head); > + > +#define resource_list_for_each_entry(entry, list) \ > + list_for_each_entry((entry), (list), node) > + > +#define resource_list_for_each_entry_safe(entry, tmp, list) \ > + list_for_each_entry_safe((entry), (tmp), (list), node) > > #endif /* __ASSEMBLY__ */ > #endif /* _LINUX_IOPORT_H */ > diff --git a/kernel/resource.c b/kernel/resource.c > index 0bcebffc4e77..414183809383 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1529,6 +1529,54 @@ int iomem_is_exclusive(u64 addr) > return err; > } > > +struct resource_list_entry *resource_list_alloc(struct resource *res, > + size_t extra_size) What about create_resource_list_entry()? Less confusing surely. > +{ > + struct resource_list_entry *entry; > + > + entry = kzalloc(sizeof(*entry) + extra_size, GFP_KERNEL); > + if (entry) { > + INIT_LIST_HEAD(&entry->node); > + entry->res = res ? res : &entry->__res; > + } > + > + return entry; > +} > +EXPORT_SYMBOL(resource_list_alloc); > + > +void resource_list_free(struct resource_list_entry *entry) > +{ > + kfree(entry); > +} > +EXPORT_SYMBOL(resource_list_free); Well, I'm not sure I like this. The name suggests that it would free the entire list and what's wrong with using kfree() directly on "entry" anyway? > + > +void resource_list_insert(struct list_head *head, > + struct resource_list_entry *entry, bool tail) I would call this resource_list_add() if anything. Also it may be better to have two helpers, one for "add" and one for "add_tail" (and perhaps define them as static inline?). And why change the ordering between "head" and "entry". That's alomost guaranteed to confuse people. > +{ > + if (tail) > + list_add_tail(&entry->node, head); > + else > + list_add(&entry->node, head); > +} > +EXPORT_SYMBOL(resource_list_insert); > + > +void resource_list_delete(struct resource_list_entry *entry) > +{ > + list_del(&entry->node); > +} > +EXPORT_SYMBOL(resource_list_delete); Couldn't this be a static inline)? Or maybe we can combine the "list_del" with "kfree" in one function? > + > +void resource_list_free_list(struct list_head *head) > +{ > + struct resource_list_entry *entry, *tmp; > + > + list_for_each_entry_safe(entry, tmp, head, node) { > + list_del(&entry->node); > + resource_list_free(entry); > + } > +} > +EXPORT_SYMBOL(resource_list_free_list); > + > static int __init strict_iomem(char *str) > { > if (strstr(str, "relaxed")) >
On 2015/1/21 9:10, Rafael J. Wysocki wrote: > On Thursday, January 08, 2015 10:33:04 AM Jiang Liu wrote: <snit> >> diff --git a/kernel/resource.c b/kernel/resource.c >> index 0bcebffc4e77..414183809383 100644 >> --- a/kernel/resource.c >> +++ b/kernel/resource.c >> @@ -1529,6 +1529,54 @@ int iomem_is_exclusive(u64 addr) >> return err; >> } >> >> +struct resource_list_entry *resource_list_alloc(struct resource *res, >> + size_t extra_size) > > What about create_resource_list_entry()? Less confusing surely. Sure, I will rename it as resource_list_create_entry(). > >> +{ >> + struct resource_list_entry *entry; >> + >> + entry = kzalloc(sizeof(*entry) + extra_size, GFP_KERNEL); >> + if (entry) { >> + INIT_LIST_HEAD(&entry->node); >> + entry->res = res ? res : &entry->__res; >> + } >> + >> + return entry; >> +} >> +EXPORT_SYMBOL(resource_list_alloc); >> + >> +void resource_list_free(struct resource_list_entry *entry) >> +{ >> + kfree(entry); >> +} >> +EXPORT_SYMBOL(resource_list_free); > > Well, I'm not sure I like this. The name suggests that it would free the > entire list and what's wrong with using kfree() directly on "entry" anyway? I just want to make interface symmetric. We may also support some type of callback when freeing resources in future. > >> + >> +void resource_list_insert(struct list_head *head, >> + struct resource_list_entry *entry, bool tail) > > I would call this resource_list_add() if anything. > > Also it may be better to have two helpers, one for "add" and one for "add_tail" > (and perhaps define them as static inline?). We can't use inline functions here because that needs pulling list.h into ioport.h, then causing building issues to header inclusion order. > > And why change the ordering between "head" and "entry". That's alomost > guaranteed to confuse people. My fault, will change in next version. > >> +{ >> + if (tail) >> + list_add_tail(&entry->node, head); >> + else >> + list_add(&entry->node, head); >> +} >> +EXPORT_SYMBOL(resource_list_insert); >> + >> +void resource_list_delete(struct resource_list_entry *entry) >> +{ >> + list_del(&entry->node); >> +} >> +EXPORT_SYMBOL(resource_list_delete); > > Couldn't this be a static inline)? Inline will cause header file inclusion order issue:( > > Or maybe we can combine the "list_del" with "kfree" in one function? There are callers which need separating list_del from kfree, so exported two interfaces here. Will add another helper interface resource_list_destroy_entry(). Regards! Gerry > >> + >> +void resource_list_free_list(struct list_head *head) >> +{ >> + struct resource_list_entry *entry, *tmp; >> + >> + list_for_each_entry_safe(entry, tmp, head, node) { >> + list_del(&entry->node); >> + resource_list_free(entry); >> + } >> +} >> +EXPORT_SYMBOL(resource_list_free_list); >> + >> static int __init strict_iomem(char *str) >> { >> if (strstr(str, "relaxed")) >> > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 21 Jan 2015, Jiang Liu wrote: > On 2015/1/21 9:10, Rafael J. Wysocki wrote: > >> + > >> +void resource_list_insert(struct list_head *head, > >> + struct resource_list_entry *entry, bool tail) > > > > I would call this resource_list_add() if anything. > > > > Also it may be better to have two helpers, one for "add" and one for "add_tail" > > (and perhaps define them as static inline?). > We can't use inline functions here because that needs pulling list.h > into ioport.h, then causing building issues to header inclusion order. Create a new header file to avoid the circular dependencies then. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/1/21 17:43, Thomas Gleixner wrote: > On Wed, 21 Jan 2015, Jiang Liu wrote: >> On 2015/1/21 9:10, Rafael J. Wysocki wrote: >>>> + >>>> +void resource_list_insert(struct list_head *head, >>>> + struct resource_list_entry *entry, bool tail) >>> >>> I would call this resource_list_add() if anything. >>> >>> Also it may be better to have two helpers, one for "add" and one for "add_tail" >>> (and perhaps define them as static inline?). >> We can't use inline functions here because that needs pulling list.h >> into ioport.h, then causing building issues to header inclusion order. > > Create a new header file to avoid the circular dependencies then. Great, create include/linux/resource_list.h for it so we could inline those simple functions. Regards, Gerry > > Thanks, > > tglx > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" 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/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 4f3febf8a589..39b548dba70b 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -333,12 +333,12 @@ static int acpi_lpss_create_device(struct acpi_device *adev, goto err_out; list_for_each_entry(rentry, &resource_list, node) - if (resource_type(&rentry->res) == IORESOURCE_MEM) { + if (resource_type(rentry->res) == IORESOURCE_MEM) { if (dev_desc->prv_size_override) pdata->mmio_size = dev_desc->prv_size_override; else - pdata->mmio_size = resource_size(&rentry->res); - pdata->mmio_base = ioremap(rentry->res.start, + pdata->mmio_size = resource_size(rentry->res); + pdata->mmio_base = ioremap(rentry->res->start, pdata->mmio_size); break; } diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c index 6ba8beb6b9d2..238e32b9cbb0 100644 --- a/drivers/acpi/acpi_platform.c +++ b/drivers/acpi/acpi_platform.c @@ -71,7 +71,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) } count = 0; list_for_each_entry(rentry, &resource_list, node) - resources[count++] = rentry->res; + resources[count++] = *rentry->res; acpi_dev_free_resource_list(&resource_list); } diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 8ea7c26d6915..c0dc038274d4 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -444,12 +444,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_interrupt); */ void acpi_dev_free_resource_list(struct list_head *list) { - struct resource_list_entry *rentry, *re; - - list_for_each_entry_safe(rentry, re, list, node) { - list_del(&rentry->node); - kfree(rentry); - } + resource_list_free_list(list); } EXPORT_SYMBOL_GPL(acpi_dev_free_resource_list); @@ -467,14 +462,14 @@ static acpi_status acpi_dev_new_resource_entry(struct resource *r, { struct resource_list_entry *rentry; - rentry = kmalloc(sizeof(*rentry), GFP_KERNEL); + rentry = resource_list_alloc(NULL, 0); if (!rentry) { c->error = -ENOMEM; return AE_NO_MEMORY; } - rentry->res = *r; + *rentry->res = *r; rentry->offset = offset; - list_add_tail(&rentry->node, c->list); + resource_list_insert(c->list, rentry, true); c->count++; return AE_OK; } diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c index de361a156b34..1ce84abe0924 100644 --- a/drivers/dma/acpi-dma.c +++ b/drivers/dma/acpi-dma.c @@ -56,10 +56,10 @@ static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp, return 0; list_for_each_entry(rentry, &resource_list, node) { - if (resource_type(&rentry->res) == IORESOURCE_MEM) - mem = rentry->res.start; - else if (resource_type(&rentry->res) == IORESOURCE_IRQ) - irq = rentry->res.start; + if (resource_type(rentry->res) == IORESOURCE_MEM) + mem = rentry->res->start; + else if (resource_type(rentry->res) == IORESOURCE_IRQ) + irq = rentry->res->start; } acpi_dev_free_resource_list(&resource_list); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 1df4a0211ae5..6d7f7edca22c 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -297,12 +297,6 @@ unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable); bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, struct resource *res); -struct resource_list_entry { - struct list_head node; - struct resource res; - resource_size_t offset; -}; - void acpi_dev_free_resource_list(struct list_head *list); int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list, int (*preproc)(struct acpi_resource *, void *), diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 2c5250222278..a6f4841ca40c 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -11,6 +11,7 @@ #ifndef __ASSEMBLY__ #include <linux/compiler.h> #include <linux/types.h> + /* * Resources are tree-like, allowing * nesting etc.. @@ -255,6 +256,30 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2) return (r1->start <= r2->end && r1->end >= r2->start); } +/* + * Common resource list management data structure and interfaces to support + * ACPI, PNP and PCI host bridge etc. + */ +struct resource_list_entry { + struct list_head node; + struct resource *res; /* In master (CPU) address space */ + resource_size_t offset; /* Translation offset for bridge */ + struct resource __res; /* Default storage for res */ +}; + +extern struct resource_list_entry *resource_list_alloc(struct resource *res, + size_t extra_size); +extern void resource_list_free(struct resource_list_entry *entry); +extern void resource_list_insert(struct list_head *head, + struct resource_list_entry *entry, bool tail); +extern void resource_list_delete(struct resource_list_entry *entry); +extern void resource_list_free_list(struct list_head *head); + +#define resource_list_for_each_entry(entry, list) \ + list_for_each_entry((entry), (list), node) + +#define resource_list_for_each_entry_safe(entry, tmp, list) \ + list_for_each_entry_safe((entry), (tmp), (list), node) #endif /* __ASSEMBLY__ */ #endif /* _LINUX_IOPORT_H */ diff --git a/kernel/resource.c b/kernel/resource.c index 0bcebffc4e77..414183809383 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1529,6 +1529,54 @@ int iomem_is_exclusive(u64 addr) return err; } +struct resource_list_entry *resource_list_alloc(struct resource *res, + size_t extra_size) +{ + struct resource_list_entry *entry; + + entry = kzalloc(sizeof(*entry) + extra_size, GFP_KERNEL); + if (entry) { + INIT_LIST_HEAD(&entry->node); + entry->res = res ? res : &entry->__res; + } + + return entry; +} +EXPORT_SYMBOL(resource_list_alloc); + +void resource_list_free(struct resource_list_entry *entry) +{ + kfree(entry); +} +EXPORT_SYMBOL(resource_list_free); + +void resource_list_insert(struct list_head *head, + struct resource_list_entry *entry, bool tail) +{ + if (tail) + list_add_tail(&entry->node, head); + else + list_add(&entry->node, head); +} +EXPORT_SYMBOL(resource_list_insert); + +void resource_list_delete(struct resource_list_entry *entry) +{ + list_del(&entry->node); +} +EXPORT_SYMBOL(resource_list_delete); + +void resource_list_free_list(struct list_head *head) +{ + struct resource_list_entry *entry, *tmp; + + list_for_each_entry_safe(entry, tmp, head, node) { + list_del(&entry->node); + resource_list_free(entry); + } +} +EXPORT_SYMBOL(resource_list_free_list); + static int __init strict_iomem(char *str) { if (strstr(str, "relaxed"))
Currently ACPI, PCI and pnp all implement the same resource list management with different data structure. We need to transfer from one data structure into another when passing resources from one subsystem into another subsystem. Sp move struct resource_list_entry from ACPI into resource core, so it could be reused by different subystems and avoid the data structure conversion. Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- drivers/acpi/acpi_lpss.c | 6 +++--- drivers/acpi/acpi_platform.c | 2 +- drivers/acpi/resource.c | 13 ++++-------- drivers/dma/acpi-dma.c | 8 +++---- include/linux/acpi.h | 6 ------ include/linux/ioport.h | 25 ++++++++++++++++++++++ kernel/resource.c | 48 ++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 85 insertions(+), 23 deletions(-)