|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-ia64-devel] [Patch 4/4] [RFC] Xwindow: Modify pci_acpi_scan_root()
Hi,
Several comments below. Thanks,
Alex
On Wed, 2007-05-30 at 14:46 +0900, Jun Kamada wrote:
> # HG changeset patch
> # User Jun Kamada <kama@xxxxxxxxxxxxxx>
> # Date 1180495840 -32400
> # Node ID 7d200afe519bb5c2f4155f3425588b9cabc6281c
> # Parent 1ee5f2385085473feee213f2ec10b8bf3b30d977
> [IA64] Issue ioremap hypercall in pci_acpi_scan_root() in order to
> map /dev/mem
>
> diff -r 1ee5f2385085 -r 7d200afe519b
> linux-2.6-xen-sparse/arch/ia64/pci/pci.c
> --- a/linux-2.6-xen-sparse/arch/ia64/pci/pci.c Wed May 30 12:30:24
> 2007 +0900
> +++ b/linux-2.6-xen-sparse/arch/ia64/pci/pci.c Wed May 30 12:30:40
> 2007 +0900
> @@ -29,6 +29,23 @@
> #include <asm/smp.h>
> #include <asm/irq.h>
> #include <asm/hw_irq.h>
> +
> +#ifdef CONFIG_XEN
This looks like a list, see below...
> +struct ioremap_issue_list {
> + unsigned long start;
> + unsigned long end;
> + struct ioremap_issue_list *next;
> +};
> +typedef struct ioremap_issue_list ioremap_issue_list_t;
> +
> +static int __add_issue_list(unsigned long, unsigned long,
> + ioremap_issue_list_t *);
> +static int __issue_ioremap(ioremap_issue_list_t *);
> +static int __make_issue_list(struct resource *,
> + ioremap_issue_list_t *);
> +static int do_ioremap_on_resource_list(struct resource *,
> +
> ioremap_issue_list_t *);
> +#endif
>
> /*
> * Low-level SAL-based PCI configuration access functions. Note that
> SAL
> @@ -341,6 +358,11 @@ pci_acpi_scan_root(struct acpi_device *d
> struct pci_bus *pbus;
> char *name;
> int pxm;
> +#ifdef CONFIG_XEN
> + ioremap_issue_list_t ioremap_issue_list_top = {
> + 0, 0, NULL
> + };
> +#endif
>
> controller = alloc_pci_controller(domain);
> if (!controller)
> @@ -375,6 +397,18 @@ pci_acpi_scan_root(struct acpi_device *d
> if (pbus)
> pcibios_setup_root_windows(pbus, controller);
>
> +#ifdef CONFIG_XEN
> + if (is_initial_xendomain()) {
> + if (do_ioremap_on_resource_list(&iomem_resource,
> + &ioremap_issue_list_top) !=
> 0) {
> + printk(KERN_ERR "%s: Counld not issue
> HYPERVISOR_ioremap "
> + "due to lack of memory or hypercall
> failure\n",
Looking at the functions below, it looks like a memory failure will
printk 3 times. Seems excessive. The hypercall failure isn't passed
back as an error though. see below.
> + __FUNCTION__);
> + goto out3;
> + }
> + }
> +#endif
> +
> return pbus;
>
> out3:
> @@ -384,6 +418,157 @@ out1:
> out1:
> return NULL;
> }
> +
Why aren't the below functions at the top of the file so they don't need
prototypes?
> +#ifdef CONFIG_XEN
> +static int
> +__add_issue_list(unsigned long start, unsigned long end,
> + ioremap_issue_list_t *top)
> +{
> + ioremap_issue_list_t *ptr, *new;
I suspect the below could be compressed into fewer cases with at least a
sanity check of 'end >= start' beforehand. Also, this looks a lot like
list handling, could it be simplified with linux/list.h?
> + for (ptr = top->next; ptr != NULL; ptr = ptr->next) {
> + if ((start <= ptr->start) &&
> + (end >= ptr->start) && (end <= ptr->end)) {
> + ptr->start = start;
> + goto out;
Just 'return 0;' for these. There's no point in a goto that does
nothing more than return.
> + } else if ((start >= ptr->start) && (start <=
> ptr->end) &&
> + end >= ptr->end) {
> + ptr->end = end;
> + goto out;
> + } else if ((start >= ptr->start) && (start <=
> ptr->end) &&
> + (end >= ptr->start) && (end <= ptr->end)) {
> + /* nothing to do */
> + goto out;
> + } else if ((start <= ptr->start) && (end >= ptr->end))
> {
> + ptr->start = start;
> + ptr->end = end;
> + goto out;
> + } else if ((start < ptr->start - 1) &&
> + (end == ptr->start - 1)) {
> + ptr->start = start;
> + goto out;
> + } else if ((start == ptr->end + 1) && (end > ptr->end
> + 1)) {
> + ptr->end = end;
> + goto out;
> + }
> + }
> +
> + 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__);
Would it be useful to print the range here so we can guess what might
not work?
> + return -1;
return -ENOMEM?
> + }
> + new->start = start;
> + new->end = end;
> + new->next = top->next;
> + top->next = new;
> +
> +out:
> + return 0;
> +}
> +
> +static int
> +__issue_ioremap(struct ioremap_issue_list *top)
> +{
> + ioremap_issue_list_t *ptr, *next;
> + unsigned int offset;
> +
> + if (top->next == NULL) {
> + return 0;
> + }
> +
It seems odd that we never use top->start & top->end. list.h could
clean this up nicely.
> + for (ptr = top->next; ptr != NULL; ptr = ptr->next) {
> + offset = HYPERVISOR_ioremap(ptr->start,
> + ptr->end - ptr->start +
> 1);
> + if (offset == ~0) {
> + printk(KERN_ERR "%s: HYPERVISOR_ioremap()
> failed\n",
> + __FUNCTION__);
> + }
> + }
Looks like an empty list bug here.
> + for (ptr = top->next, next = ptr->next;;
> + ptr = next, next = ptr->next) {
> + kfree(ptr);
> +
> + if (next == NULL)
> + break;
> + }
> +
> + 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;
> +
> + for ( ;; ) {
> + if (ptr->flags & IORESOURCE_MEM) {
> + if (__add_issue_list(ptr->start, ptr->end,
> top) != 0)
> + return -1;
If __add_issue_list() returns -ENOMEM as suggested above, should this
pass that on? For example
ret = __add_issue_list(ptr->start, ptr->end, top);
if (ret)
return ret;
Please add some comments on what the code below is trying to accomplish.
Comments are good ;)
> + }
> +
> + if (curr_lvl > prev_lvl) {
> + if (ptr->child != 0) {
> + ptr = ptr->child;
> + curr_lvl++;
> + prev_lvl++;
> + } else if (ptr->sibling != 0) {
> + ptr = ptr->sibling;
> + } else {
> + ptr = ptr->parent;
> + curr_lvl--;
> + prev_lvl++;
> + if (curr_lvl == 0)
> + goto out;
Another case where a direct return would suffice.
> + }
> + } else if (curr_lvl < prev_lvl) {
> + if (ptr->sibling != 0) {
> + ptr = ptr->sibling;
> + } else {
> + ptr = ptr->parent;
> + curr_lvl--;
> + prev_lvl--;
> + if (curr_lvl == 0)
> + goto out;
> + }
> + } else {
> + printk(KERN_ERR "%s: Internal error. "
> + "Must not reach here\n", __FUNCTION__);
return -EFAULT? or something more appropriate?
> + }
> + }
> +
> +out:
> + return 0;
> +}
> +
> +static int
> +do_ioremap_on_resource_list(struct resource *ptr,
> ioremap_issue_list_t *top) {
> + if (ptr->child != 0) {
> + if (__make_issue_list(ptr->child, top) != 0) {
> + printk(KERN_ERR "%s: Could not allocate
> memory. "
> + "HYPERVISOR_ioremap will not be issued
> \n",
> + __FUNCTION__);
no need to mention this twice, it's already printk'd by
__add_issue_list.
> + return -1;
return ret;?
> + }
> + }
> + if (ptr->sibling != 0) {
> + if (__make_issue_list(ptr->sibling, top) != 0) {
> + printk(KERN_ERR "%s: Could not allocate
> memory. "
> + "HYPERVISOR_ioremap will not be issued
> \n",
> + __FUNCTION__);
> + return -1;
> + }
> + }
> +
> + if (__issue_ioremap(top) != 0)
__issue_ioremap isn't coded to return anything but 0.
> + return -1;
> +
> + return 0;
> +}
> +#endif /* CONFIG_XEN */
>
> void pcibios_resource_to_bus(struct pci_dev *dev,
> struct pci_bus_region *region, struct resource *res)
--
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 |