|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-ia64-devel] Re: [4/4] Issue ioremap hypercall in pci_acpi_scan_root() in order to map /dev/mem
While we're waiting on staging to get pushed out, here are a few more
comments. Casting ioremap_issue_list_top as a struct ioremap_issue_list
is awkward, and I think unnecessary. The gotos are easy to remove too.
You might also want to consider a few simple variable renames, for
instance "top" is used as both a struct resource and a struct
ioremap_issue_list. A few additional cleanups below. Thanks,
Alex
On Fri, 2007-07-13 at 18:40 +0900, Jun Kamada wrote:
> # HG changeset patch
> # User Jun Kamada <kama@xxxxxxxxxxxxxx>
> # Date 1184348594 -32400
> # Node ID b82a7428f53a5d03c964fd30d21100429376e64f
> # Parent 031ea456e2756b3f7c10c00f7fcdccb4fc01baa2
> Issue ioremap hypercall in pci_acpi_scan_root() in order to map /dev/mem.
>
> Signed-off-by: Jun Kamada <kama@xxxxxxxxxxxxxx>
>
> diff -r 031ea456e275 -r b82a7428f53a arch/ia64/pci/pci.c
> --- a/arch/ia64/pci/pci.c Sat Jul 14 02:41:26 2007 +0900
> +++ b/arch/ia64/pci/pci.c Sat Jul 14 02:43:14 2007 +0900
> @@ -29,6 +29,15 @@
> #include <asm/smp.h>
> #include <asm/irq.h>
> #include <asm/hw_irq.h>
> +
> +#ifdef CONFIG_XEN
> +struct ioremap_issue_list {
> + struct list_head listp;
> + unsigned long start;
> + unsigned long end;
> +};
> +typedef struct ioremap_issue_list ioremap_issue_list_t;
> +#endif /* CONFIG_XEN */
>
> /*
> * Low-level SAL-based PCI configuration access functions. Note that SAL
> @@ -337,6 +346,182 @@ pcibios_setup_root_windows(struct pci_bu
> }
> }
>
> +#ifdef CONFIG_XEN
> +static void
> +__cleanup_issue_list(ioremap_issue_list_t *top)
s/ioremap_issue_list_t/struct list/
> +{
> + ioremap_issue_list_t *ptr, *tmp_ptr;
> +
> + list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
s/&(top->listp)/top/
> + list_del(&(ptr->listp));
> + kfree(ptr);
> + }
> +}
> +
> +static int
> +__add_issue_list(unsigned long start, unsigned long end,
> + ioremap_issue_list_t *top)
s/ioremap_issue_list_t/struct list/
> +{
> + ioremap_issue_list_t *ptr, *new;
> +
> + if (start > end) {
> + printk(KERN_ERR "%s: Internal error (start addr > end
> addr)\n",
> + __FUNCTION__);
> + return 0;
> + }
> +
> + /* Head of the resource structure list contains */
> + /* dummy val.(start=0, end=~0), so skip it */
> + if ((start == 0) && (end == ~0)) {
> + return 0;
> + }
> +
> + start &= PAGE_MASK;
> + end |= ~PAGE_MASK;
> +
> + /* We can merge specified address range into existing entry */
> + list_for_each_entry(ptr, &(top->listp), listp) {
s/&(top->listp)/top
> + if ((ptr->start > end + 1) || (ptr->end + 1 < start))
> + continue;
> + ptr->start = min(start, ptr->start);
> + ptr->end = max(end, ptr->end);
> + goto out;
> + }
> +
> + /* We could not merge, so create new entry */
> + new = kmalloc(sizeof(ioremap_issue_list_t), GFP_KERNEL);
> + if (new == NULL) {
> + printk(KERN_ERR "%s: Could not allocate memory. "
> + "HYPERVISOR_ioremap will not be issued\n",
> + __FUNCTION__);
> + return -ENOMEM;
> + }
> +
> + new->start = start;
> + new->end = end;
> +
> + /* Insert the new entry to the list by ascending order */
> + if (list_empty(&(top->listp))) {
s/&(top->listp)/top
> + list_add_tail(&(new->listp), &(top->listp));
s/&(top->listp)/top
> + goto out;
s/goto out/return 0/
> + }
> + list_for_each_entry(ptr, &(top->listp), listp) {
s/&(top->listp)/top
> + if (new->start > ptr->start)
> + continue;
> + list_add(&(new->listp), ((struct list_head *)ptr)->prev);
> + goto out;
s/goto out/return 0/
> + }
> + list_add_tail(&(new->listp), &(top->listp));
s/&(top->listp)/top
> +
> +out:
s/out://
> + return 0;
> +}
> +
> +static int
> +__make_issue_list(struct resource *ptr, ioremap_issue_list_t *top)
s/ioremap_issue_list_t/struct list/
> +{
> + int ret;
> +
> + if (ptr->child) {
> + ret = __make_issue_list(ptr->child, top);
> + if (ret)
> + return ret;
> + }
> + if (ptr->sibling) {
> + ret = __make_issue_list(ptr->sibling, top);
> + if (ret)
> + return ret;
> + }
> +
> + if (ptr->flags & IORESOURCE_MEM) {
> + ret = __add_issue_list(ptr->start, ptr->end, top);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +__compress_issue_list(ioremap_issue_list_t *top)
> +{
> + ioremap_issue_list_t *ptr, *tmp_ptr, *next;
> + int compressed;
> +
> + /* merge adjacent entries, if overlapped */
> + /* (assumes entries are sorted by ascending order) */
s/assumes// - At least I hope we're sure they're sorted ;)
> + do {
> + compressed = 0;
> +
> + list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
s/&(top->listp)/top
> + if (list_is_last((struct list_head *)ptr,
> + &(top->listp)) == 0) {
s/&(top->listp)/top
Rather than adding another level of nesting here, how about:
if (list_is_last((struct list_head *)ptr, top))
continue; /* or maybe break */
> + next = (ioremap_issue_list_t *)
> + (((struct list_head *)ptr)->next);
> + if (next->start <= (ptr->end) + 1) {
> + next->start = min(ptr->start,
> + next->start);
> + next->end = max(ptr->end,
> + next->end);
> +
> + list_del(&(ptr->listp));
> + kfree(ptr);
> + compressed = 1;
> + }
> + }
> + }
> + } while (compressed == 1);
Does this really need the do/while? We're expanding the next list entry
and removing the current, so it seems like we complete the merging in
one pass.
> +}
> +
> +static int
> +__issue_ioremap(ioremap_issue_list_t *top)
s/ioremap_issue_list_t/struct list/
> +{
> + ioremap_issue_list_t *ptr, *tmp_ptr;
> + unsigned int offset;
> +
> + list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
s/&(top->listp)/top
> + offset = HYPERVISOR_ioremap(ptr->start,
> + ptr->end - ptr->start + 1);
> + if (offset == ~0) {
> + printk(KERN_ERR "%s: HYPERVISOR_ioremap() failed. "
> + "Address Range: 0x%016lx-0x%016lx\n",
> + __FUNCTION__, ptr->start, ptr->end);
> + }
> +
> + list_del(&(ptr->listp));
> + kfree(ptr);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +do_ioremap_on_resource_list(struct resource *top)
> +{
> + struct list_head ioremap_issue_list_top = {
> + &ioremap_issue_list_top,
> + &ioremap_issue_list_top
> + };
Use:
LIST_HEAD(ioremap_issue_list_top);
> + int ret;
> +
> + ret = __make_issue_list(top,
> + (ioremap_issue_list_t *)(&ioremap_issue_list_top));
s/(ioremap_issue_list_t *)//
> + if (ret) {
> + __cleanup_issue_list((ioremap_issue_list_t *)
s/(ioremap_issue_list_t *)//
> + (&ioremap_issue_list_top));
> + return ret;
> + }
> +
> + __compress_issue_list((ioremap_issue_list_t *)
s/(ioremap_issue_list_t *)//
> + (&ioremap_issue_list_top));
> +
> + (void)__issue_ioremap((ioremap_issue_list_t *)
s/(ioremap_issue_list_t *)//
> + (&ioremap_issue_list_top));
> +
> + return 0;
> +}
> +#endif /* CONFIG_XEN */
> +
> struct pci_bus * __devinit
> pci_acpi_scan_root(struct acpi_device *device, int domain, int bus)
> {
> @@ -379,6 +564,18 @@ pci_acpi_scan_root(struct acpi_device *d
> pbus = pci_scan_bus_parented(NULL, bus, &pci_root_ops, controller);
> if (pbus)
> pcibios_setup_root_windows(pbus, controller);
> +
> +#ifdef CONFIG_XEN
> + if (is_initial_xendomain()) {
> + if (do_ioremap_on_resource_list(&iomem_resource) != 0) {
> + printk(KERN_ERR
> + "%s: Counld not issue HYPERVISOR_ioremap "
> + "due to lack of memory or hypercall failure\n",
> + __FUNCTION__);
> + goto out3;
> + }
> + }
> +#endif /* CONFIG_XEN */
>
> return pbus;
>
--
Alex Williamson HP Open Source & Linux Org.
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |