Re: [PATCH] SN: Add initial ACPI support

[Keith Owens <kaos@sgi.com>]

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.