[PATCH v2 0/3] mwifiex: add .get_tx_power and .get_antenna cfg80211 operations

[PATCH v2 0/3] mwifiex: add .get_tx_power and .get_antenna cfg80211 operations

[Javier Martinez Canillas]

Hello,

Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
ops") the wireless core warns if a driver implements a cfg80211 callback
but doesn't implements the inverse operation. The mwifiex driver has two
set operations defined without the get counterpat so warnings are shown:

    WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
    Modules linked in: mwifiex_sdio mwifiex
    CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: G        W       4.7.0-rc1-next-20160531-00006-g569df5b983f3
    Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
    Workqueue: events request_firmware_work_func
    [<c010e1ac>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14)
    [<c010af38>] (show_stack) from [<c0323b9c>] (dump_stack+0x88/0x9c)
    [<c0323b9c>] (dump_stack) from [<c011a828>] (__warn+0xe8/0x100)
    [<c011a828>] (__warn) from [<c011a8f0>] (warn_slowpath_null+0x20/0x28)
    [<c011a8f0>] (warn_slowpath_null) from [<c06a42d4>] (wiphy_new_nm+0x66c/0x6ac)
    [<c06a42d4>] (wiphy_new_nm) from [<bf1c24cc>] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
    [<bf1c24cc>] (mwifiex_register_cfg80211 [mwifiex]) from [<bf1a0018>] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
    [<bf1a0018>] (mwifiex_fw_dpc [mwifiex]) from [<c040eb74>] (request_firmware_work_func+0x30/0x58)
    [<c040eb74>] (request_firmware_work_func) from [<c012fe90>] (process_one_work+0x124/0x338)
    [<c012fe90>] (process_one_work) from [<c01300dc>] (worker_thread+0x38/0x4d4)
    [<c01300dc>] (worker_thread) from [<c01353b8>] (kthread+0xdc/0xf4)
    [<c01353b8>] (kthread) from [<c0107978>] (ret_from_fork+0x14/0x3c)

This patch series fixes the warnings by implementing callbacks for the
missing get operations.

Since the first version was posted [0], Amitkumar Karwar from Marvell
provided me their patches that adds the same. The .get_tx_power patch was
very similar to mine. So I kept my patch and added the delta as a follow
up patch but discarded my .get_antenna patch since the one provided by
Amitkumar queries the firmware so is the correct approach.

Patches were tested on an Exynos5800 Chromebook that has a mwifiex chip.

Changes since v1:
- Drop patch that reports antenna cached values and use Marvell's patch
  instead.
- Add patch that moves the .get_tx_power() logic to sta_ioctl.c file.

[0]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1160119.html

Best regards,
Javier


Javier Martinez Canillas (1):
  mwifiex: add a cfg80211 .get_tx_power operation callback

Shengzhen Li (2):
  mwifiex: move .get_tx_power logic to station ioctl file
  mwifiex: add get_antenna support for cfg80211

 drivers/net/wireless/marvell/mwifiex/cfg80211.c    | 32 ++++++++++++++
 drivers/net/wireless/marvell/mwifiex/fw.h          |  3 ++
 drivers/net/wireless/marvell/mwifiex/init.c        |  2 +
 drivers/net/wireless/marvell/mwifiex/main.h        |  4 ++
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c     | 50 +++++++++++++++-------
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++--
 drivers/net/wireless/marvell/mwifiex/sta_ioctl.c   | 18 ++++++++
 7 files changed, 100 insertions(+), 19 deletions(-)

-- 
2.5.5

[PATCH v2 1/3] mwifiex: add a cfg80211 .get_tx_power operation callback

[Javier Martinez Canillas]

The mwifiex driver implements a cfg80211 .set_tx_power operation handler
but doesn't have the inverse .get_tx_power callback.

This not only has the effect that the Tx power can't be reported to user
space tools such as iwconfig and iwlist but also that the wireless core
prints a warning when a new wiphy is created due an cfg80211 operation
being implemented without its counterpart.

After this patch, the Tx power is properly reported to user-space tools:

$ iwlist mlan0 txpower
mlan0     unknown transmit-power information.

          Current Tx-Power=13 dBm       (19 mW)

and also the following warning isn't shown anymore on the driver probe:

WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
Modules linked in: mwifiex_sdio mwifiex
CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: G        W       4.7.0-rc1-next-20160531-00006-g569df5b983f3
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: events request_firmware_work_func
[<c010e1ac>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14)
[<c010af38>] (show_stack) from [<c0323b9c>] (dump_stack+0x88/0x9c)
[<c0323b9c>] (dump_stack) from [<c011a828>] (__warn+0xe8/0x100)
[<c011a828>] (__warn) from [<c011a8f0>] (warn_slowpath_null+0x20/0x28)
[<c011a8f0>] (warn_slowpath_null) from [<c06a42d4>] (wiphy_new_nm+0x66c/0x6ac)
[<c06a42d4>] (wiphy_new_nm) from [<bf1c24cc>] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
[<bf1c24cc>] (mwifiex_register_cfg80211 [mwifiex]) from [<bf1a0018>] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
[<bf1a0018>] (mwifiex_fw_dpc [mwifiex]) from [<c040eb74>] (request_firmware_work_func+0x30/0x58)
[<c040eb74>] (request_firmware_work_func) from [<c012fe90>] (process_one_work+0x124/0x338)
[<c012fe90>] (process_one_work) from [<c01300dc>] (worker_thread+0x38/0x4d4)
[<c01300dc>] (worker_thread) from [<c01353b8>] (kthread+0xdc/0xf4)
[<c01353b8>] (kthread) from [<c0107978>] (ret_from_fork+0x14/0x3c)

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index ff948a922222..b17f3d09a2c7 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -377,6 +377,29 @@ mwifiex_cfg80211_set_tx_power(struct wiphy *wiphy,
 }
 
 /*
+ * CFG802.11 operation handler to get Tx power.
+ */
+static int
+mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
+			      struct wireless_dev *wdev,
+			      int *dbm)
+{
+	struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
+	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
+							MWIFIEX_BSS_ROLE_ANY);
+	int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
+				   HostCmd_ACT_GEN_GET, 0, NULL, true);
+
+	if (ret < 0)
+		return ret;
+
+	/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
+	*dbm = priv->tx_power_level;
+
+	return 0;
+}
+
+/*
  * CFG802.11 operation handler to set Power Save option.
  *
  * The timeout value, if provided, is currently ignored.
@@ -3940,6 +3963,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
 	.set_default_key = mwifiex_cfg80211_set_default_key,
 	.set_power_mgmt = mwifiex_cfg80211_set_power_mgmt,
 	.set_tx_power = mwifiex_cfg80211_set_tx_power,
+	.get_tx_power = mwifiex_cfg80211_get_tx_power,
 	.set_bitrate_mask = mwifiex_cfg80211_set_bitrate_mask,
 	.start_ap = mwifiex_cfg80211_start_ap,
 	.stop_ap = mwifiex_cfg80211_stop_ap,
-- 
2.5.5

[PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

From: Shengzhen Li <szli@marvell.com>

Most cfg80211 operations are just a wrappers to functions defined in the
sta_ioctl.c file, so for consistency move the .get_tx_power logic there.

Signed-off-by: Shengzhen Li <szli@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
[javier: update the subject line and commit message]
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/net/wireless/marvell/mwifiex/cfg80211.c  | 14 +++-----------
 drivers/net/wireless/marvell/mwifiex/main.h      |  2 ++
 drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index b17f3d09a2c7..ff3f63ed95e1 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
 			      int *dbm)
 {
 	struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
-	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
-							MWIFIEX_BSS_ROLE_ANY);
-	int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
-				   HostCmd_ACT_GEN_GET, 0, NULL, true);
-
-	if (ret < 0)
-		return ret;
-
-	/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
-	*dbm = priv->tx_power_level;
+	struct mwifiex_private *priv;
 
-	return 0;
+	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
+	return mwifiex_get_tx_power(priv, dbm);
 }
 
 /*
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 0207af00be42..79c28cfb7780 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1464,6 +1464,8 @@ int mwifiex_drv_get_driver_version(struct mwifiex_adapter *adapter,
 int mwifiex_set_tx_power(struct mwifiex_private *priv,
 			 struct mwifiex_power_cfg *power_cfg);
 
+int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm);
+
 int mwifiex_main_process(struct mwifiex_adapter *);
 
 int mwifiex_queue_tx_pkt(struct mwifiex_private *priv, struct sk_buff *skb);
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
index 8e0862657122..70ff9b805b5b 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
@@ -775,6 +775,24 @@ int mwifiex_set_tx_power(struct mwifiex_private *priv,
 }
 
 /*
+ * IOCTL request handler to get tx power configuration.
+ *
+ * This function prepares the correct firmware command and
+ * issues it.
+ */
+int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm)
+{
+	int ret;
+
+	ret = mwifiex_send_cmd(priv, HostCmd_CMD_TXPWR_CFG,
+			       HostCmd_ACT_GEN_GET, 0, NULL, true);
+
+	*dbm = priv->tx_power_level;
+
+	return ret;
+}
+
+/*
  * IOCTL request handler to get power save mode.
  *
  * This function prepares the correct firmware command and
-- 
2.5.5

[PATCH v2 3/3] mwifiex: add get_antenna support for cfg80211

[Javier Martinez Canillas]

From: Shengzhen Li <szli@marvell.com>

Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
ops") the wireless core warns if a driver implements a cfg80211 callback
but doesn't implements the inverse operation.

The mwifiex driver defines a .set_antenna handler but not a .get_antenna
so this not only makes the core to print a warning when creating a new
wiphy but also the antenna isn't reported to user-space apps such as iw.

This patch queries the antenna to the firmware so is properly reported to
user-space. With this patch, the wireless core does not warn anymore and:

$ iw phy phy0 info | grep Antennas
        Available Antennas: TX 0x3 RX 0x3
        Configured Antennas: TX 0x3 RX 0x3

Signed-off-by: Shengzhen Li <szli@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
[javier: expand the commit message]
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/net/wireless/marvell/mwifiex/cfg80211.c    | 16 +++++++
 drivers/net/wireless/marvell/mwifiex/fw.h          |  3 ++
 drivers/net/wireless/marvell/mwifiex/init.c        |  2 +
 drivers/net/wireless/marvell/mwifiex/main.h        |  2 +
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c     | 50 +++++++++++++++-------
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++--
 6 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index ff3f63ed95e1..ff1eefe5087b 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1819,6 +1819,21 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant)
 				HostCmd_ACT_GEN_SET, 0, &ant_cfg, true);
 }
 
+static int
+mwifiex_cfg80211_get_antenna(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant)
+{
+	struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
+	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
+							MWIFIEX_BSS_ROLE_ANY);
+	mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA,
+			 HostCmd_ACT_GEN_GET, 0, NULL, true);
+
+	*tx_ant = priv->tx_ant;
+	*rx_ant = priv->rx_ant;
+
+	return 0;
+}
+
 /* cfg80211 operation handler for stop ap.
  * Function stops BSS running at uAP interface.
  */
@@ -3962,6 +3977,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
 	.change_beacon = mwifiex_cfg80211_change_beacon,
 	.set_cqm_rssi_config = mwifiex_cfg80211_set_cqm_rssi_config,
 	.set_antenna = mwifiex_cfg80211_set_antenna,
