linux-kernel - [PATCH 30/31] staging: wfx: drop async field from struct hif

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20200907101521.66082-31-Jerome.Pouiller@silabs.com>
Date:   Mon,  7 Sep 2020 12:15:20 +0200
From:   Jerome Pouiller <Jerome.Pouiller@...abs.com>
To:     devel@...verdev.osuosl.org, linux-wireless@...r.kernel.org
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jérôme Pouiller 
        <jerome.pouiller@...abs.com>, kbuild test robot <lkp@...el.com>,
        Julia Lawall <julia.lawall@...6.fr>
Subject: [PATCH 30/31] staging: wfx: drop async field from struct hif_cmd

From: Jérôme Pouiller <jerome.pouiller@...abs.com>

The parameter "async" in wfx_cmd_send() allows to send command without
waiting for the reply. In this case, the mutex hif_cmd.lock is released
asynchronously in the context of the receiver workqueue.

However, "kbuild test robot" complains about this architecture[1] since
it is not able to follow the lock duration of hif_cmd.lock (and indeed,
the state of the driver if the hardware wouldn't reply is not well
defined).

Besides, this feature is not really necessary. It is only used by
hif_shutdown(). This function hijack the 'async' flag to run a command
that won't answer.

So, this patch removes the 'async' flag and introduces a 'no_reply' flag.
Thus, the mutex hif_cmd.lock is only acquired/released from
hif_cmd_send(). Therefore:
  - hif_shutdown() does not have to touch the private data of the struct
    hif_cmd
  - Kbuild test robot should be happy
  - the resulting code is simpler

[1] https://lore.kernel.org/driverdev-devel/alpine.DEB.2.21.1910041317381.2992@hadrien/

Reported-by: kbuild test robot <lkp@...el.com>
Reported-by: Julia Lawall <julia.lawall@...6.fr>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
---
 drivers/staging/wfx/hif_rx.c |  7 +------
 drivers/staging/wfx/hif_tx.c | 24 +++++++++---------------
 drivers/staging/wfx/hif_tx.h |  1 -
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index 6950b3e9d7cf..b40af86356f1 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c
@@ -47,12 +47,7 @@ static int hif_generic_confirm(struct wfx_dev *wdev,
 	}
 	wdev->hif_cmd.ret = status;
 
-	if (!wdev->hif_cmd.async) {
-		complete(&wdev->hif_cmd.done);
-	} else {
-		wdev->hif_cmd.buf_send = NULL;
-		mutex_unlock(&wdev->hif_cmd.lock);
-	}
+	complete(&wdev->hif_cmd.done);
 	return status;
 }
 
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index a75c6b9804ba..1bd7f773209c 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -47,7 +47,7 @@ static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
 }
 
 int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
-		 void *reply, size_t reply_len, bool async)
+		 void *reply, size_t reply_len, bool no_reply)
 {
 	const char *mib_name = "";
 	const char *mib_sep = "";
@@ -55,8 +55,6 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
 	int vif = request->interface;
 	int ret;
 
-	WARN(wdev->hif_cmd.buf_recv && wdev->hif_cmd.async, "API usage error");
-
 	// Do not wait for any reply if chip is frozen
 	if (wdev->chip_frozen)
 		return -ETIMEDOUT;
@@ -69,14 +67,18 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
 	wdev->hif_cmd.buf_send = request;
 	wdev->hif_cmd.buf_recv = reply;
 	wdev->hif_cmd.len_recv = reply_len;
-	wdev->hif_cmd.async = async;
 	complete(&wdev->hif_cmd.ready);
 
 	wfx_bh_request_tx(wdev);
 
-	// NOTE: no timeout is caught async is enabled
-	if (async)
+	if (no_reply) {
+		// Chip won't reply. Give enough time to the wq to send the
+		// buffer.
+		msleep(100);
+		wdev->hif_cmd.buf_send = NULL;
+		mutex_unlock(&wdev->hif_cmd.lock);
 		return 0;
+	}
 
 	if (wdev->poll_irq)
 		wfx_bh_poll_irq(wdev);
@@ -118,29 +120,21 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
 }
 
 // This function is special. After HIF_REQ_ID_SHUT_DOWN, chip won't reply to any
-// request anymore. We need to slightly hack struct wfx_hif_cmd for that job. Be
-// careful to only call this function during device unregister.
+// request anymore. Obviously, only call this function during device unregister.
 int hif_shutdown(struct wfx_dev *wdev)
 {
 	int ret;
 	struct hif_msg *hif;
 
-	if (wdev->chip_frozen)
-		return 0;
 	wfx_alloc_hif(0, &hif);
 	if (!hif)
 		return -ENOMEM;
 	wfx_fill_header(hif, -1, HIF_REQ_ID_SHUT_DOWN, 0);
 	ret = wfx_cmd_send(wdev, hif, NULL, 0, true);
-	// After this command, chip won't reply. Be sure to give enough time to
-	// bh to send buffer:
-	msleep(100);
-	wdev->hif_cmd.buf_send = NULL;
 	if (wdev->pdata.gpio_wakeup)
 		gpiod_set_value(wdev->pdata.gpio_wakeup, 0);
 	else
 		control_reg_write(wdev, 0);
-	mutex_unlock(&wdev->hif_cmd.lock);
 	kfree(hif);
 	return ret;
 }
diff --git a/drivers/staging/wfx/hif_tx.h b/drivers/staging/wfx/hif_tx.h
index e8aca39e7497..960d5f2fa41c 100644
--- a/drivers/staging/wfx/hif_tx.h
+++ b/drivers/staging/wfx/hif_tx.h
@@ -22,7 +22,6 @@ struct wfx_hif_cmd {
 	struct mutex      lock;
 	struct completion ready;
 	struct completion done;
-	bool              async;
 	struct hif_msg    *buf_send;
 	void              *buf_recv;
 	size_t            len_recv;
-- 
2.28.0