|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-ia64-devel] [Patch 4/4] Xwindow: Modify pci_acpi_scan_root()
Hi,
Some more comments below. Thanks for switching to the standard list
implementation, it looks better. How do you propose coordinating the
patches for xen-ia64-devel vs the one for xen-devel? Should we get the
latter in first, then proceed w/ the xen-ia64-devel patch? Thanks,
Alex
On Wed, 2007-06-13 at 15:56 +0900, Jun Kamada wrote:
> # HG changeset patch
> # User Jun Kamada <kama@xxxxxxxxxxxxxx>
> # Date 1181706151 -32400
> # Node ID 76e7ecd69d2ce751bf33784f62762c2ba8ed7162
> # Parent 7286707307032b6b3117a4f48aa4f0fdce6e7587
> Issue ioremap hypercall in pci_acpi_scan_root() in order to
> map /dev/mem.
>
> Signed-off-by: Jun Kamada <kama@xxxxxxxxxxxxxx>
>
> diff -r 728670730703 -r 76e7ecd69d2c
> linux-2.6-xen-sparse/arch/ia64/pci/pci.c
> --- a/linux-2.6-xen-sparse/arch/ia64/pci/pci.c Wed Jun 13 12:38:18
> 2007 +0900
> +++ b/linux-2.6-xen-sparse/arch/ia64/pci/pci.c Wed Jun 13 12:42:31
> 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
> @@ -332,6 +341,165 @@ pcibios_setup_root_windows(struct pci_bu
> }
> }
>
> +#ifdef CONFIG_XEN
> +
> +#define MIN(x,y) (((x) <= (y)) ? (x) : (y))
> +#define MAX(x,y) (((x) >= (y)) ? (x) : (y))
min() and max() are already defined in kernel.h, can you make use of
those?
> +static int
> +__add_issue_list(unsigned long start, unsigned long end,
> + ioremap_issue_list_t *top)
> +{
> + ioremap_issue_list_t *ptr, *new;
> +
> + if (start > end) {
> + printk(KERN_ERR "%s: Internal error (start addr > end
> addr)\n",
> + __FUNCTION__);
> + return 0;
> + }
> +
> + start &= ~(PAGE_SIZE - 1);
Use PAGE_MASK
> + end |= (PAGE_SIZE - 1);
And ~PAGE_MASK
> +
> + list_for_each_entry(ptr, &(top->listp), listp) {
> + if (((ptr->start >= start) && (ptr->start <= end + 1))
> ||
> + ((ptr->end >= start - 1) && (ptr->end <= end))
> ||
> + ((start >= ptr->start) && (start <= ptr->end) &&
> + (end >= ptr->start) && (end <= ptr->end))) {
> + ptr->start = MIN(start, ptr->start);
> + ptr->end = MAX(end, ptr->end);
> + return 0;
> + }
Can't this be greatly simplified? Instead of checking all the degrees
of overlap, just look for the non-overlap cases. Everything else is
overlap
if (ptr->start > end + 1 || ptr->end + 1 < start)
continue;
ptr->start = min(start, ptr->start);
ptr->end = max(end, ptr->end);
return 0;
However, what if this grows the range such that it bumps into the
entries around it. Do you plan to merge them? Maybe keeping the list
sorted would make this easier.
> + }
> +
> + 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;
> +
> + list_add(&(new->listp), &(top->listp));
> +
> + return 0;
> +}
> +
> +static int
> +__make_issue_list(struct resource *ptr, ioremap_issue_list_t *top) {
> + unsigned int curr_lvl = 1;
> + unsigned int prev_lvl = 0;
> + int ret;
> +
> + for ( ;; ) {
> + if (ptr->flags & IORESOURCE_MEM) {
> + ret = __add_issue_list(ptr->start, ptr->end,
> top);
> + if (ret)
> + return ret;
> + }
> +
Thanks, the comments below are helpful, but wouldn't it make sense to do
this as a recursive algorithm? Doesn't something like this do the same
thing (untested)?
int scan_foo(struct resource *res, ioremap_issue_list_t *list)
{
int ret;
if (res->child) {
ret = scan_foo(res->child, list);
if (ret)
return ret;
}
if (res->sibling) {
ret = scan_foo(res->sibling, list);
if (ret)
return ret;
}
if (ptr->flags & IORESOURCE_MEM) {
ret = __add_issue_list(res->start, res->end, list);
if (ret)
return ret;
}
return 0;
}
> + /* means that the "ptr" came here from parent (upper
> level) */
> + if (curr_lvl > prev_lvl) {
> + /* child exists, so go down to the child */
> + if (ptr->child != 0) {
> + ptr = ptr->child;
> + curr_lvl++;
> + prev_lvl++;
> + /* sibling exists, so go to the sibling */
> + } else if (ptr->sibling != 0) {
> + ptr = ptr->sibling;
> + /* no child and no sibling exist, so go up to
> parent */
> + } else {
> + ptr = ptr->parent;
> + curr_lvl--;
> + prev_lvl++;
> + if (curr_lvl == 0)
> + return 0;
> + }
> +
> + /* means that the "ptr" came here from child (lower
> level) */
> + } else if (curr_lvl < prev_lvl) {
> + /* sibling exists, so go to the sibling */
> + if (ptr->sibling != 0) {
> + ptr = ptr->sibling;
> + prev_lvl = curr_lvl - 1;
> + /* no sibling exists, so go up to parent */
> + } else {
> + ptr = ptr->parent;
> + curr_lvl--;
> + prev_lvl--;
> + if (curr_lvl == 0)
> + return 0;
> + }
> + } else {
> + printk(KERN_ERR "%s: Internal error. "
> + "Must not reach here\n", __FUNCTION__);
> + return -EFAULT;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +__issue_ioremap(struct ioremap_issue_list *top)
> +{
> + ioremap_issue_list_t *ptr, *tmp_ptr;
> + unsigned int offset;
> +
> + list_for_each_entry(ptr, &(top->listp), listp) {
> + 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_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
> + list_del(&(ptr->listp));
> + kfree(ptr);
> + }
These could be done in the same pass through the list.
> + return 0;
> +}
> +
> +static int
> +do_ioremap_on_resource_list(struct resource *ptr)
> +{
> + ioremap_issue_list_t *ioremap_issue_list_top;
> + int ret;
> +
> + ioremap_issue_list_top = kmalloc(sizeof(ioremap_issue_list_t),
> + GFP_KERNEL);
> + if (ioremap_issue_list_top == NULL)
> + return -ENOMEM;
The top of the list doesn't need to be an ioremap_issue_list_t, it can
just be a struct list. That could just be put on the stack rather than
dynamically allocated.
> +
> + INIT_LIST_HEAD(&(ioremap_issue_list_top->listp));
> +
If you took the recursive approach above, these could be eliminated,
just call scan_foo(ptr, &list).
> + if (ptr->child != 0) {
> + ret = __make_issue_list(ptr->child,
> ioremap_issue_list_top);
> + if (ret) {
memory leak - be careful to do list_del & kfree should anything fail
> + return ret;
> + }
> + }
> + if (ptr->sibling != 0) {
> + ret = __make_issue_list(ptr->sibling,
> ioremap_issue_list_top);
> + if (ret) {
memory leak - ditto
> + return ret;
> + }
> + }
> +
> + (void)__issue_ioremap(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)
> {
> @@ -374,6 +542,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 |