RE: [PATCH RESEND 2/3] iwlwifi: mei: add the driver to allow cooperation with CSME

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

> 
> On Mon, Apr 12, 2021 at 03:43:27PM +0300, Emmanuel Grumbach wrote:
> > iwlmei is a driver that handles the communication with the Wireless
> > driver of the CSME firmware.
> > More details in the documentation included in this patch.
> >
> > Co-Developed-by: Ayala Beker <ayala.beker@intel.com>
> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/Kconfig    |   13 +
> >  drivers/net/wireless/intel/iwlwifi/Makefile   |    2 +
> >  .../net/wireless/intel/iwlwifi/mei/Makefile   |    8 +
> >  .../net/wireless/intel/iwlwifi/mei/internal.h |   20 +
> >  .../net/wireless/intel/iwlwifi/mei/iwl-mei.h  |  440 ++++
> > drivers/net/wireless/intel/iwlwifi/mei/main.c | 2026 +++++++++++++++++
> > drivers/net/wireless/intel/iwlwifi/mei/net.c  |  409 ++++
> > drivers/net/wireless/intel/iwlwifi/mei/sap.h  |  736 ++++++
> >  .../wireless/intel/iwlwifi/mei/trace-data.h   |   69 +
> >  .../net/wireless/intel/iwlwifi/mei/trace.c    |   15 +
> >  .../net/wireless/intel/iwlwifi/mei/trace.h    |   62 +
> >  11 files changed, 3800 insertions(+)
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/Makefile
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/internal.h
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/iwl-mei.h
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/main.c
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/net.c
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/sap.h
> >  create mode 100644
> > drivers/net/wireless/intel/iwlwifi/mei/trace-data.h
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/trace.c
> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mei/trace.h
> >
> > diff --git a/drivers/net/wireless/intel/iwlwifi/Kconfig
> > b/drivers/net/wireless/intel/iwlwifi/Kconfig
> > index 1085afbefba8..eb68f93bbe90 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/Kconfig
> > +++ b/drivers/net/wireless/intel/iwlwifi/Kconfig
> > @@ -92,6 +92,19 @@ config IWLWIFI_BCAST_FILTERING
> >  	  If unsure, don't enable this option, as some programs might
> >  	  expect incoming broadcasts for their normal operations.
> >
> > +config IWLMEI
> > +	tristate "Enable V-PRO for WLAN"
> > +	depends on INTEL_MEI
> > +	depends on IWLMVM
> > +	help
> > +	  Enable V-PRO. This allows to communicate with the CSME firmware.
> > +	  This is supported starting from Tiger Lake platforms and has been
> > +	  tested on 9260 devices only. Enabling this option on a platform that
> > +	  has a different device and has Wireless enabled on AMT can
> prevent
> > +	  WiFi from working correctly.
> 
> What does any of that mean?  I can't understand it at all, and here I thought I
> understand new hardware terms :(
> 
> > +
> > +	  If unsure, say N.
> 
> module name?
> 
> 
> > +
> >  menu "Debugging Options"
> >
> >  config IWLWIFI_DEBUG
> > diff --git a/drivers/net/wireless/intel/iwlwifi/Makefile
> > b/drivers/net/wireless/intel/iwlwifi/Makefile
> > index 14b0db28143b..ff4d4b651c3e 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/Makefile
> > +++ b/drivers/net/wireless/intel/iwlwifi/Makefile
> > @@ -30,4 +30,6 @@ ccflags-y += -I$(src)
> >  obj-$(CONFIG_IWLDVM)	+= dvm/
> >  obj-$(CONFIG_IWLMVM)	+= mvm/
> >
> > +obj-$(CONFIG_IWLMEI)	+= mei/
> 
> Cuddle up against the line above?
> 
> > --- /dev/null
> > +++ b/drivers/net/wireless/intel/iwlwifi/mei/main.c
> > @@ -0,0 +1,2026 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> 
> I didn't think that Intel was doing dual-license for .c files in the kernel,
> especially for ones that call GPL-only kernel symbols :(
> 
> The "loonacy" of doing this for .h files kind of makes sense if you really
> squint, but even then, it's crazy.  But for .c files like this, that's not good.
> 
> You all have the right to license the code under whatever you want, so why
> keep a BSD license on it when it goes into the kernel tree?
> 
> In the past, I have been told by various Intel lawyers/legal-types that they
> were not going to do this anymore.  I know things change, but are you sure
> that this is ok with the current corporate position of Linux kernel code that
> calls other parts of the core kernel like this?
> 
> Same for all new files in this patchset.
> 
> 
> > +/*
> > + * Copyright (C) 2021 Intel Corporation  */
> > +
> > +#include <linux/etherdevice.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/rtnetlink.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/mei_cl_bus.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/wait.h>
> > +#include <linux/slab.h>
> > +#include <linux/mm.h>
> > +
> > +#include "internal.h"
> > +#include "iwl-mei.h"
> > +#include "trace.h"
> > +#include "trace-data.h"
> > +#include "sap.h"
> > +
> > +MODULE_DESCRIPTION("The Intel(R) wireless / CSME firmware
> > +interface"); MODULE_LICENSE("GPL");
> > +
> > +#define CHECK_FOR_NEWLINE(f) BUILD_BUG_ON(f[sizeof(f) - 2] != '\n')
> > +
> > +#define IWL_MEI_DEBUG(c, f, a...)		\
> > +	do {					\
> > +		CHECK_FOR_NEWLINE(f);		\
> 
> Huh?
> 
> > +		dev_dbg(&(c)->dev, f, ## a);	\
> 
> Just use dev_dbg(), don't be special for a single driver, it hurts when trying to
> read different drivers.

I took this from iwlwifi. I can change if needed, not a big deal.

> 
> 
> > +	} while (0)
> > +
> > +#define IWL_MEI_INFO(c, f, a...)		\
> > +	do {					\
> > +		CHECK_FOR_NEWLINE(f);		\
> > +		dev_info(&(c)->dev, f, ## a);	\
> > +	} while (0)
> > +
> > +#define IWL_MEI_ERR(c, f, a...)			\
> > +	do {					\
> > +		CHECK_FOR_NEWLINE(f);		\
> > +		dev_err(&(c)->dev, f, ## a);	\
> > +	} while (0)
> 
> Same for all of the above, just use dev_info() and dev_err() please.

Sure
> 
> > +
> > +#define MEI_WLAN_UUID UUID_LE(0x13280904, 0x7792, 0x4fcb, \
> > +			      0xa1, 0xaa, 0x5e, 0x70, 0xcb, 0xb1, 0xe8, 0x65)
> > +
> > +/*
> > + * Since iwlwifi calls iwlmei without any context, hold a pointer to
> > +the
> > + * mei_cl_device structure here.
> > + * Define a mutex that will synchronize all the flows between iwlwifi
> > +and
> > + * iwlmei.
> > + */
> > +static struct mei_cl_device *iwl_mei_global_cldev; static
> > +DEFINE_MUTEX(iwl_mei_mutex); static unsigned long iwl_mei_status; #if
> > +IS_ENABLED(CONFIG_DEBUG_FS) static bool defer_start_message;
> > +
> > +module_param_named(defer_start_message, defer_start_message,
> bool,
> > +0644); MODULE_PARM_DESC(defer_start_message,
> > +		 "Defer the start message Tx to CSME (default false)");
> 
> Why do you need this?  Who is going to set it to anything else, and why
> would they?  This isn't the 1990's anymore, please do not add new module
> parameters.

For testing. I need this to be able to force a certain order of initialization which is possible (and hence must be tested) but not likely to happen.
Another point is tracing. This allows me to load the module but prevent any real operation. Then, start tracing. This way, I can see the whole flow in tracing, even the very beginning.

> 
> 
> > +#endif
> > +
> > +enum iwl_mei_status_bits {
> > +	IWL_MEI_STATUS_SAP_CONNECTED,
> > +};
> > +
> > +bool iwl_mei_is_connected(void)
> > +{
> > +	return test_bit(IWL_MEI_STATUS_SAP_CONNECTED,
> &iwl_mei_status); }
> > +EXPORT_SYMBOL(iwl_mei_is_connected);
> 
> EXPORT_SYMBOL_GPL()?  I have to ask... :)
> 
> 
> > +struct iwl_mei {
> > +	wait_queue_head_t get_nvm_wq;
> > +	struct work_struct send_csa_msg_wk;
> > +	wait_queue_head_t get_ownership_wq;
> > +	struct iwl_mei_shared_mem_ptrs shared_mem;
> > +	struct mei_cl_device *cldev;
> > +	struct iwl_mei_nvm *nvm;
> > +	struct iwl_mei_filters __rcu *filters;
> > +	bool got_ownership;
> > +	bool amt_enabled;
> > +	bool csa_throttled;
> > +	bool csme_taking_ownership;
> > +	struct delayed_work csa_throttle_end_wk;
> > +	spinlock_t data_q_lock;
> > +
> > +	atomic_t sap_seq_no;
> > +	atomic_t seq_no;
> > +
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +	struct dentry *dbgfs_dir;
> > +	wait_queue_head_t debugfs_wq;
> > +	bool ping_pending;
> > +#endif
> 
> Why #ifdef?  Does the size really matter?

Size. I can drop those.

> 
> > +static ssize_t iwl_mei_write_cyclic_buf(struct mei_cl_device *cldev,
> > +					struct iwl_sap_q_ctrl_blk *notif_q,
> > +					u8 *q_head,
> > +					const struct iwl_sap_hdr *hdr)
> > +{
> > +	u32 rd = le32_to_cpu(READ_ONCE(notif_q->rd_ptr));
> > +	u32 wr = le32_to_cpu(READ_ONCE(notif_q->wr_ptr));
> > +	u32 q_sz = le32_to_cpu(notif_q->size);
> > +	size_t room_in_buf;
> > +	size_t tx_sz = sizeof(*hdr) + le16_to_cpu(hdr->len);
> > +
> > +	if (WARN_ON(rd > q_sz || wr > q_sz))
> > +		return -EINVAL;
> 
> If any of the WARN_ON() things in this driver can ever trigger, just handle
> them normally and do NOT call WARN_ON() as you just rebooted the box for
> a simple thing that you could have recovered from.

My understanding is that WARN_ON does not reboot the box which is why it was invented when we already had BUG_ON. This can't be triggered by the user space, but can be triggered by the CSME firmware that runs on the chipset.

> 
> Some of these WARN_ON() in the code feel very lazy as you control the
> caller so you "know" that this will never happen.  So don't leave them in, it's
> debugging code that you can now remove.

I can transform them in error prints, but again, my understanding is / was that WARN_ON is ok to use and won't reboot the box since it differs from BUG_ON.

> 
> > +
> > +	room_in_buf = wr >= rd ? q_sz - wr + rd : rd - wr;
> > +
> > +	/* we don't have enough room for the data to write */
> > +	if (WARN_ON(room_in_buf < tx_sz))
> > +		return -ENOSPC;
> 
> Can userspace trigger this?  If so, again, you just crashed the machine when
> panic_on_warn is enabled.
> 
> Just do a real check and handle the issue.

I'll change to a simple print.

> 
> > +static void
> > +iwl_mei_handle_rx_start_ok(struct mei_cl_device *cldev,
> > +			   const struct iwl_sap_me_msg_start_ok *rsp,
> > +			   ssize_t len)
> > +{
> > +	struct iwl_mei *mei = mei_cldev_get_drvdata(cldev);
> > +
> > +	if (len != sizeof(*rsp)) {
> > +		IWL_MEI_ERR(cldev,
> > +			    "got invalid SAP_ME_MSG_START_OK from CSME
> firmware\n");
> > +		IWL_MEI_ERR(cldev,
> > +			    "size is incorrect: %ld instead of %lu\n",
> > +			    len, sizeof(*rsp));
> > +		return;
> > +	}
> > +
> > +	if (rsp->supported_version != SAP_VERSION) {
> > +		IWL_MEI_ERR(cldev,
> > +			    "didn't get the expected version: got %d\n",
> > +			    rsp->supported_version);
> > +		return;
> > +	}
> > +
> > +	IWL_MEI_INFO(cldev,
> > +		     "Got a valid SAP_ME_MSG_START_OK from CSME
> firmware\n");
> 
> When drivers work properly, they are quiet.  This is very noisy, please
> remove debugging code like this.

I'll move to dbg level.

> 
> > +
> > +	mutex_lock(&iwl_mei_mutex);
> > +	set_bit(IWL_MEI_STATUS_SAP_CONNECTED, &iwl_mei_status);
> > +	/* wifi driver has registered already */
> > +	if (iwl_mei_cache.ops) {
> > +		WARN_ON(iwl_mei_send_sap_msg(mei->cldev,
> > +					     SAP_MSG_NOTIF_WIFIDR_UP));
> 
> Why????

Same explanation as above.

> 
> > +		iwl_mei_cache.ops->sap_connected(iwl_mei_cache.priv);
> > +	}
> > +
> > +	mutex_unlock(&iwl_mei_mutex);
> > +}
> > +
> > +static void iwl_mei_handle_csme_filters(struct mei_cl_device *cldev,
> > +					const struct iwl_sap_csme_filters
> *filters) {
> > +	struct iwl_mei *mei =
> mei_cldev_get_drvdata(iwl_mei_global_cldev);
> > +	struct iwl_mei_filters *new_filters;
> > +	struct iwl_mei_filters *old_filters;
> > +
> > +	IWL_MEI_DEBUG(cldev, "Got CSME filters\n");
> 
> ftrace is your friend, remove these pointless "the code got here!"
> lines.  You have loads of them...

This is debug. Won't appear unless you want it.
I have extensive tracing support.

> 
> > +static void iwl_mei_handle_rx_host_own_req(struct mei_cl_device
> *cldev,
> > +					   const struct iwl_sap_msg_dw *dw)
> {
> > +	struct iwl_mei *mei = mei_cldev_get_drvdata(cldev);
> > +
> > +	IWL_MEI_DEBUG(cldev, "Got ownership req response: %d\n", dw-
> >val);
> 
> Code got here!  :(

Same, this is protected by a dynamic debug knob.
I can see tons of those in mei bus driver .
$ git grep _dbg\( -- drivers/misc/mei | wc -l
228

> 
> > +
> > +	if (!dw->val) {
> > +		IWL_MEI_INFO(cldev, "Ownership req denied\n");
> 
> why?????
> 
> > +		return;
> 
> No error returned?

This is not an error. This means that the CSME firmware is not allowing the host to use the WLAN device.

> 
> > +	}
> > +
> > +	mei->got_ownership = true;
> > +	wake_up_all(&mei->get_ownership_wq);
> > +
> > +	iwl_mei_send_sap_msg(cldev,
> > +
> SAP_MSG_NOTIF_HOST_OWNERSHIP_CONFIRMED);
> > +
> > +	/* We can now start the connection, unblock rfkill */
> > +	if (iwl_mei_cache.ops)
> > +		iwl_mei_cache.ops->rfkill(iwl_mei_cache.priv, false); }
> > +
> > +static void iwl_mei_handle_pong(struct mei_cl_device *cldev,
> > +				const void *payload)
> > +{
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +	struct iwl_mei *mei = mei_cldev_get_drvdata(cldev);
> > +
> > +	mei->ping_pending = false;
> > +	wake_up_interruptible_all(&mei->debugfs_wq);
> > +#endif
> 
> Why is this dependant on debugfs?

Because only debugfs can trigger the flow that would be waiting for the pong.
I can remove this debugfs know if you like.

> 
> > +
> > +	IWL_MEI_DEBUG(cldev, "Got PONG\n");
> 
> again, ftrace.
> 
> > +static void iwl_mei_handle_sap_data(struct mei_cl_device *cldev,
> > +				    const u8 *q_head, u32 q_sz,
> > +				    u32 rd, u32 wr, ssize_t valid_rx_sz,
> > +				    struct sk_buff_head *tx_skbs) {
> > +	struct iwl_sap_hdr hdr;
> > +	struct net_device *netdev =
> > +		rcu_dereference_protected(iwl_mei_cache.netdev,
> > +
> lockdep_is_held(&iwl_mei_mutex));
> > +
> > +	if (!netdev) {
> > +		IWL_MEI_INFO(cldev, "No netdevice, dropping the Tx
> packet\n");
> 
> quiet!!!

This is actually not a usual path. A race is happening here...

> 
> Or at the least, make this an error so that a user can handle it properly.  They
> can do something about this, right?  If not, why did you just tell them this
> happened?

There isn't much I can do here. The CSME firmware is sending a packet to iwlmei to send on the WLAN link, but the link isn't valid anymore.

> 
> > +		return;
> 
> No error value?

Nobody will read this error value. This is a handler for a notification from the CSME firmware.

> 
> > +	}
> > +
> > +	while (valid_rx_sz >= sizeof(hdr)) {
> > +		struct ethhdr *ethhdr;
> > +		unsigned char *data;
> > +		struct sk_buff *skb;
> > +		u16 len;
> > +
> > +		iwl_mei_read_from_q(q_head, q_sz, &rd, wr, &hdr,
> sizeof(hdr));
> > +		valid_rx_sz -= sizeof(hdr);
> > +		len = le16_to_cpu(hdr.len);
> > +
> > +		if (valid_rx_sz < len) {
> > +			IWL_MEI_ERR(cldev,
> > +				    "Data queue is corrupted: valid data len
> %ld, len %d\n",
> > +				    valid_rx_sz, len);
> 
> What can be done if this happens?
I guess I can restart the whole handshake with the CSME firmware. I haven't implemented this flow (yet).

> 
> > +			break;
> > +		}
> > +
> > +		if (len < sizeof(*ethhdr)) {
> > +			IWL_MEI_ERR(cldev,
> > +				    "Data len is smaller than an ethernet
> header? len = %d\n",
> > +				    len);
> > +		}
> > +
> > +		valid_rx_sz -= len;
> > +
> > +		if (le16_to_cpu(hdr.type) != SAP_MSG_DATA_PACKET) {
> > +			IWL_MEI_INFO(cldev, "Unsupported message: type
> %d, len %d\n",
> > +				     le16_to_cpu(hdr.type), len);
> 
> So userspace can spam the kernel log?

This is not userspace, this is the CSME firmware.

> 
> Please revisit _ALL_ of your messages you are printing out here, it feels way
> way way too noisy, like you got the code up and working with the debug lines
> in it and forgot to remove it.

[   12.665966] iwlmei 0000:00:16.0-13280904-7792-4fcb-a1aa-5e70cbb1e865: Got a valid SAP_ME_MSG_START_OK from CSME firmware

That's the only line I am printing by default. I can remove it. I can't do better than 0 printing.

> 
> How did this get missed in the internal Intel reviews that of course was
> required to happen before sending it to others outside of the firewall to
> review?
> 
> > +			continue;
> > +		}
> > +
> > +		/* We need enough room for the WiFi header + SNAP + IV */
> > +		skb = netdev_alloc_skb(netdev, len + 26 + 8 + 8);
> > +
> > +		skb_reserve(skb, 26 + 8 + 8);
> > +		ethhdr = skb_push(skb, sizeof(*ethhdr));
> > +
> > +		iwl_mei_read_from_q(q_head, q_sz, &rd, wr,
> > +				    ethhdr, sizeof(*ethhdr));
> > +		len -= sizeof(*ethhdr);
> > +
> > +		skb_reset_mac_header(skb);
> > +		skb_reset_network_header(skb);
> > +		skb->protocol = ethhdr->h_proto;
> > +
> > +		/* we don't really want to get anything besides 802.3 packets
> */
> > +		WARN_ON_ONCE(!eth_proto_is_802_3(ethhdr->h_proto));
> > +
> > +		data = skb_put(skb, len);
> > +		iwl_mei_read_from_q(q_head, q_sz, &rd, wr, data, len);
> > +
> > +		/*
> > +		 * Enqueue the skb here so that it can be sent later when we
> > +		 * do not hold the mutex. TX'ing a packet with a mutex held is
> > +		 * possible, but it wouldn't be nice to forbid the TX path to
> > +		 * call any of iwlmei's functions, since every API from iwlmei
> > +		 * needs the mutex.
> > +		 */
> > +		__skb_queue_tail(tx_skbs, skb);
> > +	}
> > +}
> > +
> > +static void iwl_mei_handle_sap_rx_cmd(struct mei_cl_device *cldev,
> > +				      const u8 *q_head, u32 q_sz,
> > +				      u32 rd, u32 wr, ssize_t valid_rx_sz) {
> > +	struct page *p = alloc_page(GFP_KERNEL);
> > +	struct iwl_sap_hdr *hdr;
> > +
> > +	if (!p)
> > +		return;
> > +
> > +	hdr = page_address(p);
> > +
> > +	while (valid_rx_sz >= sizeof(*hdr)) {
> > +		u16 len;
> > +
> > +		iwl_mei_read_from_q(q_head, q_sz, &rd, wr, hdr,
> sizeof(*hdr));
> > +		valid_rx_sz -= sizeof(*hdr);
> > +		len = le16_to_cpu(hdr->len);
> > +
> > +		if (valid_rx_sz < len)
> > +			break;
> > +
> > +		iwl_mei_read_from_q(q_head, q_sz, &rd, wr, hdr + 1, len);
> > +
> > +		trace_iwlmei_sap_cmd(hdr, false);
> > +		iwl_mei_handle_sap_msg(cldev, hdr);
> > +		valid_rx_sz -= len;
> > +	}
> > +
> > +	/* valid_rx_sz must be 0 now... */
> > +	WARN_ON_ONCE(valid_rx_sz);
> 
> reboot!
> 
> {sigh}
> 
> <snip>
> 
> > +static void iwl_mei_dbgfs_register(struct iwl_mei *mei) {
> > +	mei->dbgfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> 
> At the root of debugfs, bold move :)

There is no other place since the mei bus doesn't provide another dir to hook on.

> 
> > +
> > +	if (!mei->dbgfs_dir)
> > +		return;
> 
> Why check this?  Just feed the result back into other debugfs_create() calls,
> no need to care.
> 
> And why save the directory name at all, can't you just look it up when you
> want to remove it anyway?

Possible.

> 
> > +
> > +	init_waitqueue_head(&mei->debugfs_wq);
> > +
> > +	debugfs_create_ulong("status", S_IRUSR,
> > +			     mei->dbgfs_dir, &iwl_mei_status);
> > +	debugfs_create_file("ping", S_IRUSR, mei->dbgfs_dir,
> > +			    mei, &iwl_mei_dbgfs_ping_ops);
> > +	debugfs_create_file("send_start_message", S_IWUSR, mei-
> >dbgfs_dir,
> > +			    mei, &iwl_mei_dbgfs_send_start_message_ops);
> > +	debugfs_create_file("req_ownserhip", S_IWUSR, mei->dbgfs_dir,
> > +			    mei, &iwl_mei_dbgfs_req_ownership_ops);
> > +}
> > +
> > +static void iwl_mei_dbgfs_unregister(struct iwl_mei *mei) {
> > +	debugfs_remove_recursive(mei->dbgfs_dir);
> > +	mei->dbgfs_dir = NULL;
> 
> Why set this to NULL?

I can remove.

> 
> > +}
> > +
> > +#else
> > +
> > +static void iwl_mei_dbgfs_register(struct iwl_mei *mei) {} static
> > +void iwl_mei_dbgfs_unregister(struct iwl_mei *mei) {}
> 
> This type of thing goes in a .h file, you know better.

A header file only for less a handful of functions?

> 
> 
> > +
> > +#endif /* CONFIG_DEBUG_FS */
> > +
> > +/**
> > + * iwl_mei_probe - the probe function called by the mei bus
> > +enumeration
> > + *
> > + * This allocates the data needed by iwlmei and sets a pointer to
> > +this data
> > + * into the mei_cl_device's drvdata.
> > + * It starts the SAP protocol by sending the SAP_ME_MSG_START without
> > + * waiting for the answer. The answer will be caught later by the Rx
> callback.
> > + */
> > +static int iwl_mei_probe(struct mei_cl_device *cldev,
> > +			 const struct mei_cl_device_id *id) {
> > +	struct iwl_mei *mei;
> > +	int ret;
> > +
> > +	mei = devm_kzalloc(&cldev->dev, sizeof(*mei), GFP_KERNEL);
> > +	if (!mei)
> > +		return -ENOMEM;
> > +
> > +	init_waitqueue_head(&mei->get_nvm_wq);
> > +	INIT_WORK(&mei->send_csa_msg_wk,
> iwl_mei_send_csa_msg_wk);
> > +	INIT_DELAYED_WORK(&mei->csa_throttle_end_wk,
> > +			  iwl_mei_csa_throttle_end_wk);
> > +	init_waitqueue_head(&mei->get_ownership_wq);
> > +	spin_lock_init(&mei->data_q_lock);
> > +
> > +	mei_cldev_set_drvdata(cldev, mei);
> > +	mei->cldev = cldev;
> > +
> > +	ret = iwl_mei_alloc_shared_mem(cldev);
> > +	if (ret)
> > +		goto free;
> > +
> > +	iwl_mei_init_shared_mem(mei);
> > +
> > +	ret = iwl_mei_enable(cldev);
> > +	if (ret)
> > +		goto free_shared_mem;
> > +
> > +	iwl_mei_dbgfs_register(mei);
> > +
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> 
> #if should not be in .c files, again, you all know better...
> 
> I'm giving up here, sorry, I can't do it anymore...
> 
> greg k-h