2.4.21-ia64-030702 arch/ia64/kernel/unwind.c

This has been fixed once but must have slipped through the cracks. It only shows up when you turn on unwind debugging.

This has been fixed once but must have slipped through the cracks. It only shows up when you turn on unwind debugging.

Quoted 2 lines · Keith Owens

This has been fixed once but must have slipped through the cracks. It only shows up when you turn on unwind debugging.

And another one, again it only shows up with unwind debugging.

arch/ia64/kernel/unwind.c +1 -1

@@ -1955,7 +1955,7 @@
 		   "  bsp    0x%lx\n"
 		   "  sof    0x%lx\n"
 		   "  ip     0x%lx\n",
-		   info->bsp, sof, info->ip);
+		   __FUNCTION__, info->bsp, sof, info->ip);
 	find_save_locs(info);
 }
 

Patches to arch/ia64/kernel/mca.c against 2.4.21-ia64-030702.

Generic:
Issue a message to identify INIT/MCA records printed on the next boot.

gp passed to SAL_VECTOR_OS_MCA should be a physical address.

Enable console logging for INIT/MCA messages.

OEM data can be variable sized, do not use sizeof().

Convert #ifdef CONFIG_SGI_SN2 to ia64_platform_is("sn2") to allow the building of generic kernels.

Quoted 4 lines · Keith Owens

Patches to arch/ia64/kernel/mca.c against 2.4.21-ia64-030702.

Generic:
Issue a message to identify INIT/MCA records printed on the next boot.

ia64_log_print() already prints a note on the next boot if the log contains saved records. That note could be expanded, but the logic around the new return values seems too complicated to me.

Quoted 1 line

Enable console logging for INIT/MCA messages.

I guess this refers to the SGI-specific addition to ia64_log_print(). I'm hoping to decrease, not increase, the error record decoding code in the kernel. Particularly for INIT, this is code that is
(1) platform-specific, and (2) only used after a non-recoverable error. It seems better to move such code to an off-line environment.

Quoted 1 line

OEM data can be variable sized, do not use sizeof().

I don't quite understand this one. Don't these:

Review

Quoted 2 lines

(quoted diff) +1 -1

-				      (int)sizeof(sal_log_plat_specific_err_info_t) - 1,
+				      ((char*)psei->oem_data - (char*)psei),

compute the same value? I see that OEM data can be of variable size (and we use "u8 oem_data[1]" as a placeholder), but isn't this expression just computing the size of the fixed part of it?

Quoted 2 lines

Convert #ifdef CONFIG_SGI_SN2 to ia64_platform_is("sn2") to allow the building of generic kernels.

I don't see any conversion -- just the addition of several ia64_platform_is() checks

which I object to, especially when they change the behavior of things that are not obviously platform-dependent. For example, I think we should come up with a strategy for breaking SAL locks and clearing INIT logs that everybody can agree on. Error handling is confusing enough without making it platform-dependent.

Quoted 11 lines · Bjorn Helgaas

Quoted 1 line · Keith Owens

OEM data can be variable sized, do not use sizeof().

I don't quite understand this one. Don't these:

Review

Quoted 2 lines

(quoted diff) +1 -1

-				      (int)sizeof(sal_log_plat_specific_err_info_t) - 1,
+				      ((char*)psei->oem_data - (char*)psei),

compute the same value? I see that OEM data can be of variable size (and we use "u8 oem_data[1]" as a placeholder), but isn't this expression just computing the size of the fixed part of it?

platform_plat_specific_err_print() maps to ia64_log_prt_oem_data() which expects a header length and a section length. ia64_log_prt_oem_data() calculates the data length as the difference of the first two parameters.

Quoted 6 lines · Bjorn Helgaas

I don't see any conversion -- just the addition of several ia64_platform_is() checks which I object to, especially when they change the behavior of things that are not obviously platform-dependent. For example, I think we should come up with a strategy for breaking SAL locks and clearing INIT logs that everybody can agree on. Error handling is confusing enough without making it platform-dependent.

I agree, but so far only SN has any requirements for special processing in mca.c. Without a second platform, it is not clear what the coding model should be. I suggest you put in the sn2 changes and we review them when another platform has similar requirements.