+	.get_antenna = mwifiex_cfg80211_get_antenna,
 	.del_station = mwifiex_cfg80211_del_station,
 	.sched_scan_start = mwifiex_cfg80211_sched_scan_start,
 	.sched_scan_stop = mwifiex_cfg80211_sched_scan_stop,
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 8e4145abdbfa..cef72343f5b6 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -462,6 +462,9 @@ enum P2P_MODES {
 #define HostCmd_ACT_SET_RX              0x0001
 #define HostCmd_ACT_SET_TX              0x0002
 #define HostCmd_ACT_SET_BOTH            0x0003
+#define HostCmd_ACT_GET_RX              0x0004
+#define HostCmd_ACT_GET_TX              0x0008
+#define HostCmd_ACT_GET_BOTH            0x000c
 
 #define RF_ANTENNA_AUTO                 0xFFFF
 
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 78c532f0d286..fbaf49056746 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -110,6 +110,8 @@ int mwifiex_init_priv(struct mwifiex_private *priv)
 	priv->tx_power_level = 0;
 	priv->max_tx_power_level = 0;
 	priv->min_tx_power_level = 0;
+	priv->tx_ant = 0;
+	priv->rx_ant = 0;
 	priv->tx_rate = 0;
 	priv->rxpd_htinfo = 0;
 	priv->rxpd_rate = 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 79c28cfb7780..2ae7ff74e1c6 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -533,6 +533,8 @@ struct mwifiex_private {
 	u16 tx_power_level;
 	u8 max_tx_power_level;
 	u8 min_tx_power_level;
+	u32 tx_ant;
+	u32 rx_ant;
 	u8 tx_rate;
 	u8 tx_htinfo;
 	u8 rxpd_htinfo;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index e436574b1698..8c658495bf66 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -313,23 +313,41 @@ static int mwifiex_cmd_rf_antenna(struct mwifiex_private *priv,
 
 	cmd->command = cpu_to_le16(HostCmd_CMD_RF_ANTENNA);
 
-	if (cmd_action != HostCmd_ACT_GEN_SET)
-		return 0;
-
-	if (priv->adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
-		cmd->size = cpu_to_le16(sizeof(struct host_cmd_ds_rf_ant_mimo) +
-					S_DS_GEN);
-		ant_mimo->action_tx = cpu_to_le16(HostCmd_ACT_SET_TX);
-		ant_mimo->tx_ant_mode = cpu_to_le16((u16)ant_cfg->tx_ant);
-		ant_mimo->action_rx = cpu_to_le16(HostCmd_ACT_SET_RX);
-		ant_mimo->rx_ant_mode = cpu_to_le16((u16)ant_cfg->rx_ant);
-	} else {
-		cmd->size = cpu_to_le16(sizeof(struct host_cmd_ds_rf_ant_siso) +
-					S_DS_GEN);
-		ant_siso->action = cpu_to_le16(HostCmd_ACT_SET_BOTH);
-		ant_siso->ant_mode = cpu_to_le16((u16)ant_cfg->tx_ant);
+	switch (cmd_action) {
+	case HostCmd_ACT_GEN_SET:
+		if (priv->adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
+			cmd->size = cpu_to_le16(sizeof(struct
+						host_cmd_ds_rf_ant_mimo)
+						+ S_DS_GEN);
+			ant_mimo->action_tx = cpu_to_le16(HostCmd_ACT_SET_TX);
+			ant_mimo->tx_ant_mode = cpu_to_le16((u16)ant_cfg->
+							    tx_ant);
+			ant_mimo->action_rx = cpu_to_le16(HostCmd_ACT_SET_RX);
+			ant_mimo->rx_ant_mode = cpu_to_le16((u16)ant_cfg->
+							    rx_ant);
+		} else {
+			cmd->size = cpu_to_le16(sizeof(struct
+						host_cmd_ds_rf_ant_siso) +
+						S_DS_GEN);
+			ant_siso->action = cpu_to_le16(HostCmd_ACT_SET_BOTH);
+			ant_siso->ant_mode = cpu_to_le16((u16)ant_cfg->tx_ant);
+		}
+		break;
+	case HostCmd_ACT_GEN_GET:
+		if (priv->adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
+			cmd->size = cpu_to_le16(sizeof(struct
+						host_cmd_ds_rf_ant_mimo) +
+						S_DS_GEN);
+			ant_mimo->action_tx = cpu_to_le16(HostCmd_ACT_GET_TX);
+			ant_mimo->action_rx = cpu_to_le16(HostCmd_ACT_GET_RX);
+		} else {
+			cmd->size = cpu_to_le16(sizeof(struct
+						host_cmd_ds_rf_ant_siso) +
+						S_DS_GEN);
+			ant_siso->action = cpu_to_le16(HostCmd_ACT_GET_BOTH);
+		}
+		break;
 	}
-
 	return 0;
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index d18c7979d723..11f19b668e13 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -469,7 +469,9 @@ static int mwifiex_ret_rf_antenna(struct mwifiex_private *priv,
 	struct host_cmd_ds_rf_ant_siso *ant_siso = &resp->params.ant_siso;
 	struct mwifiex_adapter *adapter = priv->adapter;
 
-	if (adapter->hw_dev_mcs_support == HT_STREAM_2X2)
+	if (adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
+		priv->tx_ant = le16_to_cpu(ant_mimo->tx_ant_mode);
+		priv->rx_ant = le16_to_cpu(ant_mimo->rx_ant_mode);
 		mwifiex_dbg(adapter, INFO,
 			    "RF_ANT_RESP: Tx action = 0x%x, Tx Mode = 0x%04x\t"
 			    "Rx action = 0x%x, Rx Mode = 0x%04x\n",
@@ -477,12 +479,14 @@ static int mwifiex_ret_rf_antenna(struct mwifiex_private *priv,
 			    le16_to_cpu(ant_mimo->tx_ant_mode),
 			    le16_to_cpu(ant_mimo->action_rx),
 			    le16_to_cpu(ant_mimo->rx_ant_mode));
-	else
+	} else {
+		priv->tx_ant = le16_to_cpu(ant_siso->ant_mode);
+		priv->rx_ant = le16_to_cpu(ant_siso->ant_mode);
 		mwifiex_dbg(adapter, INFO,
 			    "RF_ANT_RESP: action = 0x%x, Mode = 0x%04x\n",
 			    le16_to_cpu(ant_siso->action),
 			    le16_to_cpu(ant_siso->ant_mode));
-
+	}
 	return 0;
 }
 
-- 
2.5.5

Re: [PATCH v2 1/3] mwifiex: add a cfg80211 .get_tx_power operation callback

[Enric Balletbo Serra]

2016-06-06 19:02 GMT+02:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> The mwifiex driver implements a cfg80211 .set_tx_power operation handler
> but doesn't have the inverse .get_tx_power callback.
>
> This not only has the effect that the Tx power can't be reported to user
> space tools such as iwconfig and iwlist but also that the wireless core
> prints a warning when a new wiphy is created due an cfg80211 operation
> being implemented without its counterpart.
>
> After this patch, the Tx power is properly reported to user-space tools:
>
> $ iwlist mlan0 txpower
> mlan0     unknown transmit-power information.
>
>           Current Tx-Power=13 dBm       (19 mW)
>
> and also the following warning isn't shown anymore on the driver probe:
>
> WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
> Modules linked in: mwifiex_sdio mwifiex
> CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: G        W       4.7.0-rc1-next-20160531-00006-g569df5b983f3
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: events request_firmware_work_func
> [<c010e1ac>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14)
> [<c010af38>] (show_stack) from [<c0323b9c>] (dump_stack+0x88/0x9c)
> [<c0323b9c>] (dump_stack) from [<c011a828>] (__warn+0xe8/0x100)
> [<c011a828>] (__warn) from [<c011a8f0>] (warn_slowpath_null+0x20/0x28)
> [<c011a8f0>] (warn_slowpath_null) from [<c06a42d4>] (wiphy_new_nm+0x66c/0x6ac)
> [<c06a42d4>] (wiphy_new_nm) from [<bf1c24cc>] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
> [<bf1c24cc>] (mwifiex_register_cfg80211 [mwifiex]) from [<bf1a0018>] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
> [<bf1a0018>] (mwifiex_fw_dpc [mwifiex]) from [<c040eb74>] (request_firmware_work_func+0x30/0x58)
> [<c040eb74>] (request_firmware_work_func) from [<c012fe90>] (process_one_work+0x124/0x338)
> [<c012fe90>] (process_one_work) from [<c01300dc>] (worker_thread+0x38/0x4d4)
> [<c01300dc>] (worker_thread) from [<c01353b8>] (kthread+0xdc/0xf4)
> [<c01353b8>] (kthread) from [<c0107978>] (ret_from_fork+0x14/0x3c)
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index ff948a922222..b17f3d09a2c7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -377,6 +377,29 @@ mwifiex_cfg80211_set_tx_power(struct wiphy *wiphy,
>  }
>
>  /*
> + * CFG802.11 operation handler to get Tx power.
> + */
> +static int
> +mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
> +                             struct wireless_dev *wdev,
> +                             int *dbm)
> +{
> +       struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
> +       struct mwifiex_private *priv = mwifiex_get_priv(adapter,
> +                                                       MWIFIEX_BSS_ROLE_ANY);
> +       int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
> +                                  HostCmd_ACT_GEN_GET, 0, NULL, true);
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
> +       *dbm = priv->tx_power_level;
> +
> +       return 0;
> +}
> +
> +/*
>   * CFG802.11 operation handler to set Power Save option.
>   *
>   * The timeout value, if provided, is currently ignored.
> @@ -3940,6 +3963,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
>         .set_default_key = mwifiex_cfg80211_set_default_key,
>         .set_power_mgmt = mwifiex_cfg80211_set_power_mgmt,
>         .set_tx_power = mwifiex_cfg80211_set_tx_power,
> +       .get_tx_power = mwifiex_cfg80211_get_tx_power,
>         .set_bitrate_mask = mwifiex_cfg80211_set_bitrate_mask,
>         .start_ap = mwifiex_cfg80211_start_ap,
>         .stop_ap = mwifiex_cfg80211_stop_ap,
> --
> 2.5.5
>

This fixes the WARN_ON and makes things work, thanks.

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Enric Balletbo Serra]

2016-06-06 19:02 GMT+02:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> From: Shengzhen Li <szli@marvell.com>
>
> Most cfg80211 operations are just a wrappers to functions defined in the
> sta_ioctl.c file, so for consistency move the .get_tx_power logic there.
>
> Signed-off-by: Shengzhen Li <szli@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> [javier: update the subject line and commit message]
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>
> ---
>
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c  | 14 +++-----------
>  drivers/net/wireless/marvell/mwifiex/main.h      |  2 ++
>  drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index b17f3d09a2c7..ff3f63ed95e1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
>                               int *dbm)
>  {
>         struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
> -       struct mwifiex_private *priv = mwifiex_get_priv(adapter,
> -                                                       MWIFIEX_BSS_ROLE_ANY);
> -       int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
> -                                  HostCmd_ACT_GEN_GET, 0, NULL, true);
> -
> -       if (ret < 0)
> -               return ret;
> -
> -       /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
> -       *dbm = priv->tx_power_level;
> +       struct mwifiex_private *priv;
>
> -       return 0;
> +       priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> +       return mwifiex_get_tx_power(priv, dbm);
>  }
>
>  /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 0207af00be42..79c28cfb7780 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1464,6 +1464,8 @@ int mwifiex_drv_get_driver_version(struct mwifiex_adapter *adapter,
>  int mwifiex_set_tx_power(struct mwifiex_private *priv,
>                          struct mwifiex_power_cfg *power_cfg);
>
> +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm);
> +
>  int mwifiex_main_process(struct mwifiex_adapter *);
>
>  int mwifiex_queue_tx_pkt(struct mwifiex_private *priv, struct sk_buff *skb);
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
> index 8e0862657122..70ff9b805b5b 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
> @@ -775,6 +775,24 @@ int mwifiex_set_tx_power(struct mwifiex_private *priv,
>  }
>
>  /*
> + * IOCTL request handler to get tx power configuration.
> + *
> + * This function prepares the correct firmware command and
> + * issues it.
> + */
> +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm)
> +{
> +       int ret;
> +
> +       ret = mwifiex_send_cmd(priv, HostCmd_CMD_TXPWR_CFG,
> +                              HostCmd_ACT_GEN_GET, 0, NULL, true);
> +
> +       *dbm = priv->tx_power_level;
> +
> +       return ret;
> +}
> +
> +/*
>   * IOCTL request handler to get power save mode.
>   *
>   * This function prepares the correct firmware command and
> --
> 2.5.5
>

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Re: [PATCH v2 3/3] mwifiex: add get_antenna support for cfg80211

[Enric Balletbo Serra]

2016-06-06 19:02 GMT+02:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> From: Shengzhen Li <szli@marvell.com>
>
> Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent
> ops") the wireless core warns if a driver implements a cfg80211 callback
> but doesn't implements the inverse operation.
>
> The mwifiex driver defines a .set_antenna handler but not a .get_antenna
> so this not only makes the core to print a warning when creating a new
> wiphy but also the antenna isn't reported to user-space apps such as iw.
>
> This patch queries the antenna to the firmware so is properly reported to
> user-space. With this patch, the wireless core does not warn anymore and:
>
> $ iw phy phy0 info | grep Antennas
>         Available Antennas: TX 0x3 RX 0x3
>         Configured Antennas: TX 0x3 RX 0x3
>
> Signed-off-by: Shengzhen Li <szli@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> [javier: expand the commit message]
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>
> ---
>
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c    | 16 +++++++
>  drivers/net/wireless/marvell/mwifiex/fw.h          |  3 ++
>  drivers/net/wireless/marvell/mwifiex/init.c        |  2 +
>  drivers/net/wireless/marvell/mwifiex/main.h        |  2 +
>  drivers/net/wireless/marvell/mwifiex/sta_cmd.c     | 50 +++++++++++++++-------
>  drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++--
>  6 files changed, 64 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index ff3f63ed95e1..ff1eefe5087b 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -1819,6 +1819,21 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant)
>                                 HostCmd_ACT_GEN_SET, 0, &ant_cfg, true);
>  }
>
> +static int
> +mwifiex_cfg80211_get_antenna(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant)
> +{
> +       struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
> +       struct mwifiex_private *priv = mwifiex_get_priv(adapter,
> +                                                       MWIFIEX_BSS_ROLE_ANY);
> +       mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA,
> +                        HostCmd_ACT_GEN_GET, 0, NULL, true);
> +
> +       *tx_ant = priv->tx_ant;
> +       *rx_ant = priv->rx_ant;
> +
> +       return 0;
> +}
> +
>  /* cfg80211 operation handler for stop ap.
>   * Function stops BSS running at uAP interface.
>   */
> @@ -3962,6 +3977,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = {
>         .change_beacon = mwifiex_cfg80211_change_beacon,
>         .set_cqm_rssi_config = mwifiex_cfg80211_set_cqm_rssi_config,
>         .set_antenna = mwifiex_cfg80211_set_antenna,
> +       .get_antenna = mwifiex_cfg80211_get_antenna,
>         .del_station = mwifiex_cfg80211_del_station,
>         .sched_scan_start = mwifiex_cfg80211_sched_scan_start,
>         .sched_scan_stop = mwifiex_cfg80211_sched_scan_stop,
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 8e4145abdbfa..cef72343f5b6 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -462,6 +462,9 @@ enum P2P_MODES {
>  #define HostCmd_ACT_SET_RX              0x0001
>  #define HostCmd_ACT_SET_TX              0x0002
>  #define HostCmd_ACT_SET_BOTH            0x0003
> +#define HostCmd_ACT_GET_RX              0x0004
> +#define HostCmd_ACT_GET_TX              0x0008
> +#define HostCmd_ACT_GET_BOTH            0x000c
>
>  #define RF_ANTENNA_AUTO                 0xFFFF
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
> index 78c532f0d286..fbaf49056746 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -110,6 +110,8 @@ int mwifiex_init_priv(struct mwifiex_private *priv)
>         priv->tx_power_level = 0;
>         priv->max_tx_power_level = 0;
>         priv->min_tx_power_level = 0;
> +       priv->tx_ant = 0;
> +       priv->rx_ant = 0;
>         priv->tx_rate = 0;
>         priv->rxpd_htinfo = 0;
>         priv->rxpd_rate = 0;
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 79c28cfb7780..2ae7ff74e1c6 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -533,6 +533,8 @@ struct mwifiex_private {
>         u16 tx_power_level;
>         u8 max_tx_power_level;
>         u8 min_tx_power_level;
> +       u32 tx_ant;
> +       u32 rx_ant;
>         u8 tx_rate;
>         u8 tx_htinfo;
>         u8 rxpd_htinfo;
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> index e436574b1698..8c658495bf66 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> @@ -313,23 +313,41 @@ static int mwifiex_cmd_rf_antenna(struct mwifiex_private *priv,
>
>         cmd->command = cpu_to_le16(HostCmd_CMD_RF_ANTENNA);
>
> -       if (cmd_action != HostCmd_ACT_GEN_SET)
> -               return 0;
> -
> -       if (priv->adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
> -               cmd->size = cpu_to_le16(sizeof(struct host_cmd_ds_rf_ant_mimo) +
> -                                       S_DS_GEN);
> -               ant_mimo->action_tx = cpu_to_le16(HostCmd_ACT_SET_TX);
> -               ant_mimo->tx_ant_mode = cpu_to_le16((u16)ant_cfg->tx_ant);
> -               ant_mimo->action_rx = cpu_to_le16(HostCmd_ACT_SET_RX);
> -               ant_mimo->rx_ant_mode = cpu_to_le16((u16)ant_cfg->rx_ant);
> -       } else {
> -               cmd->size = cpu_to_le16(sizeof(struct host_cmd_ds_rf_ant_siso) +
> -                                       S_DS_GEN);
> -               ant_siso->action = cpu_to_le16(HostCmd_ACT_SET_BOTH);
> -               ant_siso->ant_mode = cpu_to_le16((u16)ant_cfg->tx_ant);
> +       switch (cmd_action) {
> +       case HostCmd_ACT_GEN_SET:
> +               if (priv->adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
> +                       cmd->size = cpu_to_le16(sizeof(struct
> +                                               host_cmd_ds_rf_ant_mimo)
> +                                               + S_DS_GEN);
> +                       ant_mimo->action_tx = cpu_to_le16(HostCmd_ACT_SET_TX);
> +                       ant_mimo->tx_ant_mode = cpu_to_le16((u16)ant_cfg->
> +                                                           tx_ant);
> +                       ant_mimo->action_rx = cpu_to_le16(HostCmd_ACT_SET_RX);
> +                       ant_mimo->rx_ant_mode = cpu_to_le16((u16)ant_cfg->
> +                                                           rx_ant);
> +               } else {
> +                       cmd->size = cpu_to_le16(sizeof(struct
> +                                               host_cmd_ds_rf_ant_siso) +
> +                                               S_DS_GEN);
> +                       ant_siso->action = cpu_to_le16(HostCmd_ACT_SET_BOTH);
> +                       ant_siso->ant_mode = cpu_to_le16((u16)ant_cfg->tx_ant);
> +               }
> +               break;
> +       case HostCmd_ACT_GEN_GET:
> +               if (priv->adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
> +                       cmd->size = cpu_to_le16(sizeof(struct
> +                                               host_cmd_ds_rf_ant_mimo) +
> +                                               S_DS_GEN);
> +                       ant_mimo->action_tx = cpu_to_le16(HostCmd_ACT_GET_TX);
> +                       ant_mimo->action_rx = cpu_to_le16(HostCmd_ACT_GET_RX);
> +               } else {
> +                       cmd->size = cpu_to_le16(sizeof(struct
> +                                               host_cmd_ds_rf_ant_siso) +
> +                                               S_DS_GEN);
> +                       ant_siso->action = cpu_to_le16(HostCmd_ACT_GET_BOTH);
> +               }
> +               break;
>         }
> -
>         return 0;
>  }
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index d18c7979d723..11f19b668e13 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -469,7 +469,9 @@ static int mwifiex_ret_rf_antenna(struct mwifiex_private *priv,
>         struct host_cmd_ds_rf_ant_siso *ant_siso = &resp->params.ant_siso;
>         struct mwifiex_adapter *adapter = priv->adapter;
>
> -       if (adapter->hw_dev_mcs_support == HT_STREAM_2X2)
> +       if (adapter->hw_dev_mcs_support == HT_STREAM_2X2) {
> +               priv->tx_ant = le16_to_cpu(ant_mimo->tx_ant_mode);
> +               priv->rx_ant = le16_to_cpu(ant_mimo->rx_ant_mode);
>                 mwifiex_dbg(adapter, INFO,
>                             "RF_ANT_RESP: Tx action = 0x%x, Tx Mode = 0x%04x\t"
>                             "Rx action = 0x%x, Rx Mode = 0x%04x\n",
> @@ -477,12 +479,14 @@ static int mwifiex_ret_rf_antenna(struct mwifiex_private *priv,
>                             le16_to_cpu(ant_mimo->tx_ant_mode),
>                             le16_to_cpu(ant_mimo->action_rx),
>                             le16_to_cpu(ant_mimo->rx_ant_mode));
> -       else
> +       } else {
> +               priv->tx_ant = le16_to_cpu(ant_siso->ant_mode);
> +               priv->rx_ant = le16_to_cpu(ant_siso->ant_mode);
>                 mwifiex_dbg(adapter, INFO,
>                             "RF_ANT_RESP: action = 0x%x, Mode = 0x%04x\n",
>                             le16_to_cpu(ant_siso->action),
>                             le16_to_cpu(ant_siso->ant_mode));
> -
> +       }
>         return 0;
>  }
>
> --
> 2.5.5
>

This fixes the WARN_ON and makes things work, thanks.

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Kalle Valo]

Javier Martinez Canillas <javier@osg.samsung.com> writes:

> From: Shengzhen Li <szli@marvell.com>
>
> Most cfg80211 operations are just a wrappers to functions defined in the
> sta_ioctl.c file, so for consistency move the .get_tx_power logic there.
>
> Signed-off-by: Shengzhen Li <szli@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> [javier: update the subject line and commit message]
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

[...]

> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
>  			      int *dbm)
>  {
>  	struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
> -	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
> -							MWIFIEX_BSS_ROLE_ANY);
> -	int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
> -				   HostCmd_ACT_GEN_GET, 0, NULL, true);
> -
> -	if (ret < 0)
> -		return ret;
> -
> -	/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
> -	*dbm = priv->tx_power_level;
> +	struct mwifiex_private *priv;
>  
> -	return 0;
> +	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> +	return mwifiex_get_tx_power(priv, dbm);
>  }

So in patch 1 you added the patch and in patch 2 you move it to a
different location? That doesn't make any sense, can't you just fold the
two patches into one so that the function is added only once.

-- 
Kalle Valo

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Kalle,

On 06/10/2016 10:30 AM, Kalle Valo wrote:
> Javier Martinez Canillas <javier@osg.samsung.com> writes:
> 
>> From: Shengzhen Li <szli@marvell.com>
>>
>> Most cfg80211 operations are just a wrappers to functions defined in the
>> sta_ioctl.c file, so for consistency move the .get_tx_power logic there.
>>
>> Signed-off-by: Shengzhen Li <szli@marvell.com>
>> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
>> [javier: update the subject line and commit message]
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> [...]
> 
>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy,
>>  			      int *dbm)
>>  {
>>  	struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy);
>> -	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
>> -							MWIFIEX_BSS_ROLE_ANY);
>> -	int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
>> -				   HostCmd_ACT_GEN_GET, 0, NULL, true);
>> -
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */
>> -	*dbm = priv->tx_power_level;
>> +	struct mwifiex_private *priv;
>>  
>> -	return 0;
>> +	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>> +	return mwifiex_get_tx_power(priv, dbm);
>>  }
> 
> So in patch 1 you added the patch and in patch 2 you move it to a
> different location? That doesn't make any sense, can't you just fold the
> two patches into one so that the function is added only once.
>

