Page MenuHomeFreeBSD

kdb: set kdb_why when entered via reboot and panic
ClosedPublic

Authored by thj on Mar 14 2022, 5:05 PM.
Tags
None
Referenced Files
F102203577: D34551.diff
Fri, Nov 8, 10:17 PM
Unknown Object (File)
Wed, Oct 30, 6:38 PM
Unknown Object (File)
Sep 27 2024, 10:15 PM
Unknown Object (File)
Sep 27 2024, 2:38 AM
Unknown Object (File)
Sep 27 2024, 12:25 AM
Unknown Object (File)
Sep 24 2024, 4:42 AM
Unknown Object (File)
Sep 23 2024, 2:28 PM
Unknown Object (File)
Sep 20 2024, 3:18 PM
Subscribers

Details

Summary

Set the kdb_why reason when we enter via the reboot and panic paths. Add a
reason for kdb_why in the reboot path.

The original downstream diff has this comment explaining why kdb_why has to be
set earlier. To me this move seems harmless for us and it helps then reduce
their diff. I am happy to add an explanation, but I am not really sure it is
needed in kdb_enter.

 void                                                                        
 kdb_enter(const char *why, const char *msg)                                 
 {                                                                           
                                                                             
        if (kdb_dbbe != NULL && kdb_active == 0) {                           
+               /* Needs to happen early because we added a KASSERT in cnputs
+                  which fails unless kdb_why is set. cnputs is called by    
+                  printf. */                                                
+               kdb_why = why;                                               
                if (msg != NULL)                                             
                        printf("KDB: enter: %s\n", msg);                     
-               kdb_why = why;                                               
                breakpoint();                                                
                kdb_why = KDB_WHY_UNSET;                                     
        }                                                                    
 }

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 44766
Build 41654: arc lint + arc unit

Event Timeline

thj requested review of this revision.Mar 14 2022, 5:05 PM
sys/kern/subr_kdb.c
306

I don't think shutdown_nice() actually enters kdb. Do we need this WHY?

sys/kern/subr_kdb.c
306

I suspect that downstream wants to check kdb_why in the cnputc() called from printf(). I don't know though of the value of checking kdb_why in cnputc() if kdb hasn't been entered yet? Understanding the reason for that KASSERT may help understand this change better.

sys/kern/subr_kdb.c
306

They allow cnputsn to print with interrupts enabled. To avoid deadlock that have special cased some subsystems which handle deadlock on their own. They assert that kdb_why is not KDB_WHY_UNSET when running with interrupts enabled in cnputsn.

This part of their change is very specific to their code base.

I guess this is fine. The changes for kdb_panic and kdb_reboot do indeed seem a bit gratuitous and non-obvious, so there is some risk they get broken in the future perhaps. OTOH, this file doesn't change much.

This revision is now accepted and ready to land.Mar 25 2022, 5:44 PM

Note that an alternative would be to have some other special variable that gets set in other places and have the KASSERT be something like KASSERT(new_var || kdb_why != KDB_WHY_UNSENT). That would be a downstream-only approach of course, but would avoid needing to move kdb_why around downstream.