(quoted diff) +5 -0

@@ -2076,6 +2141,11 @@ ia64_log_proc_dev_err_info_print (sal_lo
+		if (ia64_platform_is("sn2")) {
+			platform_plat_specific_err_print((int)slpi->header.len,
+			      0, (void*)slpi, prfunc);
+			return;
+		}
 

On Thursday 07 August 2003 6:41 pm, Keith Owens wrote:
I don't understand the changes well enough to be convinced that they're SGI-specific. For instance:

Review

Quoted 6 lines

(quoted diff) +6 -0

+#ifdef CONFIG_SMP
+    if (ia64_platform_is("sn2") && sal_lock.lock) {
+            udelay(500000);
+            sal_lock.lock = 0;
+    }
+#endif

ia64_sal_get_state_info() uses SAL_CALL(), which acquires sal_lock. So it seems that every platform would deadlock if we MCA in the middle of a previous SAL call. Or is there something peculiar about SGI firmware?

Review

Quoted 7 lines

(quoted diff) +7 -0

+    if (ia64_platform_is("sn2")) {
+            /* SN saves logs until next boot */
+            ;
+    } else {
+            /* Clear the INIT SAL logs now that they have been saved in the OS buffer */
+            ia64_sal_clear_state_info(SAL_INFO_TYPE_INIT);
+    }

You added back ia64_sal_clear_state_info() for all non-SGI platforms, which I don't think we want.

Quoted 6 lines

(quoted diff) +5 -0

@@ -2076,6 +2141,11 @@ ia64_log_proc_dev_err_info_print (sal_lo
+            if (ia64_platform_is("sn2")) {
+                    platform_plat_specific_err_print((int)slpi->header.len,
+                          0, (void*)slpi, prfunc);
+                    return;
+            }

Review ia64_log_print(int sal_info_type, prfunc · line 2377

Quoted 9 lines

(quoted diff) +7 -1

@@ -2301,7 +2371,13 @@ ia64_log_print(int sal_info_type, prfunc
-            prfunc("+MCA INIT ERROR LOG (UNIMPLEMENTED)\n");
+            if (ia64_platform_is("sn2")) {
+                    prfunc("+BEGIN HARDWARE ERROR STATE AT INIT\n");
+                    platform_err = ia64_log_platform_info_print(IA64_LOG_CURR_BUFFER(sal_info_type), prfunc);
+                    prfunc("+END HARDWARE ERROR STATE AT INIT\n");
+            } else {
+                    prfunc("+MCA INIT ERROR LOG (UNIMPLEMENTED)\n");
+            }

Is there any reason not to do these on all platforms? The "platform_plat_specific_err_print" looks a lot like a platform vector anyway, so I don't see the point of another test.

The current kernel code does not conform to those specifications. SAL_GET_STATE_INFO_SIZE is supposed to be reentrant safe so it does not deadlock, but the Linux code uses SAL_CALL() with a spinlock that will hang for SAL_GET_STATE_INFO_SIZE during an existing SAL event. I have hit this case.

The real fix is to change the non-conforming functions in sal.h to use SAL_CALL_NOLOCK. But is it safe to do so, can all the platforms handle reentrant SAL calls?

I know that SGI SAL code is reentrant safe so I break any existing SAL lock, but only on SN2. If the other platforms say that they are reentrant safe and the locking in sal.h is fixed then this sn2 specific patch is not required. Until then, we need it in the kernel.

Quoted 4 lines

I know that SGI SAL code is reentrant safe so I break any existing SAL lock, but only on SN2. If the other platforms say that they are reentrant safe and the locking in sal.h is fixed then this sn2 specific patch is not required. Until then, we need it in the kernel.

See the modification I made to these for cmc/cpe polling. I created
a new SAL call that runs with irqs enabled. SAL_{GET,CLEAR}_STATE_INFO* make use of them. Nobody has spoken up yet about any platforms that don't follow the SAL spec for these calls. Bjorn accepted the changes, but it doesn't look like they're in bk yet. They're in 2.6.0-test3. Does that eliminate this problem?