Add Quanta OEM code: ipmitool for DIMM slot readable feature #5

qctbmc · 2018-05-09T12:13:08Z

Hi, Mr. Amelkin:

I posted ticket with https://sourceforge.net/p/ipmitool/bugs/511/ on SourceForge.
I have forked ipmitool and try "Pull Request" to request merge to "ipmitool" code.

Thank you

include/ipmitool/helper.h

AlexanderAmelkin · 2018-05-10T13:50:19Z

lib/ipmi_oem.c

@@ -40,6 +40,8 @@
 static int ipmi_oem_supermicro(struct ipmi_intf * intf);
 static int ipmi_oem_ibm(struct ipmi_intf * intf);



Please remove this empty line between ibm and quanta.

AlexanderAmelkin · 2018-05-10T16:37:42Z

lib/ipmi_sel.c

+				return NULL;
+			} else if (rsp->ccode > 0) {
+				lprintf(LOG_ERR, " Error getting system info: %s",
+						val2str(rsp->ccode, completion_code_vals));


Please use tabs to indent to lprintf level, then spaces to align val2str to LOG_ERR or to the opening double quote. Please see https://github.com/ipmitool/ipmitool/wiki/Coding-Standards regarding indentation and alignment.

AlexanderAmelkin · 2018-05-10T16:38:05Z

lib/ipmi_sel.c

+				snprintf(desc, SIZE_OF_DESC, "CPU%d_%c%d",
+						 (data3 & 0xC0)>> 6,
+						 ((data3 & 0x38) >> 3)+0x41,						 
+						 (data3 & 0x07));


Please indent/align according to https://github.com/ipmitool/ipmitool/wiki/Coding-Standards

AlexanderAmelkin · 2018-05-10T16:44:07Z

lib/helper.c

@@ -1026,3 +1026,40 @@ ipmi_get_oem_id(struct ipmi_intf *intf)

 	return oem_id;
 }
+
+uint16_t
+ipmi_get_qct_paltform_id(struct ipmi_intf *intf)


This doesn't belong here. It must go to lib/ipmi_quantaoem.c just like it is done for sunoem, kontronoem, et al.

AlexanderAmelkin · 2018-05-10T16:45:46Z

include/ipmitool/ipmi.h

