Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 4FC3DC4338F
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [PATCH v6 6/7] iwlwifi: mvm: add vendor commands needed for iwlmei
References: <20210811105541.43426-1-emmanuel.grumbach@intel.com>
        <20210811105541.43426-6-emmanuel.grumbach@intel.com>
Date:   Mon, 18 Oct 2021 14:19:36 +0300
In-Reply-To: <20210811105541.43426-6-emmanuel.grumbach@intel.com> (Emmanuel
        Grumbach's message of "Wed, 11 Aug 2021 13:55:40 +0300")
Message-ID: <87czo2vauv.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:

> Add the vendor commands that must be used by the network manager
> to allow proper operation of iwlmei.
>
> * Send information on the AP CSME is connected to
> * Notify the userspace when roaming is forbidden
> * Allow the userspace to require ownership
>
> Co-Developed-by: Ayala Beker <ayala.beker@intel.com>
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

[...]

> --- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
> @@ -1952,16 +1952,16 @@ void iwl_mvm_enter_ctkill(struct iwl_mvm *mvm);
>  int iwl_mvm_send_temp_report_ths_cmd(struct iwl_mvm *mvm);
>  int iwl_mvm_ctdp_command(struct iwl_mvm *mvm, u32 op, u32 budget);
>  
> -#ifdef CONFIG_IWLMVM_VENDOR_CMDS
> +#if IS_ENABLED(CONFIG_IWLMEI)
>  
>  /* vendor commands */
>  void iwl_mvm_vendor_cmds_register(struct iwl_mvm *mvm);
>  
> -#else // IWLMVM_VENDOR_CMDS
> +#else // CONFIG_IWLMEI
>  
>  static inline void iwl_mvm_vendor_cmds_register(struct iwl_mvm *mvm) {}
>  
> -#endif // IWLMVM_VENDOR_CMDS
> +#endif // CONFIG_IWLMEI

No C++ style comments, please.

> --- /dev/null
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/vendor-cmd.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2012-2014, 2018-2021 Intel Corporation
> + * Copyright (C) 2013-2015 Intel Mobile Communications GmbH
> + * Copyright (C) 2016-2017 Intel Deutschland GmbH
> + */
> +#include "mvm.h"
> +#include <linux/nl80211-vnd-intel.h>
> +
> +static const struct nla_policy
> +iwl_mvm_vendor_attr_policy[NUM_IWL_MVM_VENDOR_ATTR] = {
> +	[IWL_MVM_VENDOR_ATTR_ROAMING_FORBIDDEN] = { .type = NLA_U8 },
> +	[IWL_MVM_VENDOR_ATTR_AUTH_MODE] = { .type = NLA_U32 },
> +	[IWL_MVM_VENDOR_ATTR_CHANNEL_NUM] = { .type = NLA_U8 },
> +	[IWL_MVM_VENDOR_ATTR_SSID] = { .type = NLA_BINARY,
> +				       .len = IEEE80211_MAX_SSID_LEN },
> +	[IWL_MVM_VENDOR_ATTR_BAND] = { .type = NLA_U8 },
> +	[IWL_MVM_VENDOR_ATTR_COLLOC_CHANNEL] = { .type = NLA_U8 },
> +	[IWL_MVM_VENDOR_ATTR_COLLOC_ADDR] = { .type = NLA_BINARY, .len = ETH_ALEN },
> +};
> +
> +__maybe_unused static struct nlattr **
> +iwl_mvm_parse_vendor_data(const void *data, int data_len)
> +{
> +	struct nlattr **tb;
> +	int err;
> +
> +	if (!data)
> +		return ERR_PTR(-EINVAL);
> +
> +	tb = kcalloc(MAX_IWL_MVM_VENDOR_ATTR + 1, sizeof(*tb), GFP_KERNEL);
> +	if (!tb)
> +		return ERR_PTR(-ENOMEM);
> +
> +	err = nla_parse(tb, MAX_IWL_MVM_VENDOR_ATTR, data, data_len,
> +			iwl_mvm_vendor_attr_policy, NULL);
> +	if (err) {
> +		kfree(tb);
> +		return ERR_PTR(err);
> +	}
> +
> +	return tb;
> +}

I was wondering why you had __maybe_unused but when noticed you are not
really using this function anywhere. So why not just remove it?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches