From 08fec4c1ab5cb39972c35add097f33e2710fdced Mon Sep 17 00:00:00 2001 From: Tyler Erickson Date: Mon, 9 Sep 2024 10:55:45 -0600 Subject: [PATCH] bug: Fixing setting internal EPC struct for SAS devices causing invalid output Fixing how the VPD and Mode pages are read for EPC drives. Standby Y and Z were mixed up in a bitfield which resulted in incorrect output. There was a second bug where the mode page header was not taken into account so the standby timer values were not correctly accessed and causing invalid results. [Seagate/openSeaChest#155] Signed-off-by: Tyler Erickson --- include/opensea_operation_version.h | 2 +- src/power_control.c | 257 +++++++++++++++------------- 2 files changed, 141 insertions(+), 118 deletions(-) diff --git a/include/opensea_operation_version.h b/include/opensea_operation_version.h index 19a53c3..df12d13 100644 --- a/include/opensea_operation_version.h +++ b/include/opensea_operation_version.h @@ -26,7 +26,7 @@ extern "C" #define OPENSEA_OPERATION_MAJOR_VERSION 8 #define OPENSEA_OPERATION_MINOR_VERSION 0 -#define OPENSEA_OPERATION_PATCH_VERSION 1 +#define OPENSEA_OPERATION_PATCH_VERSION 2 #define OPENSEA_OPERATION_VERSION COMBINE_OPERATION_VERSIONS(OPENSEA_OPERATION_MAJOR_VERSION,OPENSEA_OPERATION_MINOR_VERSION,OPENSEA_OPERATION_PATCH_VERSION) diff --git a/src/power_control.c b/src/power_control.c index 6978238..9c76dcc 100644 --- a/src/power_control.c +++ b/src/power_control.c @@ -1130,15 +1130,15 @@ eReturnValues scsi_Set_Power_Conditions(tDevice *device, bool restoreAllToDefaul //CCF fields if (powerConditions->checkConditionFlags.ccfIdleValid && !powerConditions->checkConditionFlags.ccfIdleResetDefault) { - powerConditionsPage[mpStartOffset + 39] |= powerConditions->checkConditionFlags.ccfIdleMode << 6; + powerConditionsPage[mpStartOffset + 39] |= get_bit_range_uint8(powerConditions->checkConditionFlags.ccfIdleMode, 1, 0) << 6; } if (powerConditions->checkConditionFlags.ccfStandbyValid && !powerConditions->checkConditionFlags.ccfStandbyResetDefault) { - powerConditionsPage[mpStartOffset + 39] |= powerConditions->checkConditionFlags.ccfStandbyMode << 4; + powerConditionsPage[mpStartOffset + 39] |= get_bit_range_uint8(powerConditions->checkConditionFlags.ccfStandbyMode, 1, 0) << 4; } if (powerConditions->checkConditionFlags.ccfStopValid && !powerConditions->checkConditionFlags.ccfStopResetDefault) { - powerConditionsPage[mpStartOffset + 39] |= powerConditions->checkConditionFlags.ccfStopMode << 2; + powerConditionsPage[mpStartOffset + 39] |= get_bit_range_uint8(powerConditions->checkConditionFlags.ccfStopMode, 1, 0) << 2; } } //send the modified data to the drive @@ -2133,6 +2133,7 @@ static eReturnValues scsi_Get_EPC_Settings(tDevice *device, ptrEpcSettings epcSe { return BAD_PARAMETER; } + bool powerConditionVPDsupported = true; DECLARE_ZERO_INIT_ARRAY(uint8_t, epcVPDPage, VPD_POWER_CONDITION_LEN); if (SUCCESS == get_SCSI_VPD(device, POWER_CONDITION, M_NULLPTR, M_NULLPTR, true, epcVPDPage, VPD_POWER_CONDITION_LEN, M_NULLPTR)) { @@ -2167,8 +2168,18 @@ static eReturnValues scsi_Get_EPC_Settings(tDevice *device, ptrEpcSettings epcSe epcSettings->idle_c.nominalRecoveryTimeToActiveState = M_Max(1, epcSettings->idle_c.nominalRecoveryTimeToActiveState / 100); } } - //standby y + //standby z if (epcVPDPage[4] & BIT0) + { + epcSettings->standby_z.powerConditionSupported = true; + epcSettings->standby_z.nominalRecoveryTimeToActiveState = M_BytesTo2ByteValue(epcVPDPage[8], epcVPDPage[9]); + if (epcSettings->standby_z.nominalRecoveryTimeToActiveState > 0) + { + epcSettings->standby_z.nominalRecoveryTimeToActiveState = M_Max(1, epcSettings->standby_z.nominalRecoveryTimeToActiveState / 100); + } + } + //standby y + if (epcVPDPage[4] & BIT1) { epcSettings->standby_y.powerConditionSupported = true; epcSettings->standby_y.nominalRecoveryTimeToActiveState = M_BytesTo2ByteValue(epcVPDPage[10], epcVPDPage[11]); @@ -2177,113 +2188,131 @@ static eReturnValues scsi_Get_EPC_Settings(tDevice *device, ptrEpcSettings epcSe epcSettings->standby_y.nominalRecoveryTimeToActiveState = M_Max(1, epcSettings->standby_y.nominalRecoveryTimeToActiveState / 100); } } - //standby z - if (epcVPDPage[4] & BIT1) + } + else + { + powerConditionVPDsupported = false; + } + epcSettings->settingsAffectMultipleLogicalUnits = scsi_Mode_Pages_Shared_By_Multiple_Logical_Units(device, MP_POWER_CONDTION, 0); + //now time to read the mode pages for the other information (start with current, then saved, then default) + DECLARE_ZERO_INIT_ARRAY(uint8_t, epcModePage, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN); + for (eScsiModePageControl modePageControl = MPC_CURRENT_VALUES; modePageControl <= MPC_SAVED_VALUES; ++modePageControl) + { + safe_memset(epcModePage, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN, 0, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN); + bool gotData = false; + uint8_t headerLength = MODE_PARAMETER_HEADER_10_LEN; + if (SUCCESS == scsi_Mode_Sense_10(device, MP_POWER_CONDTION, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN, 0, true, false, modePageControl, epcModePage)) { - epcSettings->standby_z.powerConditionSupported = true; - epcSettings->standby_z.nominalRecoveryTimeToActiveState = M_BytesTo2ByteValue(epcVPDPage[8], epcVPDPage[9]); - if (epcSettings->standby_z.nominalRecoveryTimeToActiveState > 0) + gotData = true; + } + else if (SUCCESS == scsi_Mode_Sense_6(device, MP_POWER_CONDTION, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_6_LEN, 0, true, modePageControl, epcModePage)) + { + gotData = true; + headerLength = MODE_PARAMETER_HEADER_6_LEN; + } + if (gotData) + { + bool *idleAenabledBit = M_NULLPTR; + uint32_t *idleAtimerSetting = M_NULLPTR; + bool *idleBenabledBit = M_NULLPTR; + uint32_t *idleBtimerSetting = M_NULLPTR; + bool *idleCenabledBit = M_NULLPTR; + uint32_t *idleCtimerSetting = M_NULLPTR; + bool *standbyYenabledBit = M_NULLPTR; + uint32_t *standbyYtimerSetting = M_NULLPTR; + bool *standbyZenabledBit = M_NULLPTR; + uint32_t *standbyZtimerSetting = M_NULLPTR; + switch (modePageControl) { - epcSettings->standby_z.nominalRecoveryTimeToActiveState = M_Max(1, epcSettings->standby_z.nominalRecoveryTimeToActiveState / 100); + case MPC_CURRENT_VALUES: + idleAenabledBit = &epcSettings->idle_a.currentTimerEnabled; + idleAtimerSetting = &epcSettings->idle_a.currentTimerSetting; + idleBenabledBit = &epcSettings->idle_b.currentTimerEnabled; + idleBtimerSetting = &epcSettings->idle_b.currentTimerSetting; + idleCenabledBit = &epcSettings->idle_c.currentTimerEnabled; + idleCtimerSetting = &epcSettings->idle_c.currentTimerSetting; + standbyYenabledBit = &epcSettings->standby_y.currentTimerEnabled; + standbyYtimerSetting = &epcSettings->standby_y.currentTimerSetting; + standbyZenabledBit = &epcSettings->standby_z.currentTimerEnabled; + standbyZtimerSetting = &epcSettings->standby_z.currentTimerSetting; + //special case. If reading the current values, check the page for the PS bit (parameters savable) to note this for all power conditions. + if (epcModePage[headerLength + 0] & BIT7) + { + epcSettings->idle_a.powerConditionSaveable = true; + epcSettings->idle_b.powerConditionSaveable = true; + epcSettings->idle_c.powerConditionSaveable = true; + epcSettings->standby_y.powerConditionSaveable = true; + epcSettings->standby_z.powerConditionSaveable = true; + } + break; + case MPC_CHANGABLE_VALUES: + idleAenabledBit = &epcSettings->idle_a.powerConditionChangeable; + idleAtimerSetting = &epcSettings->idle_a.maximumTimerSetting; + idleBenabledBit = &epcSettings->idle_b.powerConditionChangeable; + idleBtimerSetting = &epcSettings->idle_b.maximumTimerSetting; + idleCenabledBit = &epcSettings->idle_c.powerConditionChangeable; + idleCtimerSetting = &epcSettings->idle_c.maximumTimerSetting; + standbyYenabledBit = &epcSettings->standby_y.powerConditionChangeable; + standbyYtimerSetting = &epcSettings->standby_y.maximumTimerSetting; + standbyZenabledBit = &epcSettings->standby_z.powerConditionChangeable; + standbyZtimerSetting = &epcSettings->standby_z.maximumTimerSetting; + break; + case MPC_DEFAULT_VALUES: + idleAenabledBit = &epcSettings->idle_a.defaultTimerEnabled; + idleAtimerSetting = &epcSettings->idle_a.defaultTimerSetting; + idleBenabledBit = &epcSettings->idle_b.defaultTimerEnabled; + idleBtimerSetting = &epcSettings->idle_b.defaultTimerSetting; + idleCenabledBit = &epcSettings->idle_c.defaultTimerEnabled; + idleCtimerSetting = &epcSettings->idle_c.defaultTimerSetting; + standbyYenabledBit = &epcSettings->standby_y.defaultTimerEnabled; + standbyYtimerSetting = &epcSettings->standby_y.defaultTimerSetting; + standbyZenabledBit = &epcSettings->standby_z.defaultTimerEnabled; + standbyZtimerSetting = &epcSettings->standby_z.defaultTimerSetting; + break; + case MPC_SAVED_VALUES: + idleAenabledBit = &epcSettings->idle_a.savedTimerEnabled; + idleAtimerSetting = &epcSettings->idle_a.savedTimerSetting; + idleBenabledBit = &epcSettings->idle_b.savedTimerEnabled; + idleBtimerSetting = &epcSettings->idle_b.savedTimerSetting; + idleCenabledBit = &epcSettings->idle_c.savedTimerEnabled; + idleCtimerSetting = &epcSettings->idle_c.savedTimerSetting; + standbyYenabledBit = &epcSettings->standby_y.savedTimerEnabled; + standbyYtimerSetting = &epcSettings->standby_y.savedTimerSetting; + standbyZenabledBit = &epcSettings->standby_z.savedTimerEnabled; + standbyZtimerSetting = &epcSettings->standby_z.savedTimerSetting; + break; + default: + continue; } - } - epcSettings->settingsAffectMultipleLogicalUnits = scsi_Mode_Pages_Shared_By_Multiple_Logical_Units(device, MP_POWER_CONDTION, 0); - //now time to read the mode pages for the other information (start with current, then saved, then default) - DECLARE_ZERO_INIT_ARRAY(uint8_t, epcModePage, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN); - for (eScsiModePageControl modePageControl = MPC_CURRENT_VALUES; modePageControl <= MPC_SAVED_VALUES; ++modePageControl) - { - memset(epcModePage, 0, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN); - bool gotData = false; - uint8_t headerLength = MODE_PARAMETER_HEADER_10_LEN; - if (SUCCESS == scsi_Mode_Sense_10(device, MP_POWER_CONDTION, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_10_LEN, 0, true, false, modePageControl, epcModePage)) - { - gotData = true; - } - else if (SUCCESS == scsi_Mode_Sense_6(device, MP_POWER_CONDTION, MP_POWER_CONDITION_LEN + MODE_PARAMETER_HEADER_6_LEN, 0, true, modePageControl, epcModePage)) - { - gotData = true; - headerLength = MODE_PARAMETER_HEADER_6_LEN; - } - if (gotData) - { - bool *idleAenabledBit = M_NULLPTR; - uint32_t *idleAtimerSetting = M_NULLPTR; - bool *idleBenabledBit = M_NULLPTR; - uint32_t *idleBtimerSetting = M_NULLPTR; - bool *idleCenabledBit = M_NULLPTR; - uint32_t *idleCtimerSetting = M_NULLPTR; - bool *standbyYenabledBit = M_NULLPTR; - uint32_t *standbyYtimerSetting = M_NULLPTR; - bool *standbyZenabledBit = M_NULLPTR; - uint32_t *standbyZtimerSetting = M_NULLPTR; - switch (modePageControl) - { - case MPC_CURRENT_VALUES: - idleAenabledBit = &epcSettings->idle_a.currentTimerEnabled; - idleAtimerSetting = &epcSettings->idle_a.currentTimerSetting; - idleBenabledBit = &epcSettings->idle_b.currentTimerEnabled; - idleBtimerSetting = &epcSettings->idle_b.currentTimerSetting; - idleCenabledBit = &epcSettings->idle_c.currentTimerEnabled; - idleCtimerSetting = &epcSettings->idle_c.currentTimerSetting; - standbyYenabledBit = &epcSettings->standby_y.currentTimerEnabled; - standbyYtimerSetting = &epcSettings->standby_y.currentTimerSetting; - standbyZenabledBit = &epcSettings->standby_z.currentTimerEnabled; - standbyZtimerSetting = &epcSettings->standby_z.currentTimerSetting; - //special case. If reading the current values, check the page for the PS bit (parameters savable) to note this for all power conditions. - if (epcModePage[headerLength + 0] & BIT7) + //idle a + if (epcModePage[headerLength + 3] & BIT1) + { + *idleAenabledBit = true; + if (powerConditionVPDsupported == false) + { + if (modePageControl == MPC_CHANGABLE_VALUES || modePageControl == MPC_CURRENT_VALUES) { - epcSettings->idle_a.powerConditionSaveable = true; - epcSettings->idle_b.powerConditionSaveable = true; - epcSettings->idle_c.powerConditionSaveable = true; - epcSettings->standby_y.powerConditionSaveable = true; - epcSettings->standby_z.powerConditionSaveable = true; + //special case, mostly for older drives before EPC. + //SCSI has supported this mode page for a while, so if it's the "changeable" page set "supported" bits for different power conditions. + epcSettings->idle_a.powerConditionSupported = true; } - break; - case MPC_CHANGABLE_VALUES: - idleAenabledBit = &epcSettings->idle_a.powerConditionChangeable; - idleAtimerSetting = &epcSettings->idle_a.maximumTimerSetting; - idleBenabledBit = &epcSettings->idle_b.powerConditionChangeable; - idleBtimerSetting = &epcSettings->idle_b.maximumTimerSetting; - idleCenabledBit = &epcSettings->idle_c.powerConditionChangeable; - idleCtimerSetting = &epcSettings->idle_c.maximumTimerSetting; - standbyYenabledBit = &epcSettings->standby_y.powerConditionChangeable; - standbyYtimerSetting = &epcSettings->standby_y.maximumTimerSetting; - standbyZenabledBit = &epcSettings->standby_z.powerConditionChangeable; - standbyZtimerSetting = &epcSettings->standby_z.maximumTimerSetting; - break; - case MPC_DEFAULT_VALUES: - idleAenabledBit = &epcSettings->idle_a.defaultTimerEnabled; - idleAtimerSetting = &epcSettings->idle_a.defaultTimerSetting; - idleBenabledBit = &epcSettings->idle_b.defaultTimerEnabled; - idleBtimerSetting = &epcSettings->idle_b.defaultTimerSetting; - idleCenabledBit = &epcSettings->idle_c.defaultTimerEnabled; - idleCtimerSetting = &epcSettings->idle_c.defaultTimerSetting; - standbyYenabledBit = &epcSettings->standby_y.defaultTimerEnabled; - standbyYtimerSetting = &epcSettings->standby_y.defaultTimerSetting; - standbyZenabledBit = &epcSettings->standby_z.defaultTimerEnabled; - standbyZtimerSetting = &epcSettings->standby_z.defaultTimerSetting; - break; - case MPC_SAVED_VALUES: - idleAenabledBit = &epcSettings->idle_a.savedTimerEnabled; - idleAtimerSetting = &epcSettings->idle_a.savedTimerSetting; - idleBenabledBit = &epcSettings->idle_b.savedTimerEnabled; - idleBtimerSetting = &epcSettings->idle_b.savedTimerSetting; - idleCenabledBit = &epcSettings->idle_c.savedTimerEnabled; - idleCtimerSetting = &epcSettings->idle_c.savedTimerSetting; - standbyYenabledBit = &epcSettings->standby_y.savedTimerEnabled; - standbyYtimerSetting = &epcSettings->standby_y.savedTimerSetting; - standbyZenabledBit = &epcSettings->standby_z.savedTimerEnabled; - standbyZtimerSetting = &epcSettings->standby_z.savedTimerSetting; - break; - default: - continue; - } - //idle a - if (epcModePage[headerLength + 3] & BIT1) - { - *idleAenabledBit = true; } - *idleAtimerSetting = M_BytesTo4ByteValue(epcModePage[headerLength + 4], epcModePage[headerLength + 5], epcModePage[headerLength + 6], epcModePage[headerLength + 7]); + } + *idleAtimerSetting = M_BytesTo4ByteValue(epcModePage[headerLength + 4], epcModePage[headerLength + 5], epcModePage[headerLength + 6], epcModePage[headerLength + 7]); + //standby z + if (epcModePage[headerLength + 3] & BIT0) + { + *standbyZenabledBit = true; + if (modePageControl == MPC_CHANGABLE_VALUES || modePageControl == MPC_CURRENT_VALUES) + { + //special case, mostly for older drives before EPC. + //SCSI has supported this mode page for a while, so if it's the "changeable" page set "supported" bits for different power conditions. + epcSettings->standby_z.powerConditionSupported = true; + } + } + *standbyZtimerSetting = M_BytesTo4ByteValue(epcModePage[headerLength + 8], epcModePage[headerLength + 9], epcModePage[headerLength + 10], epcModePage[headerLength + 11]); + if (epcModePage[headerLength + 1] > 0x0A)//before EPC this page was shorter, so do not try to access the rest of it as the data is invalid + { //idle b if (epcModePage[headerLength + 3] & BIT2) { @@ -2297,19 +2326,13 @@ static eReturnValues scsi_Get_EPC_Settings(tDevice *device, ptrEpcSettings epcSe } *idleCtimerSetting = M_BytesTo4ByteValue(epcModePage[headerLength + 16], epcModePage[headerLength + 17], epcModePage[headerLength + 18], epcModePage[headerLength + 19]); //standby y - if (epcModePage[2] & BIT0) + if (epcModePage[headerLength + 2] & BIT0) { *standbyYenabledBit = true; } *standbyYtimerSetting = M_BytesTo4ByteValue(epcModePage[headerLength + 20], epcModePage[headerLength + 21], epcModePage[headerLength + 22], epcModePage[headerLength + 23]); - //standby z - if (epcModePage[3] & BIT0) - { - *standbyZenabledBit = true; - } - *standbyZtimerSetting = M_BytesTo4ByteValue(epcModePage[headerLength + 8], epcModePage[headerLength + 9], epcModePage[headerLength + 10], epcModePage[headerLength + 11]); - ret = SUCCESS; } + ret = SUCCESS; } } return ret; @@ -2342,7 +2365,7 @@ static void print_Power_Condition(ptrPowerConditionInfo condition, const char *c { printf(" "); } - printf("%-12"PRIu32" ", condition->currentTimerSetting); + printf("%-12" PRIu32 " ", condition->currentTimerSetting); if (condition->defaultTimerEnabled) { printf("*"); @@ -2351,7 +2374,7 @@ static void print_Power_Condition(ptrPowerConditionInfo condition, const char *c { printf(" "); } - printf("%-12"PRIu32" ", condition->defaultTimerSetting); + printf("%-12" PRIu32 " ", condition->defaultTimerSetting); if (condition->savedTimerEnabled) { printf("*"); @@ -2360,8 +2383,8 @@ static void print_Power_Condition(ptrPowerConditionInfo condition, const char *c { printf(" "); } - printf("%-12"PRIu32" ", condition->savedTimerSetting); - printf("%-12"PRIu32" ", condition->nominalRecoveryTimeToActiveState); + printf("%-12" PRIu32 " ", condition->savedTimerSetting); + printf("%-12" PRIu32 " ", condition->nominalRecoveryTimeToActiveState); if (condition->powerConditionChangeable) { printf(" Y");