I posted this patch in v1 but then Amitkumar shared his patch with me that
was very similar to mine, only that the logic was in a different location.

So I included his delta as a separate patch to try keeping attribution as
best as possible.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

RE: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Amitkumar Karwar]

Hi Kalle/Javier,

> From: Javier Martinez Canillas [mailto:javier@osg.samsung.com]
> Sent: Friday, June 10, 2016 8:07 PM
> To: Kalle Valo
> Cc: linux-kernel@vger.kernel.org; Julian Calaby; Shengzhen Li; Enric
> Balletbo i Serra; Amitkumar Karwar; netdev@vger.kernel.org; linux-
> wireless@vger.kernel.org; Nishant Sarmukadam
> Subject: Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station
> ioctl file
> 
> Hello Kalle,
> 
> On 06/10/2016 10:30 AM, Kalle Valo wrote:
> > Javier Martinez Canillas <javier@osg.samsung.com> writes:
> >
> >> From: Shengzhen Li <szli@marvell.com>
> >>
> >> Most cfg80211 operations are just a wrappers to functions defined in
> >> the sta_ioctl.c file, so for consistency move the .get_tx_power logic
> there.
> >>
> >> Signed-off-by: Shengzhen Li <szli@marvell.com>
> >> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> >> [javier: update the subject line and commit message]
> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> >
> > [...]
> >
> >> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> >> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> >> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy
> *wiphy,
> >>  			      int *dbm)
> >>  {
> >>  	struct mwifiex_adapter *adapter =
> mwifiex_cfg80211_get_adapter(wiphy);
> >> -	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
> >> -							MWIFIEX_BSS_ROLE_ANY);
> >> -	int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
> >> -				   HostCmd_ACT_GEN_GET, 0, NULL, true);
> >> -
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> -	/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler
> */
> >> -	*dbm = priv->tx_power_level;
> >> +	struct mwifiex_private *priv;
> >>
> >> -	return 0;
> >> +	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> >> +	return mwifiex_get_tx_power(priv, dbm);
> >>  }
> >
> > So in patch 1 you added the patch and in patch 2 you move it to a
> > different location? That doesn't make any sense, can't you just fold
> > the two patches into one so that the function is added only once.
> >
> 
> I posted this patch in v1 but then Amitkumar shared his patch with me
> that was very similar to mine, only that the logic was in a different
> location.
> 
> So I included his delta as a separate patch to try keeping attribution
> as best as possible.
> 

