RE: [PATCH v3 1/4] iwlwifi: mei: add the driver to allow cooperation with CSME

On 8/7/2021 8:38 PM, Grumbach, Emmanuel wrote:

quoted

quoted

quoted

quoted

quoted

+     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_OPEN !=
+                  (u32)SAP_WIFI_AUTH_TYPE_OPEN);
+     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_RSNA !=
+                  (u32)SAP_WIFI_AUTH_TYPE_RSNA);
+     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_RSNA_PSK !=
+                  (u32)SAP_WIFI_AUTH_TYPE_RSNA_PSK);
+     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_SAE !=
+                  (u32)SAP_WIFI_AUTH_TYPE_SAE);
+
+     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_NONE !=
+                  (u32)SAP_WIFI_CIPHER_ALG_NONE);
+     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_CCMP !=
+                  (u32)SAP_WIFI_CIPHER_ALG_CCMP);
+     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_GCMP !=
+                  (u32)SAP_WIFI_CIPHER_ALG_GCMP);
+     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_GCMP_256 !=
+                  (u32)SAP_WIFI_CIPHER_ALG_GCMP_256);

These look just weird, and suspicious. You are using two different
enums but they have to be same values, or what?

Exactly. I don't want the userspace to have to include all the SAP
protocol header file. OTOH, I don't want to have to translate between
vendor commands attributes values and the SAP values.

Why not? I assume you would just need a helper function with switch
statements to "translate" between enums, not much more lines of code

but

quoted

quoted

a lot cleaner code.

I disagree that it'll give us a cleaner code.
What we'll have is two different enums with two different values and

functions

quoted

that translate from one value to another. This is very bug prone. When you

want

quoted

to debug, you get a value, you always need to think of what type of enum

you're

quoted

dealing with.
I believe your suggestion is not good, but since I am tired arguing I will do it

to

quoted

make you happy.

I don't want to flare up this fire further, but have you considered to
declare the enum values needed by user-space in uapi header and simply
declare the SAP enum using those values:

#include <uapi/iwlmei.h>

enum sap_cipher {
	SAP_WIFI_CIPHER_ALG_NONE = IWL_MEI_CIPHER_NONE,
	SAP_WIFI_CIPHER_ALG_CCMP = IWL_MEI_CIPHER_CCMP,
		:
};

Seems like assurance enough that user-space api is cleanly separated and
using the same values.

Yeah, I'll take that option. Although it is weird to have the CSME firmware API
values defined by the uapi, but I guess it is the best we can do here.
I'll need to add a comment on the uapi to make sure that if anybody wants to add
a value, he'll first check the CSME firmware value.

Thanks for the suggestion!

Kalle, anything else before I send v6?