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

["Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>]

> On 8/7/2021 8:38 PM, Grumbach, Emmanuel wrote:
> >>
> >>>>> +     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.
> 

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?