main_v2 - Optimize Crater Lake - sensor monitor by DelphineChiu · Pull Request #214 · facebook/OpenBIC

DelphineChiu · 2022-03-23T07:28:48Z

Summary:
- Move src/sensor/dev to common/dev
- Move sensor table from src/sensor/plat_sensor.c to src/platform/plat_sensor_table.c
- Move some functions in src/sensor/plat_sensor.c to common/sensor/sensor.c
- Move sdr table from src/sensor/plat_sdr.c to src/platform/plat_sdr_table.c
- Move some functions in src/sensor/plat_sdr.c to common/sensor/sdr.c
-Add 6 element in sensor config struct
- mux_address: i2c mux address in front of the device.
- mux_offset: mux offset to switch to the device.
- pre_sensor_read_fn: function to call before reading sensor value.
- pre_sensor_read_args: argument for pre_sensor_read_fn.
- post_sensor_read_fn: function to call after reading sensor value.
- post_sensor_read_args: argument for post_sensor_read_fn.
- The convert sensor reading to 4 bytes response data in ipmi
OEM_1S_ACCURACY_SENSNR handler.
- byte 0 ~ 1 is decimal part (LSB).
- byte 2 ~ 3 is fraction part (LSB).
- Revise sensor_poll stack size from 1000 to 2048.
- Revise gpio_workq stack size from 1024 to 2048.

facebook-github-bot · 2022-03-24T05:31:58Z

@GoldenBug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

GoldenBug · 2022-03-24T05:36:04Z

-		*reading = (acur_cal_MBR(sensor_num, val) / 1000) & 0xFFFF;
-		sensor_config[snrcfg_sensor_num].cache = *reading;
-		sensor_config[snrcfg_sensor_num].cache_status = SNR_READ_ACUR_SUCCESS;
+		*reading = (float)val * cfg->arg0 / cfg->arg1 / 1000;


Check if reading is null at beginning of function

Hi @GoldenBug,

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This problem will be solved in those PRs.
Let me create an issue #223 for tracking.

GoldenBug · 2022-03-24T05:38:27Z

+
+bool adm1278_read(uint8_t sensor_num, float *reading)
+{
+	snr_cfg *cfg = &sensor_config[sensor_config_index_map[sensor_num]];


I think we changed all these in the main branch but we'd like to keep all these consistent.

extend "snr" -> "sensor."

Hi @GoldenBug,

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This problem will be solved in those PRs.
Let me create an issue #223 for tracking.

GoldenBug · 2022-03-24T05:39:07Z

+		printf("Sensor number %x read fail\n", sensor_num);
+		return false;
+	}
+	*reading = val;


Check that reading isn't null before dereference.

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This problem will be solved in those PRs.
Let me create an issue #223 for tracking.

GoldenBug · 2022-03-24T05:40:32Z

+		} else if (sensor_num == SENSOR_NUM_PWR_HSCIN) {
+			// m = +6123 * Rsense(mohm), b = 0, R = -2
+			val = ((((msg.data[1] << 8) | msg.data[0]) * 100) / 1530.75);
+		}


else case here of bad sensor_num is passed in

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This problem will be solved in those PRs.
Let me create an issue #223 for tracking.

GoldenBug · 2022-03-24T05:41:49Z

+int isl69260_switch_page(uint8_t sensor_num, void *args)
+{
+	snr_cfg *cfg = &sensor_config[sensor_config_index_map[sensor_num]];
+	struct isl69260_page_data *page = args;


check if args is null?

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This problem will be solved in those PRs.
Let me create an issue #223 for tracking.

GoldenBug · 2022-03-24T19:41:17Z

+			keep_data_len += rx_len;
+		}
+		if (keep_data_len == fwupdate_data_len) {
+			while (k_msgq_put(&ipmi_msgq, &current_msg, K_NO_WAIT) != 0) {


Is it possible for this to loop forever?

We have fixed it and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-03-24T19:43:22Z

+				card_type_2ou = TYPE_2OU_DPV2_8;
+			} else if (i2c_msg.data[0] == TYPE_2OU_DPV2_16) { // in case the SKU exist
+				card_type_2ou = TYPE_2OU_DPV2_16;
+			}


either an else statement in case type isn't found or a switch statement with a default: tag

We have fixed it and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-03-24T19:44:14Z

+	}
+
+	memcpy(&IPMB_config_table[0], &pal_IPMB_config_table[0], sizeof(pal_IPMB_config_table));
+	return 1;


return true

This problem has been fixed in #195 .

GoldenBug · 2022-03-24T19:44:35Z