This patch (2/3) is only for code rearrangement and adds an unnecessary wrapper function. We can simply drop the patch.

Regards,
Amitkumar

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Amitkumar,

On 06/10/2016 12:26 PM, Amitkumar Karwar wrote:
> Hi Kalle/Javier,
> 
>> From: Javier Martinez Canillas [mailto:javier@osg.samsung.com]
>> Sent: Friday, June 10, 2016 8:07 PM
>> To: Kalle Valo
>> Cc: linux-kernel@vger.kernel.org; Julian Calaby; Shengzhen Li; Enric
>> Balletbo i Serra; Amitkumar Karwar; netdev@vger.kernel.org; linux-
>> wireless@vger.kernel.org; Nishant Sarmukadam
>> Subject: Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station
>> ioctl file
>>
>> Hello Kalle,
>>
>> On 06/10/2016 10:30 AM, Kalle Valo wrote:
>>> Javier Martinez Canillas <javier@osg.samsung.com> writes:
>>>
>>>> From: Shengzhen Li <szli@marvell.com>
>>>>
>>>> Most cfg80211 operations are just a wrappers to functions defined in
>>>> the sta_ioctl.c file, so for consistency move the .get_tx_power logic
>> there.
>>>>
>>>> Signed-off-by: Shengzhen Li <szli@marvell.com>
>>>> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
>>>> [javier: update the subject line and commit message]
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>
>>> [...]
>>>
>>>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy
>> *wiphy,
>>>>  			      int *dbm)
>>>>  {
>>>>  	struct mwifiex_adapter *adapter =
>> mwifiex_cfg80211_get_adapter(wiphy);
>>>> -	struct mwifiex_private *priv = mwifiex_get_priv(adapter,
>>>> -							MWIFIEX_BSS_ROLE_ANY);
>>>> -	int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR,
>>>> -				   HostCmd_ACT_GEN_GET, 0, NULL, true);
>>>> -
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> -
>>>> -	/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler
>> */
>>>> -	*dbm = priv->tx_power_level;
>>>> +	struct mwifiex_private *priv;
>>>>
>>>> -	return 0;
>>>> +	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>>>> +	return mwifiex_get_tx_power(priv, dbm);
>>>>  }
>>>
>>> So in patch 1 you added the patch and in patch 2 you move it to a
>>> different location? That doesn't make any sense, can't you just fold
>>> the two patches into one so that the function is added only once.
>>>
>>
>> I posted this patch in v1 but then Amitkumar shared his patch with me
>> that was very similar to mine, only that the logic was in a different
>> location.
>>
>> So I included his delta as a separate patch to try keeping attribution
>> as best as possible.
>>
> 
> This patch (2/3) is only for code rearrangement and adds an unnecessary wrapper function. We can simply drop the patch.
>

