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

+     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
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
that translate from one value to another. This is very bug prone. When you want
to debug, you get a value, you always need to think of what type of enum you're
dealing with.
I believe your suggestion is not good, but since I am tired arguing I will do it to
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.

Regards,
Arend

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.