+	  RESERVED_ADDRESS, "RESERVED_ATTR", "RESERVED_ATTR" },
+};
+
+bool pal_load_ipmb_config(void)


This function always returns true. Should it be a void return type instead?

The return code is checked in ipmb_init() function.
We have fixed it and done the forced update.

GoldenBug · 2022-03-24T19:45:20Z

+	} else {
+		if (k_work_cancel_delayable(&SLP3_work) != 0) {


Make an } else if {

We have fixed it and done the forced update.
Please review it. Thanks.

facebook-github-bot · 2022-03-31T13:12:15Z

@DelphineChiu has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-04-01T17:48:20Z

@GoldenBug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

GoldenBug · 2022-04-04T23:50:54Z

 	init_adc_dev();
 	if (!(device_is_ready(dev_adc[0]) && device_is_ready(dev_adc[1]))) {
-		printk("ADC device not found\n");
+		printf("ADC device not found\n");


Print statement should identify which ADC device was not found in the event only one is missing.

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some
commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This
problem will be solved in those PRs.
Please check #223 .

GoldenBug · 2022-04-04T23:51:16Z

+		osDelay(1);
+	}
+
+	return 0;


Since this function always returns zero, I would recommend making it a void.

The optimization code is based on v2022.07.01. After that, the main branch has been phased in some
commits related to sensor monitoring. Those changes will be phased in main_v2 in the other PRs. This
problem will be solved in those PRs.
Please check #223 .

GoldenBug · 2022-04-04T23:52:25Z

+		}
+	}
+
+	printf("adm1278 initail fail\n");


