* [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
@ 2020-08-05 12:14 ` Tomer Samara
0 siblings, 0 replies; 6+ messages in thread
From: Tomer Samara @ 2020-08-05 12:14 UTC (permalink / raw)
To: jerome.pouiller; +Cc: gregkh, devel, linux-kernel
Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
which works as follow:
wfx_full_send() - simple wrapper for both wfx_fill_header()
and wfx_cmd_send().
wfx_full_send_no_reply_async() - wrapper for both but with
NULL as reply and size zero.
wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
but with false async value
wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
but also free the struct hif_msg.
Signed-off-by: Tomer Samara <tomersamara98@gmail.com>
---
Changes in v2:
- Changed these functions to static
drivers/staging/wfx/hif_tx.c | 180 ++++++++++++++++-------------------
1 file changed, 80 insertions(+), 100 deletions(-)
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 5110f9b93762..17f668fa40a0 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
{
- *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
+ *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
if (*hif)
return (*hif)->body;
else
@@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
return ret;
}
+static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
+ size_t reply_len, bool async, int if_id, unsigned int cmd,
+ int size)
+{
+ wfx_fill_header(hif, if_id, cmd, size);
+ return wfx_cmd_send(wdev, hif, reply, reply_len, async);
+}
+
+static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
+ unsigned int cmd, int size, bool async)
+{
+ return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
+}
+
+static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
+ unsigned int cmd, int size)
+{
+ return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
+}
+
+static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
+ unsigned int cmd, int size)
+{
+ int ret;
+
+ ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
+ kfree(hif);
+ return ret;
+}
+
// 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
-// carefull to only call this funcion during device unregister.
+// careful to only call this function during device unregister.
int hif_shutdown(struct wfx_dev *wdev)
{
int ret;
@@ -136,8 +166,8 @@ int hif_shutdown(struct wfx_dev *wdev)
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);
+ ret = wfx_full_send_no_reply_async(wdev, hif, -1, HIF_REQ_ID_SHUT_DOWN,
+ 0, true);
// After this command, chip won't reply. Be sure to give enough time to
// bh to send buffer:
msleep(100);
@@ -154,7 +184,6 @@ int hif_shutdown(struct wfx_dev *wdev)
int hif_configuration(struct wfx_dev *wdev, const u8 *conf, size_t len)
{
- int ret;
size_t buf_len = sizeof(struct hif_req_configuration) + len;
struct hif_msg *hif;
struct hif_req_configuration *body = wfx_alloc_hif(buf_len, &hif);
@@ -163,25 +192,20 @@ int hif_configuration(struct wfx_dev *wdev, const u8 *conf, size_t len)
return -ENOMEM;
body->length = cpu_to_le16(len);
memcpy(body->pds_data, conf, len);
- wfx_fill_header(hif, -1, HIF_REQ_ID_CONFIGURATION, buf_len);
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_CONFIGURATION,
+ buf_len);
}
int hif_reset(struct wfx_vif *wvif, bool reset_stat)
{
- int ret;
struct hif_msg *hif;
struct hif_req_reset *body = wfx_alloc_hif(sizeof(*body), &hif);
if (!hif)
return -ENOMEM;
body->reset_flags.reset_stat = reset_stat;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_RESET, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_RESET, sizeof(*body));
}
int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
@@ -198,9 +222,8 @@ int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
goto out;
}
body->mib_id = cpu_to_le16(mib_id);
- wfx_fill_header(hif, vif_id, HIF_REQ_ID_READ_MIB, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, reply, buf_len, false);
-
+ ret = wfx_full_send(wdev, hif, reply, buf_len, false, vif_id,
+ HIF_REQ_ID_READ_MIB, sizeof(*body));
if (!ret && mib_id != le16_to_cpu(reply->mib_id)) {
dev_warn(wdev->dev, "%s: confirmation mismatch request\n",
__func__);
@@ -223,7 +246,6 @@ int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
int hif_write_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
void *val, size_t val_len)
{
- int ret;
struct hif_msg *hif;
int buf_len = sizeof(struct hif_req_write_mib) + val_len;
struct hif_req_write_mib *body = wfx_alloc_hif(buf_len, &hif);
@@ -233,16 +255,14 @@ int hif_write_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
body->mib_id = cpu_to_le16(mib_id);
body->length = cpu_to_le16(val_len);
memcpy(&body->mib_data, val, val_len);
- wfx_fill_header(hif, vif_id, HIF_REQ_ID_WRITE_MIB, buf_len);
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, vif_id, HIF_REQ_ID_WRITE_MIB,
+ buf_len);
}
int hif_scan(struct wfx_vif *wvif, struct cfg80211_scan_request *req,
int chan_start_idx, int chan_num, int *timeout)
{
- int ret, i;
+ int i;
struct hif_msg *hif;
size_t buf_len =
sizeof(struct hif_req_start_scan_alt) + chan_num * sizeof(u8);
@@ -292,31 +312,25 @@ int hif_scan(struct wfx_vif *wvif, struct cfg80211_scan_request *req,
if (timeout)
*timeout = usecs_to_jiffies(tmo);
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_START_SCAN, buf_len);
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_START_SCAN, buf_len);
}
int hif_stop_scan(struct wfx_vif *wvif)
{
- int ret;
struct hif_msg *hif;
// body associated to HIF_REQ_ID_STOP_SCAN is empty
wfx_alloc_hif(0, &hif);
if (!hif)
return -ENOMEM;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_STOP_SCAN, 0);
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_STOP_SCAN, 0);
}
int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
struct ieee80211_channel *channel, const u8 *ssid, int ssidlen)
{
- int ret;
struct hif_msg *hif;
struct hif_req_join *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -341,15 +355,12 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
body->ssid_length = cpu_to_le32(ssidlen);
memcpy(body->ssid, ssid, ssidlen);
}
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_JOIN, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_JOIN, sizeof(*body));
}
int hif_set_bss_params(struct wfx_vif *wvif, int aid, int beacon_lost_count)
{
- int ret;
struct hif_msg *hif;
struct hif_req_set_bss_params *body =
wfx_alloc_hif(sizeof(*body), &hif);
@@ -358,16 +369,13 @@ int hif_set_bss_params(struct wfx_vif *wvif, int aid, int beacon_lost_count)
return -ENOMEM;
body->aid = cpu_to_le16(aid);
body->beacon_lost_count = beacon_lost_count;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_SET_BSS_PARAMS,
- sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_SET_BSS_PARAMS,
+ sizeof(*body));
}
int hif_add_key(struct wfx_dev *wdev, const struct hif_req_add_key *arg)
{
- int ret;
struct hif_msg *hif;
// FIXME: only send necessary bits
struct hif_req_add_key *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -379,34 +387,28 @@ int hif_add_key(struct wfx_dev *wdev, const struct hif_req_add_key *arg)
if (wfx_api_older_than(wdev, 1, 5))
// Legacy firmwares expect that add_key to be sent on right
// interface.
- wfx_fill_header(hif, arg->int_id, HIF_REQ_ID_ADD_KEY,
- sizeof(*body));
- else
- wfx_fill_header(hif, -1, HIF_REQ_ID_ADD_KEY, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, arg->int_id,
+ HIF_REQ_ID_ADD_KEY,
+ sizeof(*body));
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_ADD_KEY,
+ sizeof(*body));
}
int hif_remove_key(struct wfx_dev *wdev, int idx)
{
- int ret;
struct hif_msg *hif;
struct hif_req_remove_key *body = wfx_alloc_hif(sizeof(*body), &hif);
if (!hif)
return -ENOMEM;
body->entry_index = idx;
- wfx_fill_header(hif, -1, HIF_REQ_ID_REMOVE_KEY, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_REMOVE_KEY,
+ sizeof(*body));
}
int hif_set_edca_queue_params(struct wfx_vif *wvif, u16 queue,
const struct ieee80211_tx_queue_params *arg)
{
- int ret;
struct hif_msg *hif;
struct hif_req_edca_queue_params *body = wfx_alloc_hif(sizeof(*body),
&hif);
@@ -427,16 +429,13 @@ int hif_set_edca_queue_params(struct wfx_vif *wvif, u16 queue,
body->queue_id = HIF_QUEUE_ID_BACKGROUND;
if (wfx_api_older_than(wvif->wdev, 2, 0) && queue == IEEE80211_AC_BK)
body->queue_id = HIF_QUEUE_ID_BESTEFFORT;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_EDCA_QUEUE_PARAMS,
- sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_EDCA_QUEUE_PARAMS,
+ sizeof(*body));
}
int hif_set_pm(struct wfx_vif *wvif, bool ps, int dynamic_ps_timeout)
{
- int ret;
struct hif_msg *hif;
struct hif_req_set_pm_mode *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -452,16 +451,13 @@ int hif_set_pm(struct wfx_vif *wvif, bool ps, int dynamic_ps_timeout)
if (body->fast_psm_idle_period)
body->pm_mode.fast_psm = 1;
}
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_SET_PM_MODE, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_SET_PM_MODE, sizeof(*body));
}
int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
const struct ieee80211_channel *channel)
{
- int ret;
struct hif_msg *hif;
struct hif_req_start *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -476,15 +472,12 @@ int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
cpu_to_le32(wfx_rate_mask_to_hw(wvif->wdev, conf->basic_rates));
body->ssid_length = conf->ssid_len;
memcpy(body->ssid, conf->ssid, conf->ssid_len);
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_START, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_START, sizeof(*body));
}
int hif_beacon_transmit(struct wfx_vif *wvif, bool enable)
{
- int ret;
struct hif_msg *hif;
struct hif_req_beacon_transmit *body = wfx_alloc_hif(sizeof(*body),
&hif);
@@ -492,16 +485,13 @@ int hif_beacon_transmit(struct wfx_vif *wvif, bool enable)
if (!hif)
return -ENOMEM;
body->enable_beaconing = enable ? 1 : 0;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_BEACON_TRANSMIT,
- sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_BEACON_TRANSMIT,
+ sizeof(*body));
}
int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int flags, int sta_id)
{
- int ret;
struct hif_msg *hif;
struct hif_req_map_link *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -511,15 +501,12 @@ int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int flags, int sta_id)
ether_addr_copy(body->mac_addr, mac_addr);
body->map_link_flags = *(struct hif_map_link_flags *)&flags;
body->peer_sta_id = sta_id;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_MAP_LINK, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_MAP_LINK, sizeof(*body));
}
int hif_update_ie_beacon(struct wfx_vif *wvif, const u8 *ies, size_t ies_len)
{
- int ret;
struct hif_msg *hif;
int buf_len = sizeof(struct hif_req_update_ie) + ies_len;
struct hif_req_update_ie *body = wfx_alloc_hif(buf_len, &hif);
@@ -529,10 +516,8 @@ int hif_update_ie_beacon(struct wfx_vif *wvif, const u8 *ies, size_t ies_len)
body->ie_flags.beacon = 1;
body->num_ies = cpu_to_le16(1);
memcpy(body->ie, ies, ies_len);
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_UPDATE_IE, buf_len);
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_UPDATE_IE, buf_len);
}
int hif_sl_send_pub_keys(struct wfx_dev *wdev,
@@ -549,10 +534,9 @@ int hif_sl_send_pub_keys(struct wfx_dev *wdev,
memcpy(body->host_pub_key, pubkey, sizeof(body->host_pub_key));
memcpy(body->host_pub_key_mac, pubkey_hmac,
sizeof(body->host_pub_key_mac));
- wfx_fill_header(hif, -1, HIF_REQ_ID_SL_EXCHANGE_PUB_KEYS,
- sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
+ ret = wfx_full_send_no_reply_free(wdev, hif, -1,
+ HIF_REQ_ID_SL_EXCHANGE_PUB_KEYS,
+ sizeof(*body));
// Compatibility with legacy secure link
if (ret == le32_to_cpu(HIF_STATUS_SLK_NEGO_SUCCESS))
ret = 0;
@@ -561,17 +545,14 @@ int hif_sl_send_pub_keys(struct wfx_dev *wdev,
int hif_sl_config(struct wfx_dev *wdev, const unsigned long *bitmap)
{
- int ret;
struct hif_msg *hif;
struct hif_req_sl_configure *body = wfx_alloc_hif(sizeof(*body), &hif);
if (!hif)
return -ENOMEM;
memcpy(body->encr_bmp, bitmap, sizeof(body->encr_bmp));
- wfx_fill_header(hif, -1, HIF_REQ_ID_SL_CONFIGURE, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_SL_CONFIGURE,
+ sizeof(*body));
}
int hif_sl_set_mac_key(struct wfx_dev *wdev, const u8 *slk_key, int destination)
@@ -585,9 +566,8 @@ int hif_sl_set_mac_key(struct wfx_dev *wdev, const u8 *slk_key, int destination)
return -ENOMEM;
memcpy(body->key_value, slk_key, sizeof(body->key_value));
body->otp_or_ram = destination;
- wfx_fill_header(hif, -1, HIF_REQ_ID_SET_SL_MAC_KEY, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
+ ret = wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_SET_SL_MAC_KEY,
+ sizeof(*body));
// Compatibility with legacy secure link
if (ret == le32_to_cpu(HIF_STATUS_SLK_SET_KEY_SUCCESS))
ret = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
@ 2020-08-05 12:14 ` Tomer Samara
0 siblings, 0 replies; 6+ messages in thread
From: Tomer Samara @ 2020-08-05 12:14 UTC (permalink / raw)
To: jerome.pouiller; +Cc: devel, gregkh, linux-kernel
Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
which works as follow:
wfx_full_send() - simple wrapper for both wfx_fill_header()
and wfx_cmd_send().
wfx_full_send_no_reply_async() - wrapper for both but with
NULL as reply and size zero.
wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
but with false async value
wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
but also free the struct hif_msg.
Signed-off-by: Tomer Samara <tomersamara98@gmail.com>
---
Changes in v2:
- Changed these functions to static
drivers/staging/wfx/hif_tx.c | 180 ++++++++++++++++-------------------
1 file changed, 80 insertions(+), 100 deletions(-)
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 5110f9b93762..17f668fa40a0 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
{
- *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
+ *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
if (*hif)
return (*hif)->body;
else
@@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
return ret;
}
+static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
+ size_t reply_len, bool async, int if_id, unsigned int cmd,
+ int size)
+{
+ wfx_fill_header(hif, if_id, cmd, size);
+ return wfx_cmd_send(wdev, hif, reply, reply_len, async);
+}
+
+static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
+ unsigned int cmd, int size, bool async)
+{
+ return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
+}
+
+static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
+ unsigned int cmd, int size)
+{
+ return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
+}
+
+static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
+ unsigned int cmd, int size)
+{
+ int ret;
+
+ ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
+ kfree(hif);
+ return ret;
+}
+
// 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
-// carefull to only call this funcion during device unregister.
+// careful to only call this function during device unregister.
int hif_shutdown(struct wfx_dev *wdev)
{
int ret;
@@ -136,8 +166,8 @@ int hif_shutdown(struct wfx_dev *wdev)
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);
+ ret = wfx_full_send_no_reply_async(wdev, hif, -1, HIF_REQ_ID_SHUT_DOWN,
+ 0, true);
// After this command, chip won't reply. Be sure to give enough time to
// bh to send buffer:
msleep(100);
@@ -154,7 +184,6 @@ int hif_shutdown(struct wfx_dev *wdev)
int hif_configuration(struct wfx_dev *wdev, const u8 *conf, size_t len)
{
- int ret;
size_t buf_len = sizeof(struct hif_req_configuration) + len;
struct hif_msg *hif;
struct hif_req_configuration *body = wfx_alloc_hif(buf_len, &hif);
@@ -163,25 +192,20 @@ int hif_configuration(struct wfx_dev *wdev, const u8 *conf, size_t len)
return -ENOMEM;
body->length = cpu_to_le16(len);
memcpy(body->pds_data, conf, len);
- wfx_fill_header(hif, -1, HIF_REQ_ID_CONFIGURATION, buf_len);
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_CONFIGURATION,
+ buf_len);
}
int hif_reset(struct wfx_vif *wvif, bool reset_stat)
{
- int ret;
struct hif_msg *hif;
struct hif_req_reset *body = wfx_alloc_hif(sizeof(*body), &hif);
if (!hif)
return -ENOMEM;
body->reset_flags.reset_stat = reset_stat;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_RESET, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_RESET, sizeof(*body));
}
int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
@@ -198,9 +222,8 @@ int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
goto out;
}
body->mib_id = cpu_to_le16(mib_id);
- wfx_fill_header(hif, vif_id, HIF_REQ_ID_READ_MIB, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, reply, buf_len, false);
-
+ ret = wfx_full_send(wdev, hif, reply, buf_len, false, vif_id,
+ HIF_REQ_ID_READ_MIB, sizeof(*body));
if (!ret && mib_id != le16_to_cpu(reply->mib_id)) {
dev_warn(wdev->dev, "%s: confirmation mismatch request\n",
__func__);
@@ -223,7 +246,6 @@ int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
int hif_write_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
void *val, size_t val_len)
{
- int ret;
struct hif_msg *hif;
int buf_len = sizeof(struct hif_req_write_mib) + val_len;
struct hif_req_write_mib *body = wfx_alloc_hif(buf_len, &hif);
@@ -233,16 +255,14 @@ int hif_write_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
body->mib_id = cpu_to_le16(mib_id);
body->length = cpu_to_le16(val_len);
memcpy(&body->mib_data, val, val_len);
- wfx_fill_header(hif, vif_id, HIF_REQ_ID_WRITE_MIB, buf_len);
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, vif_id, HIF_REQ_ID_WRITE_MIB,
+ buf_len);
}
int hif_scan(struct wfx_vif *wvif, struct cfg80211_scan_request *req,
int chan_start_idx, int chan_num, int *timeout)
{
- int ret, i;
+ int i;
struct hif_msg *hif;
size_t buf_len =
sizeof(struct hif_req_start_scan_alt) + chan_num * sizeof(u8);
@@ -292,31 +312,25 @@ int hif_scan(struct wfx_vif *wvif, struct cfg80211_scan_request *req,
if (timeout)
*timeout = usecs_to_jiffies(tmo);
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_START_SCAN, buf_len);
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_START_SCAN, buf_len);
}
int hif_stop_scan(struct wfx_vif *wvif)
{
- int ret;
struct hif_msg *hif;
// body associated to HIF_REQ_ID_STOP_SCAN is empty
wfx_alloc_hif(0, &hif);
if (!hif)
return -ENOMEM;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_STOP_SCAN, 0);
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_STOP_SCAN, 0);
}
int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
struct ieee80211_channel *channel, const u8 *ssid, int ssidlen)
{
- int ret;
struct hif_msg *hif;
struct hif_req_join *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -341,15 +355,12 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
body->ssid_length = cpu_to_le32(ssidlen);
memcpy(body->ssid, ssid, ssidlen);
}
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_JOIN, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_JOIN, sizeof(*body));
}
int hif_set_bss_params(struct wfx_vif *wvif, int aid, int beacon_lost_count)
{
- int ret;
struct hif_msg *hif;
struct hif_req_set_bss_params *body =
wfx_alloc_hif(sizeof(*body), &hif);
@@ -358,16 +369,13 @@ int hif_set_bss_params(struct wfx_vif *wvif, int aid, int beacon_lost_count)
return -ENOMEM;
body->aid = cpu_to_le16(aid);
body->beacon_lost_count = beacon_lost_count;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_SET_BSS_PARAMS,
- sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_SET_BSS_PARAMS,
+ sizeof(*body));
}
int hif_add_key(struct wfx_dev *wdev, const struct hif_req_add_key *arg)
{
- int ret;
struct hif_msg *hif;
// FIXME: only send necessary bits
struct hif_req_add_key *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -379,34 +387,28 @@ int hif_add_key(struct wfx_dev *wdev, const struct hif_req_add_key *arg)
if (wfx_api_older_than(wdev, 1, 5))
// Legacy firmwares expect that add_key to be sent on right
// interface.
- wfx_fill_header(hif, arg->int_id, HIF_REQ_ID_ADD_KEY,
- sizeof(*body));
- else
- wfx_fill_header(hif, -1, HIF_REQ_ID_ADD_KEY, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, arg->int_id,
+ HIF_REQ_ID_ADD_KEY,
+ sizeof(*body));
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_ADD_KEY,
+ sizeof(*body));
}
int hif_remove_key(struct wfx_dev *wdev, int idx)
{
- int ret;
struct hif_msg *hif;
struct hif_req_remove_key *body = wfx_alloc_hif(sizeof(*body), &hif);
if (!hif)
return -ENOMEM;
body->entry_index = idx;
- wfx_fill_header(hif, -1, HIF_REQ_ID_REMOVE_KEY, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_REMOVE_KEY,
+ sizeof(*body));
}
int hif_set_edca_queue_params(struct wfx_vif *wvif, u16 queue,
const struct ieee80211_tx_queue_params *arg)
{
- int ret;
struct hif_msg *hif;
struct hif_req_edca_queue_params *body = wfx_alloc_hif(sizeof(*body),
&hif);
@@ -427,16 +429,13 @@ int hif_set_edca_queue_params(struct wfx_vif *wvif, u16 queue,
body->queue_id = HIF_QUEUE_ID_BACKGROUND;
if (wfx_api_older_than(wvif->wdev, 2, 0) && queue == IEEE80211_AC_BK)
body->queue_id = HIF_QUEUE_ID_BESTEFFORT;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_EDCA_QUEUE_PARAMS,
- sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_EDCA_QUEUE_PARAMS,
+ sizeof(*body));
}
int hif_set_pm(struct wfx_vif *wvif, bool ps, int dynamic_ps_timeout)
{
- int ret;
struct hif_msg *hif;
struct hif_req_set_pm_mode *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -452,16 +451,13 @@ int hif_set_pm(struct wfx_vif *wvif, bool ps, int dynamic_ps_timeout)
if (body->fast_psm_idle_period)
body->pm_mode.fast_psm = 1;
}
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_SET_PM_MODE, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_SET_PM_MODE, sizeof(*body));
}
int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
const struct ieee80211_channel *channel)
{
- int ret;
struct hif_msg *hif;
struct hif_req_start *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -476,15 +472,12 @@ int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
cpu_to_le32(wfx_rate_mask_to_hw(wvif->wdev, conf->basic_rates));
body->ssid_length = conf->ssid_len;
memcpy(body->ssid, conf->ssid, conf->ssid_len);
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_START, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_START, sizeof(*body));
}
int hif_beacon_transmit(struct wfx_vif *wvif, bool enable)
{
- int ret;
struct hif_msg *hif;
struct hif_req_beacon_transmit *body = wfx_alloc_hif(sizeof(*body),
&hif);
@@ -492,16 +485,13 @@ int hif_beacon_transmit(struct wfx_vif *wvif, bool enable)
if (!hif)
return -ENOMEM;
body->enable_beaconing = enable ? 1 : 0;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_BEACON_TRANSMIT,
- sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_BEACON_TRANSMIT,
+ sizeof(*body));
}
int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int flags, int sta_id)
{
- int ret;
struct hif_msg *hif;
struct hif_req_map_link *body = wfx_alloc_hif(sizeof(*body), &hif);
@@ -511,15 +501,12 @@ int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int flags, int sta_id)
ether_addr_copy(body->mac_addr, mac_addr);
body->map_link_flags = *(struct hif_map_link_flags *)&flags;
body->peer_sta_id = sta_id;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_MAP_LINK, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_MAP_LINK, sizeof(*body));
}
int hif_update_ie_beacon(struct wfx_vif *wvif, const u8 *ies, size_t ies_len)
{
- int ret;
struct hif_msg *hif;
int buf_len = sizeof(struct hif_req_update_ie) + ies_len;
struct hif_req_update_ie *body = wfx_alloc_hif(buf_len, &hif);
@@ -529,10 +516,8 @@ int hif_update_ie_beacon(struct wfx_vif *wvif, const u8 *ies, size_t ies_len)
body->ie_flags.beacon = 1;
body->num_ies = cpu_to_le16(1);
memcpy(body->ie, ies, ies_len);
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_UPDATE_IE, buf_len);
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_UPDATE_IE, buf_len);
}
int hif_sl_send_pub_keys(struct wfx_dev *wdev,
@@ -549,10 +534,9 @@ int hif_sl_send_pub_keys(struct wfx_dev *wdev,
memcpy(body->host_pub_key, pubkey, sizeof(body->host_pub_key));
memcpy(body->host_pub_key_mac, pubkey_hmac,
sizeof(body->host_pub_key_mac));
- wfx_fill_header(hif, -1, HIF_REQ_ID_SL_EXCHANGE_PUB_KEYS,
- sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
+ ret = wfx_full_send_no_reply_free(wdev, hif, -1,
+ HIF_REQ_ID_SL_EXCHANGE_PUB_KEYS,
+ sizeof(*body));
// Compatibility with legacy secure link
if (ret == le32_to_cpu(HIF_STATUS_SLK_NEGO_SUCCESS))
ret = 0;
@@ -561,17 +545,14 @@ int hif_sl_send_pub_keys(struct wfx_dev *wdev,
int hif_sl_config(struct wfx_dev *wdev, const unsigned long *bitmap)
{
- int ret;
struct hif_msg *hif;
struct hif_req_sl_configure *body = wfx_alloc_hif(sizeof(*body), &hif);
if (!hif)
return -ENOMEM;
memcpy(body->encr_bmp, bitmap, sizeof(body->encr_bmp));
- wfx_fill_header(hif, -1, HIF_REQ_ID_SL_CONFIGURE, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_SL_CONFIGURE,
+ sizeof(*body));
}
int hif_sl_set_mac_key(struct wfx_dev *wdev, const u8 *slk_key, int destination)
@@ -585,9 +566,8 @@ int hif_sl_set_mac_key(struct wfx_dev *wdev, const u8 *slk_key, int destination)
return -ENOMEM;
memcpy(body->key_value, slk_key, sizeof(body->key_value));
body->otp_or_ram = destination;
- wfx_fill_header(hif, -1, HIF_REQ_ID_SET_SL_MAC_KEY, sizeof(*body));
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
+ ret = wfx_full_send_no_reply_free(wdev, hif, -1, HIF_REQ_ID_SET_SL_MAC_KEY,
+ sizeof(*body));
// Compatibility with legacy secure link
if (ret == le32_to_cpu(HIF_STATUS_SLK_SET_KEY_SUCCESS))
ret = 0;
--
2.25.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
2020-08-05 12:14 ` Tomer Samara
@ 2020-08-10 9:38 ` Jérôme Pouiller
-1 siblings, 0 replies; 6+ messages in thread
From: Jérôme Pouiller @ 2020-08-10 9:38 UTC (permalink / raw)
To: Tomer Samara; +Cc: gregkh, devel, linux-kernel
Hello Tomer,
On Wednesday 5 August 2020 14:14:42 CEST Tomer Samara wrote:
>
> Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
> wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
> which works as follow:
> wfx_full_send() - simple wrapper for both wfx_fill_header()
> and wfx_cmd_send().
> wfx_full_send_no_reply_async() - wrapper for both but with
> NULL as reply and size zero.
> wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
> but with false async value
> wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
> but also free the struct hif_msg.
>
> Signed-off-by: Tomer Samara <tomersamara98@gmail.com>
> ---
> Changes in v2:
> - Changed these functions to static
>
> drivers/staging/wfx/hif_tx.c | 180 ++++++++++++++++-------------------
> 1 file changed, 80 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> index 5110f9b93762..17f668fa40a0 100644
> --- a/drivers/staging/wfx/hif_tx.c
> +++ b/drivers/staging/wfx/hif_tx.c
> @@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
>
> static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
> {
> - *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
> + *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
This change is not related to the rest of the patch. It should probably be
split out.
> if (*hif)
> return (*hif)->body;
> else
> @@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
> return ret;
> }
>
> +static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
> + size_t reply_len, bool async, int if_id, unsigned int cmd,
> + int size)
> +{
> + wfx_fill_header(hif, if_id, cmd, size);
> + return wfx_cmd_send(wdev, hif, reply, reply_len, async);
> +}
This function takes 8 parameters. Are you sure it simplifies the code?
In add, it does two actions: modify hif and send it. I would keep these
two actions separated.
> +
> +static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> + unsigned int cmd, int size, bool async)
> +{
> + return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
> +}
Does it make sense to have a parameter 'async' to
wfx_full_send_no_reply_async()? It is weird to call this function with
async=false, no?
> +
> +static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> + unsigned int cmd, int size)
> +{
> + return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
> +}
> +
> +static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> + unsigned int cmd, int size)
> +{
> + int ret;
> +
> + ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
> + kfree(hif);
> + return ret;
> +}
One more time, sending the data and releasing are unrelated actions.
Indeed, it saves a few lines of code, but is it really an improvement?
> +
> // 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
> -// carefull to only call this funcion during device unregister.
> +// careful to only call this function during device unregister.
Not related to the rest of the patch.
[...]
As it stands, I think this change does not improve the code. Obviously, it
is a subjective opinion. What the other developers think about it?
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
@ 2020-08-10 9:38 ` Jérôme Pouiller
0 siblings, 0 replies; 6+ messages in thread
From: Jérôme Pouiller @ 2020-08-10 9:38 UTC (permalink / raw)
To: Tomer Samara; +Cc: devel, gregkh, linux-kernel
Hello Tomer,
On Wednesday 5 August 2020 14:14:42 CEST Tomer Samara wrote:
>
> Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
> wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
> which works as follow:
> wfx_full_send() - simple wrapper for both wfx_fill_header()
> and wfx_cmd_send().
> wfx_full_send_no_reply_async() - wrapper for both but with
> NULL as reply and size zero.
> wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
> but with false async value
> wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
> but also free the struct hif_msg.
>
> Signed-off-by: Tomer Samara <tomersamara98@gmail.com>
> ---
> Changes in v2:
> - Changed these functions to static
>
> drivers/staging/wfx/hif_tx.c | 180 ++++++++++++++++-------------------
> 1 file changed, 80 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> index 5110f9b93762..17f668fa40a0 100644
> --- a/drivers/staging/wfx/hif_tx.c
> +++ b/drivers/staging/wfx/hif_tx.c
> @@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
>
> static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
> {
> - *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
> + *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
This change is not related to the rest of the patch. It should probably be
split out.
> if (*hif)
> return (*hif)->body;
> else
> @@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
> return ret;
> }
>
> +static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
> + size_t reply_len, bool async, int if_id, unsigned int cmd,
> + int size)
> +{
> + wfx_fill_header(hif, if_id, cmd, size);
> + return wfx_cmd_send(wdev, hif, reply, reply_len, async);
> +}
This function takes 8 parameters. Are you sure it simplifies the code?
In add, it does two actions: modify hif and send it. I would keep these
two actions separated.
> +
> +static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> + unsigned int cmd, int size, bool async)
> +{
> + return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
> +}
Does it make sense to have a parameter 'async' to
wfx_full_send_no_reply_async()? It is weird to call this function with
async=false, no?
> +
> +static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> + unsigned int cmd, int size)
> +{
> + return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
> +}
> +
> +static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> + unsigned int cmd, int size)
> +{
> + int ret;
> +
> + ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
> + kfree(hif);
> + return ret;
> +}
One more time, sending the data and releasing are unrelated actions.
Indeed, it saves a few lines of code, but is it really an improvement?
> +
> // 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
> -// carefull to only call this funcion during device unregister.
> +// careful to only call this function during device unregister.
Not related to the rest of the patch.
[...]
As it stands, I think this change does not improve the code. Obviously, it
is a subjective opinion. What the other developers think about it?
--
Jérôme Pouiller
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
2020-08-10 9:38 ` Jérôme Pouiller
@ 2020-08-18 14:02 ` Greg KH
-1 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2020-08-18 14:02 UTC (permalink / raw)
To: Jérôme Pouiller; +Cc: Tomer Samara, devel, linux-kernel
On Mon, Aug 10, 2020 at 11:38:33AM +0200, Jérôme Pouiller wrote:
> Hello Tomer,
>
> On Wednesday 5 August 2020 14:14:42 CEST Tomer Samara wrote:
> >
> > Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
> > wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
> > which works as follow:
> > wfx_full_send() - simple wrapper for both wfx_fill_header()
> > and wfx_cmd_send().
> > wfx_full_send_no_reply_async() - wrapper for both but with
> > NULL as reply and size zero.
> > wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
> > but with false async value
> > wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
> > but also free the struct hif_msg.
> >
> > Signed-off-by: Tomer Samara <tomersamara98@gmail.com>
> > ---
> > Changes in v2:
> > - Changed these functions to static
> >
> > drivers/staging/wfx/hif_tx.c | 180 ++++++++++++++++-------------------
> > 1 file changed, 80 insertions(+), 100 deletions(-)
> >
> > diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> > index 5110f9b93762..17f668fa40a0 100644
> > --- a/drivers/staging/wfx/hif_tx.c
> > +++ b/drivers/staging/wfx/hif_tx.c
> > @@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
> >
> > static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
> > {
> > - *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
> > + *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
>
> This change is not related to the rest of the patch. It should probably be
> split out.
>
> > if (*hif)
> > return (*hif)->body;
> > else
> > @@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
> > return ret;
> > }
> >
> > +static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
> > + size_t reply_len, bool async, int if_id, unsigned int cmd,
> > + int size)
> > +{
> > + wfx_fill_header(hif, if_id, cmd, size);
> > + return wfx_cmd_send(wdev, hif, reply, reply_len, async);
> > +}
>
> This function takes 8 parameters. Are you sure it simplifies the code?
>
> In add, it does two actions: modify hif and send it. I would keep these
> two actions separated.
>
> > +
> > +static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> > + unsigned int cmd, int size, bool async)
> > +{
> > + return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
> > +}
>
> Does it make sense to have a parameter 'async' to
> wfx_full_send_no_reply_async()? It is weird to call this function with
> async=false, no?
>
> > +
> > +static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> > + unsigned int cmd, int size)
> > +{
> > + return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
> > +}
> > +
> > +static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> > + unsigned int cmd, int size)
> > +{
> > + int ret;
> > +
> > + ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
> > + kfree(hif);
> > + return ret;
> > +}
>
> One more time, sending the data and releasing are unrelated actions.
> Indeed, it saves a few lines of code, but is it really an improvement?
>
> > +
> > // 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
> > -// carefull to only call this funcion during device unregister.
> > +// careful to only call this function during device unregister.
>
> Not related to the rest of the patch.
>
> [...]
>
> As it stands, I think this change does not improve the code. Obviously, it
> is a subjective opinion. What the other developers think about it?
I agree with you, this just makes things more complex for no good
reason.
Now dropped from my review queue.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
@ 2020-08-18 14:02 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2020-08-18 14:02 UTC (permalink / raw)
To: Jérôme Pouiller; +Cc: devel, Tomer Samara, linux-kernel
On Mon, Aug 10, 2020 at 11:38:33AM +0200, Jérôme Pouiller wrote:
> Hello Tomer,
>
> On Wednesday 5 August 2020 14:14:42 CEST Tomer Samara wrote:
> >
> > Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
> > wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
> > which works as follow:
> > wfx_full_send() - simple wrapper for both wfx_fill_header()
> > and wfx_cmd_send().
> > wfx_full_send_no_reply_async() - wrapper for both but with
> > NULL as reply and size zero.
> > wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
> > but with false async value
> > wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
> > but also free the struct hif_msg.
> >
> > Signed-off-by: Tomer Samara <tomersamara98@gmail.com>
> > ---
> > Changes in v2:
> > - Changed these functions to static
> >
> > drivers/staging/wfx/hif_tx.c | 180 ++++++++++++++++-------------------
> > 1 file changed, 80 insertions(+), 100 deletions(-)
> >
> > diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> > index 5110f9b93762..17f668fa40a0 100644
> > --- a/drivers/staging/wfx/hif_tx.c
> > +++ b/drivers/staging/wfx/hif_tx.c
> > @@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
> >
> > static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
> > {
> > - *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
> > + *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
>
> This change is not related to the rest of the patch. It should probably be
> split out.
>
> > if (*hif)
> > return (*hif)->body;
> > else
> > @@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg *request,
> > return ret;
> > }
> >
> > +static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
> > + size_t reply_len, bool async, int if_id, unsigned int cmd,
> > + int size)
> > +{
> > + wfx_fill_header(hif, if_id, cmd, size);
> > + return wfx_cmd_send(wdev, hif, reply, reply_len, async);
> > +}
>
> This function takes 8 parameters. Are you sure it simplifies the code?
>
> In add, it does two actions: modify hif and send it. I would keep these
> two actions separated.
>
> > +
> > +static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> > + unsigned int cmd, int size, bool async)
> > +{
> > + return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
> > +}
>
> Does it make sense to have a parameter 'async' to
> wfx_full_send_no_reply_async()? It is weird to call this function with
> async=false, no?
>
> > +
> > +static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> > + unsigned int cmd, int size)
> > +{
> > + return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
> > +}
> > +
> > +static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int if_id,
> > + unsigned int cmd, int size)
> > +{
> > + int ret;
> > +
> > + ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
> > + kfree(hif);
> > + return ret;
> > +}
>
> One more time, sending the data and releasing are unrelated actions.
> Indeed, it saves a few lines of code, but is it really an improvement?
>
> > +
> > // 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
> > -// carefull to only call this funcion during device unregister.
> > +// careful to only call this function during device unregister.
>
> Not related to the rest of the patch.
>
> [...]
>
> As it stands, I think this change does not improve the code. Obviously, it
> is a subjective opinion. What the other developers think about it?
I agree with you, this just makes things more complex for no good
reason.
Now dropped from my review queue.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-18 14:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-05 12:14 [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c Tomer Samara
2020-08-05 12:14 ` Tomer Samara
2020-08-10 9:38 ` Jérôme Pouiller
2020-08-10 9:38 ` Jérôme Pouiller
2020-08-18 14:02 ` Greg KH
2020-08-18 14:02 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.