Hi, Alex-san,
Thank you for your helpful comments.
I will attach a modified patch.
Thanks
On Thu, 19 Jul 2007 13:04:44 -0600
Alex Williamson <alex.williamson@xxxxxx> wrote:
>
> 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.
>
Jun Kamada
Linux Technology Development Div.
Server Systems Unit
Fujitsu Ltd.
kama@xxxxxxxxxxxxxx
new_paravirt_pci.patch
Description: Binary data
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel
|
Home