[coreboot-gerrit] Change in coreboot[master]: [RFC]src/soc/intel/common/block/pmc: Don't link get_int_option in SMM

[Angel Pons (Code Review)]

Attention is currently required from: Anjaneya "Reddy" Chagam, Patrick Rudolph, Jonathan Zhang, Johnny Lin, Tim Wawrzynczak, Morgan Jang, Andrey Petrov.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55114 )

Change subject: [RFC]src/soc/intel/common/block/pmc: Don't link get_int_option in SMM
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

File src/soc/intel/common/block/include/intelblocks/pmclib.h:

https://review.coreboot.org/c/coreboot/+/55114/comment/1b69aa24_328ebb0e 
PS1, Line 223: 				 const enum pmc_after_g3_power_state power_on_after_fail);
To avoid having to change each and every , I wouldn't change the signature of the `pmc_set_power_failure_state` function. Instead, add another function that takes the `state` parameter:

void pmc_set_power_failure_state(bool target_on)
 {
    const unsigned int state = get_uint_option("power_on_after_fail",
    				CONFIG_MAINBOARD_POWER_FAILURE_STATE);
    pmc_set_power_failure_state_val(target_on, state);
 }

void pmc_set_power_failure_state_val(bool target_on, const enum pmc_after_g3_power_state state)
 {
    bool on;

switch (state) {
    case MAINBOARD_POWER_STATE_OFF:
    	printk(BIOS_INFO, "Set power off after power failure.\n");
    	on = false;
    	break;
    case MAINBOARD_POWER_STATE_ON:
    	printk(BIOS_INFO, "Set power on after power failure.\n");
    	on = true;
    	break;
    case MAINBOARD_POWER_STATE_PREVIOUS:
    	printk(BIOS_INFO, "Keep power state after power failure.\n");
    	on = target_on;
    	break;
    default:
    	printk(BIOS_WARNING, "WARNING: Unknown power-failure state: %d\n", state);
    	on = false;
    	break;
    }

pmc_soc_set_afterg3_en(on);
 }

To prevent using `pmc_set_power_failure_state()` from within SMM, I can think of three options (sorted, as per my personal preferences, from best to worst):

1. If this works, simply force a linker error:

void pmc_set_power_failure_state(bool target_on)
 {
    /* Option API can't be safely used from within SMM */
    if (ENV_SMM)
    	dead_code();

const unsigned int state = get_uint_option("power_on_after_fail",
    				CONFIG_MAINBOARD_POWER_FAILURE_STATE);
    pmc_set_power_failure_state_val(target_on, state);
 }

2. Guard the function definition with some preprocessor:

/* Option API can't be safely used from within SMM */
 #if !ENV_SMM
 void pmc_set_power_failure_state(bool target_on)
 {
    const unsigned int state = get_uint_option("power_on_after_fail",
    				CONFIG_MAINBOARD_POWER_FAILURE_STATE);
    pmc_set_power_failure_state_val(target_on, state);
 }
 #endif

3. Put the function definition in another compilation unit. Where, though? This also breaks locality of reference. I don't like this idea.

File src/soc/intel/common/block/pmc/pmclib.c:

https://review.coreboot.org/c/coreboot/+/55114/comment/39a0c080_e21ac331 
PS1, Line 582: power_on_after_fail
How about `state` for the sake of brevity?