Agreed.

Kalle,

Patch 3/3 applies cleanly even after dropping patch 2/3.
Is that Ok for you or do you want me to re-resend a v3
with only patches 1/3 and 3/3?

> Regards,
> Amitkumar
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Kalle Valo]

Javier Martinez Canillas <javier@osg.samsung.com> writes:

>> This patch (2/3) is only for code rearrangement and adds an
>> unnecessary wrapper function. We can simply drop the patch.
>
> Agreed.
>
> Kalle,
>
> Patch 3/3 applies cleanly even after dropping patch 2/3.
> Is that Ok for you or do you want me to re-resend a v3
> with only patches 1/3 and 3/3?

I can drop patch 2, no need to resend. Thanks.

-- 
Kalle Valo

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Kalle,

On 06/10/2016 03:54 PM, Kalle Valo wrote:
> Javier Martinez Canillas <javier@osg.samsung.com> writes:
> 
>>> This patch (2/3) is only for code rearrangement and adds an
>>> unnecessary wrapper function. We can simply drop the patch.
>>
>> Agreed.
>>
>> Kalle,
>>
>> Patch 3/3 applies cleanly even after dropping patch 2/3.
>> Is that Ok for you or do you want me to re-resend a v3
>> with only patches 1/3 and 3/3?
> 
> I can drop patch 2, no need to resend. Thanks.
> 