We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-04T23:54:40Z

 		if (rx_len > 0) {
 			if (DEBUG_IPMB) {
-				printf("Received an IPMB message from bus(%d) data[%d](",
+				printf("Recieved an IPMB message from bus(%d) data[%d](",


We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-04T23:54:54Z

 					/* Notify the client about the new request */
 					if (DEBUG_IPMB) {
-						printf("[%s] Received a request message, netfn(0x%x) InfS(0x%x) seq_s(%d)\n",
+						printf("[%s] Recieved a request message, netfn(0x%x) InfS(0x%x) seq_s(%d)\n",


We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-05T00:00:02Z

+void add_full_sdr_table(SDR_Full_sensor add_item)
+{
+	if (get_sdr_index(add_item.sensor_num) != SENSOR_NUM_MAX) {
+		printf("add sensor num [%x] is already exists\n", add_item.sensor_num);


"add sensor num [%x] already exists\n"

We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-05T00:00:34Z

+		       sensor_num);
+		return;
+	}
+	if (mbr_type == MBR_M) {


This probably is better implemented as a switch statement.

We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-05T00:07:40Z

-			    SNR_READ_ACUR_SUCCESS) {
-			*reading = sensor_config[SnrNum_SnrCfg_map[sensor_num]].cache;
+	} else if (read_mode == GET_FROM_CACHE) {
+		if (sensor_config[sensor_config_index_map[sensor_num]].cache_status ==


Please change this to a switch/case statement

We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-05T00:08:06Z

+				return j;
+			} else if (i == sensor_config_num) {
+				return SENSOR_NUM_MAX;
+			}


Need else statement

Hi @GoldenBug,
We have fixed this and done the forced update.
Please review it. Thanks.

GoldenBug · 2022-04-05T00:09:29Z

+};
+
+snr_cfg fix_class2_sensor_config_table[] = {
+	/* number,                  type,       port,      address,      offset,


Are these tables placeholders for future work? If so, we should add a TODO to indicate the remaining work here (and in the next two empty tables as well)

We have fixed this and done the forced update.
Please review it. Thanks.

facebook-github-bot · 2022-04-07T06:54:03Z

@DelphineChiu has updated the pull request. You must reimport the pull request before landing.

Summary: - Move src/sensor/dev to common/dev - Move sensor table from src/sensor/plat_sensor.c to src/platform/plat_sensor_table.c - Move some functions in src/sensor/plat_sensor.c to common/sensor/sensor.c - Move sdr table from src/sensor/plat_sdr.c to src/platform/plat_sdr_table.c - Move some functions in src/sensor/plat_sdr.c to common/sensor/sdr.c -Add 6 element in sensor config struct - mux_address: i2c mux address in front of the device. - mux_offset: mux offset to switch to the device. - pre_sensor_read_fn: function to call before reading sensor value. - pre_sensor_read_args: argument for pre_sensor_read_fn. - post_sensor_read_fn: function to call after reading sensor value. - post_sensor_read_args: argument for post_sensor_read_fn. - The convert sensor reading to 4 bytes response data in ipmi OEM_1S_ACCURACY_SENSNR handler. - byte 0 ~ 1 is decimal part (LSB). - byte 2 ~ 3 is fraction part (LSB). - Revise sensor_poll stack size from 1000 to 2048. - Revise gpio_workq stack size from 1024 to 2048.

facebook-github-bot · 2022-04-07T07:39:36Z

@DelphineChiu has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-04-07T21:05:01Z

@GoldenBug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: - Move src/sensor/dev to common/dev - Move sensor table from src/sensor/plat_sensor.c to src/platform/plat_sensor_table.c - Move some functions in src/sensor/plat_sensor.c to common/sensor/sensor.c - Move sdr table from src/sensor/plat_sdr.c to src/platform/plat_sdr_table.c - Move some functions in src/sensor/plat_sdr.c to common/sensor/sdr.c -Add 6 element in sensor config struct - mux_address: i2c mux address in front of the device. - mux_offset: mux offset to switch to the device. - pre_sensor_read_fn: function to call before reading sensor value. - pre_sensor_read_args: argument for pre_sensor_read_fn. - post_sensor_read_fn: function to call after reading sensor value. - post_sensor_read_args: argument for post_sensor_read_fn. - The convert sensor reading to 4 bytes response data in ipmi OEM_1S_ACCURACY_SENSNR handler. - byte 0 ~ 1 is decimal part (LSB). - byte 2 ~ 3 is fraction part (LSB). - Revise sensor_poll stack size from 1000 to 2048. - Revise gpio_workq stack size from 1024 to 2048. Pull Request resolved: #214 Reviewed By: garnermic Differential Revision: D35106136 Pulled By: GoldenBug fbshipit-source-id: 810d6e8dd3d544d41f43372a193a4ef7aa6045cf

Summary: - Move src/sensor/dev to common/dev - Move sensor table from src/sensor/plat_sensor.c to src/platform/plat_sensor_table.c - Move some functions in src/sensor/plat_sensor.c to common/sensor/sensor.c - Move sdr table from src/sensor/plat_sdr.c to src/platform/plat_sdr_table.c - Move some functions in src/sensor/plat_sdr.c to common/sensor/sdr.c -Add 6 element in sensor config struct - mux_address: i2c mux address in front of the device. - mux_offset: mux offset to switch to the device. - pre_sensor_read_fn: function to call before reading sensor value. - pre_sensor_read_args: argument for pre_sensor_read_fn. - post_sensor_read_fn: function to call after reading sensor value. - post_sensor_read_args: argument for post_sensor_read_fn. - The convert sensor reading to 4 bytes response data in ipmi OEM_1S_ACCURACY_SENSNR handler. - byte 0 ~ 1 is decimal part (LSB). - byte 2 ~ 3 is fraction part (LSB). - Revise sensor_poll stack size from 1000 to 2048. - Revise gpio_workq stack size from 1024 to 2048. Pull Request resolved: #214 Reviewed By: garnermic Differential Revision: D35941625 Pulled By: GoldenBug fbshipit-source-id: 6026f97d3e707b2bde1987f4db29b73c93e3e281

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 23, 2022

GoldenBug suggested changes Mar 24, 2022

View reviewed changes

BonnieLo mentioned this pull request Mar 30, 2022

main_v2 - Optimize Crater Lake IPMB #195

Closed

DelphineChiu changed the title ~~Optimize Crater Lake - sensor monitor (main_v2)~~ Mar 31, 2022

BonnieLo force-pushed the PR/new-cl-sensor branch from 44437b2 to e654f7d Compare March 31, 2022 13:12

BonnieLo mentioned this pull request Mar 31, 2022

Move sensor monitoring changes from main to main_v2 #223

Closed

GoldenBug suggested changes Apr 5, 2022

View reviewed changes

BonnieLo force-pushed the PR/new-cl-sensor branch from e654f7d to 7a5a673 Compare April 7, 2022 06:54

BonnieLo force-pushed the PR/new-cl-sensor branch from 7a5a673 to 1d0a96a Compare April 7, 2022 07:39

This was referenced Apr 7, 2022

main_v2 - Optimize Crater Lake - Fix Warnings #224

Closed

main_v2 - Refactor Baseboard layer #225

Closed

GoldenBug closed this Apr 8, 2022

Uh oh!

Conversation

DelphineChiu commented Mar 23, 2022

facebook-github-bot commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 31, 2022

facebook-github-bot commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 7, 2022

facebook-github-bot commented Apr 7, 2022

facebook-github-bot commented Apr 7, 2022

Labels

5 participants