WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ia64-devel

[Xen-ia64-devel] Re: [4/4] Issue ioremap hypercall in pci_acpi_scan_root

To: Jun Kamada <kama@xxxxxxxxxxxxxx>
Subject: [Xen-ia64-devel] Re: [4/4] Issue ioremap hypercall in pci_acpi_scan_root() in order to map /dev/mem
From: Alex Williamson <alex.williamson@xxxxxx>
Date: Thu, 19 Jul 2007 13:04:44 -0600
Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Thu, 19 Jul 2007 12:11:45 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20070713184001.AC11.KAMA@xxxxxxxxxxxxxx>
List-help: <mailto:xen-ia64-devel-request@lists.xensource.com?subject=help>
List-id: Discussion of the ia64 port of Xen <xen-ia64-devel.lists.xensource.com>
List-post: <mailto:xen-ia64-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: HP OSLO R&D
References: <20070615134015.12B7.KAMA@xxxxxxxxxxxxxx> <20070713180637.AC08.KAMA@xxxxxxxxxxxxxx> <20070713184001.AC11.KAMA@xxxxxxxxxxxxxx>
Sender: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
   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