John Keller (on Sat, 14 Jan 2006 11:37:47 -0600 (CST)) wrote:
>First phase in introducing ACPI support to SN.
>Index: acpi_support/arch/ia64/sn/kernel/io_init.c
>===================================================================
>--- acpi_support.orig/arch/ia64/sn/kernel/io_init.c 2006-01-14
>09:26:29.761674100 -0600
>+++ acpi_support/arch/ia64/sn/kernel/io_init.c 2006-01-14 09:45:06.418505048
>-0600
>+#define SN_ACPI_BASE_SUPPORT (acpi_gbl_DSDT->oem_revision >= 0x20101)
These mini tests are conventionally defined as pseudo functions, i.e.
#define SN_ACPI_BASE_SUPPORT() ...
>+static void __init sn_acpi_setup(void);
Forward declarations do not include __init/__exit etc. I know that the
kernel has several examples of this, but they are wrong.
> static struct list_head sn_sysdata_list;
>
>@@ -157,26 +162,96 @@
Patches look nicer with function names in the '@@' line.
export QUILT_DIFF_OPTS=-p in ~/.bashrc.
>+inline uint64_t
>+sal_ioif_init(void)
Make that static with no inline, let gcc decide if it should be inlined
or not.
>+ struct ia64_sal_retval ret_stuff;
>+ ret_stuff.status = 0;
>+ ret_stuff.v0 = 0;
>+
>+ SAL_CALL_NOLOCK(ret_stuff,
>+ (u64) SN_SAL_IOIF_INIT,
>+ 0, 0, 0, 0, 0, 0, 0);
Is that really meant to be a NOLOCK call? Can SAL cope with another
SAL call being executed at the same time as SN_SAL_IOIF_INIT?
+static void __init
+sn_hubdev_init(struct hubdev_info *hubdev)
+{
+
+ struct sn_flush_device_list *sn_flush_device_list;
sn_flush_device_list no longer exists. Prarit Bhargava changed it in
https://www.redhat.com/archives/fedora-devel-list/2006-January/msg00123.html
and that patch is now in the IA64 git tree.
>+ if (!hubdev->hdi_flush_nasid_list.widget_p)
>+ return;
>+
>+ hubdev->hdi_flush_nasid_list.widget_p =
>+ kmalloc((HUB_WIDGET_ID_MAX + 1) *
>+ sizeof(struct sn_flush_device_list *), GFP_KERNEL);
Overwriting hubdev->hdi_flush_nasid_list.widget_p only if it is already
set does not look right. Should the test be this?
if (hubdev->hdi_flush_nasid_list.widget_p)
return;
>+ memset(hubdev->hdi_flush_nasid_list.widget_p, 0x0,
>+ (HUB_WIDGET_ID_MAX + 1) *
>+ sizeof(struct sn_flush_device_list *));
You memset hubdev->hdi_flush_nasid_list.widget_p without testing if
kmalloc succeeded :(.
BTW, replace all occurrences of kmalloc + memset with kzalloc.
>+ for (widget = 0; widget <= HUB_WIDGET_ID_MAX; widget++) {
>+ sn_flush_device_list = kmalloc(DEV_PER_WIDGET *
>+ sizeof(struct
>+ sn_flush_device_list),
>+ GFP_KERNEL);
>+ memset(sn_flush_device_list, 0x0,
>+ DEV_PER_WIDGET *
>+ sizeof(struct sn_flush_device_list));
Missing test if kmalloc succeeded.
>+ status = sal_get_widget_dmaflush_list(hubdev->hdi_nasid, widget,
>+ (uint64_t)
>+ __pa (sn_flush_device_list));
>+ if (status) {
>+ kfree(sn_flush_device_list);
>+ return;
>+ }
>+
>+ spin_lock_init(&sn_flush_device_list->sfdl_flush_lock);
The spinlock was moved out of sn_flush_device_list to its own
structure, so the prom is not exposed to changes in the size of kernel
structures. You need to (a) change sn_flush_device_list to
sn_flush_device_common and (b) allocate the related
sn_flush_device_kernel and format it.
At this point the patch does not make any sense. It only applies over
the top of Prarit's patch (i.e. against a recent ia64 git tree), but
you are deleting all the sn_flush_device_kernel code that Prarit
recently added and appear to be going back to the old
sn_flush_device_list code that Prarit removed because it broke the
prom.
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