I saw that you sent your pull request for v4.8 but the
patches from this series were not included:

https://lkml.org/lkml/2016/6/21/400

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Kalle Valo]

Javier Martinez Canillas <javier@osg.samsung.com> writes:

>>> Patch 3/3 applies cleanly even after dropping patch 2/3.
>>> Is that Ok for you or do you want me to re-resend a v3
>>> with only patches 1/3 and 3/3?
>> 
>> I can drop patch 2, no need to resend. Thanks.
>> 
>
> I saw that you sent your pull request for v4.8 but the
> patches from this series were not included:
>
> https://lkml.org/lkml/2016/6/21/400

I had forgotten to change the state of patches 1 and 3 from 'Changes
Requested' back to 'New' and that's why I missed them. I did that now so
they are back in my queue:

https://patchwork.kernel.org/patch/9158855/
https://patchwork.kernel.org/patch/9158851/

Thanks for checking.

-- 
Kalle Valo

Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file

[Javier Martinez Canillas]

Hello Kalle,

On 06/22/2016 02:17 AM, Kalle Valo wrote:
> Javier Martinez Canillas <javier@osg.samsung.com> writes:
> 
>>>> Patch 3/3 applies cleanly even after dropping patch 2/3.
>>>> Is that Ok for you or do you want me to re-resend a v3
>>>> with only patches 1/3 and 3/3?
>>>
>>> I can drop patch 2, no need to resend. Thanks.
>>>
>>
>> I saw that you sent your pull request for v4.8 but the
>> patches from this series were not included:
>>
>> https://lkml.org/lkml/2016/6/21/400
> 
> I had forgotten to change the state of patches 1 and 3 from 'Changes
> Requested' back to 'New' and that's why I missed them. I did that now so
> they are back in my queue:
> 
> https://patchwork.kernel.org/patch/9158855/
> https://patchwork.kernel.org/patch/9158851/
> 
> Thanks for checking.
> 