@@ -250,6 +250,7 @@ struct ipmi_rs {
 #define IPMI_BMC_SLAVE_ADDR		0x20
 #define IPMI_REMOTE_SWID		0x81

+#define IPMI_NETFN_QCTOEM		0x36


This doesn't belong here and must go to include/ipmitool/ipmi_quantaoem.h just like it is done for sunoem, et al.

qctbmc · 2018-05-23T12:19:32Z

Hi, Mr. Amelkin:

Thanks for your advice, I have fixed the typo and move Quanta's code to new files.
Please let me know if my code need to be modified.

Thanks

AlexanderAmelkin

Please fix the comments and make sure that your code compiles without warnings (warnings from existing header files do not count of course).

AlexanderAmelkin · 2018-05-29T12:50:06Z

include/ipmitool/ipmi_quantaoem.h

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2009, 2014, Oracle and/or its affiliates. All rights reserved.


Are you sure you want to put your new code under Oracle's copyright?
The disclaimer below isn't even attributed to Oracle and mentions SUN instead. I guess you borrowed it from some of the files with broken copyright/license statement.

Originally, it was a standard 2-clause BSD license (https://opensource.org/licenses/BSD-2-Clause).
Unless you work for Oracle, I think you need to use a copyright/license statement like the one used in lib/create_pen_list. Probably you want to specify Quanta or yourself as the copyright owner.

AlexanderAmelkin · 2018-05-29T12:53:15Z

lib/ipmi_quantaoem.c

@@ -0,0 +1,176 @@
+/*
+ * Copyright (c) 2009, 2014, Oracle and/or its affiliates. All rights reserved.


Same here. Oracle?

AlexanderAmelkin · 2018-05-29T12:54:38Z

lib/ipmi_quantaoem.c

+	req.msg.netfn = IPMI_NETFN_QCTOEM;
+	req.msg.cmd   = 0x65;	
+	req.msg.data = msg_data;	
+	req.msg.data_len = 4;


Always use sizeof() in such cases. There must be no way for discrepancy to sneak in later when someone decides to change the size of the structure but forgets to update the magic number here. Although this scenario is unlikely in this particular case, it's just a good habit to avoid duplicating data (and code) whenever possible.

Still need to fix this. Use sizeof(msg_data).

AlexanderAmelkin · 2018-05-29T12:58:00Z

lib/ipmi_quantaoem.c

+
+	memset(&req, 0, sizeof(req));
+	req.msg.netfn = IPMI_NETFN_QCTOEM;
+	req.msg.cmd   = 0x65;	


Don't use "magic number". Define a descriptive macro, e.g. QUANTA_GET_PLATFORM_ID.

AlexanderAmelkin · 2018-05-29T13:34:46Z

lib/ipmi_quantaoem.c

+		return 0;
+	}
+	oem_id = rsp->data[0];
+	lprintf(LOG_DEBUG,"Platform ID: %x", oem_id);


What's the purpose of having a 16-bit uint16_t oem_id to assign an 8-bit value to it and then print using an integer (32- or 64-bit) conversion specifier %x?

Besides, it looks like it is actually platform_id, not oem_id (which would be IPMI_OEM_QUANTA).

AlexanderAmelkin · 2018-05-29T14:11:49Z

lib/ipmi_sel.c

@@ -1244,6 +1244,9 @@ ipmi_get_oem_desc(struct ipmi_intf * intf, struct sel_event_record * rec)
 	case IPMI_OEM_SUPERMICRO_47488:
 		desc = get_supermicro_evt_desc(intf, rec);
 		break;
+	case IPMI_OEM_QUANTA:		
+		desc = get_quanta_evt_desc(intf, rec);


You get warning: implicit declaration of function ‘get_quanta_evt_desc’ here, and also (as a result) warning: implicit declaration of function ‘get_quanta_evt_desc’. Please fix.

AlexanderAmelkin · 2018-05-29T14:14:27Z

lib/ipmi_quantaoem.c

+	int data2;
+	int data3;
+	int sensor_type;
+	uint8_t i = 0;


This variable is unused.

AlexanderAmelkin · 2018-05-29T14:16:31Z

lib/ipmi_quantaoem.c

+				snprintf(desc, SIZE_OF_DESC, "CPU%d_%c%d",
+				         (data3 & 0xC0)>> 6,
+				         ((data3 & 0x38) >> 3)+0x41,						 
+				         (data3 & 0x07));


What are all these magic numbers? Macros with descriptive names are required.

AlexanderAmelkin · 2018-05-29T14:16:47Z

lib/ipmi_quantaoem.c

+			/* check the platform type */
+			oem_id = ipmi_get_qct_platform_id(intf);
+
+			if(oem_id == 0x02)


What is 0x02 ? A descriptive macro please.

AlexanderAmelkin · 2018-05-29T14:17:11Z

lib/ipmi_quantaoem.c

+	struct ipmi_rs *rsp;
+	struct ipmi_rq req;
+	char *desc = NULL;
+	int chipset_type = 0;


This variable is never used.

AlexanderAmelkin · 2018-06-17T08:39:12Z

@qctbmc, will you please fix it? It's out of sync with master already and the longer you wait, the more out of sync it will get.

qctbmc · 2018-07-03T08:59:10Z

Hi, Mr. Amelkin:

Sorry to reply late, I already sync with master code and modify Quanta's code from your comment.
Thanks for your advice, please let me know if my code need to be modified.

Thanks

AlexanderAmelkin · 2018-07-04T13:27:46Z

lib/ipmi_quantaoem.c

@@ -60,16 +61,18 @@
 #include <ipmitool/ipmi_quantaoem.h>
 #include <ipmitool/ipmi_raw.h>

-#define	SIZE_OF_DESC	128	/* Max Size of the description String to be displyed for the Each sel entry */
+#define	SIZE_OF_DESC		128	/* Max Size of the description String to be displyed for the Each sel entry */


Why so many spaces before and after macro name? Doesn't look aligned to anything.
Anyway, please avoid "pretty" formatting. It tends to deteriorate over time and is hard to maintain.
There is a clause on this in Coding Standards.

AlexanderAmelkin · 2018-07-04T13:29:08Z

lib/ipmi_quantaoem.c

+	req.msg.netfn = IPMI_NETFN_QCTOEM;
+	req.msg.cmd   = 0x65;	
+	req.msg.data = msg_data;	
+	req.msg.data_len = 4;


Still need to fix this. Use sizeof(msg_data).

AlexanderAmelkin · 2018-07-04T13:29:24Z

lib/ipmi_quantaoem.c

 	/* Check for the Standard Event type == 0x6F */
 	if (rec->sel_type.standard_type.event_type != 0x6F) {
 		return NULL;
 	}
 	/* Allocate mem for te Description string */
-	desc = malloc(sizeof(char) *SIZE_OF_DESC);
+	desc = malloc(sizeof(char) * SIZE_OF_DESC);


sizeof(char) is useless here. Your SIZE_OF_DESC is already in bytes. Size of a char is by definition 1 byte.

AlexanderAmelkin · 2018-07-04T13:31:30Z

lib/ipmi_quantaoem.c

 	if (desc == NULL) {
 		lprintf(LOG_ERR, "ipmitool: malloc failure");
 		return NULL;
 	}
-	memset(desc, '\0', SIZE_OF_DESC);
+	memset(desc, '0', SIZE_OF_DESC);


'\0' was 0x00, now '0' is 0x30. Is this really what you wanted? I thought you were going to set all bytes to 0, not to '0'. There is a difference.

AlexanderAmelkin · 2018-07-04T13:33:42Z

include/ipmitool/ipmi_quantaoem.h

+{
+	GRANTLY = 0,
+	PURLEY = 2
+} Code_Base;


Please call this enum type in an oem-specific way in order to avoid potential conflicts. And please don't use CamelCase in this project please. I've updated the Coding Standards on this topic.

Call it, for example, oem_qct_platform_id_t or oem_qct_platform_t.

AlexanderAmelkin · 2018-07-05T09:08:11Z

lib/ipmi_quantaoem.c

- * Copyright (c) 2009, 2014, Oracle and/or its affiliates. All rights reserved.
- *
+ * Copyright (c) 2018 Quanta Computer Inc. All Rights Reserved.
+ * 


This trailing space (and the one below) must not be here.

AlexanderAmelkin · 2018-07-05T09:16:24Z

include/ipmitool/ipmi_quantaoem.h

+ * INCIDENTAL OR PUNITIVE DAMAGES, HOWEVER CAUSED AND REGARDLESS OF THE 
+ * THEORY OF LIABILITY, ARISING OUT OF THE USE OF OR INABILITY TO USE 
+ * THIS SOFTWARE, EVEN IF SUN HAS BEEN ADVISED OF THE POSSIBILITY OF 
+ * SUCH DAMAGES.


Please in future avoid such re-formatting as they appear like major changes when in fact you only moved the line breaks.

Please remove the trailing whitespaces in this new block.

Please make sure your committed code doesn't contain traling whitespaces (including tabs) anywhere.

AlexanderAmelkin · 2018-07-05T09:17:53Z

lib/ipmi_quantaoem.c

 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
- *
+ * 


Trailing whitespace again. Here and below.

AlexanderAmelkin · 2018-07-05T09:18:17Z

lib/ipmi_quantaoem.c

@@ -80,7 +83,7 @@ ipmi_get_qct_platform_id(struct ipmi_intf *intf)

 	memset(&req, 0, sizeof(req));
 	req.msg.netfn = IPMI_NETFN_QCTOEM;
-	req.msg.cmd   = 0x65;	
+	req.msg.cmd   = BMC_GET_OEM_INFO;	


Trailing whitespace here.

AlexanderAmelkin · 2018-07-05T09:18:42Z

lib/ipmi_quantaoem.c

-				         (data3 & 0xC0)>> 6,
-				         ((data3 & 0x38) >> 3)+0x41,						 
-				         (data3 & 0x07));
+						 CPU_NUM( data ),			 


Trailing whitespace

AlexanderAmelkin · 2018-07-05T10:30:55Z

Also, your 'Update from ipmitool master' commit is not the right way to rebase to master.
To properly rebase use this on your branch, do the following:

git fetch ipmitool/master # provided that you have a remote called 'ipmitool'
git co master # this is your branch you requested to pull from
git rebase -i ipmitool/master

During the interactive rebase, drop your 'Update' commit, then resolve all
the conflicts and finish rebasing, then forcecefully update your branch:

git push -f

Test that you can do git merge --ff-only on ipmitool/master from your master branch:

git co -b usptream-master ipmitool/master
git merge --ff-only master

qctbmc · 2018-07-10T03:29:12Z

Hi, Mr. Amelkin:

Thanks for your great advice, I re-sync master ipmitool code and add my modify code.
Please let me know if my code need to be modified.

Thanks

AlexanderAmelkin · 2018-07-11T11:18:50Z

lib/ipmi_quantaoem.c

+#define CHANNEL_NUM(x)	(CHANNEL_BASE + CHANNEL_OFFSET(x))
+
+#define DIMM_MASK	0x07
+#define DIMM_NUM(x)	((x) & CPU_MASK)


CPU_MASK ? Really?

AlexanderAmelkin · 2018-07-11T11:19:50Z

lib/ipmi_quantaoem.c

+#define CHANNEL_BASE	0x41
+#define CHANNEL_SHIFT	3
+#define CHANNEL_MASK	0x07
+#define CHANNEL_OFFSET(x)	(((x) >> CHANNEL_SHIFT) & CHANNEL_MASK)


Don't use TABs for spaces or alignment! TABs are only for block level indentation.
You have lots of TABs throughout your code (in all files) where spaces must be used. This is not the only place. I will not comment the rest. Please fix them all. And get rid of all trailing whitespace (spaces and TABs alike).

AlexanderAmelkin · 2018-07-11T11:22:26Z

lib/ipmi_quantaoem.c

+	msg_data[0] = 0x4C;
+	msg_data[1] = 0x1C;
+	msg_data[2] = 0x00;
+	msg_data[3] = 0x02;


What are these magic numbers? Please use descriptive macros instead.

AlexanderAmelkin · 2018-07-11T11:25:43Z

lib/ipmi_quantaoem.c

+	    lprintf(LOG_ERR, "Get Platform ID command failed");
+	    return 0;
+	}
+	if (rsp->ccode > 0) {


it's uint8_t, and can't be negative, so this check is basically for non-zero. Hence, please use if (rsp->ccode) {.

AlexanderAmelkin · 2018-07-11T13:37:28Z

lib/ipmi_quantaoem.c

+	    return 0;
+	}
+	if (rsp->ccode > 0) {
+	    lprintf(LOG_ERR, "Get Platform ID command failed: %#x %s", rsp->ccode, val2str(rsp->ccode, completion_code_vals));


Please wrap to fit into 80 characters, align the wrapped line with spaces to the opening parenthesis.

AlexanderAmelkin · 2018-07-11T13:38:23Z

lib/ipmi_quantaoem.c

+#define	GET_PLATFORM_ID_DATA_SIZE	4
+
+uint8_t
+oem_qct_get_qct_platform_id(struct ipmi_intf *intf)


This is totally up to you, but I'd remove the second qct here.

AlexanderAmelkin · 2018-07-11T13:57:52Z

lib/ipmi_quantaoem.c

+	sensor_type = rec->sel_type.standard_type.sensor_type;
+	switch (sensor_type) {
+		case SENSOR_TYPE_MEMORY:
+					memset(&req, 0, sizeof (req));


Please refer to Coding Standards on switch formatting. This is NOT the way.

AlexanderAmelkin · 2018-07-11T13:59:41Z

lib/ipmi_quantaoem.c

+					    if (desc != NULL) {
+					    	free(desc);
+					    	desc = NULL;
+					    }


Please do Linux-kernel-style centralized cleanup, see here

AlexanderAmelkin · 2018-07-11T14:00:55Z

lib/ipmi_quantaoem.c

+					/* check the platform type */
+					platform_id = oem_qct_get_qct_platform_id(intf);
+					if(OEM_QCT_PLATFORM_PURLEY == platform_id) {
+					   snprintf(desc, SIZE_OF_DESC, "CPU%d_%c%d",


Is this an indent? Doesn't look so. TAB is required.

qctbmc · 2018-07-27T13:54:53Z

Hi, Mr. Amelkin:

Thanks for your great advice, I re-sync master ipmitool code and add my modify code.
Please let me know if my code need to be modified.

Thanks

AlexanderAmelkin · 2018-07-28T16:15:41Z

Ok, I have fixed some more minor issues and merged this.

JPlanche · 2022-05-30T10:17:33Z

I am very sceptical about this patch, it gives me wrong DIMM locations for UE/CE (vs. the technical PDF QCT support gave me) (server is T22HF-1U).
In fact I think it should not be used as is.

AlexanderAmelkin · 2022-09-02T12:00:48Z

@JPlanche, well, I believe the code was sumbitted by Quanta Cloud Technology BMC team, so I assumed they know what they do about their products. If you think there is a bug, please submit your own PR with a fix or at least create a separate issue. Commenting a merged PR is quite useless in terms of getting the problem fixed. You may reference this PR from your issue or new PR if you want though.

AlexanderAmelkin suggested changes May 10, 2018

View reviewed changes

AlexanderAmelkin suggested changes May 29, 2018

View reviewed changes

AlexanderAmelkin suggested changes Jul 5, 2018

View reviewed changes

qctbmc closed this Jul 9, 2018

qctbmc force-pushed the master branch from 2efd21b to 0310208 Compare July 9, 2018 07:58

qctbmc reopened this Jul 10, 2018

AlexanderAmelkin suggested changes Jul 11, 2018

View reviewed changes

qctbmc force-pushed the master branch from ffcd195 to 172eeb4 Compare July 27, 2018 12:56

oem: Add basic support for Quanta

f134708

AlexanderAmelkin approved these changes Jul 28, 2018

View reviewed changes

AlexanderAmelkin merged commit 5c033c0 into ipmitool:master Jul 28, 2018

utkarsh2102 mentioned this pull request Jul 1, 2021

Time for a release, maybe? #303

Closed

JPlanche mentioned this pull request Dec 16, 2022

Wrong DIMMs given in SEL with Quanta QCT HW #385

Open

		@@ -40,6 +40,8 @@
		static int ipmi_oem_supermicro(struct ipmi_intf * intf);
		static int ipmi_oem_ibm(struct ipmi_intf * intf);

		@@ -0,0 +1,47 @@
		/*
		* Copyright (c) 2009, 2014, Oracle and/or its affiliates. All rights reserved.

		@@ -0,0 +1,176 @@
		/*
		* Copyright (c) 2009, 2014, Oracle and/or its affiliates. All rights reserved.

Add Quanta OEM code: ipmitool for DIMM slot readable feature #5

Add Quanta OEM code: ipmitool for DIMM slot readable feature #5

Conversation

qctbmc commented May 9, 2018

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

qctbmc commented May 23, 2018

AlexanderAmelkin left a 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

AlexanderAmelkin commented Jun 17, 2018

qctbmc commented Jul 3, 2018

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

AlexanderAmelkin commented Jul 5, 2018

qctbmc commented Jul 10, 2018

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

qctbmc commented Jul 27, 2018

AlexanderAmelkin commented Jul 28, 2018

JPlanche commented May 30, 2022 • edited

AlexanderAmelkin commented Sep 2, 2022

JPlanche commented May 30, 2022 •

edited