Ok, thanks for the information.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [v2, 1/3] mwifiex: add a cfg80211 .get_tx_power operation callback

[Kalle Valo]

Javier Martinez Canillas <javier@osg.samsung.com> wrote:
> The mwifiex driver implements a cfg80211 .set_tx_power operation handler
> but doesn't have the inverse .get_tx_power callback.
> 
> This not only has the effect that the Tx power can't be reported to user
> space tools such as iwconfig and iwlist but also that the wireless core
> prints a warning when a new wiphy is created due an cfg80211 operation
> being implemented without its counterpart.
> 
> After this patch, the Tx power is properly reported to user-space tools:
> 
> $ iwlist mlan0 txpower
> mlan0     unknown transmit-power information.
> 
>           Current Tx-Power=13 dBm       (19 mW)
> 
> and also the following warning isn't shown anymore on the driver probe:
> 
> WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac
> Modules linked in: mwifiex_sdio mwifiex
> CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: G        W       4.7.0-rc1-next-20160531-00006-g569df5b983f3
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: events request_firmware_work_func
> [<c010e1ac>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14)
> [<c010af38>] (show_stack) from [<c0323b9c>] (dump_stack+0x88/0x9c)
> [<c0323b9c>] (dump_stack) from [<c011a828>] (__warn+0xe8/0x100)
> [<c011a828>] (__warn) from [<c011a8f0>] (warn_slowpath_null+0x20/0x28)
> [<c011a8f0>] (warn_slowpath_null) from [<c06a42d4>] (wiphy_new_nm+0x66c/0x6ac)
> [<c06a42d4>] (wiphy_new_nm) from [<bf1c24cc>] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex])
> [<bf1c24cc>] (mwifiex_register_cfg80211 [mwifiex]) from [<bf1a0018>] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex])
> [<bf1a0018>] (mwifiex_fw_dpc [mwifiex]) from [<c040eb74>] (request_firmware_work_func+0x30/0x58)
> [<c040eb74>] (request_firmware_work_func) from [<c012fe90>] (process_one_work+0x124/0x338)
> [<c012fe90>] (process_one_work) from [<c01300dc>] (worker_thread+0x38/0x4d4)
> [<c01300dc>] (worker_thread) from [<c01353b8>] (kthread+0xdc/0xf4)
> [<c01353b8>] (kthread) from [<c0107978>] (ret_from_fork+0x14/0x3c)
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks, 2 patches applied to wireless-drivers-next.git:

7d54bacadce1 mwifiex: add a cfg80211 .get_tx_power operation callback
3ee712857958 mwifiex: add get_antenna support for cfg80211

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9158855/